Add canvas to set of elements that do not allow style sharing in order to provoke RenderLayer creation
Created attachment 164545 [details] Patch
Related chromium bug: http://crbug.com/144686 James, does this seem like the correct approach to you? I'm trying to figure out how this can be tested. I'm not sure if the original bug would reproduce in a layout test, though I'll try it.
I seem unable to trigger the bug in a layout test. I also wonder whether it would be a better (or even correct) fix to instead allow style sharing for iframes, plugins, and canvas and change setStyle to continue when the new/old style is the same object but skip the style diff computation and set diff = StyleDifferenceEqual.
Comment on attachment 164545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164545&action=review > Source/WebCore/rendering/RenderObject.cpp:1713 > + // The answer to requiresLayer() for plugins, iframes, and canvas can change outside of the style system, we do a setNeedsStyleRecalc(SyntheticStyleChange) in HTMLCanvasElement whenever the compositing status of a canvas changes, so I'm not sure that this comment change is accurate depending on what you mean by "outside of the style system"
Bugfix seems pretty reasonable, but I think we want a layout test. Does the test case in http://code.google.com/p/chromium/issues/detail?id=144686#c4 reproduce reliably?
You might be able to make a test similar to the iframe test (blame the changes that added iframes to the elements that disable sharing).
James, it does repro reliably within Chromium but didn't when I ran it in DRT. Simon, I will take a look. Thanks for the pointer.
Created attachment 164909 [details] Patch
I was able to create a test that reproduces the bug without the code change (and works correctly with the code change).
Comment on attachment 164545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164545&action=review >> Source/WebCore/rendering/RenderObject.cpp:1713 >> + // The answer to requiresLayer() for plugins, iframes, and canvas can change outside of the style system, > > we do a setNeedsStyleRecalc(SyntheticStyleChange) in HTMLCanvasElement whenever the compositing status of a canvas changes, so I'm not sure that this comment change is accurate depending on what you mean by "outside of the style system" Hopefully this is clearer in the new patch.
Comment on attachment 164909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164909&action=review > LayoutTests/fast/canvas/canvas-render-layer.html:5 > +<head> > + <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> > +</head> You can remove this. > LayoutTests/fast/canvas/canvas-render-layer.html:20 > + if (window.testRunner) { > + testRunner.waitUntilDone(); > + } No need for braces. > LayoutTests/fast/canvas/canvas-render-layer.html:27 > + if (window.testRunner) { > + testRunner.display(); > + } Ditto. Note that display() doesn't always do the same thing on Mac and Chromium. If you're using it to force layer creation, that might not work on Mac synchronously. Can you make a test that doesn't use display()?
Created attachment 165378 [details] Patch
Sorry for the slow cycle time; I was out of the office. It turns out the display() wasn't necessary at all.
Comment on attachment 165378 [details] Patch Attachment 165378 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13997610 New failing tests: fast/canvas/canvas-render-layer.html
Comment on attachment 165378 [details] Patch Could you add canvas-render-layer-expected.txt to this patch? I think the tests will fail without it.
Created attachment 165428 [details] Patch
(In reply to comment #15) > (From update of attachment 165378 [details]) > Could you add canvas-render-layer-expected.txt to this patch? I think the tests will fail without it. Done in the latest patch. There are different expectations for the normal and virtual/gpu runs. The former should not create layers while the latter should. At senorblanco's suggestion I had modified TestExpectations to mark the test as expected to fail, pending baselinig. But based on your request to add the canvas-render-layer-expected.txt file I'm wondering if that was actually incorrect.
Comment on attachment 165428 [details] Patch Attachment 165428 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13989793 New failing tests: fast/canvas/canvas-render-layer.html
Marking the test as failing in platform/chromium/TestExpectations won't help with the failure on the mac bot - you need to either add expectations that will apply to mac or mark it as failing for that platform.
Created attachment 165784 [details] Adds test expectation for Mac
Comment on attachment 165784 [details] Adds test expectation for Mac Attachment 165784 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14018571 New failing tests: platform/chromium/virtual/gpu/fast/canvas/canvas-render-layer.html fast/canvas/canvas-render-layer.html
Comment on attachment 165784 [details] Adds test expectation for Mac Attachment 165784 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13969061 New failing tests: fast/canvas/canvas-render-layer.html
Created attachment 165829 [details] Change test expectations to expected to fail for mac and chromium
Attachment 165829 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky. LayoutTests/platform/chromium/TestExpectations:972: Test lacks BUG modifier. [test/expectations] [5] WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky. WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky. WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky. WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky. WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky. WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky. WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky. WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky. WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky. WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky. WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky. WARNING: Using the chromium port without having the downstream skia_test_expectations.txt file checked out. Expectations related things might be wonky. Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Dirk: Do you know why the style bot is barfing like this?
(In reply to comment #25) > Dirk: Do you know why the style bot is barfing like this? If it keeps spamming like this, we need to turn off style checking for TestExpectation files.
I filed https://bugs.webkit.org/show_bug.cgi?id=97699 and attached a patch.
Is it a known issue that the checker fails on pre-existing errors? E.g.: LayoutTests/platform/chromium/TestExpectations:972: Test lacks BUG modifier. [test/expectations] [5]
(In reply to comment #28) > Is it a known issue that the checker fails on pre-existing errors? E.g.: > > LayoutTests/platform/chromium/TestExpectations:972: Test lacks BUG modifier. [test/expectations] [5] I don't recall if this is known or not, so I think that makes it not known :). Feel free to file a bug; maybe something has regressed.
(In reply to comment #29) > (In reply to comment #28) > > Is it a known issue that the checker fails on pre-existing errors? E.g.: > > > > LayoutTests/platform/chromium/TestExpectations:972: Test lacks BUG modifier. [test/expectations] [5] > > I don't recall if this is known or not, so I think that makes it not known :). Feel free to file a bug; maybe something has regressed. Ok, I filed https://bugs.webkit.org/show_bug.cgi?id=97717
Comment on attachment 165829 [details] Change test expectations to expected to fail for mac and chromium The latest patch passes the bots (leaving aside the bogus style complaint).
Comment on attachment 165829 [details] Change test expectations to expected to fail for mac and chromium View in context: https://bugs.webkit.org/attachment.cgi?id=165829&action=review > LayoutTests/fast/canvas/canvas-render-layer.html:34 > + }, 5); is there a particular reason these are 5ms delays instead of 0? typically we want tests to finish as quickly as possible
Created attachment 166240 [details] Patch
(In reply to comment #32) > (From update of attachment 165829 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165829&action=review > > > LayoutTests/fast/canvas/canvas-render-layer.html:34 > > + }, 5); > > is there a particular reason these are 5ms delays instead of 0? typically we want tests to finish as quickly as possible I thought I had determined that this was necessary to repro the bug. That may have been true with an earlier version of the test, but it does repro with a zero timeout (as in the patch just uploaded).
Comment on attachment 166240 [details] Patch Clearing flags on attachment: 166240 Committed r129934: <http://trac.webkit.org/changeset/129934>
All reviewed patches have been landed. Closing bug.
Filed bug 97940 for cleaning up the test and it's expectations.