97013 – Add canvas to set of elements that do not allow style sharing in order to provoke RenderLayer creation

RESOLVED FIXED 97013

Add canvas to set of elements that do not allow style sharing in order to provoke RenderLayer creation

https://bugs.webkit.org/show_bug.cgi?id=97013

Summary Add canvas to set of elements that do not allow style sharing in order to pro...

Brian Salomon

Reported 2012-09-18 06:30:43 PDT

Add canvas to set of elements that do not allow style sharing in order to provoke RenderLayer creation

Attachments
Patch (3.37 KB, patch) 2012-09-18 06:33 PDT, Brian Salomon	no flags	Details Formatted Diff Diff
Patch (7.16 KB, patch) 2012-09-20 06:46 PDT, Brian Salomon	no flags	Details Formatted Diff Diff
Patch (7.05 KB, patch) 2012-09-24 07:35 PDT, Brian Salomon	no flags	Details Formatted Diff Diff
Patch (9.65 KB, patch) 2012-09-24 12:24 PDT, Brian Salomon	no flags	Details Formatted Diff Diff
Adds test expectation for Mac (10.10 KB, patch) 2012-09-26 06:38 PDT, Brian Salomon	no flags	Details Formatted Diff Diff
Change test expectations to expected to fail for mac and chromium (7.49 KB, patch) 2012-09-26 10:33 PDT, Brian Salomon	no flags	Details Formatted Diff Diff
Patch (7.81 KB, patch) 2012-09-28 06:37 PDT, Brian Salomon	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Brian Salomon

Comment 1 2012-09-18 06:33:09 PDT

Created attachment 164545 [details] Patch

Brian Salomon

Comment 2 2012-09-18 06:37:37 PDT

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.

Brian Salomon

Comment 3 2012-09-18 09:09:03 PDT

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.

James Robinson

Comment 4 2012-09-18 10:53:19 PDT

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"

James Robinson

Comment 5 2012-09-18 10:54:45 PDT

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?

Simon Fraser (smfr)

Comment 6 2012-09-18 10:56:37 PDT

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

Brian Salomon

Comment 7 2012-09-18 12:10:01 PDT

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.

Brian Salomon

Comment 8 2012-09-20 06:46:59 PDT

Created attachment 164909 [details] Patch

Brian Salomon

Comment 9 2012-09-20 06:48:31 PDT

I was able to create a test that reproduces the bug without the code change (and works correctly with the code change).

Brian Salomon

Comment 10 2012-09-20 06:48:56 PDT

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.

Simon Fraser (smfr)

Comment 11 2012-09-20 10:25:23 PDT

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

Brian Salomon

Comment 12 2012-09-24 07:35:30 PDT

Created attachment 165378 [details] Patch

Brian Salomon

Comment 13 2012-09-24 07:37:41 PDT

Sorry for the slow cycle time; I was out of the office. It turns out the display() wasn't necessary at all.

Build Bot

Comment 14 2012-09-24 07:58:12 PDT

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

James Robinson

Comment 15 2012-09-24 10:48:17 PDT

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.

Brian Salomon

Comment 16 2012-09-24 12:24:24 PDT

Created attachment 165428 [details] Patch

Brian Salomon

Comment 17 2012-09-24 12:28:12 PDT

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

Build Bot

Comment 18 2012-09-24 14:03:25 PDT

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

James Robinson

Comment 19 2012-09-24 17:10:10 PDT

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.

Brian Salomon

Comment 20 2012-09-26 06:38:53 PDT

Created attachment 165784 [details] Adds test expectation for Mac

WebKit Review Bot

Comment 21 2012-09-26 08:48:15 PDT

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

Build Bot

Comment 22 2012-09-26 09:39:45 PDT

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

Brian Salomon

Comment 23 2012-09-26 10:33:42 PDT

Created attachment 165829 [details] Change test expectations to expected to fail for mac and chromium

WebKit Review Bot

Comment 24 2012-09-26 10:39:06 PDT

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.

Eric Seidel (no email)

Comment 25 2012-09-26 10:43:33 PDT

Dirk: Do you know why the style bot is barfing like this?

Adam Barth

Comment 26 2012-09-26 10:46:32 PDT

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

Adam Barth

Comment 27 2012-09-26 10:53:03 PDT

I filed https://bugs.webkit.org/show_bug.cgi?id=97699 and attached a patch.

Brian Salomon

Comment 28 2012-09-26 12:04:04 PDT

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]

Dirk Pranke

Comment 29 2012-09-26 12:09:30 PDT

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

Brian Salomon

Comment 30 2012-09-26 12:31:13 PDT

(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

Brian Salomon

Comment 31 2012-09-27 06:54:26 PDT

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

James Robinson

Comment 32 2012-09-27 18:06:34 PDT

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

Brian Salomon

Comment 33 2012-09-28 06:37:18 PDT

Created attachment 166240 [details] Patch

Brian Salomon

Comment 34 2012-09-28 06:39:02 PDT

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

WebKit Review Bot

Comment 35 2012-09-28 12:47:48 PDT

Comment on attachment 166240 [details] Patch Clearing flags on attachment: 166240 Committed r129934: <http://trac.webkit.org/changeset/129934>

WebKit Review Bot

Comment 36 2012-09-28 12:47:53 PDT

All reviewed patches have been landed. Closing bug.

Ojan Vafai

Comment 37 2012-09-28 14:35:09 PDT

Filed bug 97940 for cleaning up the test and it's expectations.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component New Bugs

Assignee

Brian Salomon

Reported

2012-09-18 06:30 PDT

Modified

2012-09-28 14:35 PDT History

CC List

15 users Show

URL

Keywords

Depends on

Blocks