New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 619584 link

Starred by 2 users
Status: WontFix
Owner:
mastiz@chromium.org
Closed: Mar 2018
Cc:
█ sfiera@chromium.org
█ markusheintz@chromium.org
█ noyau@chromium.org
treib@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature

Blocked on:
issue 650589
issue 603026
Blocking:
issue 619585



Sign in to add a comment

Make MostVisitedSites a KeyedService

Project Member Reported by sfiera@chromium.org, Jun 13 2016 
Currently a MostVisitedSites object is instantiated each time the new tab page is opened. It should become a long-lived KeyedService. Tasks include:

- Support addition/removal of observers, instead of setting a single one.
- Configure the number of tiles in a different way, one of:
- - Pick a sufficiently high, constant value (12?).
- - Accept num_tiles in the constructor.
- - Allow each observer to pick make a request, and use the max.
- Change it to a KeyedService subclass, with a factory in chrome/.

Comment 1 by sfiera@chromium.org, Jun 13 2016

Blocking: 619585

Comment 2 by markusheintz@chromium.org, Jun 13 2016

Cc: markusheintz@chromium.org

Comment 3 by nepper@chromium.org, Jun 14 2016

Cc: sfiera@chromium.org
Labels: -Restrict-View-Google -Pri-2 -zine-mr-untriaged zine-ntp-pe Pri-3 Type-Feature
Owner: ----
Status: Available (was: Untriaged)

Comment 4 by sfiera@chromium.org, Jun 15 2016

Owner: sfiera@chromium.org

Comment 5 by treib@chromium.org, Jun 15 2016

Cc: treib@chromium.org
Status: Assigned (was: Available)

Comment 6 by treib@chromium.org, Jul 15 2016

Blocking: 607111

Comment 7 by fi...@chromium.org, Jul 29 2016

Labels: zine-triaged

Comment 8 by sfiera@chromium.org, Sep 27 2016

Blockedon: 650589

Comment 9 by sfiera@chromium.org, Sep 27 2016

Cc: noyau@chromium.org

Comment 10 by mastiz@chromium.org, Dec 9 2016

Owner: mastiz@chromium.org
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 14 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/78efbde6ae8fa8409079dc0ef14eadf447ffd8d2

commit 78efbde6ae8fa8409079dc0ef14eadf447ffd8d2
Author: mastiz <mastiz@chromium.org>
Date: Wed Dec 14 17:07:07 2016

[Popular Sites] Split PopularSites interface and PopularSitesImpl

The introduction of an interface allows dependent classes
(MostVisitedSites) to inject doubles in tests.

BUG= 619584 

Review-Url: https://codereview.chromium.org/2570783003
Cr-Commit-Position: refs/heads/master@{#438536}

[modify] https://crrev.com/78efbde6ae8fa8409079dc0ef14eadf447ffd8d2/chrome/browser/ntp_tiles/chrome_popular_sites_factory.cc
[modify] https://crrev.com/78efbde6ae8fa8409079dc0ef14eadf447ffd8d2/chrome/browser/prefs/browser_prefs.cc
[modify] https://crrev.com/78efbde6ae8fa8409079dc0ef14eadf447ffd8d2/components/ntp_tiles/most_visited_sites.h
[modify] https://crrev.com/78efbde6ae8fa8409079dc0ef14eadf447ffd8d2/components/ntp_tiles/popular_sites.cc
[modify] https://crrev.com/78efbde6ae8fa8409079dc0ef14eadf447ffd8d2/components/ntp_tiles/popular_sites.h
[modify] https://crrev.com/78efbde6ae8fa8409079dc0ef14eadf447ffd8d2/components/ntp_tiles/popular_sites_unittest.cc
[modify] https://crrev.com/78efbde6ae8fa8409079dc0ef14eadf447ffd8d2/components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc
[modify] https://crrev.com/78efbde6ae8fa8409079dc0ef14eadf447ffd8d2/components/ntp_tiles/webui/popular_sites_internals_message_handler.cc
[modify] https://crrev.com/78efbde6ae8fa8409079dc0ef14eadf447ffd8d2/ios/chrome/browser/ntp_tiles/ios_popular_sites_factory.cc
[modify] https://crrev.com/78efbde6ae8fa8409079dc0ef14eadf447ffd8d2/ios/chrome/browser/prefs/browser_prefs.mm
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b06f14242378cb8ce63fd7ee3ec89d4287757c49

commit b06f14242378cb8ce63fd7ee3ec89d4287757c49
Author: mastiz <mastiz@chromium.org>
Date: Thu Dec 15 12:32:17 2016

[Popular Sites] Move PopularSitesImpl to dedicated file

Trivial follow-up to https://codereview.chromium.org/2570783003/ where
it was suggested to move the new class to a dedicated file.

BUG= 619584 

Review-Url: https://codereview.chromium.org/2575823002
Cr-Commit-Position: refs/heads/master@{#438811}

[modify] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/chrome/browser/ntp_tiles/chrome_popular_sites_factory.cc
[modify] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/chrome/browser/prefs/browser_prefs.cc
[modify] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/components/ntp_tiles/BUILD.gn
[modify] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/components/ntp_tiles/most_visited_sites.h
[modify] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/components/ntp_tiles/popular_sites.h
[rename] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/components/ntp_tiles/popular_sites_impl.cc
[add] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/components/ntp_tiles/popular_sites_impl.h
[rename] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/components/ntp_tiles/popular_sites_impl_unittest.cc
[modify] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/ios/chrome/browser/ntp_tiles/ios_popular_sites_factory.cc
[modify] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/ios/chrome/browser/prefs/browser_prefs.mm
Project Member

Comment 13 by bugdroid1@chromium.org, Dec 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b06f14242378cb8ce63fd7ee3ec89d4287757c49

commit b06f14242378cb8ce63fd7ee3ec89d4287757c49
Author: mastiz <mastiz@chromium.org>
Date: Thu Dec 15 12:32:17 2016

[Popular Sites] Move PopularSitesImpl to dedicated file

Trivial follow-up to https://codereview.chromium.org/2570783003/ where
it was suggested to move the new class to a dedicated file.

BUG= 619584 

Review-Url: https://codereview.chromium.org/2575823002
Cr-Commit-Position: refs/heads/master@{#438811}

[modify] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/chrome/browser/ntp_tiles/chrome_popular_sites_factory.cc
[modify] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/chrome/browser/prefs/browser_prefs.cc
[modify] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/components/ntp_tiles/BUILD.gn
[modify] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/components/ntp_tiles/most_visited_sites.h
[modify] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/components/ntp_tiles/popular_sites.h
[rename] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/components/ntp_tiles/popular_sites_impl.cc
[add] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/components/ntp_tiles/popular_sites_impl.h
[rename] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/components/ntp_tiles/popular_sites_impl_unittest.cc
[modify] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/ios/chrome/browser/ntp_tiles/ios_popular_sites_factory.cc
[modify] https://crrev.com/b06f14242378cb8ce63fd7ee3ec89d4287757c49/ios/chrome/browser/prefs/browser_prefs.mm
Project Member

Comment 14 by bugdroid1@chromium.org, Dec 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8dc93c5196fbf8d6309f065276f4766ba5e00abf

commit 8dc93c5196fbf8d6309f065276f4766ba5e00abf
Author: mastiz <mastiz@chromium.org>
Date: Thu Dec 15 14:01:34 2016

ntp_tiles: Refactor existing MostVisitedSites tests

Expectations are now more explicitly inlined in tests without an
intermediate Check() function. Among other things, this allows better
failure messages including line numbers.

This is a preceding step before adding new tests, which require a few
other patches submitted first.

BUG= 619584 

Review-Url: https://codereview.chromium.org/2567333003
Cr-Commit-Position: refs/heads/master@{#438824}

[modify] https://crrev.com/8dc93c5196fbf8d6309f065276f4766ba5e00abf/components/ntp_tiles/most_visited_sites.h
[modify] https://crrev.com/8dc93c5196fbf8d6309f065276f4766ba5e00abf/components/ntp_tiles/most_visited_sites_unittest.cc
Project Member

Comment 15 by bugdroid1@chromium.org, Dec 16 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d5d7b4a47a878e25b55294cf09751340aaf302e2

commit d5d7b4a47a878e25b55294cf09751340aaf302e2
Author: mastiz <mastiz@chromium.org>
Date: Fri Dec 16 21:41:38 2016

[SuggestionsService] Split SuggestionsService interface from impl

The introduction of an interface allows dependent classes
(primarily MostVisitedSites) to inject doubles in tests, since it's
otherwise hard due to the number of dependencies.

A small signature change is adopted: GetSuggestionsDataFromCache() now
returns base::Optional, which allows clients to distinguish cache
misses from cache containing an empty tileset. Client code has been
updated to prevent behavioral changes.

BUG= 619584 

Review-Url: https://codereview.chromium.org/2568133005
Cr-Commit-Position: refs/heads/master@{#439195}

[modify] https://crrev.com/d5d7b4a47a878e25b55294cf09751340aaf302e2/chrome/browser/search/suggestions/suggestions_service_factory.cc
[modify] https://crrev.com/d5d7b4a47a878e25b55294cf09751340aaf302e2/chrome/browser/search/suggestions/suggestions_ui.cc
[modify] https://crrev.com/d5d7b4a47a878e25b55294cf09751340aaf302e2/chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc
[modify] https://crrev.com/d5d7b4a47a878e25b55294cf09751340aaf302e2/components/ntp_tiles/most_visited_sites.cc
[modify] https://crrev.com/d5d7b4a47a878e25b55294cf09751340aaf302e2/components/suggestions/BUILD.gn
[modify] https://crrev.com/d5d7b4a47a878e25b55294cf09751340aaf302e2/components/suggestions/suggestions_service.h
[rename] https://crrev.com/d5d7b4a47a878e25b55294cf09751340aaf302e2/components/suggestions/suggestions_service_impl.cc
[add] https://crrev.com/d5d7b4a47a878e25b55294cf09751340aaf302e2/components/suggestions/suggestions_service_impl.h
[rename] https://crrev.com/d5d7b4a47a878e25b55294cf09751340aaf302e2/components/suggestions/suggestions_service_impl_unittest.cc
[modify] https://crrev.com/d5d7b4a47a878e25b55294cf09751340aaf302e2/ios/chrome/browser/suggestions/suggestions_service_factory.mm
Project Member

Comment 16 by bugdroid1@chromium.org, Dec 17 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/451ce6c31e4a2643271bc29e68a103109069fb38

commit 451ce6c31e4a2643271bc29e68a103109069fb38
Author: mastiz <mastiz@chromium.org>
Date: Sat Dec 17 08:13:02 2016

ntp_tiles: Add tests covering tile-fetching logic

The previous tests only covered a tiny part of the component, which is
the merging logic, but not the actual combinations of events that can
happen during the fetching of tiles from the backends (top sites,
suggestions service, popular sites).

Mocks are used extensively due to the lack of fakes, and because the
interfaces are really big. It's somewhat painful to read, but
alternatives are not better.

The tests surface one bug which is not addressed here, left more a
follow-up patch and represented with a TODO.

Popular Sites fetching is still not covered by these tests, and
another TODO is added for this purpose.

BUG= 619584 

Review-Url: https://codereview.chromium.org/2579813002
Cr-Commit-Position: refs/heads/master@{#439335}

[modify] https://crrev.com/451ce6c31e4a2643271bc29e68a103109069fb38/components/ntp_tiles/most_visited_sites.cc
[modify] https://crrev.com/451ce6c31e4a2643271bc29e68a103109069fb38/components/ntp_tiles/most_visited_sites_unittest.cc

Comment 17 by mastiz@chromium.org, Oct 4 2017

Blocking: -607111

Comment 18 by fi...@chromium.org, Jan 12 2018

Components: -UI>Browser>NewTabPage UI>Browser>ContentSuggestions

Comment 19 by mastiz@chromium.org, Mar 16 2018

Status: WontFix (was: Assigned)
It's not likely that this bug gets addressed anytime soon, and interest in dependent bugs has decreased, so I'll just close it.
►

Sign in to add a comment