Summary: | [chromium] Add WebWidget API for accessing the current WebCompositor | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Robinson <jamesr> | ||||||||||||
Component: | New Bugs | Assignee: | 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
James Robinson
2011-09-30 16:06:32 PDT
Created attachment 109364 [details]
Patch
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... (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. Created attachment 109382 [details]
Patch
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. Created attachment 109385 [details]
fix silly bug in compositorForIdentifier()...maybe i need tests
Created attachment 109389 [details]
add basic test for compositorForIdentifier
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 Created attachment 109432 [details]
Patch for landing
Comment on attachment 109432 [details] Patch for landing Clearing flags on attachment: 109432 Committed r96481: <http://trac.webkit.org/changeset/96481> All reviewed patches have been landed. Closing bug. Committed r96529: <http://trac.webkit.org/changeset/96529> |