Bug 65338 - Remove LegacyDefaultOptionalArguments flag from HTML DOM IDL files
Summary: Remove LegacyDefaultOptionalArguments flag from HTML DOM IDL files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-28 13:07 PDT by Mark Pilgrim (Google)
Modified: 2011-08-05 00:31 PDT (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Pilgrim (Google) 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.
Comment 1 Mark Pilgrim (Google) 2011-07-28 13:11:36 PDT
Created attachment 102290 [details]
Patch
Comment 2 Adam Barth 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.
Comment 3 Mark Pilgrim (Google) 2011-07-28 14:30:23 PDT
Created attachment 102300 [details]
Patch
Comment 4 Mark Pilgrim (Google) 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.
Comment 5 Adam Barth 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.
Comment 6 Adam Barth 2011-08-02 13:47:46 PDT
Comment on attachment 102300 [details]
Patch

You decided not to add the test?
Comment 7 WebKit Review Bot 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
Comment 8 Mark Pilgrim (Google) 2011-08-02 14:57:28 PDT
Created attachment 102702 [details]
Patch
Comment 9 WebKit Review Bot 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
Comment 10 WebKit Review Bot 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
Comment 11 Mark Pilgrim (Google) 2011-08-03 13:14:12 PDT
Created attachment 102819 [details]
Patch
Comment 12 Mark Pilgrim (Google) 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).
Comment 13 Adam Barth 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!
Comment 14 Mark Pilgrim (Google) 2011-08-03 14:45:46 PDT
I'm confused; why is this commit-queue-?
Comment 15 Adam Barth 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.
Comment 16 Mark Pilgrim (Google) 2011-08-03 15:29:36 PDT
Created attachment 102840 [details]
Patch
Comment 17 WebKit Review Bot 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>
Comment 18 WebKit Review Bot 2011-08-03 16:21:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Ryosuke Niwa 2011-08-03 18:32:53 PDT
canPlayType is inherited from HTMLMediaElement and that takes one argument, which used to be optional for HTMLVideoElement.
Comment 21 Philippe Normand 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
Comment 22 Eric Carlson 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.
Comment 23 Ryosuke Niwa 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.
Comment 24 Ryosuke Niwa 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.
Comment 25 Ryosuke Niwa 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.
Comment 26 Adam Barth 2011-08-05 00:31:47 PDT
I've landed http://trac.webkit.org/changeset/92455 in attempt to fix these issues.