Bug 37313

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:

Keywords:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

35784

Attachments:

Description	Flags
Added a way for the chrome-client to disable compositing even if it's enabled in settings	none
changed some names, and moved the chrome-client check to the settings-cache phase. it still works :)	none
name changes, better punctuation grammar :)	simon.fraser: review+, simon.fraser: commit-queue-
AC -> accelerated compositing	simon.fraser: review-, simon.fraser: commit-queue-
take 4: /me was being sloppy with const	none

Jocelyn Turcotte

Reported 2010-04-09 01:29:37 PDT

Right now there is two ways to enable/disable accelerated compositing for the QGraphicsWebView: - WTF_USE_ACCELERATED_COMPOSITING at compile time - QWebSettings::AcceleratedCompositingEnabled at runtime Since this needs Settings::setAcceleratedCompositingEnabled(true) to work in WebCore, and that it needs to be disabled for QWebView, the setting is disabled on the page when QWebView::setPage is called. The problem is that the runtime flags API currently allows the user to mess up WebCore by setting the flag on a QWebPage already associated with a QWebView. The bug is currently present in QtLauncher by going on maps.google.com. Zooming in/out on the map will result on the map being drawn white during the animation. To disable the setting and see the correct behavior you have to: - Enable QGraphicsWebView to enable the accelerated composition menu - Disable accelerated composition - Disable QGraphicsWebView We were wondering if we actually need the runtime flag since: - Once accelerated composition works correctly, there will be few cases where the developer would want to disable it for specific pages - The QWebSettings api is have effects on QWebPage, however this api feels to fit more on QGraphicsWebView. How should this be handled when the developer implements its own view over QWebPage? And arguments to keep the runtime flag: - Accelerated composition might require additional resources and memory to operate. A developer might want to save these resources for some page and enable it for slimer pages needing faster animations without having two version of QtWebKit compiled for each scenario. If we keep the flag then I think we should verify in QWebSettings that a compatible web view is attached to the QWebPage before forwarding the call to WebCore.

Attachments
Added a way for the chrome-client to disable compositing even if it's enabled in settings (6.57 KB, patch) 2010-04-19 14:08 PDT, Noam Rosenthal	no flags	Details Formatted Diff Diff
changed some names, and moved the chrome-client check to the settings-cache phase. it still works :) (6.61 KB, patch) 2010-04-19 15:05 PDT, Noam Rosenthal	no flags	Details Formatted Diff Diff
name changes, better punctuation grammar :) (6.62 KB, patch) 2010-04-19 17:12 PDT, Noam Rosenthal	simon.fraser: review+ simon.fraser: commit-queue-	Details Formatted Diff Diff
AC -> accelerated compositing (6.68 KB, patch) 2010-04-19 17:24 PDT, Noam Rosenthal	simon.fraser: review- simon.fraser: commit-queue-	Details Formatted Diff Diff
take 4: /me was being sloppy with const (6.71 KB, patch) 2010-04-19 17:42 PDT, Noam Rosenthal	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Noam Rosenthal

Comment 1 2010-04-09 08:19:03 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.

Benjamin Poulain

Comment 2 2010-04-09 08:38:07 PDT

(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 :)

Kenneth Rohde Christiansen

Comment 3 2010-04-09 10:22:20 PDT

(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.

Noam Rosenthal

Comment 4 2010-04-19 14:08:51 PDT

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...

Noam Rosenthal

Comment 5 2010-04-19 15:05:15 PDT

Created attachment 53726 [details] changed some names, and moved the chrome-client check to the settings-cache phase. it still works :)

Simon Fraser (smfr)

Comment 6 2010-04-19 16:10:27 PDT

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?

Simon Fraser (smfr)

Comment 7 2010-04-19 16:20:57 PDT

Also, the way we did this on Mac was to override the Settings value: http://trac.webkit.org/changeset/55100

Simon Fraser (smfr)

Comment 8 2010-04-19 16:29:53 PDT

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.

Noam Rosenthal

Comment 9 2010-04-19 16:33:56 PDT

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)

Noam Rosenthal

Comment 10 2010-04-19 17:12:15 PDT

Created attachment 53745 [details] name changes, better punctuation grammar :) applied feedback from Simon Fraser; re-uploading

Simon Fraser (smfr)

Comment 11 2010-04-19 17:19:07 PDT

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.

Noam Rosenthal

Comment 12 2010-04-19 17:24:45 PDT

Created attachment 53746 [details] AC -> accelerated compositing de-acronymed

Simon Fraser (smfr)

Comment 13 2010-04-19 17:33:25 PDT

Comment on attachment 53746 [details] AC -> accelerated compositing Not all my suggestions were responded to.

Noam Rosenthal

Comment 14 2010-04-19 17:42:19 PDT

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.

WebKit Commit Bot

Comment 15 2010-04-21 03:09:57 PDT

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>

WebKit Commit Bot

Comment 16 2010-04-21 03:10:04 PDT

All reviewed patches have been landed. Closing bug.

Simon Hausmann

Comment 17 2010-04-26 03:25:22 PDT

Revision r57961 cherry-picked into qtwebkit-2.0 with commit 003263d41f7ac488a40904fdc1b8ee91261853d5

Note You need to log in before you can comment on or make changes to this bug.