Bug 69181 – [chromium] Add WebWidget API for accessing the current WebCompositor

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Add WebWidget API for accessing the current WebCompositor

[James Robinson]

Created attachment 109364 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 109364 [details]
Patch

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

> Source/WebKit/chromium/public/WebWidget.h:176
> +    // The WebCompositor may only be accessed and is destroyed from the compositor thread.

nit: This almost sounds like it is saying that you need to call this WebWidget::compositor()
method on the compositor thread.  Instead, maybe tweak the text slightly like so:

  // The WebCompositor's methods may only be called on the compositor thread.

I'm concerned there may still be a race-condition at setup time.  Could the WebCompositor
get destroyed, on the compositor thread, before the consumer has a chance to set the
WebCompositorClient pointer on the WebCompositor?

Hmm...

[James Robinson]

(In reply to comment #2)
> (From update of attachment 109364 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109364&action=review
> 
> > Source/WebKit/chromium/public/WebWidget.h:176
> > +    // The WebCompositor may only be accessed and is destroyed from the compositor thread.
> 
> nit: This almost sounds like it is saying that you need to call this WebWidget::compositor()
> method on the compositor thread.  Instead, maybe tweak the text slightly like so:
> 
>   // The WebCompositor's methods may only be called on the compositor thread.
> 

sure, will do

> I'm concerned there may still be a race-condition at setup time.  Could the WebCompositor
> get destroyed, on the compositor thread, before the consumer has a chance to set the
> WebCompositorClient pointer on the WebCompositor?
> 
> Hmm...

The way things work today, the compositor can only be destroyed from a main-thread initiated action.  It can't get destroyed before (say) WebWidgetClient::didActivateAcceleratedCompositing() returns, but it could get destroyed as soon as the main thread's message loop is destroyed.  When the compositor is shut down, it posts a task to the compositor thread and spins the message loop until it hits the quit task. So it seems like the contract here is that in order to not be racy, the WebWidgetClient implementation has to post a task on the compositor thread to register its WebCompositorClient before it returns from WebWidgetClient::didActivateAcceleratedCompositing().  that's what I have coded up locally in render_widget.cc - i'm calling CompositorThread::AddCompositor() which posts a task.

[James Robinson]

Created attachment 109382 [details]
Patch

[James Robinson]

Adds the weak binding mechanism we talked about.  The intended use is that the WebWidgetClient implementation gets a didEnableAcceleratedCompositing() call with an ID, posts a task to the compositor thread with that ID, and then on the compositor thread looks up a WebCompositor* and registers its WebCompositorClient on that thread so it can get the willShutdown() call.

[James Robinson]

Created attachment 109385 [details]
fix silly bug in compositorForIdentifier()...maybe i need tests

[James Robinson]

Created attachment 109389 [details]
add basic test for compositorForIdentifier

[Darin Fisher (:fishd, Google)]

Comment on attachment 109389 [details]
add basic test for compositorForIdentifier

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

R=me w/ these suggested changes.

> Source/WebKit/chromium/public/WebCompositor.h:43
> +    WEBKIT_EXPORT static WebCompositor* compositorForIdentifier(int);

nit: at callsites, this will look like WebCompositor::compositorForIdentifier(x), but
since that says "compositor" twice, how about WebCompositor::fromIdentifier(x) instead?
kinda like String::fromUTF8, etc.

> Source/WebKit/chromium/public/WebWidget.h:44
> +class WebCompositor;

nit: can delete this now

> Source/WebKit/chromium/public/WebWidgetClient.h:61
> +    virtual void didEnableAcceleratedCompositing(int compositorIdentifier) { }

nit: I originally wanted to use the word "enable" here, but vangelis talked
me out of it, since we already use the term "enable" in the WebSettings API:
WebSettings::setAcceleratedCompositingEnabled(bool).  That's why we settled
on "activate", and then this term is used pretty consistently on the Chrome
side too IIRC.  it is OK to temporarily override a method name.

  didActivateAcceleratedCompositing(int compositorIdentifier) { }
  didDeactivateAcceleratedCompositing() { }

Hmm, actually... I wonder why we need to say "accelerated" in these method
names.  Everywhere else we pretty much leave off that prefix.

Could just go with this:

  didActivateCompositor(int compositorIdentifier) { }
  didDeactivateCompositor() { }

> Source/WebKit/chromium/src/WebCompositorImpl.cpp:73
> +

nit: extra new line

[James Robinson]

Created attachment 109432 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 109432 [details]
Patch for landing

Clearing flags on attachment: 109432

Committed r96481: <http://trac.webkit.org/changeset/96481>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

Committed r96529: <http://trac.webkit.org/changeset/96529>