Bug 98048

Created attachment 166508 [details] Patch

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

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?

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

Created attachment 166829 [details] Patch

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

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.

Created attachment 166861 [details] Patch

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

Created attachment 166871 [details] Patch Rather not bothering with the constness here as it affects exported symbols.

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

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

Created attachment 166873 [details] Patch

Comment on attachment 166873 [details] Patch Ops, forget about gtk...

Created attachment 166874 [details] Patch

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

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

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

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

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

Created attachment 167087 [details] Patch

Comment on attachment 167087 [details] Patch Clearing flags on attachment: 167087 Committed r130389: <http://trac.webkit.org/changeset/130389>

All reviewed patches have been landed. Closing bug.

This broke chromium.

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

(In reply to comment #24) > This broke chromium. Give some info about how it broke it, please!

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

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

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