Allow CopyFromBackingStore to fallback to copying from the renderer side if the accelerated surface…

Add RenderWidgetHost::GetSnapshotFromRenderer method to content/ interface for snapshotting for cases where we might not be able to use CopyFromBackingStore because the browser side can't access the backing store or accelerated surface due to driver issues or WinXP.

Consolidate renderer-side snapshotting into that method and move clients (tabsApi, tabCapture and NTP) to the new method.

Let the Linux CopyFromCompositingSurface always use this since the current one is incorrect if the tab is in the background or is covered by a window.

Remove Linux-GTK snapshotting workarounds.

BUG=188867, 174957, 132301
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189969

[justinlin, 2013-03-15 06:19:15]

Hi reviewers, ptal.

I think maybe platform-specific CopyFromCompositingSurface implementations
should be able to fallback to the renderer side copy, but that's a much bigger
change.

We want to mostly fix this problem for WinXP and systems with driver issues to
at least return something instead of a black screen first with this change.

[Alpha Left Google, 2013-03-15 06:35:59]

I'm confused by this change.

It seems to me this change tries to ask the renderer to take a snapshot and
sends it back to the renderer. Why is SnapshotTabHelper not capable of doing
this? Is this new method better?

It appears to me this implementation asks renderer to draw into a skbitmap
through the WebView interface, does it work for accelerated composited content
like WebGL and Flash?

I'm not sure this patch actually solves the problem but touches a lot of
unrelated places.

[justinlin, 2013-03-15 06:43:47]

SnapshotTabHelper would need to be moved to content/ for us to use it. I started
moving it, but it seemed like it made sense to just make the
CopyFromBackingStore do this, and it simplifies a number of things.

I haven't tested WebGL and Flash, but I can take a look tomorrow. This is still
better than nothing, which is what we used to have. This does exactly what
SnapshotTabHelper used to do, so it should fix the problem. I can test on Nick's
desktop tomorrow.

This also fixes the Linux problem for us and we don't need to do that
composited-swap anymore.

[justinlin, 2013-03-16 02:24:22]

Cleaned up some things and reduced scope of this. Will do deletions in separate
CL.

jamesr@: ptal at everything except
c/b/renderer_host/render_widget_host_view_gtk*

Thanks!

[justinlin, 2013-03-16 02:27:08]

I mean c/b/renderer_host/render_widget_host_view_*

[jamesr, 2013-03-18 01:32:19]

https://codereview.chromium.org/12881005/diff/9009/content/common/view_messag...
File content/common/view_messages.h (right):

https://codereview.chromium.org/12881005/diff/9009/content/common/view_messag...
content/common/view_messages.h:1389: // Sent from the browser to ask the
renderer for a snapshot of the current view.
This IPC pair already exists:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/rend...

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/rend...

Please don't add duplicate code (or in this case, very nearly duplicate code)

[justinlin, 2013-03-18 03:21:14]

https://codereview.chromium.org/12881005/diff/9009/content/common/view_messag...
File content/common/view_messages.h (right):

https://codereview.chromium.org/12881005/diff/9009/content/common/view_messag...
content/common/view_messages.h:1389: // Sent from the browser to ask the
renderer for a snapshot of the current view.
On 2013/03/18 01:32:19, jamesr wrote:
> This IPC pair already exists:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/rend...
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/rend...
> 
> Please don't add duplicate code (or in this case, very nearly duplicate code)

Right, but we would like to use it from content/, hence the goal of this CL is
to move that functionality into content/, and then delete SnapshotTabHelper in a
separate CL (patch #1 did the delete, but I would like to do it separately).

[jamesr, 2013-03-18 03:49:20]

On 2013/03/18 03:21:14, justinlin wrote:
>
https://codereview.chromium.org/12881005/diff/9009/content/common/view_messag...
> File content/common/view_messages.h (right):
> 
>
https://codereview.chromium.org/12881005/diff/9009/content/common/view_messag...
> content/common/view_messages.h:1389: // Sent from the browser to ask the
> renderer for a snapshot of the current view.
> On 2013/03/18 01:32:19, jamesr wrote:
> > This IPC pair already exists:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/rend...
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/rend...
> > 
> > Please don't add duplicate code (or in this case, very nearly duplicate
code)
> 
> Right, but we would like to use it from content/, hence the goal of this CL is
> to move that functionality into content/, and then delete SnapshotTabHelper in
a
> separate CL (patch #1 did the delete, but I would like to do it separately).

I'd prefer keeping one mechanism in the tree at a time.  That means making sure
the new mechanism does everything the old one did and passes all of the tests
for the old mechanism, but I think that's a good thing.

[justinlin, 2013-03-18 07:45:11]

OK, I put back the deletions into this CL and the tests seem to pass.

+skerner who added this to the tabs API.

Hi Sam, I know you added this a long time ago, but does this seem like a
reasonable change? As far as I know, we should always either be able to copy
from backing store or compositing surface, except for cases like WinXP or driver
issues, so CopyFromBackingStore shouldn't fail. This CL moves the renderer-side
copy directly into the CopyFromCompositingSurface implementation for cases where
the accelerated surface is not available from the browser side.

[Sam Kerner (Chrome), 2013-03-18 13:49:51]

https://codereview.chromium.org/12881005/diff/31001/chrome/browser/extensions...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/12881005/diff/31001/chrome/browser/extensions...
chrome/browser/extensions/api/tabs/tabs_api.cc:1753:
SendResultFromBitmap(bitmap);
It has been a while since I was in this code.  Is the backing store guaranteed
to give an image if it is possible to get one?  I wrote the fallback path you
removed because at the time, the backing store was an LRU cache of bitmaps.  The
cache had limited size, so that a miss was common for user with many tabs open.

https://codereview.chromium.org/12881005/diff/31001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/12881005/diff/31001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.h:455: // browser side
might fail due to buggy drivers or other issues.
Unless something changed, this should note that copying from the browser side
might fail because the browser side's cache has limited resources, and
intentionally discards images of tabs that are not visible and unused for a
while.

[justinlin, 2013-03-18 18:10:01]

Thanks, I did the experiment of continuously nuking the backing store via
BackingStoreManager::RemoveAllBackingStores() (which looks like is what manages
the cache) and trying to snapshot, and you're right it will fail. Fixed that
issue by using the renderer side copy path if the backing store is not
available.

ptal, thanks

https://codereview.chromium.org/12881005/diff/31001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/12881005/diff/31001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.h:455: // browser side
might fail due to buggy drivers or other issues.
I mentioned when the backing store is not available, which is more or less the
same issue.

As far as I can tell (after looking through the platform specific
CopyFromBackingStore's), it can't actually fail in the copy unless there's some
Skia issue.

[Sam Kerner (Chrome), 2013-03-18 19:30:52]

https://codereview.chromium.org/12881005/diff/35001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/12881005/diff/35001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:589:
GetSnapshotFromRenderer(accelerated_copy_rect,

You are changing the semantics of this method from "Look in the backing store
cache (or GPU surface) for an image.  Return it on a hit" to "Get the image. 
Use the cache if possible, Otherwise, get it from the renderer."


If you do this, you need to look at all the users and update them.  Users will
have some dead code, because they handle cache misses that can not happen after
your change.  Your change does the cleanup for CaptureVisibleTab's
implementation.  The only other user I knew of was thumbnail generation for the
new tab page, but others may have been added over time.  

I would change the name of the method to something like "GetImageAsBitmap()", to
make it clear that you are doing something potentially expensive (IPC to the
renderer).

You can also choose to not change the semantics of the method, and restore the
old code in captureVisibleTab to ask the renderer for a snapshot on a backing
store cache miss.

[justinlin, 2013-03-18 21:07:59]

OK, how about: just add GetSnapshotFromRenderer() to the RenderWidgetHost public
interface? And, I'll undo all my inside CopyFromBackingStore.

jamesr@: Any objections?

We can't keep the old code because we want to move this functionality into
content/ to be used there, so SnapshotTabHelper probably needs to go. I thought
it might be simpler to have CopyFromBackingStore handle these failure cases
implicitly, but if it makes things too messy, we can just use it explicitly too.

[justinlin, 2013-03-18 21:40:34]

Made the changes, ptal.

[Sam Kerner (Chrome), 2013-03-18 22:08:32]

On 2013/03/18 21:40:34, justinlin wrote:
> Made the changes, ptal.

LGTM for extension code.  I looked at the rest and see no issues, but that code
is not an area I was an expert in.

[Sam Kerner (Chrome), 2013-03-18 22:08:46]

https://codereview.chromium.org/12881005/diff/46001/chrome/browser/extensions...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/12881005/diff/46001/chrome/browser/extensions...
chrome/browser/extensions/api/tabs/tabs_api.cc:1754: SendInternalError();
Indent should be two spaces.

[jamesr, 2013-03-18 22:41:18]

Is the code you are adding used for NTP thumbnails?  The resizing logic for
those is subtle and I want to make sure we preserve it, but I can't tell if it's
the same as this logic or not.

https://codereview.chromium.org/12881005/diff/41001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12881005/diff/41001/content/renderer/render_v...
content/renderer/render_view_impl.cc:1911: const WebSize& size = view->size();
I'm not sure about this - the size of a WebWidget is its logical size, not
physical pixel.  On a high DPI device these won't be the same.  I think you want
to be talking in terms of the device/physical pixels here and probably just want
to intersect with physical_backing_size_

https://codereview.chromium.org/12881005/diff/52001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/12881005/diff/52001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:635: bitmap,
skia::ImageOperations::RESIZE_GOOD, dst_size.width(),
why RESIZE_GOOD? what did the old code use?

[jamesr, 2013-03-18 22:44:01]

I'm thinking about chrome/browser/thumbnails/render_widget_snapshot_taker.cc

[justinlin, 2013-03-18 23:27:51]

No, as far as I could tell, the only thing that used the renderer-side snapshot
is the tabsAPI, and now the tabCapture API will use it too after this CL.

However, it might have similar issues that might require this, but that can be
fixed later if it's an actual issue (it might just time things right so that the
CopyFromBackingStore doesn't fail).

Thanks, ptal

https://codereview.chromium.org/12881005/diff/41001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12881005/diff/41001/content/renderer/render_v...
content/renderer/render_view_impl.cc:1911: const WebSize& size = view->size();
On 2013/03/18 22:41:18, jamesr wrote:
> I'm not sure about this - the size of a WebWidget is its logical size, not
> physical pixel.  On a high DPI device these won't be the same.  I think you
want
> to be talking in terms of the device/physical pixels here and probably just
want
> to intersect with physical_backing_size_

Done. Yea, this old stuff didn't really work with HiDPI. Fixed now.

https://codereview.chromium.org/12881005/diff/52001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/12881005/diff/52001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:635: bitmap,
skia::ImageOperations::RESIZE_GOOD, dst_size.width(),
On 2013/03/18 22:41:18, jamesr wrote:
> why RESIZE_GOOD? what did the old code use?

The old code didn't really do the resize (CopyFromBackingStore in general only
does the resize in the accelerated path). I think it makes sense to just remove
it from this method. It's kind of janky to just use some arbitrary resize here
anyway; better let the caller figure it out.

[jamesr, 2013-03-18 23:57:22]

On 2013/03/18 23:27:51, justinlin wrote:
> No, as far as I could tell, the only thing that used the renderer-side
snapshot
> is the tabsAPI, and now the tabCapture API will use it too after this CL.
> 
> However, it might have similar issues that might require this, but that can be
> fixed later if it's an actual issue (it might just time things right so that
the
> CopyFromBackingStore doesn't fail).
> 
> Thanks, ptal
> 

Well, then how do the NTP thumbnails work?  Is there any way to share that?  It
seems we have a few ways to do the same thing and I'd like to understand how
they interact (or do not interact) and how their requirements are the same or
different.  Having many similar mechanisms for the same goal is very confusing.

[justinlin, 2013-03-19 09:42:12]

(Note: just realized the method I added is not in the right place: it should be
in RVH, not RWH).

It looks to me like the RenderWidgetSnapshotTaker is pretty much the same on the
browser side as this new "GetSnapshotFromRenderer" (it just has a lot more
boilerplate, which I can think be removed now), but it's different when it gets
into the renderer. RenderWidgetSnapshotTaker uses a WebWidget to paint into the
SkBitmap instead of a WebView. I'm not entirely sure what the ramifications of
this are, but it caused a lot of crashes when I tried to use the WebWidget to
paint in this CL instead (testing with tabCapture), and it also seemed slower.
Do you know why this would be different? I thought it would be more or less the
same, but the code with the WebWidget version is much more complex. So, at first
glance, it doesn't seem like these two are compatible, but I do think
RenderWidgetSnapshotTaker could be simplified.

[justinlin, 2013-03-19 19:58:45]

So, in other words: the chrome.tabs.getCapturedTabs API and the tabCapture API
will now share pretty much the same mechanism for an expensive fallback for when
CopyFromBackingStore fails.

The NTP (and some of the testing framework) does something similar, but I'm not
convinced that it can be consolidated because it operates on the WebWidget
instead of the WebView. It looks like it would take quite a few changes to try
to consolidate, if it even makes sense; something probably better left for a
separate CL?

[jamesr, 2013-03-19 20:01:13]

On 2013/03/19 19:58:45, justinlin wrote:
> So, in other words: the chrome.tabs.getCapturedTabs API and the tabCapture API
> will now share pretty much the same mechanism for an expensive fallback for
when
> CopyFromBackingStore fails.
> 
> The NTP (and some of the testing framework) does something similar, but I'm
not
> convinced that it can be consolidated because it operates on the WebWidget
> instead of the WebView. It looks like it would take quite a few changes to try
> to consolidate, if it even makes sense; something probably better left for a
> separate CL?

I'm not sure I understand what you mean by WebWidget vs WebView.  Both are
interfaces.  WebView is a subclass of WebWidget, so all implementations of
WebView are also implementations of WebWidget, but not all WebWidgets are
WebViews.  Are you saying that the difference is that one of these mechanisms
works for WebViews that are not WebWidgets?  Why?

For a given object that is both a WebView and a WebWidget it doesn't make any
difference which interface the paint call is made on since there's only one
implementation (in WebViewImpl).

[justinlin, 2013-03-19 20:09:41]

On 2013/03/19 20:01:13, jamesr wrote:
> On 2013/03/19 19:58:45, justinlin wrote:
> > So, in other words: the chrome.tabs.getCapturedTabs API and the tabCapture
API
> > will now share pretty much the same mechanism for an expensive fallback for
> when
> > CopyFromBackingStore fails.
> > 
> > The NTP (and some of the testing framework) does something similar, but I'm
> not
> > convinced that it can be consolidated because it operates on the WebWidget
> > instead of the WebView. It looks like it would take quite a few changes to
try
> > to consolidate, if it even makes sense; something probably better left for a
> > separate CL?
> 
> I'm not sure I understand what you mean by WebWidget vs WebView.  Both are
> interfaces.  WebView is a subclass of WebWidget, so all implementations of
> WebView are also implementations of WebWidget, but not all WebWidgets are
> WebViews.  Are you saying that the difference is that one of these mechanisms
> works for WebViews that are not WebWidgets?  Why?
> 
> For a given object that is both a WebView and a WebWidget it doesn't make any
> difference which interface the paint call is made on since there's only one
> implementation (in WebViewImpl).

Yes, I agree that it should be compatible, but the painting method in
render_widget.cc is a lot more complex:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

I'm not really sure I understand all the paths in there.

Also, one difference is it scales the layout, which is not something we want.

[jamesr, 2013-03-19 20:14:47]

On 2013/03/19 20:09:41, justinlin wrote:
> On 2013/03/19 20:01:13, jamesr wrote:
> > On 2013/03/19 19:58:45, justinlin wrote:
> > > So, in other words: the chrome.tabs.getCapturedTabs API and the tabCapture
> API
> > > will now share pretty much the same mechanism for an expensive fallback
for
> > when
> > > CopyFromBackingStore fails.
> > > 
> > > The NTP (and some of the testing framework) does something similar, but
I'm
> > not
> > > convinced that it can be consolidated because it operates on the WebWidget
> > > instead of the WebView. It looks like it would take quite a few changes to
> try
> > > to consolidate, if it even makes sense; something probably better left for
a
> > > separate CL?
> > 
> > I'm not sure I understand what you mean by WebWidget vs WebView.  Both are
> > interfaces.  WebView is a subclass of WebWidget, so all implementations of
> > WebView are also implementations of WebWidget, but not all WebWidgets are
> > WebViews.  Are you saying that the difference is that one of these
mechanisms
> > works for WebViews that are not WebWidgets?  Why?
> > 
> > For a given object that is both a WebView and a WebWidget it doesn't make
any
> > difference which interface the paint call is made on since there's only one
> > implementation (in WebViewImpl).
> 
> Yes, I agree that it should be compatible, but the painting method in
> render_widget.cc is a lot more complex:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> 
> I'm not really sure I understand all the paths in there.
> 
> Also, one difference is it scales the layout, which is not something we want.

Ah, so you're talking about content::RenderWidget vs content::RenderView?  It
doesn't make a lot of sense to me to have different snapshot grabbing mechanisms
for views vs widgets.  RW::PaintRect is used for production rendering, so it may
do some things differently.

[justinlin, 2013-03-19 20:36:48]

On 2013/03/19 20:14:47, jamesr wrote:
> On 2013/03/19 20:09:41, justinlin wrote:
> > On 2013/03/19 20:01:13, jamesr wrote:
> > > On 2013/03/19 19:58:45, justinlin wrote:
> > > > So, in other words: the chrome.tabs.getCapturedTabs API and the
tabCapture
> > API
> > > > will now share pretty much the same mechanism for an expensive fallback
> for
> > > when
> > > > CopyFromBackingStore fails.
> > > > 
> > > > The NTP (and some of the testing framework) does something similar, but
> I'm
> > > not
> > > > convinced that it can be consolidated because it operates on the
WebWidget
> > > > instead of the WebView. It looks like it would take quite a few changes
to
> > try
> > > > to consolidate, if it even makes sense; something probably better left
for
> a
> > > > separate CL?
> > > 
> > > I'm not sure I understand what you mean by WebWidget vs WebView.  Both are
> > > interfaces.  WebView is a subclass of WebWidget, so all implementations of
> > > WebView are also implementations of WebWidget, but not all WebWidgets are
> > > WebViews.  Are you saying that the difference is that one of these
> mechanisms
> > > works for WebViews that are not WebWidgets?  Why?
> > > 
> > > For a given object that is both a WebView and a WebWidget it doesn't make
> any
> > > difference which interface the paint call is made on since there's only
one
> > > implementation (in WebViewImpl).
> > 
> > Yes, I agree that it should be compatible, but the painting method in
> > render_widget.cc is a lot more complex:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> > 
> > I'm not really sure I understand all the paths in there.
> > 
> > Also, one difference is it scales the layout, which is not something we
want.
> 
> Ah, so you're talking about content::RenderWidget vs content::RenderView?  It
> doesn't make a lot of sense to me to have different snapshot grabbing
mechanisms
> for views vs widgets.  RW::PaintRect is used for production rendering, so it
may
> do some things differently.

Well, I think I agree with that; So, the current way to get a snapshot from the
browser side is CopyFromBackingStore.

For the edge cases (WinXP/backing store unavailable), we now have the views
based "GetSnapshotFromRenderer" method.

I would argue we could move the NTP to using that instead and have that be the
way to get a snapshot from the renderer, but leave tests using the
render_widget_snapshot_taker class since the RenderWidget version of the paint
is different in that it scales the window/relayouts and also has stuff like
"PaintDebugBorders".

On RenderWidget vs RenderView painting: Aren't most render widgets already
render views anyway (except stuff like flash or something)? I guess part of it
is I don't really understand the render_widget version, but I know and tested
that the views version works (and it was previously used).

Would that make sense? I could move the NTP in a separate CL; that's the only
thing in the browser that uses render_widget_snapshot_taker.

[justinlin, 2013-03-20 23:13:55]

So, I changed GetSnapshotFromRenderer to operate on WebWidgets exactly like
render_widget_snapshot_taker (I looked through the code and they're equivalent,
except widget works on flash widgets too), so they are now pretty much the same
except for 2 things:
- GetSnapshotFromRenderer uses SkBitmap across the IPC instead of TransportDIB.
This might have a performance penalty, but it looks like using TransportDIB
doesn't work for Linux/X11 (see TODO in render_widget_snapshot_taker.cc) which
we need, and it also makes things a lot more complex. I don't think the penalty
is significant for the 3 use cases that will use this right now.
- render_widget_snapshot_taker has a feature to scale the WebWidget before
snapshotting. That doesn't seem like something you'd typically want when asking
for a snapshot, and in the code, that parameter is only used for tests.

So, I moved the 3 cases (tabsAPI, tabCapture, NTP) to use the same mechanics
with the GetSnapshotFromRenderer method now. They don't use the scaling, so they
can be moved (they wouldn't want that kind of scaling anyway: they'd want a
bitmap scale, not a layout scale).

The next step is just to do get rid of render_widget_snapshot_taker (if the
possible performance penalty is OK) and remove PaintAtRect from
RenderWidgetHost. What's blocking is mostly verifying that it's OK to just use
SkBitmap serialization instead of shared memory for tests. Not sure how it will
impact tests.

I think it'd be better to do that separately since this CL already touches a lot
of stuff. ptal?

[justinlin, 2013-03-21 00:33:51]

+mazda: ptal @ render_widget_host_view_gtk.cc; this is better than the current
implementation because currently capturing background tabs doesn't work in
composited mode (you get black). It also doesn't have the problem with
overlapping windows.

[jamesr, 2013-03-22 00:05:33]

This looks really nice.  I'm only very familiar with the */renderer/* systems,
but the IPCs changes also look good to my non-OWNERS eyes.  I think you'll need
specific reviewers look at the rest (mazda@ for the thumbnailing stuff, jam@ for
the content APIs)

https://codereview.chromium.org/12881005/diff/76051/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/12881005/diff/76051/content/renderer/render_w...
content/renderer/render_widget.cc:1837: base::Bind(OnSnapshotFailed,
make_scoped_refptr(this), routing_id_));
This functional style is highly uncommon in chromium and IMO overly complicated.
 If you want to make sure that some code happens if some of the intermediate
operations fail, move the operations that may fail to a helper function that
returns a bool and then deal with the response in the caller.

[justinlin, 2013-03-22 00:43:39]

thanks! ptal

jam@: ptal for content/ interface changes
mazda@: ping for linux change

https://codereview.chromium.org/12881005/diff/76051/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/12881005/diff/76051/content/renderer/render_w...
content/renderer/render_widget.cc:1837: base::Bind(OnSnapshotFailed,
make_scoped_refptr(this), routing_id_));
On 2013/03/22 00:05:33, jamesr wrote:
> This functional style is highly uncommon in chromium and IMO overly
complicated.
>  If you want to make sure that some code happens if some of the intermediate
> operations fail, move the operations that may fail to a helper function that
> returns a bool and then deal with the response in the caller.

Done.

[mazda, 2013-03-22 00:51:29]

render_widget_host_view_gtk.cc lgtm

https://codereview.chromium.org/12881005/diff/6002/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_gtk.cc (left):

https://codereview.chromium.org/12881005/diff/6002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_gtk.cc:1048: // NOTE:
|output| is initialized with the size of |src_subrect|, and |dst_size|
Can you keep the part that says |dst_size| is ignored?

[justinlin, 2013-03-22 00:57:21]

https://codereview.chromium.org/12881005/diff/6002/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_gtk.cc (left):

https://codereview.chromium.org/12881005/diff/6002/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_gtk.cc:1048: // NOTE:
|output| is initialized with the size of |src_subrect|, and |dst_size|
On 2013/03/22 00:51:29, mazda wrote:
> Can you keep the part that says |dst_size| is ignored?

Done. Yes, sorry this code has changed a few times; at some point it did the
resize in software.

[Avi (use Gerrit), 2013-03-22 18:21:21]

I'm super excited to see consolidation and cleanup of the snapshotting code.
I've done no work on backing stores, but I'm willing to give my lgtm as a stamp
since you've got your experts happy.

Make sure to double-check with jam when he gets back, but thanks for this!

[justinlin, 2013-03-22 18:40:03]

thanks!

sky@: ptal for OWNERS approval for the chrome/ changes? it's mostly just
deletions except the snapshot code that Sam Kerner looked at.
cevans@: ping for IPC changes?

[sky, 2013-03-22 19:39:22]

LGTM

https://codereview.chromium.org/12881005/diff/103003/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.h (right):

https://codereview.chromium.org/12881005/diff/103003/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.h:198: void
GetSnapshotFromRendererComplete(bool succeeded,
Add descriptions for these methods.

[Cris Neckar, 2013-03-22 22:45:24]

IPC changes LGTM

filed https://code.google.com/p/chromium/issues/detail?id=223284 for a
prexisting bug but this CL seems to actually make it harder to hit so I say land
it.

[commit-bot: I haz the power, 2013-03-22 23:28:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/12881005/119001

[justinlin, 2013-03-23 01:50:27]

Committed patchset #17 manually as r189969 (presubmit successful).

[jam, 2013-04-22 19:59:52]

https://codereview.chromium.org/12881005/diff/119001/content/common/view_mess...
File content/common/view_messages.h (right):

https://codereview.chromium.org/12881005/diff/119001/content/common/view_mess...
content/common/view_messages.h:1393: IPC_MESSAGE_ROUTED2(ViewHostMsg_Snapshot,
nit: i just saw this. this second IPC is in the wrong place, see how the file
puts all the ViewMsg_* messages first, then all the ViewHostMsg_*