Bug 65353 – Remove LegacyDefaultOptionalArguments flag from DOMWindow.idl

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[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 DOMWindow.idl. 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.

(Note: this required a small change to the code generator to allow [RequireAllArguments] without the "=Raise" parameter, as required by scrollBy, scrollTo, scroll, moveBy, moveTo, resizeBy, and resizeTo methods.)

[Mark Pilgrim (Google)]

Created attachment 102322 [details]
Patch

[Darin Adler]

Comment on attachment 102322 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102322&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1983
>                  if ($dataNode->extendedAttributes->{"LegacyDefaultOptionalArguments"}) {
>                      $requiresAllArguments = $function->signature->extendedAttributes->{"RequiresAllArguments"};
>                  } else {
> -                    $requiresAllArguments = "Raise";
> +                    $requiresAllArguments = $function->signature->extendedAttributes->{"RequiresAllArguments"} || "Raise";
>                  }

There is probably a cleaner way to write this that doesn’t repeat the expression function->signature->extendedAttributes->{"RequiresAllArguments"}.

> Source/WebCore/page/DOMWindow.idl:74
> +        [Custom] DOMWindow open(in [Optional=CallWithDefaultValue] DOMString url,

I’m curious, what does “call with default value” mean? What is the default value?

[Mark Pilgrim (Google)]

(In reply to comment #2)
> (From update of attachment 102322 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102322&action=review
> 
> There is probably a cleaner way to write this that doesn’t repeat the expression function->signature->extendedAttributes->{"RequiresAllArguments"}.

After a few more patches in the coming few days, we will be removing the LegacyDefaultOptionalArguments altogether, so I'd argue it's not that big a deal.

> 
> > Source/WebCore/page/DOMWindow.idl:74
> > +        [Custom] DOMWindow open(in [Optional=CallWithDefaultValue] DOMString url,
> 
> I’m curious, what does “call with default value” mean? What is the default value?

It varies by type.

[Adam Barth]

> (From update of attachment 102322 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102322&action=review
> 
> > Source/WebCore/page/DOMWindow.idl:74
> > +        [Custom] DOMWindow open(in [Optional=CallWithDefaultValue] DOMString url,
> 
> I’m curious, what does “call with default value” mean? What is the default value?

Basically, it's whatever undefined maps to for the given type.  In the case of DOMString, I think that's the null string.  For pointer types, it ends up as zero.  "Optional" (without the value) uses C++ overloading to dispatch the different calls.  We struggled a bit to come up with an appropriate name.  If you've got a better suggestion, we're all ears.  :)

[Adam Barth]

Comment on attachment 102322 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102322&action=review

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1983
>>                  if ($dataNode->extendedAttributes->{"LegacyDefaultOptionalArguments"}) {
>>                      $requiresAllArguments = $function->signature->extendedAttributes->{"RequiresAllArguments"};
>>                  } else {
>> -                    $requiresAllArguments = "Raise";
>> +                    $requiresAllArguments = $function->signature->extendedAttributes->{"RequiresAllArguments"} || "Raise";
>>                  }
> 
> There is probably a cleaner way to write this that doesn’t repeat the expression function->signature->extendedAttributes->{"RequiresAllArguments"}.

I would just address this comment.  It's easy to fix.  :)

> Source/WebCore/page/DOMWindow.idl:205
> -        [EnabledAtRuntime=FileSystem] void webkitRequestFileSystem(in unsigned short type, in long long size, in [Callback, Optional] FileSystemCallback successCallback, in [Callback, Optional] ErrorCallback errorCallback);
> -        [EnabledAtRuntime=FileSystem] void webkitResolveLocalFileSystemURL(in DOMString url, in [Callback, Optional] EntryCallback successCallback, in [Callback, Optional] ErrorCallback errorCallback);
> +        [EnabledAtRuntime=FileSystem] void webkitRequestFileSystem(in [Optional=CallWithDefaultValue] unsigned short type, 
> +                                                                   in [Optional=CallWithDefaultValue] long long size, 
> +                                                                   in [Callback, Optional] FileSystemCallback successCallback, 
> +                                                                   in [Callback, Optional] ErrorCallback errorCallback);
> +        [EnabledAtRuntime=FileSystem] void webkitResolveLocalFileSystemURL(in [Optional=CallWithDefaultValue] DOMString url, 
> +                                                                           in [Callback, Optional] EntryCallback successCallback, 
> +                                                                           in [Callback, Optional] ErrorCallback errorCallback);

I'd skip these.  FileSystem is pretty new.

> Source/WebCore/page/DOMWindow.idl:250
>  #if defined(ENABLE_REQUEST_ANIMATION_FRAME)
>          // WebKit animation extensions
>          long webkitRequestAnimationFrame(in [Callback] RequestAnimationFrameCallback callback, in Element element);
> -        void webkitCancelRequestAnimationFrame(in long id);
> +        void webkitCancelRequestAnimationFrame(in [Optional=CallWithDefaultValue] long id);
>  #endif

This API is already pretty new.  I'd skip it.

[Mark Pilgrim (Google)]

Created attachment 102808 [details]
Patch

[Mark Pilgrim (Google)]

Skipped FileSystem and animationFrame APIs. Refactored code generators as per Darin's comment.

[WebKit Review Bot]

Comment on attachment 102808 [details]
Patch

Clearing flags on attachment: 102808

Committed r92313: <http://trac.webkit.org/changeset/92313>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.