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
Patch (21.41 KB, patch)
2011-07-28 14:30 PDT, Mark Pilgrim (Google)
no flags
Patch (22.89 KB, patch)
2011-08-02 14:57 PDT, Mark Pilgrim (Google)
no flags
Patch (27.07 KB, patch)
2011-08-03 13:14 PDT, Mark Pilgrim (Google)
no flags
Patch (26.66 KB, patch)
2011-08-03 15:29 PDT, Mark Pilgrim (Google)
no flags
Mark Pilgrim (Google)
Comment 1 2011-07-28 13:11:36 PDT
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
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
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
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
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 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.