Bug 37313 – [Qt] Fix or remove the runtime flag for accelerated compositing.

• Status: CLOSED
• Resolution: FIXED
• Component: WebKit Qt

[Jocelyn Turcotte]

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.

[Noam Rosenthal]

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]

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

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

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]

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 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)]

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

[Simon Fraser (smfr)]

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

Created attachment 53745 [details]
name changes, better punctuation grammar :)

applied feedback from Simon Fraser; re-uploading

[Simon Fraser (smfr)]

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 @@
> [phone]  No'am Rosenthal  <[e-mail]>
> +
> +        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]

Created attachment 53746 [details]
AC -> accelerated compositing

de-acronymed

[Simon Fraser (smfr)]

Comment on attachment 53746 [details]
AC -> accelerated compositing

Not all my suggestions were responded to.

[Noam Rosenthal]

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

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

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