ntp_tiles: Migrate to multi-observer model

ntp_tiles: Migrate to multi-observer model

No behavioral changes except a subtle change affecting Finch metrics,
described later below. The goal is to bring the component one step
closer to becoming a service, where multiple observers will need to
coexist.

MostVisitedSites' API now replaces SetMostVisitedURLsObserver() with
AddObserver()/RemoveObserver(). There are two important changes:

1. The maximum number of tiles is no longer specified, since each
   observer could have a different interest. Instead, clients are
   expected to truncate the tileset manually.

2. AddObserver() doesn't trigger a refresh. Instead, clients are
   expected to call Refresh() explicitly.

Finch actives for Popular Sites studies will increase due to
ShouldShowPopularSites() being called more often. This is because
the component will internally always try to build a tileset of size 12
(and pull Popular Sites if enabled). This change might also have a
performance impact (due to PopularSites::StartFetch).

BUG=619584
WIP: to adopt multiobserver.

[mastiz, 2017-01-10 09:17:42]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 09:17:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 09:25:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 09:25:32]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[mastiz, 2017-01-10 12:08:48]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 12:09:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 12:54:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 12:54:40]

Dry run: This issue passed the CQ dry run.

[mastiz, 2017-01-10 15:18:48]

mastiz@chromium.org changed reviewers:
+ sfiera@chromium.org

[mastiz, 2017-01-10 15:18:49]

I think this brings the component one step closer to becoming a service. Not a
great state for long term, but perhaps acceptable as transitional solution.

Note that, in this first version, there's some nontrivial logic (truncation of
tileset) in the bridge class and equivalent, I'm not sure if it's worth pushing
this logic to upper layers.

[mastiz, 2017-01-10 15:19:34]

Description was changed from

==========
ntp_tiles: Migrate to multi-observer model

No behavioral changes except a subtle change affecting Finch metrics,
described later below.

MostVisitedSites' API now replaces SetMostVisitedURLsObserver() with
AddObserver()/RemoveObserver(). There are two important changes:

1. The maximum number of tiles is no longer specified, since each
   observer could have a different interest. Instead, clients are
   expected to truncate the tileset manually.

2. AddObserver() doesn't trigger a refresh. Instead, clients are
   expected to call Refresh() explicitly.

Finch actives for Popular Sites studies will increase due to
ShouldShowPopularSites() being called more often. This is because
the component will internally always try to build a tileset of size 12
(and pull Popular Sites if enabled). This change might also have a
performance impact (due to PopularSites::StartFetch).

BUG=619584

WIP: to adopt multiobserver.
==========

to

==========
ntp_tiles: Migrate to multi-observer model

No behavioral changes except a subtle change affecting Finch metrics,
described later below. The goal is to bring the component one step
closer to becoming a service, where multiple observers will need to
coexist.

MostVisitedSites' API now replaces SetMostVisitedURLsObserver() with
AddObserver()/RemoveObserver(). There are two important changes:

1. The maximum number of tiles is no longer specified, since each
   observer could have a different interest. Instead, clients are
   expected to truncate the tileset manually.

2. AddObserver() doesn't trigger a refresh. Instead, clients are
   expected to call Refresh() explicitly.

Finch actives for Popular Sites studies will increase due to
ShouldShowPopularSites() being called more often. This is because
the component will internally always try to build a tileset of size 12
(and pull Popular Sites if enabled). This change might also have a
performance impact (due to PopularSites::StartFetch).

BUG=619584

WIP: to adopt multiobserver.
==========

[sfiera, 2017-01-18 09:58:33]

https://codereview.chromium.org/2619993002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right):

https://codereview.chromium.org/2619993002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/most_visited_sites_bridge.cc:121:
java_observer_.reset(new JavaObserver(env, j_observer, num_sites));
Can we change JavaObserver to call {Add,Remove}Observer() in its
constructor/destructor? This would match the behavior of the ios implementation
(and if we choose one behavior, that one seems better than this one).

I think I would keep the Refresh() external, though (as it is in both places).

https://codereview.chromium.org/2619993002/diff/20001/chrome/browser/search/i...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/2619993002/diff/20001/chrome/browser/search/i...
chrome/browser/search/instant_service.cc:106:
most_visited_sites_->AddObserver(this);
Here, you're not doing any filtering to produce 8 tiles. I assume that means we
can provide >8 tiles to JS now. Have you tested this with the bing or yandex
NTPs?

https://codereview.chromium.org/2619993002/diff/20001/components/ntp_tiles/mo...
File components/ntp_tiles/most_visited_sites.h (right):

https://codereview.chromium.org/2619993002/diff/20001/components/ntp_tiles/mo...
components/ntp_tiles/most_visited_sites.h:112: // once the request completes.
This is now also necessary after every AddObserver() call, right? (there is no
way to get the current state, only updates)

https://codereview.chromium.org/2619993002/diff/20001/ios/chrome/browser/ui/n...
File ios/chrome/browser/ui/ntp/google_landing_controller.mm (right):

https://codereview.chromium.org/2619993002/diff/20001/ios/chrome/browser/ui/n...
ios/chrome/browser/ui/ntp/google_landing_controller.mm:1110: if
(_mostVisitedData.size() > kMaxNumMostVisitedFavicons)
We're resizing to save memory, but it's not related to the number of tiles we're
actually going to show, is it? I think it would be good to have a comment in
this function explaining that we might show fewer than _mostVisitedData.size()
tiles, and a TODO to correct the impressions metrics accordingly (ios will make
you create a bug for a TODO; P3/sfiera is OK).