Add support for components/ntp_tiles in InstantService

Add support for components/ntp_tiles in InstantService

This will allow us to get server-side suggestions on the local NTP.

BUG=514752
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/bb0c5af55c5614d10981fa0dc5e9d92028e7ae2b
Cr-Commit-Position: refs/heads/master@{#437576}

[Marc Treib, 2016-11-28 13:01:27]

Description was changed from

==========
Add support for components/ntp_tiles in InstantService

This will allow us to get server-side suggestions on the local NTP.

BUG=514752
==========

to

==========
Add support for components/ntp_tiles in InstantService

This will allow us to get server-side suggestions on the local NTP.

BUG=514752
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Marc Treib, 2016-11-28 13:03:52]

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

[commit-bot: I haz the power, 2016-11-28 13:04:04]

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

[Marc Treib, 2016-11-28 13:07:00]

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

[Marc Treib, 2016-11-28 13:07:00]

Ready for a first look!

[sfiera, 2016-11-28 13:11:12]

Quick look at //components/…, haven't started the full review yet.

https://codereview.chromium.org/2532103002/diff/1/components/ntp_tiles/most_v...
File components/ntp_tiles/most_visited_sites.h (right):

https://codereview.chromium.org/2532103002/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites.h:115: void ClearBlacklistedUrls();
Wasn't it already possible to clear the blacklist on Android? How is it doing
that?

https://codereview.chromium.org/2532103002/diff/1/components/ntp_tiles/ntp_ti...
File components/ntp_tiles/ntp_tile.h (right):

https://codereview.chromium.org/2532103002/diff/1/components/ntp_tiles/ntp_ti...
components/ntp_tiles/ntp_tile.h:40: // Only valid for source ==
SUGGESTIONS_SERVICE (empty otherwise).
Are they always non-empty for the suggestions service? What happens if a site
doesn't have a favicon, or we haven't yet visited it to produce the thumbnail?

[Marc Treib, 2016-11-28 13:19:29]

https://codereview.chromium.org/2532103002/diff/1/components/ntp_tiles/most_v...
File components/ntp_tiles/most_visited_sites.h (right):

https://codereview.chromium.org/2532103002/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites.h:115: void ClearBlacklistedUrls();
On 2016/11/28 13:11:12, sfiera wrote:
> Wasn't it already possible to clear the blacklist on Android? How is it doing
> that?

No, I don't think that was possible on Android. (In fact, even the "undo
blacklist" action is fairly new on Android...)

https://codereview.chromium.org/2532103002/diff/1/components/ntp_tiles/ntp_ti...
File components/ntp_tiles/ntp_tile.h (right):

https://codereview.chromium.org/2532103002/diff/1/components/ntp_tiles/ntp_ti...
components/ntp_tiles/ntp_tile.h:40: // Only valid for source ==
SUGGESTIONS_SERVICE (empty otherwise).
On 2016/11/28 13:11:12, sfiera wrote:
> Are they always non-empty for the suggestions service? What happens if a site
> doesn't have a favicon, or we haven't yet visited it to produce the thumbnail?

I believe the favicon URL always points to some chrome://favicons/ URL, and may
or may not 404.

The thumbnail URL can point to a local chrome:// (or chrome-search://, I forget)
URL and/or an online URL pointing to a thumbnail service. I think it can be
empty in some cases.

[commit-bot: I haz the power, 2016-11-28 15:47:42]

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

[commit-bot: I haz the power, 2016-11-28 15:47:43]

Dry run: This issue passed the CQ dry run.

[sfiera, 2016-11-29 10:24:03]

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/resources/lo...
File chrome/browser/resources/local_ntp/most_visited_single.js (right):

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/resources/lo...
chrome/browser/resources/local_ntp/most_visited_single.js:305: data.tileSource =
data.isServerSide ?
isServerSide is added by this CL, right? Can we just set the int from the
beginning?

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/search/insta...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/search/insta...
chrome/browser/search/instant_service.cc:120: if (most_visited_sites_) {
base::MakeUnique<>() cannot return nullptr, can it? So this can be merged into
the previous block.

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/search/insta...
chrome/browser/search/instant_service.cc:123: scoped_refptr<history::TopSites>
top_sites =
We repeat this call a lot below. Is there a reason we don't want to keep a ref
in this class? The below functions generally look like this:

  if (most_visited_sites_) {
    …
  } else {
    scoped_refptr<history::TopSites> top_sites = …;
    if (top_sites_) {
      …
    }
  }

and they would look better to me like this:

  if (most_visited_sites_) {
    …
  } else if (top_sites_) {
    …
  }

https://codereview.chromium.org/2532103002/diff/1/components/ntp_tiles/ntp_ti...
File components/ntp_tiles/ntp_tile.h (right):

https://codereview.chromium.org/2532103002/diff/1/components/ntp_tiles/ntp_ti...
components/ntp_tiles/ntp_tile.h:40: // Only valid for source ==
SUGGESTIONS_SERVICE (empty otherwise).
On 2016/11/28 13:19:29, Marc Treib wrote:
> On 2016/11/28 13:11:12, sfiera wrote:
> > Are they always non-empty for the suggestions service? What happens if a
site
> > doesn't have a favicon, or we haven't yet visited it to produce the
thumbnail?
> 
> I believe the favicon URL always points to some chrome://favicons/ URL, and
may
> or may not 404.
> 
> The thumbnail URL can point to a local chrome:// (or chrome-search://, I
forget)
> URL and/or an online URL pointing to a thumbnail service. I think it can be
> empty in some cases.

I would appreciate this kind of information in the comment :)

[Marc Treib, 2016-11-29 15:19:26]

Patchset #2 (id:20001) has been deleted

[Marc Treib, 2016-11-29 15:20:13]

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/resources/lo...
File chrome/browser/resources/local_ntp/most_visited_single.js (right):

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/resources/lo...
chrome/browser/resources/local_ntp/most_visited_single.js:305: data.tileSource =
data.isServerSide ?
On 2016/11/29 10:24:03, sfiera wrote:
> isServerSide is added by this CL, right? Can we just set the int from the
> beginning?

Yes, it's added by this CL. What do you mean by setting the int from the
beginning?

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/search/insta...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/search/insta...
chrome/browser/search/instant_service.cc:120: if (most_visited_sites_) {
On 2016/11/29 10:24:03, sfiera wrote:
> base::MakeUnique<>() cannot return nullptr, can it? So this can be merged into
> the previous block.

Done.

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/search/insta...
chrome/browser/search/instant_service.cc:123: scoped_refptr<history::TopSites>
top_sites =
On 2016/11/29 10:24:03, sfiera wrote:
> We repeat this call a lot below. Is there a reason we don't want to keep a ref
> in this class? The below functions generally look like this:
> 
>   if (most_visited_sites_) {
>     …
>   } else {
>     scoped_refptr<history::TopSites> top_sites = …;
>     if (top_sites_) {
>       …
>     }
>   }
> 
> and they would look better to me like this:
> 
>   if (most_visited_sites_) {
>     …
>   } else if (top_sites_) {
>     …
>   }

I don't see a reason not to keep a ref to the TopSites. So, done.

https://codereview.chromium.org/2532103002/diff/1/components/ntp_tiles/ntp_ti...
File components/ntp_tiles/ntp_tile.h (right):

https://codereview.chromium.org/2532103002/diff/1/components/ntp_tiles/ntp_ti...
components/ntp_tiles/ntp_tile.h:40: // Only valid for source ==
SUGGESTIONS_SERVICE (empty otherwise).
On 2016/11/29 10:24:03, sfiera wrote:
> On 2016/11/28 13:19:29, Marc Treib wrote:
> > On 2016/11/28 13:11:12, sfiera wrote:
> > > Are they always non-empty for the suggestions service? What happens if a
> site
> > > doesn't have a favicon, or we haven't yet visited it to produce the
> thumbnail?
> > 
> > I believe the favicon URL always points to some chrome://favicons/ URL, and
> may
> > or may not 404.
> > 
> > The thumbnail URL can point to a local chrome:// (or chrome-search://, I
> forget)
> > URL and/or an online URL pointing to a thumbnail service. I think it can be
> > empty in some cases.
> 
> I would appreciate this kind of information in the comment :)

Not sure how much of this we want to "fixate" in the API - e.g. the favicon URL
may be passed down from the server (though that doesn't happen in practice these
days AFAIK).
Well, I tried to add some info, PTAL.

[sfiera, 2016-11-29 15:27:46]

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/resources/lo...
File chrome/browser/resources/local_ntp/most_visited_single.js (right):

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/resources/lo...
chrome/browser/resources/local_ntp/most_visited_single.js:305: data.tileSource =
data.isServerSide ?
On 2016/11/29 15:20:13, Marc Treib wrote:
> On 2016/11/29 10:24:03, sfiera wrote:
> > isServerSide is added by this CL, right? Can we just set the int from the
> > beginning?
> 
> Yes, it's added by this CL. What do you mean by setting the int from the
> beginning?

I mean, you start with an enum in InstantService, which you map to a bool, which
you map back to the enum for logging. It would be simpler to set data.tileSource
at the beginning, instead of mapping it through data.isServerSide, wouldn't it?

The NTPLoggingTileSource enum values are equal to the corresponding ntp_tiles::
ones, right? So you don't even need a mapping.

[Marc Treib, 2016-12-01 14:49:40]

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/resources/lo...
File chrome/browser/resources/local_ntp/most_visited_single.js (right):

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/resources/lo...
chrome/browser/resources/local_ntp/most_visited_single.js:305: data.tileSource =
data.isServerSide ?
On 2016/11/29 15:27:46, sfiera wrote:
> On 2016/11/29 15:20:13, Marc Treib wrote:
> > On 2016/11/29 10:24:03, sfiera wrote:
> > > isServerSide is added by this CL, right? Can we just set the int from the
> > > beginning?
> > 
> > Yes, it's added by this CL. What do you mean by setting the int from the
> > beginning?
> 
> I mean, you start with an enum in InstantService, which you map to a bool,
which
> you map back to the enum for logging. It would be simpler to set
data.tileSource
> at the beginning, instead of mapping it through data.isServerSide, wouldn't
it?

Ah, now I see - yes, that makes a lot of sense. (I was reusing an existing (but
unused) is_server_side_suggestion flag in InstantMostVisitedItem...)

> The NTPLoggingTileSource enum values are equal to the corresponding
ntp_tiles::
> ones, right? So you don't even need a mapping.

I *really* don't want to rely on the fact that the enum values just so happen to
be equal. IMO the solution is to get rid of NTPLoggingTileSource completely, and
just use the ntp_tiles:: version everywhere. I'll make a separate CL and rebase
this one on top of that.

[Marc Treib, 2016-12-07 14:01:31]

PTAL again!

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/resources/lo...
File chrome/browser/resources/local_ntp/most_visited_single.js (right):

https://codereview.chromium.org/2532103002/diff/1/chrome/browser/resources/lo...
chrome/browser/resources/local_ntp/most_visited_single.js:305: data.tileSource =
data.isServerSide ?
On 2016/12/01 14:49:40, Marc Treib wrote:
> On 2016/11/29 15:27:46, sfiera wrote:
> > On 2016/11/29 15:20:13, Marc Treib wrote:
> > > On 2016/11/29 10:24:03, sfiera wrote:
> > > > isServerSide is added by this CL, right? Can we just set the int from
the
> > > > beginning?
> > > 
> > > Yes, it's added by this CL. What do you mean by setting the int from the
> > > beginning?
> > 
> > I mean, you start with an enum in InstantService, which you map to a bool,
> which
> > you map back to the enum for logging. It would be simpler to set
> data.tileSource
> > at the beginning, instead of mapping it through data.isServerSide, wouldn't
> it?
> 
> Ah, now I see - yes, that makes a lot of sense. (I was reusing an existing
(but
> unused) is_server_side_suggestion flag in InstantMostVisitedItem...)
> 
> > The NTPLoggingTileSource enum values are equal to the corresponding
> ntp_tiles::
> > ones, right? So you don't even need a mapping.
> 
> I *really* don't want to rely on the fact that the enum values just so happen
to
> be equal. IMO the solution is to get rid of NTPLoggingTileSource completely,
and
> just use the ntp_tiles:: version everywhere. I'll make a separate CL and
rebase
> this one on top of that.

Done.

https://codereview.chromium.org/2532103002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/local_ntp/most_visited_single.js (left):

https://codereview.chromium.org/2532103002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/local_ntp/most_visited_single.js:307: data.tileSource =
NTPTileSource.TOP_SITES;
Not necessary to set this anymore here; data.tileSource already has the correct
value.

[sfiera, 2016-12-07 14:10:00]

LGTM

https://codereview.chromium.org/2532103002/diff/100001/chrome/browser/search/...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/2532103002/diff/100001/chrome/browser/search/...
chrome/browser/search/instant_service.cc:117: // TODO(treib): Add supervisor.
Another TODO: share this with Android in a factory. Can be TODO(sfiera).

[Marc Treib, 2016-12-08 11:10:40]

https://codereview.chromium.org/2532103002/diff/100001/chrome/browser/search/...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/2532103002/diff/100001/chrome/browser/search/...
chrome/browser/search/instant_service.cc:117: // TODO(treib): Add supervisor.
On 2016/12/07 14:10:00, sfiera wrote:
> Another TODO: share this with Android in a factory. Can be TODO(sfiera).

Done.

[Marc Treib, 2016-12-08 11:12:34]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org, kenrb@chromium.org

[Marc Treib, 2016-12-08 11:12:35]

+bauerb for browser_prefs.cc

+kenrb for render_messages.h

PTAL!

[Bernhard Bauer, 2016-12-08 14:57:52]

browser_prefs LGTM

[kenrb, 2016-12-09 14:26:12]

ipc lgtm

[Marc Treib, 2016-12-09 14:26:41]

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

[commit-bot: I haz the power, 2016-12-09 14:26:54]

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

[Marc Treib, 2016-12-09 14:27:08]

Thanks all!

+mastiz FYI

[commit-bot: I haz the power, 2016-12-09 16:05:11]

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

[commit-bot: I haz the power, 2016-12-09 16:05:12]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-12-09 16:13:08]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-12-09 16:13:10]

The patchset sent to the CQ was uploaded after l-g-t-m from sfiera@chromium.org
Link to the patchset: https://codereview.chromium.org/2532103002/#ps120001
(title: "TODO (and rebase)")

[commit-bot: I haz the power, 2016-12-09 16:13:22]

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, 2016-12-09 16:19:07]

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

[commit-bot: I haz the power, 2016-12-09 16:19:08]

Failed to apply patch for components/ntp_tiles/ntp_tile.h:
While running git apply --index -p1;
  error: patch failed: components/ntp_tiles/ntp_tile.h:24
  error: components/ntp_tiles/ntp_tile.h: patch does not apply

Patch:       components/ntp_tiles/ntp_tile.h
Index: components/ntp_tiles/ntp_tile.h
diff --git a/components/ntp_tiles/ntp_tile.h b/components/ntp_tiles/ntp_tile.h
index
fa37cf8568c096836159447c3213f6fee9ab712e..3ece726c7a63fb2060c5643fddd0f2331955090a
100644
--- a/components/ntp_tiles/ntp_tile.h
+++ b/components/ntp_tiles/ntp_tile.h
@@ -24,6 +24,12 @@ struct NTPTile {
   // Only valid for source == WHITELIST (empty otherwise).
   base::FilePath whitelist_icon_path;
 
+  // Only valid for source == SUGGESTIONS_SERVICE (empty otherwise).
+  // May point to a local chrome:// URL or to a remote one. May be empty.
+  GURL thumbnail_url;
+  // This won't be empty, but might 404 etc.
+  GURL favicon_url;
+
   NTPTile();
   NTPTile(const NTPTile&);
   ~NTPTile();

[Marc Treib, 2016-12-09 16:21:35]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-12-09 16:21:35]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
kenrb@chromium.org, sfiera@chromium.org
Link to the patchset: https://codereview.chromium.org/2532103002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-09 16:21:52]

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, 2016-12-09 17:34:20]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481300495195840,
"parent_rev": "29caa30982ea0f7e7813c14c69c87c2e666ff621", "commit_rev":
"b367298873834dbde914a45a1b23fa35a92e20b3"}

[commit-bot: I haz the power, 2016-12-09 17:34:46]

Description was changed from

==========
Add support for components/ntp_tiles in InstantService

This will allow us to get server-side suggestions on the local NTP.

BUG=514752
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add support for components/ntp_tiles in InstantService

This will allow us to get server-side suggestions on the local NTP.

BUG=514752
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2532103002
==========

[commit-bot: I haz the power, 2016-12-09 17:34:47]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-12-12 14:35:50]

Description was changed from

==========
Add support for components/ntp_tiles in InstantService

This will allow us to get server-side suggestions on the local NTP.

BUG=514752
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2532103002
==========

to

==========
Add support for components/ntp_tiles in InstantService

This will allow us to get server-side suggestions on the local NTP.

BUG=514752
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/bb0c5af55c5614d10981fa0dc5e9d92028e7ae2b
Cr-Commit-Position: refs/heads/master@{#437576}
==========

[commit-bot: I haz the power, 2016-12-12 14:35:52]

Patchset 7 (id:??) landed as
https://crrev.com/bb0c5af55c5614d10981fa0dc5e9d92028e7ae2b
Cr-Commit-Position: refs/heads/master@{#437576}