Summary: | [Qt] Fix or remove the runtime flag for accelerated compositing. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jocelyn Turcotte <jturcotte> | ||||||||||||
Component: | WebKit Qt | Assignee: | Noam Rosenthal <noam> | ||||||||||||
Status: | CLOSED FIXED | ||||||||||||||
Severity: | Normal | CC: | benjamin, commit-queue, hausmann, kenneth, noam, simon.fraser, tonikitoo | ||||||||||||
Priority: | P2 | Keywords: | Qt | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 35784 | ||||||||||||||
Attachments: |
|
Description
Jocelyn Turcotte
2010-04-09 01:29:37 PDT
I think the runtime flag is necessary. It's a "browser-space" decision: e.g. WRT or the browser should decide whether or not to use AC. Also at least for now it's extremely useful for testing. We can put some cleverness that disables that option completely for QWebView if we're worried about people messing with it in that way. (In reply to comment #1) > I think the runtime flag is necessary. It's a "browser-space" decision: e.g. > WRT or the browser should decide whether or not to use AC. What are the use case to disable it at runtime? I think it might add unneeded complexity for developers, especially if it is uncommon to disable it. We should probably move this discussions outside of this bug report :) (In reply to comment #2) > (In reply to comment #1) > > I think the runtime flag is necessary. It's a "browser-space" decision: e.g. > > WRT or the browser should decide whether or not to use AC. > > What are the use case to disable it at runtime? > I think it might add unneeded complexity for developers, especially if it is > uncommon to disable it. > > We should probably move this discussions outside of this bug report :) It is not essential at runtime, but a per app basis, thus we need a setting for it. Enabling/disabling it at runtime is mostly useful for debugging. Created attachment 53719 [details]
Added a way for the chrome-client to disable compositing even if it's enabled in settings
Since this adds some API to ChromeClient, and additional code in the path to decide which layers to composite, it will need some more eyes...
Created attachment 53726 [details]
changed some names, and moved the chrome-client check to the settings-cache phase. it still works :)
Comment on attachment 53726 [details]
changed some names, and moved the chrome-client check to the settings-cache phase. it still works :)
We already have Settings::acceleratedCompositingEnabled(), and a build flag. Do we need yet another way to control this?
Also, the way we did this on Mac was to override the Settings value: http://trac.webkit.org/changeset/55100 Comment on attachment 53726 [details] changed some names, and moved the chrome-client check to the settings-cache phase. it still works :) > diff --git a/WebCore/page/ChromeClient.h b/WebCore/page/ChromeClient.h > index b710c91..d47134d 100644 > --- a/WebCore/page/ChromeClient.h > +++ b/WebCore/page/ChromeClient.h > @@ -214,6 +214,9 @@ namespace WebCore { > // Sets a flag to specify that the view needs to be updated, so we need > // to do an eager layout before the drawing. > virtual void scheduleCompositingLayerSync() = 0; > + // return whether or not the client can render the composited layer, > + // regardless of the settings > + virtual bool canRenderCompositedLayers() { return true; } Rename to allowsAcceleratedCompositing(), and make it |const|. > diff --git a/WebCore/rendering/RenderLayerCompositor.cpp b/WebCore/rendering/RenderLayerCompositor.cpp > index 9430613..2d707b3 100644 > --- a/WebCore/rendering/RenderLayerCompositor.cpp > +++ b/WebCore/rendering/RenderLayerCompositor.cpp > @@ -131,6 +131,15 @@ void RenderLayerCompositor::cacheAcceleratedCompositingFlags() > showRepaintCounter = settings->showRepaintCounter(); > } > > + // we allow the chrome to override the settings, in case the page is rendered > + // on a chrome that doesn't allow accelerated compositing Sentence case, and terminating period please. r=me with those changes. Comment on attachment 53726 [details]
changed some names, and moved the chrome-client check to the settings-cache phase. it still works :)
even though the settings approach might work for the current use case - in the future we might better granularity for the chrome to decide whether or not to allow AC (e.g. allow it only on a GL viewport)
Created attachment 53745 [details]
name changes, better punctuation grammar :)
applied feedback from Simon Fraser; re-uploading
Comment on attachment 53745 [details] name changes, better punctuation grammar :) > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > index 36db5d8..d796d7b 100644 > --- a/WebCore/ChangeLog > +++ b/WebCore/ChangeLog > @@ -1,3 +1,29 @@ > +2010-04-19 No'am Rosenthal <noam.rosenthal@nokia.com> > + > + Reviewed by NOBODY (OOPS!). > + > + [Qt] Fix or remove the runtime flag for accelerated compositing. > + > + This adds a way for a chrome client to disallow layers from becoming composited, > + even if the settings enable AC. Please expand AC. You're the only people who use that contraction :) > diff --git a/WebCore/page/ChromeClient.h b/WebCore/page/ChromeClient.h > @@ -214,6 +214,9 @@ namespace WebCore { > // Sets a flag to specify that the view needs to be updated, so we need > // to do an eager layout before the drawing. > virtual void scheduleCompositingLayerSync() = 0; > + // Returns whether or not the client can render the composited layer, > + // regardless of the settings. > + virtual bool allowsAcceleratedCompositing() { return true; } Method should be |const| > diff --git a/WebCore/platform/qt/QWebPageClient.h b/WebCore/platform/qt/QWebPageClient.h > index f03ff08..491a9f3 100644 > --- a/WebCore/platform/qt/QWebPageClient.h > +++ b/WebCore/platform/qt/QWebPageClient.h > @@ -55,6 +55,7 @@ public: > // if scheduleSync is true, we schedule a sync ourselves. otherwise, > // we wait for the next update and sync the layers then. > virtual void markForSync(bool scheduleSync = false) {} > + virtual bool allowsAcceleratedCompositing() { return false; } Fix this to match the constness in the API. > diff --git a/WebCore/rendering/RenderLayerCompositor.cpp b/WebCore/rendering/RenderLayerCompositor.cpp > index 9430613..6f856b0 100644 > --- a/WebCore/rendering/RenderLayerCompositor.cpp > +++ b/WebCore/rendering/RenderLayerCompositor.cpp > @@ -131,6 +131,15 @@ void RenderLayerCompositor::cacheAcceleratedCompositingFlags() > showRepaintCounter = settings->showRepaintCounter(); > } > > + // We allow the chrome to override the settings, in case the page is rendered > + // on a chrome that doesn't allow accelerated compositing. I think a comment like: // Allow the ChromeClient to veto use of accelerated compositing. would be clearer. Created attachment 53746 [details]
AC -> accelerated compositing
de-acronymed
Comment on attachment 53746 [details]
AC -> accelerated compositing
Not all my suggestions were responded to.
Created attachment 53749 [details]
take 4: /me was being sloppy with const
should hopefully be ok now - I triple checked it for style, consts etc.
Comment on attachment 53749 [details] take 4: /me was being sloppy with const Clearing flags on attachment: 53749 Committed r57961: <http://trac.webkit.org/changeset/57961> All reviewed patches have been landed. Closing bug. Revision r57961 cherry-picked into qtwebkit-2.0 with commit 003263d41f7ac488a40904fdc1b8ee91261853d5 |