Summary: | Remove unnecessary [Custom] attribute in CanvasRenderingContext2D.idl | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Raymond <rgbbones> | ||||||||
Component: | Canvas | Assignee: | Raymond <rgbbones> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, kling, mdelaney7, ojan, ossy, pnormand, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Raymond
2011-12-29 21:57:03 PST
Created attachment 120779 [details]
Patch
Yay Comment on attachment 120779 [details] Patch Clearing flags on attachment: 120779 Committed r103849: <http://trac.webkit.org/changeset/103849> All reviewed patches have been landed. Closing bug. The following tests are failing on the Mac bot after this change: canvas/philip/tests/2d.missingargs.html canvas/philip/tests/2d.pattern.image.string.html canvas/philip/tests/2d.pattern.image.undefined.html http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r103854%20(35962)/canvas/philip/tests/2d.missingargs-pretty-diff.html looks pretty clearly like a progression since it's reporting fewer failures. http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r103854%20(35962)/canvas/philip/tests/2d.pattern.image.string-pretty-diff.html and http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r103854%20(35962)/canvas/philip/tests/2d.pattern.image.undefined-pretty-diff.html look fine. It's just a change in what kind of exception we're throwing from TYPE_MISMATCH_ERR to Type Error. I would recommend rebaselining these results. They're more consistent with the kinds of exceptions we throw throughout WebKit (which isn't surprising since we're deleting custom code). Reopen not to forget fix layout test fixes. (In reply to comment #7) > Reopen not to forget fix layout test fixes. typo fix: s/fix\ //g I skipped them on Qt to paint the bot green: http://trac.webkit.org/changeset/103895 Please unskip them with the proper fix. Thanks. Also skipped in GTK. Created attachment 121068 [details]
Patch
Comment on attachment 121068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121068&action=review > LayoutTests/canvas/philip/tests/2d.pattern.image.string.html:15 > -try { var _thrown = false; ctx.createPattern('../images/red.png', 'repeat');} catch (e) { if (e.code != DOMException.TYPE_MISMATCH_ERR) _fail("Failed assertion: expected exception of type TYPE_MISMATCH_ERR, got: "+e.message); _thrown = true; } finally { _assert(_thrown, "should throw exception of type TYPE_MISMATCH_ERR: ctx.createPattern('../images/red.png', 'repeat')"); } > +try { var _thrown = false; ctx.createPattern('../images/red.png', 'repeat');} catch (e) { if (!(e instanceof TypeError)) _fail("Failed assertion: expected exception of type TypeError, got: "+e.message); _thrown = true; } finally { _assert(_thrown, "should throw exception of type TypeError: ctx.createPattern('../images/red.png', 'repeat')"); } I don't think we're supposed to change these tests. They're a test suite written by philip. We should just update the expected results. > LayoutTests/canvas/philip/tests/2d.pattern.image.undefined.html:17 > -} catch (e) { if (e.code != DOMException.TYPE_MISMATCH_ERR) _fail("Failed assertion: expected exception of type TYPE_MISMATCH_ERR, got: "+e.message); _thrown = true; } finally { _assert(_thrown, "should throw exception of type TYPE_MISMATCH_ERR: ctx.createPattern(undefined, 'repeat')"); } > +} catch (e) { if (!(e instanceof TypeError)) _fail("Failed assertion: expected exception of type TypeError, got: "+e.message); _thrown = true; } finally { _assert(_thrown, "should throw exception of type TypeError: ctx.createPattern(undefined, 'repeat')"); } Ditto. (In reply to comment #12) > I don't think we're supposed to change these tests. They're a test suite written by philip. We should just update the expected results. > Ok, I see. I will update the patch. Thx. Is there any more background doc about why these test case it self should not be modified? At the same time, I am also puzzled that why I also need to remove entries in LayoutTests/platform/chromium/test-expectations.txt to have the run_layout_test script not complaining "an unexpected pass". I check the log of the LayoutTests/platform/chromium/test-expectations.txt , seems these entries are not added after 2011-12-29, so why they are there at all, am I missing anything here? > Is there any more background doc about why these test case it self should not be modified? Unfortunately not. There's been some talk about creating a folder for imported test suites. For now, you just need to know the test suites. > At the same time, I am also puzzled that why I also need to remove entries in LayoutTests/platform/chromium/test-expectations.txt to have the run_layout_test script not complaining "an unexpected pass". I check the log of the LayoutTests/platform/chromium/test-expectations.txt , seems these entries are not added after 2011-12-29, so why they are there at all, am I missing anything here? I'm not sure I understand. It looks like these tests are marked as expected to fail. Once you fix them, they'll pass, which means we shouldn't expect them to fail anymore (i.e., we should remove them from the file). Your patch looks like its doing the right thing. Created attachment 121074 [details]
Patch
Comment on attachment 121074 [details]
Patch
Great. Thanks.
(In reply to comment #14) > > At the same time, I am also puzzled that why I also need to remove entries in LayoutTests/platform/chromium/test-expectations.txt to have the run_layout_test script not complaining "an unexpected pass". I check the log of the LayoutTests/platform/chromium/test-expectations.txt , seems these entries are not added after 2011-12-29, so why they are there at all, am I missing anything here? > > I'm not sure I understand. It looks like these tests are marked as expected to fail. Once you fix them, they'll pass, which means we shouldn't expect them to fail anymore (i.e., we should remove them from the file). Your patch looks like its doing the right thing. I mean It seems to me they are marked as expected to fail before the first patch been merged, But at that time, they should expected to success instead of fail. Then why they are marked? So I am a little worry about that is there anything that I miss. Maybe they were failing for a different reason? (In reply to comment #18) > Maybe they were failing for a different reason? This is what puzzled me, for I did run LayoutTest on chromium port, And now it actually don't fail.... Anyway, let buildbot check it again. Comment on attachment 121074 [details] Patch Clearing flags on attachment: 121074 Committed r104019: <http://trac.webkit.org/changeset/104019> All reviewed patches have been landed. Closing bug. |