Bug 109125 – [chromium] Clean up WebWidget / WebLayerTreeView interactions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Clean up WebWidget / WebLayerTreeView interactions

[James Robinson]

Created attachment 186975 [details]
Patch

[James Robinson]

This probably doesn't even compile by itself.  In conjunction with https://codereview.chromium.org/12226051, chrome itself seems to work fine but some WebKit-side tests don't.  The goals of this patch are in the chromium-side codereview's description.

[James Robinson]

Comment on attachment 186975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186975&action=review

> Source/WebKit/chromium/public/WebWidget.h:256
> +    // Instrumentation method that marks beginning of frame update that includes
> +    // things like animate()/layout()/paint().
> +    virtual void instrumentBeginFrame() { instrumentWillBeginFrame(); }
> +
> +    // instrumentWillBeginFrame will be followed by either
> +    // instrumentDidBeginFrame or instrumentDidCancelFrame.
> +    virtual void instrumentWillBeginFrame() { }
> +    virtual void instrumentDidCancelFrame() { }
> +    virtual void instrumentDidBeginFrame() { }
> +    virtual void instrumentWillComposite() { }

caseq/pfeldman:  I'm not super happy about these calls being on WebWidget, but I'm not sure exactly what to do.  The basic issue here is that the inspector instrumentation wants to know about steps in the compositing process, but other than these calls WebKit doesn't have any involvement with these steps.  InspectorInstrumentation doesn't appear to be exposed directly.  I could add add these calls to WebWidget as this patch does but it's fairly ugly.  I could also add another tear-off interface, or perhaps put these on WebDevToolsAgent which hangs off of WebView.  Logically these steps can apply to any WebWidget but I think WebViews are the only sorts of Widgets that the inspector can collect timeline data for.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Peter Beverloo (cr-android ews)]

Comment on attachment 186975 [details]
Patch

Attachment 186975 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16408029

[WebKit Review Bot]

Comment on attachment 186975 [details]
Patch

Attachment 186975 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16404050

[WebKit Review Bot]

Comment on attachment 186975 [details]
Patch

Attachment 186975 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16435152

[Andrey Kosyakov]

Comment on attachment 186975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186975&action=review

>> Source/WebKit/chromium/public/WebWidget.h:256
>> +    virtual void instrumentWillComposite() { }
> 
> caseq/pfeldman:  I'm not super happy about these calls being on WebWidget, but I'm not sure exactly what to do.  The basic issue here is that the inspector instrumentation wants to know about steps in the compositing process, but other than these calls WebKit doesn't have any involvement with these steps.  InspectorInstrumentation doesn't appear to be exposed directly.  I could add add these calls to WebWidget as this patch does but it's fairly ugly.  I could also add another tear-off interface, or perhaps put these on WebDevToolsAgent which hangs off of WebView.  Logically these steps can apply to any WebWidget but I think WebViews are the only sorts of Widgets that the inspector can collect timeline data for.

We discussed it with Pavel and the consensus was that for the time being the best place for the methods would be on WebDevToolsAgent -- I can help with that if you like. Another idea that Nat proposed would be to use our emerging trace event based instrumentation, which would allow to cut down on plumbing quite a bit, but we thought it would perhaps be too early for it.

[James Robinson]

Created attachment 189620 [details]
Patch

[James Robinson]

This patch passes layout and unit tests, but doesn't work entirely correctly (i.e. devtools instrumentation does not work as expected) without some chromium-side changes.

[James Robinson]

Created attachment 190850 [details]
Patch

[James Robinson]

It's alive!  This patch works.  What it does is move the rest of the compositor-interaction responsibilies to WebWidget / WebWidgetClient and leaves WebLayerTreeViewClient as just a shell.  Once this lands and the chromium side that moves calls over to the new interfaces, WebLayerTreeViewClient will go away completely.  WebViewImpl (or other WebWidget implementations) will use WebWidgetClient for widget-specific compositor communication and use WebLayerTreeView to talk directly to the compositor implementation for things that aren't widget-specific.

[James Robinson]

Created attachment 190852 [details]
more fixups to ScrollingCoordinatorChromiumTest

[James Robinson]

Oh - this also depends on https://codereview.chromium.org/12377029/ landing + rolling in to Source/WebKit/chromium/DEPS

[WebKit Review Bot]

Comment on attachment 190852 [details]
more fixups to ScrollingCoordinatorChromiumTest

Attachment 190852 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16822361

[WebKit Review Bot]

Comment on attachment 190852 [details]
more fixups to ScrollingCoordinatorChromiumTest

Attachment 190852 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16863126

[Peter Beverloo (cr-android ews)]

Comment on attachment 190852 [details]
more fixups to ScrollingCoordinatorChromiumTest

Attachment 190852 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/16778589

[James Robinson]

Comment on attachment 190852 [details]
more fixups to ScrollingCoordinatorChromiumTest

View in context: https://bugs.webkit.org/attachment.cgi?id=190852&action=review

> Source/Platform/chromium/public/WebLayerTreeViewClient.h:-76
> -    virtual void scheduleComposite() = 0;

I'll have to muck about with content shell a bit before deleting this virtual.

[James Robinson]

Created attachment 191089 [details]
fix WebTestProxy compilation

[James Robinson]

This fixes the WebTestProxy compile

[WebKit Review Bot]

Comment on attachment 191089 [details]
fix WebTestProxy compilation

Attachment 191089 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16885043

[WebKit Review Bot]

Comment on attachment 191089 [details]
fix WebTestProxy compilation

Attachment 191089 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16668335

[James Robinson]

Created attachment 191315 [details]
rebased

[James Robinson]

This should build and work fine against current chromium.  It deprecates the WebLayerTreeViewClient implementations in WebKit, but leaves them in place so the callers can be removed on the chromium side.  Mind having a look, Enne?

[Adrienne Walker]

Comment on attachment 191315 [details]
rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=191315&action=review

R=me.

> Source/WebKit/chromium/src/WebViewImpl.h:156
> +    virtual WebInputHandler* createInputHandler() OVERRIDE;
> +    virtual void applyScrollAndScale(const WebSize&, float);

Just a drive-by comment, but what's with the inconsistent use of OVERRIDE in this block if these are all WebWidget methods?

[James Robinson]

Comment on attachment 191315 [details]
rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=191315&action=review

>> Source/WebKit/chromium/src/WebViewImpl.h:156
>> +    virtual void applyScrollAndScale(const WebSize&, float);
> 
> Just a drive-by comment, but what's with the inconsistent use of OVERRIDE in this block if these are all WebWidget methods?

No reason. I'll add OVERRIDES to the rest of this file as a style follow-up

[WebKit Review Bot]

Comment on attachment 191315 [details]
rebased

Attachment 191315 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16986028

New failing tests:
inspector/debugger/pause-in-internal-script.html

[WebKit Review Bot]

Comment on attachment 191315 [details]
rebased

Clearing flags on attachment: 191315

Committed r144784: <http://trac.webkit.org/changeset/144784>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.