Popular sites on the NTP: add thumbnails!

Popular sites on the NTP: add thumbnails!

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

[Marc Treib, 2015-09-04 13:29:12]

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

[Marc Treib, 2015-09-04 13:29:12]

Thumbnails!!!1

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

https://codereview.chromium.org/1314493013/diff/40001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:244: return
suggestions_service->GetPageThumbnailWithURL(
I guess it's a bit hacky to go through the SuggestionsService here, even though
it might not be active. But it has all the thumbnail fetching logic set up
nicely. :)
The alternative would be to pull all of that out of components/suggestions and
put it somewhere else, probably a standalone component? That doesn't seem worth
the trouble, especially if we eventually want to move to ML.

[Bernhard Bauer, 2015-09-04 13:42:41]

OMG thumbnails!!!111oneone

LGTM w/ two nits:

https://codereview.chromium.org/1314493013/diff/40001/components/suggestions/...
File components/suggestions/image_manager.cc (right):

https://codereview.chromium.org/1314493013/diff/40001/components/suggestions/...
components/suggestions/image_manager.cc:61: image_url_map_[url] = image_url;
Add a thread check?

https://codereview.chromium.org/1314493013/diff/40001/components/suggestions/...
File components/suggestions/suggestions_service.h (right):

https://codereview.chromium.org/1314493013/diff/40001/components/suggestions/...
components/suggestions/suggestions_service.h:84: base::Callback<void(const
GURL&, const SkBitmap*)> callback);
Pass the callback by const reference?

[Marc Treib, 2015-09-04 13:49:03]

https://codereview.chromium.org/1314493013/diff/40001/components/suggestions/...
File components/suggestions/image_manager.cc (right):

https://codereview.chromium.org/1314493013/diff/40001/components/suggestions/...
components/suggestions/image_manager.cc:61: image_url_map_[url] = image_url;
On 2015/09/04 13:42:41, Bernhard Bauer wrote:
> Add a thread check?

Done.

https://codereview.chromium.org/1314493013/diff/40001/components/suggestions/...
File components/suggestions/suggestions_service.h (right):

https://codereview.chromium.org/1314493013/diff/40001/components/suggestions/...
components/suggestions/suggestions_service.h:84: base::Callback<void(const
GURL&, const SkBitmap*)> callback);
On 2015/09/04 13:42:41, Bernhard Bauer wrote:
> Pass the callback by const reference?

Done.
I was just blindly copying GetPageThumbnail here.. changed that to const ref as
well.

[Marc Treib, 2015-09-04 13:49:37]

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

[Marc Treib, 2015-09-04 13:49:38]

+mathp for components/suggestions/ - PTAL!
Context:
https://docs.google.com/document/d/1rAaC1mlERlBuxjIc415HmmrcQ5LSAT5bM1zQMbAFc...

[Mathieu, 2015-09-04 14:03:47]

mathp@chromium.org changed reviewers:
+ fserb@chromium.org

[Mathieu, 2015-09-04 14:07:34]

On 2015/09/04 13:49:38, Marc Treib wrote:
> +mathp for components/suggestions/ - PTAL!
> Context:
>
https://docs.google.com/document/d/1rAaC1mlERlBuxjIc415HmmrcQ5LSAT5bM1zQMbAFc...

Could you test for the suggestions changes, thanks!

[Marc Treib, 2015-09-07 10:00:41]

On 2015/09/04 14:07:34, Mathieu Perreault wrote:
> On 2015/09/04 13:49:38, Marc Treib wrote:
> > +mathp for components/suggestions/ - PTAL!
> > Context:
> >
>
https://docs.google.com/document/d/1rAaC1mlERlBuxjIc415HmmrcQ5LSAT5bM1zQMbAFc...
> 
> Could you test for the suggestions changes, thanks!

Tests added, PTAL!

[Mathieu, 2015-09-08 14:39:22]

Thanks Marc, lgtm

[Marc Treib, 2015-09-08 14:42:05]

Patchset #6 (id:100001) has been deleted

[Marc Treib, 2015-09-08 14:42:12]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-09-08 14:42:12]

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/1314493013/#ps80001
(title: "add tests")

[commit-bot: I haz the power, 2015-09-08 14:42:30]

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

[commit-bot: I haz the power, 2015-09-08 14:44:04]

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

[commit-bot: I haz the power, 2015-09-08 14:44:05]

Try jobs failed on following builders:
  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...)

[Marc Treib, 2015-09-08 15:29:01]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-09-08 15:29:02]

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

[commit-bot: I haz the power, 2015-09-08 15:29:09]

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

[commit-bot: I haz the power, 2015-09-08 15:56:51]

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

[commit-bot: I haz the power, 2015-09-08 15:56:52]

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

[Marc Treib, 2015-09-08 16:05:41]

The CQ bit was checked by treib@chromium.org

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

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

[commit-bot: I haz the power, 2015-09-08 16:33:37]

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

[commit-bot: I haz the power, 2015-09-08 16:33:38]

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

[Marc Treib, 2015-09-09 07:38:08]

Patchset #7 (id:140001) has been deleted

[Marc Treib, 2015-09-09 07:38:50]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-09-09 07:38:50]

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

[commit-bot: I haz the power, 2015-09-09 07:39:58]

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

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

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-09-09 09:09:31]

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