Popular sites on the NTP: re-download popular suggestions once per Chrome run

Popular sites on the NTP: re-download popular suggestions once per Chrome run

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

[Marc Treib, 2015-09-08 14:26:35]

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

[Marc Treib, 2015-09-08 14:26:35]

Next part :)  (based on knn's pending CL, and will need to be rebased on top of
maybelle's)
PTAL!

[Bernhard Bauer, 2015-09-08 16:06:59]

lgtm

[Marc Treib, 2015-09-09 08:11:05]

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

[Marc Treib, 2015-09-09 08:11:05]

+battre for c/b/net changes. PTAL!

[Marc Treib, 2015-09-09 15:38:20]

Patchset #2 (id:20001) has been deleted

[Marc Treib, 2015-09-11 16:15:02]

On 2015/09/09 08:11:05, Marc Treib wrote:
> +battre for c/b/net changes. PTAL!

Ping!

[battre, 2015-09-14 14:31:25]

What do you think of adding a unit test?

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

https://codereview.chromium.org/1308723005/diff/60001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:137: bool NeedPopularSites(const
PrefService* prefs, size_t num_tiles) {
Please document the meaning of num_tiles.

https://codereview.chromium.org/1308723005/diff/60001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:147: bool is_personal;
please initialize this variable.

https://codereview.chromium.org/1308723005/diff/60001/chrome/browser/net/file...
File chrome/browser/net/file_downloader.cc (right):

https://codereview.chromium.org/1308723005/diff/60001/chrome/browser/net/file...
chrome/browser/net/file_downloader.cc:68: DLOG(WARNING) << "HTTP error " <<
response_code;
Nit: This DLOG is pretty useless because it contains no context about the file
being downloaded.

[Marc Treib, 2015-09-18 11:44:24]

Immediate comments addressed.

Agreed that a unit test for FileDownloader would make sense. Can I add that in a
followup CL though? I have a few dependent CLs waiting on this one...

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

https://codereview.chromium.org/1308723005/diff/60001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:137: bool NeedPopularSites(const
PrefService* prefs, size_t num_tiles) {
On 2015/09/14 14:31:25, battre wrote:
> Please document the meaning of num_tiles.

Done.

https://codereview.chromium.org/1308723005/diff/60001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:147: bool is_personal;
On 2015/09/14 14:31:25, battre wrote:
> please initialize this variable.

Done.

https://codereview.chromium.org/1308723005/diff/60001/chrome/browser/net/file...
File chrome/browser/net/file_downloader.cc (right):

https://codereview.chromium.org/1308723005/diff/60001/chrome/browser/net/file...
chrome/browser/net/file_downloader.cc:68: DLOG(WARNING) << "HTTP error " <<
response_code;
On 2015/09/14 14:31:25, battre wrote:
> Nit: This DLOG is pretty useless because it contains no context about the file
> being downloaded.

I added the download URL, also to the DLOG above.

[Marc Treib, 2015-09-22 08:58:02]

Ping!
Is it okay to land this as-is, and add the test in a followup? I promise I won't
forget :)

[battre, 2015-09-22 09:07:57]

lgtm

[Marc Treib, 2015-09-22 09:46:21]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-09-22 09:46:21]

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/1308723005/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2015-09-22 09:46:28]

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

[commit-bot: I haz the power, 2015-09-22 10:53:13]

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

[commit-bot: I haz the power, 2015-09-22 10:53:14]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2015-09-22 10:56:51]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2015-09-22 10:57:11]

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

[commit-bot: I haz the power, 2015-09-22 12:38:16]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-09-22 12:40:24]

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