Certain CCSettings were made global to be accessible by a LayerChromium that has not been added to the tree. This is unfortunate and it also turns out to be unnecessary; the setting can be plumbed to GraphicsLayerChromium who can do the check.
Created attachment 148013 [details] Patch
Created attachment 148014 [details] Patch
Created attachment 148018 [details] Patch Rebasing. Hopefully the new patch will apply cleanly.
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Created attachment 150655 [details] Patch Rebasing patch.
I think reverts typically refer to the revision number - this is a revert of r120360 (bug 88384), right? I'm not sure I get it - if we just revert this patch, won't we reintroduce the bug it was supposed to fix? Bug 89298 is marked as blocked on this bug and is intended to fix the bug again, but we need to keep things working. I don't really get the motivation for changing this. https://bugs.webkit.org/show_bug.cgi?id=89298 requires placing ugly #if PLATFORM(CHROMIUM) blocks in cross-platform code and I can't tell that there is any corresponding improvement. Do we have a requirement to have different settings for different compositors in the same process, or is this just a stylistic concern?
(In reply to comment #6) > I think reverts typically refer to the revision number - this is a revert of r120360 (bug 88384), right? I'm not sure I get it - if we just revert this patch, won't we reintroduce the bug it was supposed to fix? Bug 89298 is marked as blocked on this bug and is intended to fix the bug again, but we need to keep things working. > > I don't really get the motivation for changing this. https://bugs.webkit.org/show_bug.cgi?id=89298 requires placing ugly #if PLATFORM(CHROMIUM) blocks in cross-platform code and I can't tell that there is any corresponding improvement. Do we have a requirement to have different settings for different compositors in the same process, or is this just a stylistic concern? I didn't like resorting to globals just to pass the accelerated animation preference around. At the time, it looked like it was the only way, but once I saw that it could be plumbed relatively easily, I thought that would be a safer option (and would discourage further use of global settings). That work seemed to naturally split into a couple of patches. My hope was that if they were both approved, they could be landed in quick succession, and we wouldn't notice the brief reintroduction of the bug.
(In reply to comment #7) > My hope was that if they were both approved, they could be landed in quick succession, and we wouldn't notice the brief reintroduction of the bug. That's a very dangerous way to live, and not something I'm comfortable with. There are all sorts of reasons it could break - we might pick up a roll that has one and not the other, one or both halves could be reverted, etc etc. We have to keep trunk shippable at all times since we ship from trunk continually.
(In reply to comment #8) > (In reply to comment #7) > > My hope was that if they were both approved, they could be landed in quick succession, and we wouldn't notice the brief reintroduction of the bug. > > That's a very dangerous way to live, and not something I'm comfortable with. There are all sorts of reasons it could break - we might pick up a roll that has one and not the other, one or both halves could be reverted, etc etc. We have to keep trunk shippable at all times since we ship from trunk continually. That's true. I will merge the two patches and upload into one bug (I believe they need to be rebased anyhow).
If we have to touch cross-platform code to add this setting, I don't think it's worth it.
Comment on attachment 150655 [details] Patch R- since this will reintroduce bug 88384
Marked LayoutTest bugs, bugs with Chromium IDs, and some others as WontFix. Test failure bugs still are trackable via TestExpectations or disabled unit tests.