WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
65338
Remove LegacyDefaultOptionalArguments flag from HTML DOM IDL files
https://bugs.webkit.org/show_bug.cgi?id=65338
Summary
Remove LegacyDefaultOptionalArguments flag from HTML DOM IDL files
Mark Pilgrim (Google)
Reported
2011-07-28 13:07:37 PDT
As discussed in IRC, we are migrating our IDL files away from the interface-level "LegacyDefaultOptionalArguments" flag and onto argument-level [Optional] or [Optional=CallWithDefaultValue] flags. This patch migrates all remaining HTML DOM-related IDL files. It does not change any behavior, i.e. it does not make any arguments required that were previously optional, nor vice-versa. All existing tests pass.
Attachments
Patch
(27.91 KB, patch)
2011-07-28 13:11 PDT
,
Mark Pilgrim (Google)
no flags
Details
Formatted Diff
Diff
Patch
(21.41 KB, patch)
2011-07-28 14:30 PDT
,
Mark Pilgrim (Google)
no flags
Details
Formatted Diff
Diff
Patch
(22.89 KB, patch)
2011-08-02 14:57 PDT
,
Mark Pilgrim (Google)
no flags
Details
Formatted Diff
Diff
Patch
(27.07 KB, patch)
2011-08-03 13:14 PDT
,
Mark Pilgrim (Google)
no flags
Details
Formatted Diff
Diff
Patch
(26.66 KB, patch)
2011-08-03 15:29 PDT
,
Mark Pilgrim (Google)
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Mark Pilgrim (Google)
Comment 1
2011-07-28 13:11:36 PDT
Created
attachment 102290
[details]
Patch
Adam Barth
Comment 2
2011-07-28 13:19:49 PDT
Comment on
attachment 102290
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=102290&action=review
Lots of minor comments. I'd like to see one more round.
> Source/WebCore/html/DOMTokenList.idl:37 > + [ConvertNullStringTo=Null] DOMString item(in [Optional=CallWithDefaultValue] unsigned long index); > + boolean contains(in [Optional=CallWithDefaultValue] DOMString token) raises(DOMException); > + void add(in [Optional=CallWithDefaultValue] DOMString token) raises(DOMException); > + void remove(in [Optional=CallWithDefaultValue] DOMString token) raises(DOMException); > + boolean toggle(in [Optional=CallWithDefaultValue] DOMString token) raises(DOMException);
I'd skip these. DOMTokenList is relatively new and these functions are useless without these arguments.
> Source/WebCore/html/DOMURL.idl:35 > + [ConvertNullStringTo=Undefined] DOMString createObjectURL(in [Optional=CallWithDefaultValue] Blob blob); > + void revokeObjectURL(in [Optional=CallWithDefaultValue] DOMString url);
I'd skip these here. DOMURL is very new and these functions are useless without these arguments.
> Source/WebCore/html/HTMLAllCollection.idl:39 > - [Custom] Node item(in unsigned long index); > - [Custom] Node namedItem(in DOMString name); > + [Custom] Node item(in [Optional=CallWithDefaultValue] unsigned long index); > + [Custom] Node namedItem(in [Optional=CallWithDefaultValue] DOMString name); > > // FIXME: This should return an HTMLAllCollection. > - NodeList tags(in DOMString name); > + NodeList tags(in [Optional=CallWithDefaultValue] DOMString name);
Can you test what IE does here? HTMLAllCollection exists to mimic IE.
> Source/WebCore/html/HTMLAnchorElement.idl:57 > - DOMString getParameter(in DOMString name); > + DOMString getParameter(in [Optional=CallWithDefaultValue] DOMString name);
You can skip this one. getParameter is very new.
> Source/WebCore/html/HTMLButtonElement.idl:43 > - void setCustomValidity(in [ConvertUndefinedOrNullToNullString] DOMString error); > + void setCustomValidity(in [ConvertUndefinedOrNullToNullString,Optional=CallWithDefaultValue] DOMString error);
I bet we can skip this one too.
> Source/WebCore/html/HTMLFieldSetElement.idl:28 > - void setCustomValidity(in [ConvertUndefinedOrNullToNullString] DOMString error); > + void setCustomValidity(in [ConvertUndefinedOrNullToNullString,Optional=CallWithDefaultValue] DOMString error);
This one too.
> Source/WebCore/html/HTMLInputElement.idl:76 > - void setCustomValidity(in [ConvertUndefinedOrNullToNullString] DOMString error); > + void setCustomValidity(in [ConvertUndefinedOrNullToNullString,Optional=CallWithDefaultValue] DOMString error);
ditto
> Source/WebCore/html/HTMLInputElement.idl:81 > - void setValueForUser(in [ConvertNullToNullString] DOMString value); > + void setValueForUser(in [ConvertNullToNullString,Optional=CallWithDefaultValue] DOMString value);
This doesn't appear to be a JavaScript API, so this attribute probably isn't needed.
> Source/WebCore/html/HTMLKeygenElement.idl:47 > - void setCustomValidity(in [ConvertUndefinedOrNullToNullString] DOMString error); > + void setCustomValidity(in [ConvertUndefinedOrNullToNullString,Optional=CallWithDefaultValue] DOMString error);
setCustomValidity => skip
> Source/WebCore/html/HTMLMediaElement.idl:48 > - DOMString canPlayType(in DOMString type); > + DOMString canPlayType(in [Optional=CallWithDefaultValue] DOMString type);
This one is also new and probably skippable.
> Source/WebCore/html/HTMLOptionsCollection.idl:39 > + Node item(in [Optional=CallWithDefaultValue] unsigned long index); > + Node namedItem(in [Optional=CallWithDefaultValue] DOMString name);
This isn't a JS API.
> Source/WebCore/html/HTMLSelectElement.idl:63 > - void remove(in long index); > + void remove(in [Optional=CallWithDefaultValue] long index);
Not a JS API.
> Source/WebCore/html/TimeRanges.idl:34 > - float start(in unsigned long index) > + float start(in [Optional=CallWithDefaultValue] unsigned long index) > raises (DOMException); > - float end(in unsigned long index) > + float end(in [Optional=CallWithDefaultValue] unsigned long index)
These are also pretty new and can likely be skipped.
Mark Pilgrim (Google)
Comment 3
2011-07-28 14:30:23 PDT
Created
attachment 102300
[details]
Patch
Mark Pilgrim (Google)
Comment 4
2011-07-28 14:31:31 PDT
Addressed all feedback. In reference to HTMLAllCollection, I tested IE9 under Windows 7 and discovered that .item() treats a missing argument as .item(0), but the other 2 methods throw an exception if the argument is missing. I've adjusted the IDL to match.
Adam Barth
Comment 5
2011-07-28 15:24:16 PDT
Comment on
attachment 102300
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=102300&action=review
> Source/WebCore/html/HTMLAllCollection.idl:35 > - [Custom] Node item(in unsigned long index); > + [Custom] Node item(in [Optional=CallWithDefaultValue] unsigned long index);
It be worth adding a test that documents the behavior you tested from IE.
Adam Barth
Comment 6
2011-08-02 13:47:46 PDT
Comment on
attachment 102300
[details]
Patch You decided not to add the test?
WebKit Review Bot
Comment 7
2011-08-02 14:28:19 PDT
Comment on
attachment 102300
[details]
Patch Rejecting
attachment 102300
[details]
from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2 Last 500 characters of output: l/HTMLTableRowElement.idl patching file Source/WebCore/html/HTMLTableSectionElement.idl patching file Source/WebCore/html/HTMLTextAreaElement.idl Hunk #2 FAILED at 48. 1 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/html/HTMLTextAreaElement.idl.rej patching file Source/WebCore/html/HTMLVideoElement.idl patching file Source/WebCore/html/TimeRanges.idl Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1 Full output:
http://queues.webkit.org/results/9300034
Mark Pilgrim (Google)
Comment 8
2011-08-02 14:57:28 PDT
Created
attachment 102702
[details]
Patch
WebKit Review Bot
Comment 9
2011-08-02 16:01:31 PDT
Comment on
attachment 102702
[details]
Patch Rejecting
attachment 102702
[details]
from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2 Last 500 characters of output: ms/ValidityState-customError.html = TEXT Regressions: Unexpected image mismatch : (5) fast/text/atsui-multiple-renderers.html = IMAGE fast/text/international/danda-space.html = IMAGE fast/text/international/thai-baht-space.html = IMAGE fast/text/international/thai-line-breaks.html = IMAGE platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE Regressions: Unexpected image and text mismatch : (1) svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT Full output:
http://queues.webkit.org/results/9289802
WebKit Review Bot
Comment 10
2011-08-02 16:27:18 PDT
Comment on
attachment 102702
[details]
Patch
Attachment 102702
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/9290876
New failing tests: fast/files/revoke-blob-url.html fast/forms/ValidityState-customError.html fast/files/create-blob-url-crash.html
Mark Pilgrim (Google)
Comment 11
2011-08-03 13:14:12 PDT
Created
attachment 102819
[details]
Patch
Mark Pilgrim (Google)
Comment 12
2011-08-03 13:14:50 PDT
Fixed failing tests that were relying on looser required-arguments behavior (that Adam says we should make stricter).
Adam Barth
Comment 13
2011-08-03 13:42:59 PDT
Comment on
attachment 102819
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=102819&action=review
Your patch would have saved us from a crasher! :)
> Source/WebCore/ChangeLog:8 > +2011-08-03 Mark Pilgrim <
pilgrim@chromium.org
> > + > + Remove LegacyDefaultOptionalArguments flag from HTML DOM IDL files > +
https://bugs.webkit.org/show_bug.cgi?id=65338
> + > + Reviewed by Adam Barth. > + > + No new tests, all existing tests pass.
Stereo changelogs!
Mark Pilgrim (Google)
Comment 14
2011-08-03 14:45:46 PDT
I'm confused; why is this commit-queue-?
Adam Barth
Comment 15
2011-08-03 15:02:20 PDT
(In reply to
comment #14
)
> I'm confused; why is this commit-queue-?
You included two copies of your ChangeLog entry.
Mark Pilgrim (Google)
Comment 16
2011-08-03 15:29:36 PDT
Created
attachment 102840
[details]
Patch
WebKit Review Bot
Comment 17
2011-08-03 16:21:12 PDT
Comment on
attachment 102840
[details]
Patch Clearing flags on attachment: 102840 Committed
r92327
: <
http://trac.webkit.org/changeset/92327
>
WebKit Review Bot
Comment 18
2011-08-03 16:21:17 PDT
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 19
2011-08-03 18:30:30 PDT
This patch appears to have broken media/video-can-play-type.html on at least Snow Leopard and GTK:
http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r92327%20(16217)/media/video-can-play-type-pretty-diff.html
http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r92327%20(1502)/media/video-can-play-type-pretty-diff.html
Ryosuke Niwa
Comment 20
2011-08-03 18:32:53 PDT
canPlayType is inherited from HTMLMediaElement and that takes one argument, which used to be optional for HTMLVideoElement.
Philippe Normand
Comment 21
2011-08-04 00:16:07 PDT
(In reply to
comment #20
)
> canPlayType is inherited from HTMLMediaElement and that takes one argument, which used to be optional for HTMLVideoElement.
I think the test needs either a rebaseline, maybe we should also remove the canPlayType() line
Eric Carlson
Comment 22
2011-08-04 13:11:10 PDT
(In reply to
comment #21
)
> > maybe we should also remove the canPlayType() line
This seems like the correct fix.
Ryosuke Niwa
Comment 23
2011-08-05 00:20:39 PDT
And you didn't do the rebaseline for 24 hours and went on to land other IDL patches? Please read
http://www.webkit.org/coding/contributing.html#landing
carefully before you make any further contribution to the project.
Ryosuke Niwa
Comment 24
2011-08-05 00:25:40 PDT
Oops, apparently the author never came back to this bug :( Didn't mean to criticize Philippe or Eric here.
Ryosuke Niwa
Comment 25
2011-08-05 00:30:36 PDT
Sorry about the confusion, Philippe and Eric. I'm going to send my angry email to Mark instead now.
Adam Barth
Comment 26
2011-08-05 00:31:47 PDT
I've landed
http://trac.webkit.org/changeset/92455
in attempt to fix these issues.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug