do not fetch topsite information if not shown

[AndriusA]

Relates to brave/brave-browser#9481

Mitigates performance issues with topsites when user has thousands of bookmarks (see brave/brave-browser#9481 (comment)) by not fetching topsite information when the widget is disabled.

Looking at chromium side of bookmarks code, looking up by { url: url } goes to execution path that checks URL index as a hashmap, vs. looking up by just url having to iterate the entire list of all the bookmarks each time (the former is faster when done).

Submitter Checklist:

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Verified that these changes build without errors on
  • Android
  • iOS
  • Linux
  • macOS
  • Windows
• Verified that these changes pass automated tests (unit, browser, security tests) on
  • iOS
  • Linux
  • macOS
  • Windows
• Verified that all lint errors/warnings are resolved (npm run lint)
• Ran git rebase master (if needed).
• Ran git rebase -i to squash commits (if needed).
• Tagged reviewers and labelled the pull request as needed.
• Request a security/privacy review as needed.
• Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone
• Public documentation has been updated as necessary. For instance:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protections
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Custom-Headers
  • https://github.com/brave/brave-browser/wiki/Web-compatibility-issues-with-tracking-protection
  • https://github.com/brave/brave-browser/wiki/QA-Guide

Test Plan:

Reviewer Checklist:

• New files have MPL-2.0 license header.
• Request a security/privacy review as needed.
• Adequate test coverage exists to prevent regressions
• Verify test plan is specified in PR before merging to source

After-merge Checklist:

• The associated issue milestone is set to the smallest version that the
  changes has landed on.
• All relevant documentation has been updated.

[cezaraugusto]

++

[petemill]

This is great. Small change requested so far.

[petemill]

Do we have a simple test plan with a sample data set that can be used to confirm the issue is fixed and the performance impact?

[cezaraugusto]

@petemill I think this PR goal is just not to call any chrome API if users disabled the feature

[AndriusA]

correct, and to call search api with a different parameter (per https://developer.chrome.com/extensions/bookmarks#method-search)

Can share bookmarks used for testing

[petemill]

Sure but we still need to validate that turning the API request off fixes the issue users have encountered and we can do that with test data.

[petemill]

If you can, please squash the "code review comments" in to the PR commit so we can keep the changes together and easier to explain when looking in the future

[AndriusA]

PTAL

[petemill]

Seems to be getting a build error, which makes sense.

@AndriusA @cezaraugusto are you not getting this error?

ERR! /home/travis/build/brave/brave-core/components/brave_new_tab_ui/api/initialData.ts
ERR! [tsl] ERROR in /home/travis/build/brave/brave-core/components/brave_new_tab_ui/api/initialData.ts(57,7)
ERR!       TS2322: Type 'Stats | undefined' is not assignable to type 'Stats'.
ERR!   Type 'undefined' is not assignable to type 'Stats'.
ERR! /home/travis/build/brave/brave-core/components/brave_new_tab_ui/api/initialData.ts
ERR! [tsl] ERROR in /home/travis/build/brave/brave-core/components/brave_new_tab_ui/api/initialData.ts(59,7)
ERR!       TS2322: Type 'MostVisitedURL[] | undefined' is not assignable to type 'MostVisitedURL[]'.
ERR!   Type 'undefined' is not assignable to type 'MostVisitedURL[]'.

[AndriusA]

weirdly - no, locally everything seemed fine. Possibly something didn't get picked up for incremental build.

Feel free to fix it up or throw out altogether, looks like assumptions about every source of data always being there are baked in all the way, even if user has disabled them

[petemill]

@AndriusA if this is P1 we should definitely fix, right? Might be as simple as setting the correct nullable type on 1 property in the store. Do you know the rough perf improvement on the change of { url: url }?

[AndriusA]

that's made obsolete by #5490 so we may not have a major performance issue on it for now

the topSites and stats shouldn't be as heavy and while I don't think it makes sense to load that data even when not shown, it requires more time than I'm prepared to spend to keep the exact same ux of widgets appearing fully loaded on a toggle