Bug 75376 - Remove unnecessary [Custom] attribute in CanvasRenderingContext2D.idl
Summary: Remove unnecessary [Custom] attribute in CanvasRenderingContext2D.idl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raymond
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-29 21:57 PST by Raymond
Modified: 2012-01-04 00:49 PST (History)
7 users (show)

See Also:


Attachments
Patch (4.74 KB, patch)
2011-12-29 22:07 PST, Raymond
no flags Details | Formatted Diff | Diff
Patch (8.26 KB, patch)
2012-01-03 22:29 PST, Raymond
no flags Details | Formatted Diff | Diff
Patch (7.09 KB, patch)
2012-01-03 22:57 PST, Raymond
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raymond 2011-12-29 21:57:03 PST
Remove unnecessary [Custom] attribute in CanvasRenderingContext2D.idl
Comment 1 Raymond 2011-12-29 22:07:35 PST
Created attachment 120779 [details]
Patch
Comment 2 Adam Barth 2011-12-29 23:12:17 PST
Yay
Comment 3 WebKit Review Bot 2011-12-30 12:40:23 PST
Comment on attachment 120779 [details]
Patch

Clearing flags on attachment: 120779

Committed r103849: <http://trac.webkit.org/changeset/103849>
Comment 4 WebKit Review Bot 2011-12-30 12:40:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Andreas Kling 2011-12-30 18:44:09 PST
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
Comment 6 Adam Barth 2011-12-30 18:54:38 PST
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).
Comment 7 Csaba Osztrogonác 2012-01-01 09:54:30 PST
Reopen not to forget fix layout test fixes.
Comment 8 Csaba Osztrogonác 2012-01-01 09:55:29 PST
(In reply to comment #7)
> Reopen not to forget fix layout test fixes.
typo fix: s/fix\ //g
Comment 9 Csaba Osztrogonác 2012-01-02 01:54:49 PST
I skipped them on Qt to paint the bot green: http://trac.webkit.org/changeset/103895

Please unskip them with the proper fix. Thanks.
Comment 10 Philippe Normand 2012-01-02 02:01:17 PST
Also skipped in GTK.
Comment 11 Raymond 2012-01-03 22:29:27 PST
Created attachment 121068 [details]
Patch
Comment 12 Adam Barth 2012-01-03 22:31:22 PST
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.
Comment 13 Raymond 2012-01-03 22:39:45 PST
(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?
Comment 14 Adam Barth 2012-01-03 22:51:54 PST
> 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.
Comment 15 Raymond 2012-01-03 22:57:53 PST
Created attachment 121074 [details]
Patch
Comment 16 Adam Barth 2012-01-03 22:59:12 PST
Comment on attachment 121074 [details]
Patch

Great.  Thanks.
Comment 17 Raymond 2012-01-03 23:05:24 PST
(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.
Comment 18 Adam Barth 2012-01-03 23:16:59 PST
Maybe they were failing for a different reason?
Comment 19 Raymond 2012-01-03 23:24:52 PST
(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 20 WebKit Review Bot 2012-01-04 00:48:58 PST
Comment on attachment 121074 [details]
Patch

Clearing flags on attachment: 121074

Committed r104019: <http://trac.webkit.org/changeset/104019>
Comment 21 WebKit Review Bot 2012-01-04 00:49:04 PST
All reviewed patches have been landed.  Closing bug.