Bug 69181

Summary: [chromium] Add WebWidget API for accessing the current WebCompositor
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 69251    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
fix silly bug in compositorForIdentifier()...maybe i need tests
none
add basic test for compositorForIdentifier
none
Patch for landing none

Description James Robinson 2011-09-30 16:06:32 PDT
[chromium] Add WebWidget API for accessing the current WebCompositor
Comment 1 James Robinson 2011-09-30 16:08:07 PDT
Created attachment 109364 [details]
Patch
Comment 2 Darin Fisher (:fishd, Google) 2011-09-30 16:12:52 PDT
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...
Comment 3 James Robinson 2011-09-30 16:19:05 PDT
(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.
Comment 4 James Robinson 2011-09-30 17:35:31 PDT
Created attachment 109382 [details]
Patch
Comment 5 James Robinson 2011-09-30 17:36:28 PDT
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.
Comment 6 James Robinson 2011-09-30 17:44:21 PDT
Created attachment 109385 [details]
fix silly bug in compositorForIdentifier()...maybe i need tests
Comment 7 James Robinson 2011-09-30 18:27:54 PDT
Created attachment 109389 [details]
add basic test for compositorForIdentifier
Comment 8 Darin Fisher (:fishd, Google) 2011-09-30 21:29:54 PDT
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
Comment 9 James Robinson 2011-10-02 17:05:48 PDT
Created attachment 109432 [details]
Patch for landing
Comment 10 WebKit Review Bot 2011-10-02 18:10:52 PDT
Comment on attachment 109432 [details]
Patch for landing

Clearing flags on attachment: 109432

Committed r96481: <http://trac.webkit.org/changeset/96481>
Comment 11 WebKit Review Bot 2011-10-02 18:10:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 James Robinson 2011-10-03 12:51:17 PDT
Committed r96529: <http://trac.webkit.org/changeset/96529>