Generate thumbnails in the browser process.

Generate thumbnails in the browser process.

The feature is now behind --enable-in-browser-thumbnailing flag.

The in-browser thumbnailing works as follows:

- The scroll offset is sent from the renderer to the browser.
- The thumbnail is taken when the page info is sent from the renderer process.
- Since thumbnails are generated in the browser, we can avoid the generation when unnecessary (ex. off-the-record mode, or New Tab Page).
- The quality of thumbnails is as good as before, as the patch doesn't change timing heuristics.
- The drawback is that we cannot take thumbnails from background tabs.

New functions are added to ThumbnailGenerator for clipping thumbnails, with tests.

BUG=65936
TEST=add unit tests for thumbnail_generator. confirmed that the thumbnails are updated with and without --enable-in-browser-thumbnailing

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72115

[brettw, 2011-01-18 18:25:37]

http://codereview.chromium.org/6246007/diff/1/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (left):

http://codereview.chromium.org/6246007/diff/1/chrome/renderer/render_view.cc#...
chrome/renderer/render_view.cc:1281: bool RenderView::CaptureThumbnail(WebView*
view,
I think we probably want to keep the old code and have your new feature
controlled by a command-line switch. This way, you can land a
partially-implemented patch and continue to work without disturbing people on
the dev channel, and it can be turned off on the beta channel if we think it
might be causing problems or isn't ready.

Hopefully adding this is self-explanatory. The thing that people usually forget
is to modify BrowserRenderProcessHost::PropagateBrowserCommandLineToRenderer if
they need the command line switch in the renderer.

http://codereview.chromium.org/6246007/diff/1/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6246007/diff/1/chrome/renderer/render_view.cc#...
chrome/renderer/render_view.cc:4979: Send(new
ViewHostMsg_UpdateScrollOffset(routing_id_, gfx_scroll_offset));
If you need something in RenderWidget, you can just make a virtual function that
RenderView provides an implementation for. An example is
GetBitmapForOptimizedPluginPaint. Potentially the default one could just return
Size(0, 0) and then RenderView would provide the "real" one. Would this allow
you to implement this more cleanly?

[satorux1, 2011-01-19 01:36:32]

Thank you for the great feedback!

http://codereview.chromium.org/6246007/diff/1/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (left):

http://codereview.chromium.org/6246007/diff/1/chrome/renderer/render_view.cc#...
chrome/renderer/render_view.cc:1281: bool RenderView::CaptureThumbnail(WebView*
view,
On 2011/01/18 18:25:42, brettw wrote:
> I think we probably want to keep the old code and have your new feature
> controlled by a command-line switch. This way, you can land a
> partially-implemented patch and continue to work without disturbing people on
> the dev channel, and it can be turned off on the beta channel if we think it
> might be causing problems or isn't ready.
> 
> Hopefully adding this is self-explanatory. The thing that people usually
forget
> is to modify BrowserRenderProcessHost::PropagateBrowserCommandLineToRenderer
if
> they need the command line switch in the renderer.

That's a great idea. I'll change it accordingly.

http://codereview.chromium.org/6246007/diff/1/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6246007/diff/1/chrome/renderer/render_view.cc#...
chrome/renderer/render_view.cc:4979: Send(new
ViewHostMsg_UpdateScrollOffset(routing_id_, gfx_scroll_offset));
On 2011/01/18 18:25:42, brettw wrote:
> If you need something in RenderWidget, you can just make a virtual function
that
> RenderView provides an implementation for. An example is
> GetBitmapForOptimizedPluginPaint. Potentially the default one could just
return
> Size(0, 0) and then RenderView would provide the "real" one. Would this allow
> you to implement this more cleanly?

Sounds like a good idea. I'll give it a try.

[satorux1, 2011-01-19 08:47:14]

Brett,

Just added --enable-in-browser-thumbnailing per your suggestion. I think the
patch is now ready for a code review. Please take a look.

[brettw, 2011-01-20 00:30:51]

Let me summarize what I was originally thinking for this feature. One of the
problems is that this is a larger project and makes sense to do in several
passes. I think something very much like your current patch would make a good
first step. But I'd like to get your opinion on this and we may want to make
some changes depending on what we want to do next.

I was originally thinking that the feature would be implemented entirely within
the thumbnail generator by just watching events and deciding when a good time
to take a thumbnail would be. The OnPageContents message in TabContents is
actually irrelevant to what it means to be a good thumbnail, it just happens to
be the time we used to do it.

I was thinking the scroll position would be sent in the
ViewHostMsg_UpdateRect_Params structure, and a paint would be one of the events
that the thumnbial generator looks for when deciding to capture a thumbnail.
This would avoid having to have an extra IPC message and the extra data on
RenderView to track the scroll position.

The thumbnail generator would listen for these paint events along with the
"backing store discarded" messages we currently listen for, plus the load state
changes, and decide when the best time to capture a thumbnail would be. If the
notification is sent before the events are actually processed, the thumbnail
generator has the option to compute the thumbnail very lazily, waiting until
the state "gets worse" before taking the thumbnail.

As an example, when the page is loading, it will get a bunch of paints at
scroll (0x0). These would likely score lower according to our heuristic because
the load hasn't completed. Then the load completes and we get a paint, which
scores more highly, but we still don't need to take a thumbnail until something
"bad" happens. An example would be a paint message with a scroll offset, a
paint message for a new page, or a backing store destroyed message. The paint
manager would notice that the backing store is about to become "worse" and will
save a thumbnail using the current (better quality) contents.


If you agree this is a good idea, I think your current patch is largely what we
want as a first step. It doesn't need all the heuristics, but it does move the
thumbnail work to the browser process where we can continue enhancing the
logic.

I haven't thought through all the logic. I'm interested in your thoughts on
whether its practical in this patch to hook up the scroll position to the paint
message and move the code in TabContents to the thumbnail generator, or if we
should wait for a subsequent patch, or possibly something in between?

http://codereview.chromium.org/6246007/diff/7010/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6246007/diff/7010/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.cc:2260: 
Style nit: probably don't need these extra lines here.

[satorux1, 2011-01-20 03:06:29]

Brett,

Thank you for sharing your thoughts. I agree with you that it would be nicer
to implement this feature entirely in ThumbnailGenerator. I originally
thought about using StartThumbnailing() and related code, but the existing
code seemed to be designed for the overview mode of Chrome OS, and looked
not easy to reuse for our purpose here. Hence I chose to start from a
simpler patch instead.

I also agree with you that we should to do this in several passes. In this
first patch, we are able to generate thumbnails with the same quality as
before, though we cannot generate thumbnails if tabs are in the background.
I think this is a good first step.

I think we have two goals for this project: efficiency and quality. What
about attacking the two goals separately:

Efficiency:

1. Go with this patch as a first step, with necessary changes
2. Implement heuristics to skip thumbnail generation. For instance, if a URL
is not in the top 20 sites, skip it. For top 20 sites, we can also skip if
what we already have is good and new enough. With heuristics like these, we
can reduce the number of thumbnail generations significantly.

Quality:

1. Move thumbnail generation timing from TabContents::OnPageContents() to
ThumbnailGenerator.
2. Implement heuristics about when would be best to generate thumbnails.

For Chrome OS, I think the first goal, efficiency, is more important for
now, so I'd like to start from this side. Does this sound good to you?

Satoru

On Thu, Jan 20, 2011 at 9:30 AM, <brettw@chromium.org> wrote:

> Let me summarize what I was originally thinking for this feature. One of
> the
> problems is that this is a larger project and makes sense to do in several
> passes. I think something very much like your current patch would make a
> good
> first step. But I'd like to get your opinion on this and we may want to
> make
> some changes depending on what we want to do next.

> I was originally thinking that the feature would be implemented entirely
> within
> the thumbnail generator by just watching events and deciding when a good
> time
> to take a thumbnail would be. The OnPageContents message in TabContents is
> actually irrelevant to what it means to be a good thumbnail, it just
> happens to
> be the time we used to do it.

> I was thinking the scroll position would be sent in the
> ViewHostMsg_UpdateRect_Params structure, and a paint would be one of the
> events
> that the thumnbial generator looks for when deciding to capture a
> thumbnail.
> This would avoid having to have an extra IPC message and the extra data on
> RenderView to track the scroll position.

> The thumbnail generator would listen for these paint events along with the
> "backing store discarded" messages we currently listen for, plus the load
> state
> changes, and decide when the best time to capture a thumbnail would be. If
> the
> notification is sent before the events are actually processed, the
> thumbnail
> generator has the option to compute the thumbnail very lazily, waiting
> until
> the state "gets worse" before taking the thumbnail.

> As an example, when the page is loading, it will get a bunch of paints at
> scroll (0x0). These would likely score lower according to our heuristic
> because
> the load hasn't completed. Then the load completes and we get a paint,
> which
> scores more highly, but we still don't need to take a thumbnail until
> something
> "bad" happens. An example would be a paint message with a scroll offset, a
> paint message for a new page, or a backing store destroyed message. The
> paint
> manager would notice that the backing store is about to become "worse" and
> will
> save a thumbnail using the current (better quality) contents.


> If you agree this is a good idea, I think your current patch is largely
> what we
> want as a first step. It doesn't need all the heuristics, but it does move
> the
> thumbnail work to the browser process where we can continue enhancing the
> logic.

> I haven't thought through all the logic. I'm interested in your thoughts  
> on
> whether its practical in this patch to hook up the scroll position to the
> paint
> message and move the code in TabContents to the thumbnail generator, or if
> we
> should wait for a subsequent patch, or possibly something in between?



>
http://codereview.chromium.org/6246007/diff/7010/chrome/browser/tab_contents/...
> File chrome/browser/tab_contents/tab_contents.cc (right):


>
http://codereview.chromium.org/6246007/diff/7010/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/tab_contents.cc:2260:
> Style nit: probably don't need these extra lines here.


> http://codereview.chromium.org/6246007/

[satorux1, 2011-01-20 05:43:39]

+derat for his thought on the overview mode.

Per your feedback, moved the scroll offset into ViewHostMsg_UpdateRect_Params.
We still need to keep this in RenderWidgetHost as we haven't moved the thumbnail
generation trigger from TabContents to ThumbnailGenerator. At least, we could
get rid of one IPC with this change.

Re doing everything in ThumbnailGenerator, the existing code like
StartThumbnailing() seems to be designed for the overview mode of Chrome OS,
which has been disabled for a while. Do we have a plan to re-enable this
feature? Otherwise, we could modify the existing code for our purpose, without
worrying about breaking the overview mode feature.

http://codereview.chromium.org/6246007/diff/7010/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6246007/diff/7010/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.cc:2260: 
On 2011/01/20 00:30:51, brettw wrote:
> Style nit: probably don't need these extra lines here.

Done.

[Daniel Erat, 2011-01-20 18:25:13]

Removing the existing overview implementation (both from Chrome and from the
window manager) would greatly simplify the code, but when I checked last week,
the UX people hadn't decided whether they want something similar to what's
already there or something completely different. :-/

On 2011/01/20 05:43:39, satorux1 wrote:
> +derat for his thought on the overview mode.
> 
> Per your feedback, moved the scroll offset into ViewHostMsg_UpdateRect_Params.
> We still need to keep this in RenderWidgetHost as we haven't moved the
thumbnail
> generation trigger from TabContents to ThumbnailGenerator. At least, we could
> get rid of one IPC with this change.
> 
> Re doing everything in ThumbnailGenerator, the existing code like
> StartThumbnailing() seems to be designed for the overview mode of Chrome OS,
> which has been disabled for a while. Do we have a plan to re-enable this
> feature? Otherwise, we could modify the existing code for our purpose, without
> worrying about breaking the overview mode feature.
> 
>
http://codereview.chromium.org/6246007/diff/7010/chrome/browser/tab_contents/...
> File chrome/browser/tab_contents/tab_contents.cc (right):
> 
>
http://codereview.chromium.org/6246007/diff/7010/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/tab_contents.cc:2260: 
> On 2011/01/20 00:30:51, brettw wrote:
> > Style nit: probably don't need these extra lines here.
> 
> Done.

[brettw, 2011-01-21 00:09:17]

LGTM

I have a few comments that are mostly trivial. If you agree with these
suggestions, feel free to just check it in so you don't have to wait another
whole day for me.

http://codereview.chromium.org/6246007/diff/13001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6246007/diff/13001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:2237: void
TabContents::UpdateThumbnailIfNecessary(const GURL& url) {
I'm always trying to get people to not add stuff to TabContents since it's
already so large. How about moving this function to the ThumbnailGenerator and
have the TabContents call it (possibly providing "this" as an argument). Then
the code is more in the right place when we refactor this later.

This will also allow you to remove the thumbnail_score.h include.

http://codereview.chromium.org/6246007/diff/13001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/thumbnail_generator.h (right):

http://codereview.chromium.org/6246007/diff/13001/chrome/browser/tab_contents...
chrome/browser/tab_contents/thumbnail_generator.h:45: kNone = 0,
Can you rename this to kNoOptions (or something similar) so it's more unique?

http://codereview.chromium.org/6246007/diff/13001/chrome/browser/tab_contents...
chrome/browser/tab_contents/thumbnail_generator.h:108: static double
CalculateBoringScore(SkBitmap* bitmap);
Moving the thumbnail computation, you can remove this from the public section of
the header and put it as a static method in the .cc file (some people prefer a
const private function on this class, which is also OK if you like that better).

[satorux1, 2011-01-21 04:53:43]

http://codereview.chromium.org/6246007/diff/13001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6246007/diff/13001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:2237: void
TabContents::UpdateThumbnailIfNecessary(const GURL& url) {
On 2011/01/21 00:09:17, brettw wrote:
> I'm always trying to get people to not add stuff to TabContents since it's
> already so large. How about moving this function to the ThumbnailGenerator and
> have the TabContents call it (possibly providing "this" as an argument). Then
> the code is more in the right place when we refactor this later.
> 
> This will also allow you to remove the thumbnail_score.h include.

That's a good idea. Moved accordingly.

http://codereview.chromium.org/6246007/diff/13001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/thumbnail_generator.h (right):

http://codereview.chromium.org/6246007/diff/13001/chrome/browser/tab_contents...
chrome/browser/tab_contents/thumbnail_generator.h:45: kNone = 0,
On 2011/01/21 00:09:17, brettw wrote:
> Can you rename this to kNoOptions (or something similar) so it's more unique?

Done.