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.
Created attachment 102290 [details] Patch
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.
Created attachment 102300 [details] Patch
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.
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.
Comment on attachment 102300 [details] Patch You decided not to add the test?
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
Created attachment 102702 [details] Patch
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
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
Created attachment 102819 [details] Patch
Fixed failing tests that were relying on looser required-arguments behavior (that Adam says we should make stricter).
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!
I'm confused; why is this commit-queue-?
(In reply to comment #14) > I'm confused; why is this commit-queue-? You included two copies of your ChangeLog entry.
Created attachment 102840 [details] Patch
Comment on attachment 102840 [details] Patch Clearing flags on attachment: 102840 Committed r92327: <http://trac.webkit.org/changeset/92327>
All reviewed patches have been landed. Closing bug.
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
canPlayType is inherited from HTMLMediaElement and that takes one argument, which used to be optional for HTMLVideoElement.
(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
(In reply to comment #21) > > maybe we should also remove the canPlayType() line This seems like the correct fix.
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.
Oops, apparently the author never came back to this bug :( Didn't mean to criticize Philippe or Eric here.
Sorry about the confusion, Philippe and Eric. I'm going to send my angry email to Mark instead now.
I've landed http://trac.webkit.org/changeset/92455 in attempt to fix these issues.