Bug 65338 – Remove LegacyDefaultOptionalArguments flag from HTML DOM IDL files

• Component: DOM
• Status: RESOLVED FIXED

[Mark Pilgrim (Google)]

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.

[Mark Pilgrim (Google)]

Created attachment 102290 [details]
Patch

[Adam Barth]

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

Created attachment 102300 [details]
Patch

[Mark Pilgrim (Google)]

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 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 on attachment 102300 [details]
Patch

You decided not to add the test?

[WebKit Review Bot]

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

Created attachment 102702 [details]
Patch

[WebKit Review Bot]

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

Created attachment 102819 [details]
Patch

[Mark Pilgrim (Google)]

Fixed failing tests that were relying on looser required-arguments behavior (that Adam says we should make stricter).

[Adam Barth]

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
> [phone]  Mark Pilgrim  <[e-mail]>
> +
> +        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)]

I'm confused; why is this commit-queue-?

[Adam Barth]

(In reply to comment #14)
> I'm confused; why is this commit-queue-?

You included two copies of your ChangeLog entry.

[Mark Pilgrim (Google)]

Created attachment 102840 [details]
Patch

[WebKit Review Bot]

Comment on attachment 102840 [details]
Patch

Clearing flags on attachment: 102840

Committed r92327: <http://trac.webkit.org/changeset/92327>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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]

canPlayType is inherited from HTMLMediaElement and that takes one argument, which used to be optional for HTMLVideoElement.

[Philippe Normand]

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

(In reply to comment #21)
> 
> maybe we should also remove the canPlayType() line

This seems like the correct fix.

[Ryosuke Niwa]

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]

Oops, apparently the author never came back to this bug :(  Didn't mean to criticize Philippe or Eric here.

[Ryosuke Niwa]

Sorry about the confusion, Philippe and Eric.

I'm going to send my angry email to Mark instead now.

[Adam Barth]

I've landed http://trac.webkit.org/changeset/92455 in attempt to fix these issues.