Implementing chrome://thumbnails page to view URLs in TopSites and cached thumbnails.

Implementing chrome://thumbnails page to view URLs in TopSites and cached thumbnails.

BUG=298240
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226411

[huangs, 2013-09-25 18:18:00]

This is a preliminary, functional CL that loads images with chrome://thumb. A
screen shot is attached with the Issue 284634.

Perhaps we can embed thumbnails into the HTML using "data:image/png;base64,...".
This way, can get rid of the extra HasPageThumbnail() interface, and have a
single HTML page that can be easily copied and reproduced for debugging. WDYT?

[huangs, 2013-09-25 19:50:41]

Embedded the thumbnails via Base64. Now the page is self-contained, and the
rather redundant HasPageThumbnail() interface is no longer needed. PTAL.

[huangs, 2013-09-25 19:51:36]

There are some residual cleanups in the files I touched. I think we're better
off keeping them, but let me know if I should revert them to keep the CL pure.

[beaudoin, 2013-09-25 21:36:36]

LGTM, nits.

https://codereview.chromium.org/24632002/diff/6001/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/thumbnail_list_source.cc (right):

https://codereview.chromium.org/24632002/diff/6001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/thumbnail_list_source.cc:41: // If |show_thumbnail|
== true, then render elements in |mv_list| that have
NIT: that have _a_ thumbnail

https://codereview.chromium.org/24632002/diff/6001/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/thumbnail_list_source.h (right):

https://codereview.chromium.org/24632002/diff/6001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/thumbnail_list_source.h:25: class
ThumbnailListSource : public content::URLDataSource {
Add a comment to explain what that is?

[huangs, 2013-09-25 21:49:26]

OWNER review for sky@, PTAL.

https://codereview.chromium.org/24632002/diff/6001/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/thumbnail_list_source.cc (right):

https://codereview.chromium.org/24632002/diff/6001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/thumbnail_list_source.cc:41: // If |show_thumbnail|
== true, then render elements in |mv_list| that have
On 2013/09/25 21:36:37, beaudoin wrote:
> NIT: that have _a_ thumbnail

Done.

https://codereview.chromium.org/24632002/diff/6001/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/thumbnail_list_source.h (right):

https://codereview.chromium.org/24632002/diff/6001/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/thumbnail_list_source.h:25: class
ThumbnailListSource : public content::URLDataSource {
On 2013/09/25 21:36:37, beaudoin wrote:
> Add a comment to explain what that is?

Done.

[huangs, 2013-09-30 17:23:49]

Ping sky@ for OWNER review.

[sky, 2013-09-30 20:46:16]

https://codereview.chromium.org/24632002/diff/13001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/thumbnail_list_source.cc (right):

https://codereview.chromium.org/24632002/diff/13001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/thumbnail_list_source.cc:34: const
history::MostVisitedURL* mv_url;
Can you copy the url here. No lifetime issues to worry about when you do that.

https://codereview.chromium.org/24632002/diff/13001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/thumbnail_list_source.cc:37:
MostVisitedURLWithImage() : mv_url(NULL) {}
nit: functions before members with structs too.

https://codereview.chromium.org/24632002/diff/13001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/thumbnail_list_source.cc:96: base::Unretained(this),
Why the unretained here?

https://codereview.chromium.org/24632002/diff/13001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/thumbnail_list_source.cc:121: int num_mv =
visited_list.size();
nit: const

[huangs, 2013-09-30 22:07:11]

PTAL.

https://codereview.chromium.org/24632002/diff/13001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/ntp/thumbnail_list_source.cc (right):

https://codereview.chromium.org/24632002/diff/13001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/thumbnail_list_source.cc:34: const
history::MostVisitedURL* mv_url;
I was being silly in trying to wrap history::MostVisitedURL into a struct, so I
can pass a single const vector<T>. Switching to passing 2 const vector<T>, one
for MostVisitedURL and one for base64 image makes more sense. Restructured.

https://codereview.chromium.org/24632002/diff/13001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/thumbnail_list_source.cc:37:
MostVisitedURLWithImage() : mv_url(NULL) {}
On 2013/09/30 20:46:16, sky wrote:
> nit: functions before members with structs too.

Done.

https://codereview.chromium.org/24632002/diff/13001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/thumbnail_list_source.cc:96: base::Unretained(this),
Changed to weak_ptr_factory_.GetWeakPtr(), emulating TopSitesQuerier in
top_sites_impl_unittest.cc, but without CancelRequest().

https://codereview.chromium.org/24632002/diff/13001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/ntp/thumbnail_list_source.cc:121: int num_mv =
visited_list.size();
On 2013/09/30 20:46:16, sky wrote:
> nit: const

Done.

[sky, 2013-09-30 22:37:55]

LGTM

[commit-bot: I haz the power, 2013-10-01 14:50:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/24632002/32001

[commit-bot: I haz the power, 2013-10-01 14:50:19]

Failed to apply patch for chrome/browser/search/instant_service.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/search/instant_service.cc
  Hunk #1 succeeded at 27 with fuzz 1 (offset 4 lines).
  Hunk #2 FAILED at 96.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/search/instant_service.cc.rej

Patch:       chrome/browser/search/instant_service.cc
Index: chrome/browser/search/instant_service.cc
diff --git a/chrome/browser/search/instant_service.cc
b/chrome/browser/search/instant_service.cc
index
62e8580219b32d5f12b5399ba1ae5057c5085115..c5d34e178fb4f372142ba3dfa1f4a35d6609e80e
100644
--- a/chrome/browser/search/instant_service.cc
+++ b/chrome/browser/search/instant_service.cc
@@ -23,6 +23,7 @@
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/webui/favicon_source.h"
+#include "chrome/browser/ui/webui/ntp/thumbnail_list_source.h"
 #include "chrome/browser/ui/webui/ntp/thumbnail_source.h"
 #include "chrome/browser/ui/webui/theme_source.h"
 #include "content/public/browser/browser_thread.h"
@@ -95,6 +96,7 @@ InstantService::InstantService(Profile* profile)
 
   content::URLDataSource::Add(profile, new ThumbnailSource(profile, false));
   content::URLDataSource::Add(profile, new ThumbnailSource(profile, true));
+  content::URLDataSource::Add(profile, new ThumbnailListSource(profile));
   content::URLDataSource::Add(profile, new FaviconSource(
       profile, FaviconSource::FAVICON));
   content::URLDataSource::Add(profile, new LocalNtpSource(profile));

[commit-bot: I haz the power, 2013-10-01 16:09:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/24632002/37001

[commit-bot: I haz the power, 2013-10-01 16:45:16]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-01 18:04:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/24632002/51001

[commit-bot: I haz the power, 2013-10-01 19:12:40]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-10-01 23:50:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/24632002/51001

[commit-bot: I haz the power, 2013-10-02 01:13:56]

Retried try job too often on win_rel for step(s) chrome_frame_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-02 04:53:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/24632002/51001

[commit-bot: I haz the power, 2013-10-02 07:07:05]

Change committed as 226411