[webkit-reviews] review denied: [Bug 65338] Remove LegacyDefaultOptionalArguments flag from HTML DOM IDL files : [Attachment 102290] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Mark Pilgrim <pilgrim at chromium.org>'s
request for review:
Bug 65338: Remove LegacyDefaultOptionalArguments flag from HTML DOM IDL files
https://bugs.webkit.org/show_bug.cgi?id=65338

Attachment 102290: Patch
https://bugs.webkit.org/attachment.cgi?id=102290&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.