Bug 98048 – Don't allow compositing to be disabled in forced compositing mode

• Component: Tools / Tests
• Status: RESOLVED INVALID

[Balazs Kelemen]

... because it makes the whole system confused. It brakes the syncronisation between UI and web process rendering, giving me empty pixel dumps with the patch applied from bug 95992. In fact we don't support the non-accelerated path so don't pretend that we do.

[Balazs Kelemen]

Created attachment 166508 [details]
Patch

[Balazs Kelemen]

For example css3/filters tests would broke after bug 95992 lands because of this.

[Jocelyn Turcotte]

Comment on attachment 166508 [details]
Patch

This feels a bit hidden in a corner and I think this doesn't need to be restricted to layout tests. What about making sure that this setting has no effect and gets overridden if forceCompositingMode() == true in DrawingAreaImpl?

[Simon Fraser (smfr)]

(In reply to comment #3)
> (From update of attachment 166508 [details])
> This feels a bit hidden in a corner and I think this doesn't need to be restricted to layout tests. What about making sure that this setting has no effect and gets overridden if forceCompositingMode() == true in DrawingAreaImpl?

Agreed. Seems like the wrong place to enforce this.

[Balazs Kelemen]

Created attachment 166829 [details]
Patch

[Build Bot]

Comment on attachment 166829 [details]
Patch

Attachment 166829 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14123661

[Jocelyn Turcotte]

Comment on attachment 166829 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166829&action=review

> Source/WebCore/page/Settings.h:512
> -        void setForceCompositingMode(bool flag) { m_forceCompositingMode = flag; }
> +        void setForceCompositingMode(bool flag)
> +        {

Settings.h is pretty clean already, I would send this to the .cpp if you expand it.

> Source/WebCore/page/Settings.h:513
> +            setAcceleratedCompositingEnabled(true);

I wouldn't call this if flag == false.

> Source/WebCore/page/Settings.h:514
> +            m_forceCompositingMode = flag;

Since setAcceleratedCompositingEnabled depends on m_forceCompositingMode, I would set this before calling setAcceleratedCompositingEnabled, just to be clean in case somebody updates this code in the future.

[Balazs Kelemen]

Created attachment 166861 [details]
Patch

[Build Bot]

Comment on attachment 166861 [details]
Patch

Attachment 166861 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14130715

[Balazs Kelemen]

Created attachment 166871 [details]
Patch

Rather not bothering with the constness here as it affects exported symbols.

[Build Bot]

Comment on attachment 166871 [details]
Patch

Attachment 166871 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14126749

[Balazs Kelemen]

I was wrong, this is about the setForceCompositingMode became non-inline so it should been exported.

[Balazs Kelemen]

Created attachment 166873 [details]
Patch

[Balazs Kelemen]

Comment on attachment 166873 [details]
Patch

Ops, forget about gtk...

[Balazs Kelemen]

Created attachment 166874 [details]
Patch

[Build Bot]

Comment on attachment 166874 [details]
Patch

Attachment 166874 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14152115

New failing tests:
http/tests/workers/terminate-during-sync-operation.html

[Build Bot]

Comment on attachment 166874 [details]
Patch

Attachment 166874 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14137596

[Balazs Kelemen]

Could you help me fixing the build? Things that I don't know:

 * I added the new symbol to symbol.filters but the Gtk build is still not happy. Is the symbol I added correct? I constructed the mandled by cloning it from another function with the same signature and changed the name. Should I edit any other file for the Gtk build?
 * Which file should I edit for the Windows build?

Both platforms want to use setAcceleratedCompositingEnabled from InternalSettings.cpp (libWebCoreTestSupport).

[Balazs Kelemen]

(In reply to comment #18)
> Could you help me fixing the build? Things that I don't know:
> 
>  * I added the new symbol to symbol.filters but the Gtk build is still not happy. Is the symbol I added correct? I constructed the mangled name by cloning it from another function with the same signature and changed the name. Should I edit any other file for the Gtk build?
>  * Which file should I edit for the Windows build?
> 
> Both platforms want to use setAcceleratedCompositingEnabled from InternalSettings.cpp (libWebCoreTestSupport).

Added more people in hope that they could help me with this :)

[Jocelyn Turcotte]

(In reply to comment #18)
>  * I added the new symbol to symbol.filters but the Gtk build is still not happy. Is the symbol I added correct? I constructed the mandled by cloning it from another function with the same signature and changed the name. Should I edit any other file for the Gtk build?
setMockScrollbarsEnabled is static, it's not the same signature.
By looking at methods present in both symbols.filter and WebCore.exp.in it seems like the only difference in mangling is the extra underscore at the beginning.

>  * Which file should I edit for the Windows build?
> 
I'm just guessing but it seems to be Source/WebKit2/win/WebKit2.def and Source/WebKit2/win/WebKit2CFLite.def.
The WebKit2 dll seems to links in their WebKit1 API as well.

[Balazs Kelemen]

Created attachment 167087 [details]
Patch

[Balazs Kelemen]

Comment on attachment 167087 [details]
Patch

Clearing flags on attachment: 167087

Committed r130389: <http://trac.webkit.org/changeset/130389>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This broke chromium.

[Vangelis Kokkevis]

(In reply to comment #24)
> This broke chromium.

It's also a bad idea to change the value of one setting based on that of another. There's logic in RenderLayerCompositor::cacheAcceleratedCompositingFlags() to set the value of m_forceCompositingMode based on a handful of conditions (including the isAcceleratedCompositingEnabled Setting). 

This seems like an awkward fix to a problem that lies elsewhere in the code.

[Balazs Kelemen]

(In reply to comment #24)
> This broke chromium.

Give some info about how it broke it, please!

[Balazs Kelemen]

(In reply to comment #25)
> (In reply to comment #24)
> > This broke chromium.
> 
> It's also a bad idea to change the value of one setting based on that of another. There's logic in RenderLayerCompositor::cacheAcceleratedCompositingFlags() to set the value of m_forceCompositingMode based on a handful of conditions (including the isAcceleratedCompositingEnabled Setting). 
> 
> This seems like an awkward fix to a problem that lies elsewhere in the code.

This function only sets m_forceCompositingMode to true if acceleratedCompositingEnabled is true so I think it doesn't conflict with the patch. This patch can break things in two cases:
 * if you set forceCompositingMode somewhere and expect that acceleratedCompositing is not necessarily true
 * if you want to disable acceleratedCompositing while forceCompositingMode is true

Both situations seem to be a bit strange.
Anyway, I'm going to roll out the patch because it seems to be a bad idea to add such logic to Settings instead of letting it's users handle that.

[Balazs Kelemen]

Reverted in http://trac.webkit.org/changeset/131162

[Vangelis Kokkevis]

(In reply to comment #28)
> Reverted in http://trac.webkit.org/changeset/131162

Thank you for reverting! Chrome uses the two flags independently and isAcceleratedCompositingEnabled is somewhat of a master switch (most often triggered by our GPU blacklist) to turn all other compositing off.