Popular sites on the NTP

Popular sites on the NTP

Download a list of suggested sites from gstatic and display them on the NTP if there are not enough personalized suggestions.

BUG=516618
Committed: https://crrev.com/cffa650f655e0c47e77d6375cc073728ea08c5e2
Cr-Commit-Position: refs/heads/master@{#342084}

[Marc Treib, 2015-08-03 15:17:17]

Patchset #2 (id:20001) has been deleted

[Marc Treib, 2015-08-03 15:19:42]

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

[Marc Treib, 2015-08-03 15:19:43]

PTAL!

[Bernhard Bauer, 2015-08-03 17:04:51]

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
File chrome/browser/android/file_downloader.cc (right):

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/file_downloader.cc:22:
FileDownloader::FileDownloader(const GURL& url,
Can we move this into platform independent code and replace usages of
SupervisedUserBlacklistDownloader with it?

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:424: return GURL(url).host() ==
GURL(popular_site.url).host();
Could you pull the GURL(popular_site.url).host() out of the loop?

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.h (right):

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.h:29: class GURL;
Ordering: GURL before PopularSites

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
File chrome/browser/android/popular_sites.cc (right):

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/popular_sites.cc:25:
"https://www.gstatic.com/chrome/ntp/prototype0.json";
How about giving this a name that mentions popular sites?

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/popular_sites.cc:36: new
std::vector<PopularSites::Site>);
Allocate the new vector after you've verified that the file is readable and can
be parsed? Otherwise you'll return an empty vector, which will not trigger the
failure code path.

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/popular_sites.cc:100: if (sites) sites_.swap(*sites);
That's Java style 😃 Chromium C++ style is to put the body on a new line.

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
File chrome/browser/android/popular_sites.h (right):

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/popular_sites.h:26: class PopularSites {
Document this class?

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/popular_sites.h:31: std::string title;
Can this be a base::string16?

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/popular_sites.h:32: std::string url;
And this a GURL?

[Marc Treib, 2015-08-04 08:16:42]

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
File chrome/browser/android/file_downloader.cc (right):

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/file_downloader.cc:22:
FileDownloader::FileDownloader(const GURL& url,
On 2015/08/03 17:04:50, Bernhard Bauer wrote:
> Can we move this into platform independent code and replace usages of
> SupervisedUserBlacklistDownloader with it?

Dammit, I was sure I had written a comment saying just that (and wondering how
this exact helper class doesn't exist elsewhere yet). Where do you want to move
it? chrome/browser/net?

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:424: return GURL(url).host() ==
GURL(popular_site.url).host();
On 2015/08/03 17:04:50, Bernhard Bauer wrote:
> Could you pull the GURL(popular_site.url).host() out of the loop?

Obsolete now that it's already a GURL.

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.h (right):

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.h:29: class GURL;
On 2015/08/03 17:04:50, Bernhard Bauer wrote:
> Ordering: GURL before PopularSites

Done.

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
File chrome/browser/android/popular_sites.cc (right):

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/popular_sites.cc:25:
"https://www.gstatic.com/chrome/ntp/prototype0.json";
On 2015/08/03 17:04:51, Bernhard Bauer wrote:
> How about giving this a name that mentions popular sites?

popular_sites_IN.json? -> out-of-band discussion

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/popular_sites.cc:36: new
std::vector<PopularSites::Site>);
On 2015/08/03 17:04:50, Bernhard Bauer wrote:
> Allocate the new vector after you've verified that the file is readable and
can
> be parsed? Otherwise you'll return an empty vector, which will not trigger the
> failure code path.

Done.

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/popular_sites.cc:100: if (sites) sites_.swap(*sites);
On 2015/08/03 17:04:50, Bernhard Bauer wrote:
> That's Java style 😃 Chromium C++ style is to put the body on a new line.

Done.

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
File chrome/browser/android/popular_sites.h (right):

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/popular_sites.h:26: class PopularSites {
On 2015/08/03 17:04:51, Bernhard Bauer wrote:
> Document this class?

Done.

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/popular_sites.h:31: std::string title;
On 2015/08/03 17:04:51, Bernhard Bauer wrote:
> Can this be a base::string16?

It could, but why? We're getting UTF8 from the json.

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/popular_sites.h:32: std::string url;
On 2015/08/03 17:04:51, Bernhard Bauer wrote:
> And this a GURL?

Hm, yeah, I guess that makes for a somewhat cleaner interface. Done.

[Bernhard Bauer, 2015-08-04 08:30:02]

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
File chrome/browser/android/file_downloader.cc (right):

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/file_downloader.cc:22:
FileDownloader::FileDownloader(const GURL& url,
On 2015/08/04 08:16:41, Marc Treib wrote:
> On 2015/08/03 17:04:50, Bernhard Bauer wrote:
> > Can we move this into platform independent code and replace usages of
> > SupervisedUserBlacklistDownloader with it?
> 
> Dammit, I was sure I had written a comment saying just that (and wondering how
> this exact helper class doesn't exist elsewhere yet). Where do you want to
move
> it? chrome/browser/net?

SG.

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
File chrome/browser/android/popular_sites.h (right):

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/popular_sites.h:31: std::string title;
On 2015/08/04 08:16:42, Marc Treib wrote:
> On 2015/08/03 17:04:51, Bernhard Bauer wrote:
> > Can this be a base::string16?
> 
> It could, but why? We're getting UTF8 from the json.

It's a user-visible string, and we tend to store these as base::string16 (see
also the std::vector<base::string16> they end up in). DictionaryValue::GetString
also has an overload for base::string16*, so you don't even need to do the
conversion yourself.

[Marc Treib, 2015-08-04 08:57:51]

Patchset #4 (id:80001) has been deleted

[Marc Treib, 2015-08-04 08:58:43]

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
File chrome/browser/android/file_downloader.cc (right):

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/file_downloader.cc:22:
FileDownloader::FileDownloader(const GURL& url,
On 2015/08/04 08:30:02, Bernhard Bauer wrote:
> On 2015/08/04 08:16:41, Marc Treib wrote:
> > On 2015/08/03 17:04:50, Bernhard Bauer wrote:
> > > Can we move this into platform independent code and replace usages of
> > > SupervisedUserBlacklistDownloader with it?
> > 
> > Dammit, I was sure I had written a comment saying just that (and wondering
how
> > this exact helper class doesn't exist elsewhere yet). Where do you want to
> move
> > it? chrome/browser/net?
> 
> SG.

Done.

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
File chrome/browser/android/popular_sites.h (right):

https://codereview.chromium.org/1265983007/diff/40001/chrome/browser/android/...
chrome/browser/android/popular_sites.h:31: std::string title;
On 2015/08/04 08:30:02, Bernhard Bauer wrote:
> On 2015/08/04 08:16:42, Marc Treib wrote:
> > On 2015/08/03 17:04:51, Bernhard Bauer wrote:
> > > Can this be a base::string16?
> > 
> > It could, but why? We're getting UTF8 from the json.
> 
> It's a user-visible string, and we tend to store these as base::string16 (see
> also the std::vector<base::string16> they end up in).
DictionaryValue::GetString
> also has an overload for base::string16*, so you don't even need to do the
> conversion yourself.

Alright, done.

[Bernhard Bauer, 2015-08-04 09:19:56]

lgtm

[Marc Treib, 2015-08-04 09:45:28]

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

[Marc Treib, 2015-08-04 09:45:28]

+battre for c/b/net. I'm basically just moving an existing helper class there,
since it's now used in more than one place. PTAL!

[battre, 2015-08-04 10:57:15]

chrome/browser/net LGTM

[Marc Treib, 2015-08-04 16:02:39]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-08-04 16:02:39]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1265983007/#ps120001
(title: "URL")

[commit-bot: I haz the power, 2015-08-04 16:03:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1265983007/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1265983007/120001

[commit-bot: I haz the power, 2015-08-04 16:06:30]

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

[commit-bot: I haz the power, 2015-08-04 16:06:30]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Marc Treib, 2015-08-05 09:01:53]

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

[Marc Treib, 2015-08-05 09:01:53]

+rkaplow for histograms.xml - can I get an OWNERS approval, please? Thanks!

[rkaplow, 2015-08-05 15:28:41]

lgtm

[Marc Treib, 2015-08-05 15:41:07]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-08-05 15:41:08]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
battre@chromium.org
Link to the patchset: https://codereview.chromium.org/1265983007/#ps160001
(title: "histograms")

[commit-bot: I haz the power, 2015-08-05 15:41:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1265983007/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1265983007/160001

[commit-bot: I haz the power, 2015-08-05 17:15:46]

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

[commit-bot: I haz the power, 2015-08-05 17:15:47]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Marc Treib, 2015-08-06 07:36:56]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2015-08-06 07:37:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1265983007/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1265983007/160001

[commit-bot: I haz the power, 2015-08-06 07:41:45]

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

[commit-bot: I haz the power, 2015-08-06 07:41:45]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Marc Treib, 2015-08-06 08:06:46]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-08-06 08:06:46]

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

[commit-bot: I haz the power, 2015-08-06 08:07:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1265983007/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1265983007/180001

[commit-bot: I haz the power, 2015-08-06 09:12:34]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2015-08-06 09:13:08]

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