Bug 75376 – Remove unnecessary [Custom] attribute in CanvasRenderingContext2D.idl

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Raymond]

Remove unnecessary [Custom] attribute in CanvasRenderingContext2D.idl

[Raymond]

Created attachment 120779 [details]
Patch

[Adam Barth]

Yay

[WebKit Review Bot]

Comment on attachment 120779 [details]
Patch

Clearing flags on attachment: 120779

Committed r103849: <http://trac.webkit.org/changeset/103849>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andreas Kling]

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

[Adam Barth]

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

[Csaba Osztrogonác]

Reopen not to forget fix layout test fixes.

[Csaba Osztrogonác]

(In reply to comment #7)
> Reopen not to forget fix layout test fixes.
typo fix: s/fix\ //g

[Csaba Osztrogonác]

I skipped them on Qt to paint the bot green: http://trac.webkit.org/changeset/103895

Please unskip them with the proper fix. Thanks.

[Philippe Normand]

Also skipped in GTK.

[Raymond]

Created attachment 121068 [details]
Patch

[Adam Barth]

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.

[Raymond]

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

[Adam Barth]

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

[Raymond]

Created attachment 121074 [details]
Patch

[Adam Barth]

Comment on attachment 121074 [details]
Patch

Great.  Thanks.

[Raymond]

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

[Adam Barth]

Maybe they were failing for a different reason?

[Raymond]

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

[WebKit Review Bot]

Comment on attachment 121074 [details]
Patch

Clearing flags on attachment: 121074

Committed r104019: <http://trac.webkit.org/changeset/104019>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.