Expose the capturePicture feature in RenderView for Android WebView legacy API support.

Expose the capturePicture feature in RenderView for Android WebView legacy API support.

These methods are required to implement WebView.capturePicture and WebView.PictureListener.onNewPicture.
- http://developer.android.com/reference/android/webkit/WebView.html#capturePicture()
- http://developer.android.com/reference/android/webkit/WebView.PictureListener.html

BUG=167908, 167913
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182106

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

[Leandro Graciá Gil, 2013-01-11 13:11:33]

joi: changes in content/public to expose the API.
jamesr: implementation of these methods in content/renderer and
webkit/compositor_bindings
joth: changes to android_webview/ code and, if you want, a look on the exposed
APIs.

This patch depends on 11575049. Cannot use the trybots until it lands.

Thanks!

https://codereview.chromium.org/11861008/diff/1/content/public/renderer/rende...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/11861008/diff/1/content/public/renderer/rende...
content/public/renderer/render_view.h:175: virtual void
SetCapturePictureCallback(
Some extra context here. We need to support both the renderer calling us back on
new frame contents (WebView.PictureListener.onNewPicture) and we explicitly
requesting the current contents (WebView.capturePicture). The last is a
synchronous API which will require a synchronous IPC in the Android WebView
code. I suggest providing the picture also in the callback case (not just
notifying about a new one being available) in order to reduce the number of such
synchronous IPC calls.

Getting the SkPicture is a non-trivial task with a very good chance of having a
performance impact. We need to do this for legacy WebView API support, but this
approach should not be used by anyone else as it implies (or will imply in the
future) playing back the drawing commands of every single layer in the full
frame. Because of this I'm proposing these methods to be Android-only and
guarded by a command line flag. I'm open to any other approaches.

https://codereview.chromium.org/11861008/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11861008/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:6590: return web_layer_tree_view_ &&
CommandLine::ForCurrentProcess()->HasSwitch(
This patch depends on https://codereview.chromium.org/11575049/ landing
successfully and introducing web_layer_tree_view_ in RenderWidget.

[Jói, 2013-01-11 18:40:08]

This LGTM with nits below, but let's loop in jam@ as it is an interface
addition.

Cheers,
Jói

https://codereview.chromium.org/11861008/diff/1/content/public/renderer/rende...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/11861008/diff/1/content/public/renderer/rende...
content/public/renderer/render_view.h:171: // Sets a callback to receive a new
SkPicture with the contents of every new
Can there be exactly 0 or 1 callback, or can there be multiple?

Also, would like to see more of the documentation from your comment added to the
documentation that goes into the file here, i.e. why this is Android-only and
why it's guarded by a command-line flag.

https://codereview.chromium.org/11861008/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/11861008/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.h:738: OVERRIDE;
nit: Suggest wrapping at the ( so that OVERRIDE isn't lonely.

[joth, 2013-01-11 21:41:17]

Fine with me as it is, if consensus is to do it this way, but IMO we can pull
more of this grot up out of content/ layer if preferred... (and avoid the
command line flag back-dooring).

https://codereview.chromium.org/11861008/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11861008/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:6596: void
RenderViewImpl::didBecomeReadyForAdditionalInput() {
I propose we should separate out the two different primitives here, and extract
upwards a lot of the policy decisions into the embedder and out of content/ as a
result:

1/ Here, we just push  a new notification to RV observers:-
FOR_EACH_OBSERVER(RenderViewObserver, observers_,
DidBecomeReadyForAdditionalInput();
i.e. this can be done unconditionally, regardless of OS_ANDROID or any funky
command line switch.

2/ We add the RenderViewImpl::CapturePicture() as you have it above, but as it's
not (of itself) spontaneous doing anything at all I'd argue a command line
switch is not needed. Just a warning on the function that it's not necessarily
"free" to call, and perhaps we also keep it under OS_ANDROID.

3/ The RenderViewImpl::SetCapturePictureCallback can be removed entirely. We
just implement that logic (receiving DidBecomeReadyForAdditionalInput
notifications and calling CapturePicture() if appropriate) up in the
anroid_webview layer.

[jamesr, 2013-01-11 21:53:25]

https://codereview.chromium.org/11861008/diff/1/content/public/renderer/rende...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/11861008/diff/1/content/public/renderer/rende...
content/public/renderer/render_view.h:15: #include
"third_party/skia/include/core/SkPicture.h"
please don't include this header just to have references to
skia::RefPtr<SkPictures> - a forward declaration should suffice

https://codereview.chromium.org/11861008/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11861008/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:6596: void
RenderViewImpl::didBecomeReadyForAdditionalInput() {
why are you hooking on didBecomeReadyForAdditionalInput()? this operation
doesn't have anything to do with input event throttling

https://codereview.chromium.org/11861008/diff/1/webkit/compositor_bindings/we...
File webkit/compositor_bindings/web_layer_tree_view_impl.h (right):

https://codereview.chromium.org/11861008/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_layer_tree_view_impl.h:12: #include
"third_party/skia/include/core/SkPicture.h"
don't include this header, forward declare SkPicture

https://codereview.chromium.org/11861008/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_layer_tree_view_impl.h:35:
WEBKIT_COMPOSITOR_BINDINGS_EXPORT skia::RefPtr<SkPicture> capturePicture();
why are you exposing this on WLTVI instead of just calling the cc::LayerTreeHost
function from your content code?

[Leandro Graciá Gil, 2013-01-12 00:26:08]

https://codereview.chromium.org/11861008/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11861008/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:6596: void
RenderViewImpl::didBecomeReadyForAdditionalInput() {
On 2013/01/11 21:53:26, jamesr wrote:
> why are you hooking on didBecomeReadyForAdditionalInput()? this operation
> doesn't have anything to do with input event throttling

The intention is to trigger the callback on every new frame contents. This is
supposed to be called on didCommit by the WebKit side, isn't it? That's what
enne and nduca suggested us (if I interpreted it right).

Following Joth's suggestion, is there anything that we could use already exposed
to RenderViewObservers?

https://codereview.chromium.org/11861008/diff/1/webkit/compositor_bindings/we...
File webkit/compositor_bindings/web_layer_tree_view_impl.h (right):

https://codereview.chromium.org/11861008/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_layer_tree_view_impl.h:35:
WEBKIT_COMPOSITOR_BINDINGS_EXPORT skia::RefPtr<SkPicture> capturePicture();
On 2013/01/11 21:53:26, jamesr wrote:
> why are you exposing this on WLTVI instead of just calling the
cc::LayerTreeHost
> function from your content code?

How else can we access cc::LayerTreeHost directly from RenderView? If there is
an alternative way I'm happy to try it.

[Leandro Graciá Gil, 2013-01-15 20:46:11]

https://codereview.chromium.org/11861008/diff/1/content/public/renderer/rende...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/11861008/diff/1/content/public/renderer/rende...
content/public/renderer/render_view.h:15: #include
"third_party/skia/include/core/SkPicture.h"
On 2013/01/11 21:53:26, jamesr wrote:
> please don't include this header just to have references to
> skia::RefPtr<SkPictures> - a forward declaration should suffice

Done.

https://codereview.chromium.org/11861008/diff/1/content/public/renderer/rende...
content/public/renderer/render_view.h:171: // Sets a callback to receive a new
SkPicture with the contents of every new
On 2013/01/11 18:40:08, Jói wrote:
> Can there be exactly 0 or 1 callback, or can there be multiple?
> 
> Also, would like to see more of the documentation from your comment added to
the
> documentation that goes into the file here, i.e. why this is Android-only and
> why it's guarded by a command-line flag.

Removing and implementing via RenderViewObserver.

https://codereview.chromium.org/11861008/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/11861008/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.h:738: OVERRIDE;
On 2013/01/11 18:40:08, Jói wrote:
> nit: Suggest wrapping at the ( so that OVERRIDE isn't lonely.

Removing method from RenderView.

https://codereview.chromium.org/11861008/diff/1/webkit/compositor_bindings/we...
File webkit/compositor_bindings/web_layer_tree_view_impl.h (right):

https://codereview.chromium.org/11861008/diff/1/webkit/compositor_bindings/we...
webkit/compositor_bindings/web_layer_tree_view_impl.h:12: #include
"third_party/skia/include/core/SkPicture.h"
On 2013/01/11 21:53:26, jamesr wrote:
> don't include this header, forward declare SkPicture

Done.

[jamesr, 2013-01-16 04:08:09]

Input throttling and picture capturing are completely different things and
really aren't related at all.  You're just (ab)using one callback because
currently they're wired up to the same thing, but that's extremely fragile.  You
should make any new notification/observer/whatever be named in terms of the
timing that you want and make that explicit.

[Leandro Graciá Gil, 2013-01-16 04:44:41]

On 2013/01/16 04:08:09, jamesr wrote:
> Input throttling and picture capturing are completely different things and
> really aren't related at all.  You're just (ab)using one callback because
> currently they're wired up to the same thing, but that's extremely fragile. 
You
> should make any new notification/observer/whatever be named in terms of the
> timing that you want and make that explicit.

Sounds good to me, but the question is which notification should we use / expose
to detect changes in the frame content? Looks like following didCommit is not a
good idea, at least not with the current name.

[Leandro Graciá Gil, 2013-01-18 22:33:47]

Using didCommit explicitly. The patch now depends on this WebKit patch too:

https://bugs.webkit.org/show_bug.cgi?id=107325

[jamesr, 2013-01-18 22:36:12]

On 2013/01/18 22:33:47, Leandro Graciá Gil wrote:
> Using didCommit explicitly. The patch now depends on this WebKit patch too:
> 
> https://bugs.webkit.org/show_bug.cgi?id=107325

I think the result of our discussion was that didCommit doesn't match what
legacy WebView did very well - for instance, it'll happen on scrolls even if
nothing is invalidated.  Shouldn't we figure out what timing we really want
first?

[Leandro Graciá Gil, 2013-01-18 22:50:28]

On 2013/01/18 22:36:12, jamesr wrote:
> On 2013/01/18 22:33:47, Leandro Graciá Gil wrote:
> > Using didCommit explicitly. The patch now depends on this WebKit patch too:
> > 
> > https://bugs.webkit.org/show_bug.cgi?id=107325
> 
> I think the result of our discussion was that didCommit doesn't match what
> legacy WebView did very well - for instance, it'll happen on scrolls even if
> nothing is invalidated.  Shouldn't we figure out what timing we really want
> first?

We discussed this more and concluded we should use didCommit for correctness in
fixed position elements and for scroll invalidation in our temporary
CapturePicture-based SW rendering code (we need that so we can start testing
until we have a proper SW renderer for WebView in place). We'll explore not
triggering PictureListener callbacks for scroll-only changes in the future.

[Leandro Graciá Gil, 2013-02-06 11:44:58]

joth & jamesr: ping.  Once CL 11575049 lands this should be rebased and ready to
go. Any further changes for this patch?

[Leandro Graciá Gil, 2013-02-06 14:37:00]

This patch is now based on the RenderWidgetCompositor object introduced by CL
12210005 to access LayerTreeHost. Now this patch should enable software
rendering in WebView.

[joth, 2013-02-06 18:08:25]

lgtm

[jamesr, 2013-02-12 20:00:35]

https://codereview.chromium.org/11861008/diff/22007/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/11861008/diff/22007/content/renderer/render_v...
content/renderer/render_view_impl.h:814: virtual void didCommitCompositorFrame()
OVERRIDE;
this is in a comment block indicating that it's a RenderWidget override, but I
don't see this function in RenderWidget.  Did you intend to put this in the
WebWidgetClient overrides section?  If so drop the OVERRIDE annotation - we
don't use that when implementing WebKit API defined in the WebKit repo.

Even better would be to implement a function on RenderWidget and call that
explicitly from RenderWidgetCompositor instead of bouncing through WebKit API

[Leandro Graciá Gil, 2013-02-12 21:08:56]

https://codereview.chromium.org/11861008/diff/22007/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/11861008/diff/22007/content/renderer/render_v...
content/renderer/render_view_impl.h:814: virtual void didCommitCompositorFrame()
OVERRIDE;
On 2013/02/12 20:00:35, jamesr wrote:
> this is in a comment block indicating that it's a RenderWidget override, but I
> don't see this function in RenderWidget.  Did you intend to put this in the
> WebWidgetClient overrides section?  If so drop the OVERRIDE annotation - we
> don't use that when implementing WebKit API defined in the WebKit repo.
> 
> Even better would be to implement a function on RenderWidget and call that
> explicitly from RenderWidgetCompositor instead of bouncing through WebKit API

Done.

[jamesr, 2013-02-12 21:23:14]

No, I'm suggesting that you not use the WebKit API at all.  Implement a new
function on RenderWidget with chromium style naming, call that from
RenderWidgetCompositor, and override it in RenderViewImpl

[Leandro Graciá Gil, 2013-02-12 23:00:41]

On 2013/02/12 21:23:14, jamesr wrote:
> No, I'm suggesting that you not use the WebKit API at all.  Implement a new
> function on RenderWidget with chromium style naming, call that from
> RenderWidgetCompositor, and override it in RenderViewImpl

Got it. I think it should be fine now.

Thanks for taking a look to this!

[jamesr, 2013-02-12 23:01:36]

lgtm

I think we'll definitely have to iterate on the timing of this, but this is a
decent place to start

[Leandro Graciá Gil, 2013-02-12 23:15:34]

On 2013/02/12 23:01:36, jamesr wrote:
> lgtm
> 
> I think we'll definitely have to iterate on the timing of this, but this is a
> decent place to start

Yes, definitely. Thanks for taking a look.

[commit-bot: I haz the power, 2013-02-12 23:16:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11861008/44003

[commit-bot: I haz the power, 2013-02-13 03:31:49]

Change committed as 182106

[Leandro Graciá Gil, 2013-02-14 13:46:55]

jam: this patch was reverted due to some timeouts caused by synchronous IPC
issues. In this case SwapOut was preventing the reply messages to be sent back,
deadlocking the browser. I've implemented a fix for this that should only affect
WebView. Could you please take a look?

The affected files are:
- android_webview/common/aw_content_client.cc
- android_webview/common/aw_content_client.h
- content/common/swapped_out_messages.cc
- content/public/common/content_client.cc
- content/public/common/content_client.h

Thanks!

[mkosiba (inactive), 2013-02-14 14:03:16]

https://codereview.chromium.org/11861008/diff/28011/content/common/swapped_ou...
File content/common/swapped_out_messages.cc (left):

https://codereview.chromium.org/11861008/diff/28011/content/common/swapped_ou...
content/common/swapped_out_messages.cc:39: return false;
I think it might be fine to simply say that replies to sync messages are always
allowed rather than adding yet another method to the content/ API.

I don't think such a change could 'hurt' the rest of Chromium since, IIRC,
outside of WebView, the channel would disallow a sync message anyway, right?

[Leandro Graciá Gil, 2013-02-14 14:25:13]

https://codereview.chromium.org/11861008/diff/28011/content/common/swapped_ou...
File content/common/swapped_out_messages.cc (left):

https://codereview.chromium.org/11861008/diff/28011/content/common/swapped_ou...
content/common/swapped_out_messages.cc:39: return false;
On 2013/02/14 14:03:16, Martin Kosiba wrote:
> I think it might be fine to simply say that replies to sync messages are
always
> allowed rather than adding yet another method to the content/ API.
> 
> I don't think such a change could 'hurt' the rest of Chromium since, IIRC,
> outside of WebView, the channel would disallow a sync message anyway, right?

I think the question is if renderer -> browser synchronous IPC messages should
send back a reply to renderers being swapped out. If that's fine or we simply
don't care because other cases won't use a sync channel, then we probably want
to add IPC_REPLY_ID here rather than adding a new API.

Is there any other code in Chrome triggering browser -> renderer synchronous
IPCs? We do only for legacy WebView API compatibility.

[jamesr, 2013-02-14 18:35:15]

> Is there any other code in Chrome triggering browser -> renderer synchronous
> IPCs? We do only for legacy WebView API compatibility.

No, that won't work in chromium's IPC model since it leads to deadlocks.

[Leandro Graciá Gil, 2013-02-14 18:39:56]

On 2013/02/14 18:35:15, jamesr wrote:
> > Is there any other code in Chrome triggering browser -> renderer synchronous
> > IPCs? We do only for legacy WebView API compatibility.
> 
> No, that won't work in chromium's IPC model since it leads to deadlocks.

We are aware of that, and this was discussed with jam some time ago. We use it
in a few exceptional cases due to the requirements the legacy WebView API
enforces on us. For example, CapturePicture and the synchronous FindInPage
method.

Still, it works as long as there is no synchronous message from the renderer to
the browser blocking the UI thread at the same time.

[jam, 2013-02-14 21:14:53]

On 2013/02/14 13:46:55, Leandro Graciá Gil wrote:
> jam: this patch was reverted due to some timeouts caused by synchronous IPC
> issues. In this case SwapOut was preventing the reply messages to be sent
back,
> deadlocking the browser. I've implemented a fix for this that should only
affect
> WebView. Could you please take a look?
> 
> The affected files are:
> - android_webview/common/aw_content_client.cc
> - android_webview/common/aw_content_client.h
> - content/common/swapped_out_messages.cc
> - content/public/common/content_client.cc
> - content/public/common/content_client.h
> 
> Thanks!

lgtm

[Leandro Graciá Gil, 2013-02-14 21:16:31]

On 2013/02/14 21:14:53, jam wrote:
> On 2013/02/14 13:46:55, Leandro Graciá Gil wrote:
> > jam: this patch was reverted due to some timeouts caused by synchronous IPC
> > issues. In this case SwapOut was preventing the reply messages to be sent
> back,
> > deadlocking the browser. I've implemented a fix for this that should only
> affect
> > WebView. Could you please take a look?
> > 
> > The affected files are:
> > - android_webview/common/aw_content_client.cc
> > - android_webview/common/aw_content_client.h
> > - content/common/swapped_out_messages.cc
> > - content/public/common/content_client.cc
> > - content/public/common/content_client.h
> > 
> > Thanks!
> 
> lgtm

Thanks for taking a look so fast, jam!

Landing this again...

[commit-bot: I haz the power, 2013-02-14 21:19:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11861008/28011

[commit-bot: I haz the power, 2013-02-14 22:12:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/11861008/28011

[commit-bot: I haz the power, 2013-02-15 06:24:21]

Change committed as 182629