See: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-createpattern and http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas_CR/#dom-context-2d-createpattern Chrome and Firefox also have this behavior.
Created attachment 230534 [details] Patch
Comment on attachment 230534 [details] Patch Attachment 230534 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6656302818263040 New failing tests: canvas/philip/tests/2d.pattern.image.incomplete.empty.html fast/canvas/canvas-empty-image-pattern.html fast/dom/gc-9.html
Created attachment 230554 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 230557 [details] Patch
Comment on attachment 230557 [details] Patch Attachment 230557 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4690787807789056 New failing tests: fast/dom/gc-9.html
Created attachment 230559 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 230557 [details] Patch Attachment 230557 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5324877452017664 New failing tests: fast/dom/gc-9.html
Created attachment 230562 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 230564 [details] Patch
Comment on attachment 230564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230564&action=review > Source/WebCore/ChangeLog:13 > + * canvas/philip/tests/2d.pattern.image.broken.html: > + * canvas/philip/tests/2d.pattern.image.incomplete.empty.html: This is an imported test suite; do your changes match upstream?
(In reply to comment #10) > (From update of attachment 230564 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230564&action=review > > > Source/WebCore/ChangeLog:13 > > + * canvas/philip/tests/2d.pattern.image.broken.html: > > + * canvas/philip/tests/2d.pattern.image.incomplete.empty.html: > > This is an imported test suite; do your changes match upstream? yes,this test suite is now maintained by the w3c. I updated this test yesterday but it needs to be regenerated. see http://www.w3c-test.org/2dcontext/fill-and-stroke-styles/2d.pattern.image.broken.html I'm looking over the cases where WK is not spec compliant
Comment on attachment 230564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230564&action=review > LayoutTests/fast/canvas/canvas-empty-image-pattern.html:2 > +<title>Canvas test: filling a pattern with an empty image should throw an exception.</title> This is a crash test, why do you change it? > LayoutTests/fast/dom/gc-9-expected.txt:-15 > -PASS: document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(new Image(), 'no-repeat').myCustomProperty should be undefined and is. Why do the pass messages disappear? > LayoutTests/fast/dom/gc-9.html:-124 > - [ "document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(new Image(), 'no-repeat')" ], // CanvasPattern Why did you remove this test?
Comment on attachment 230564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230564&action=review >> LayoutTests/fast/canvas/canvas-empty-image-pattern.html:2 >> +<title>Canvas test: filling a pattern with an empty image should throw an exception.</title> > > This is a crash test, why do you change it? Not sure if I follow. The test will now throw an exception. Should I remove it completely? >> LayoutTests/fast/dom/gc-9-expected.txt:-15 >> -PASS: document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(new Image(), 'no-repeat').myCustomProperty should be undefined and is. > > Why do the pass messages disappear? because I removed the test that creates an empty image. >> LayoutTests/fast/dom/gc-9.html:-124 >> - [ "document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(new Image(), 'no-repeat')" ], // CanvasPattern > > Why did you remove this test? createPattern(new Image()) will throw an exception as per the spec so this test no longer works.
Comment on attachment 230564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230564&action=review recover canvas-empty-image-pattern.html and I am r+ the patch. > LayoutTests/canvas/philip/tests/2d.pattern.image.incomplete.empty.html:23 > +var _thrown = undefined; > +try{ > + ctx.createPattern(img, 'repeat'); > +}catch(e) { > + _thrown = e; > +} > +_assert(_thrown && _thrown.name == "InvalidStateError" && _thrown.code == DOMException.INVALID_STATE_ERR, "should throw InvalidStateError"); You assured before that these changes were made in the official test suite as well, right? Then I am fine with the changes. >>> LayoutTests/fast/canvas/canvas-empty-image-pattern.html:2 >>> +<title>Canvas test: filling a pattern with an empty image should throw an exception.</title> >> >> This is a crash test, why do you change it? > > Not sure if I follow. The test will now throw an exception. Should I remove it completely? Not, just leave the comment unchanged. > LayoutTests/fast/canvas/canvas-empty-image-pattern.html:4 > +<script src="../../resources/js-test-pre.js"></script> don't include more things. crash tests are done to demonstrate that we do not fail again under the same circumstances. > LayoutTests/fast/canvas/canvas-empty-image-pattern.html:16 > +var _thrown = undefined; > +try { > + canvas.fillStyle = canvas.createPattern(new Image, "repeat") > +} catch(e) { > + _thrown = e; > +} > +shouldBe('_thrown && _thrown.name == "InvalidStateError" && _thrown.code == DOMException.INVALID_STATE_ERR', 'true'); Just let it throw and update the expected file. > LayoutTests/fast/canvas/canvas-empty-image-pattern.html:19 > +<script src="../../resources/js-test-post.js"></script> Ditto. >>> LayoutTests/fast/dom/gc-9.html:-124 >>> - [ "document.getElementsByTagName('canvas')[0].getContext('2d').createPattern(new Image(), 'no-repeat')" ], // CanvasPattern >> >> Why did you remove this test? > > createPattern(new Image()) will throw an exception as per the spec so this test no longer works. Ok.
Created attachment 230949 [details] Patch
Comment on attachment 230949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230949&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1756 > + if (!cachedImage || cachedImage->status() == CachedResource::LoadError) { Does the test case cover the "!cachedImage" case or only the LoadError case?
Comment on attachment 230949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230949&action=review >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1756 >> + if (!cachedImage || cachedImage->status() == CachedResource::LoadError) { > > Does the test case cover the "!cachedImage" case or only the LoadError case? Just the LoadError. When is there no cachedImage?
Comment on attachment 230949 [details] Patch Clearing flags on attachment: 230949 Committed r168400: <http://trac.webkit.org/changeset/168400>
All reviewed patches have been landed. Closing bug.