Bug 65353 - Remove LegacyDefaultOptionalArguments flag from DOMWindow.idl
Summary: Remove LegacyDefaultOptionalArguments flag from DOMWindow.idl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-28 18:51 PDT by Mark Pilgrim (Google)
Modified: 2011-08-03 14:51 PDT (History)
2 users (show)

See Also:


Attachments
Patch (11.30 KB, patch)
2011-07-28 18:53 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (9.11 KB, patch)
2011-08-03 12:19 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 18:51:58 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 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.)
Comment 1 Mark Pilgrim (Google) 2011-07-28 18:53:12 PDT
Created attachment 102322 [details]
Patch
Comment 2 Darin Adler 2011-07-28 18:59:46 PDT
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?
Comment 3 Mark Pilgrim (Google) 2011-07-28 19:17:42 PDT
(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.
Comment 4 Adam Barth 2011-07-28 22:38:24 PDT
> (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.  :)
Comment 5 Adam Barth 2011-07-29 10:50:05 PDT
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.
Comment 6 Mark Pilgrim (Google) 2011-08-03 12:19:51 PDT
Created attachment 102808 [details]
Patch
Comment 7 Mark Pilgrim (Google) 2011-08-03 12:20:46 PDT
Skipped FileSystem and animationFrame APIs. Refactored code generators as per Darin's comment.
Comment 8 WebKit Review Bot 2011-08-03 14:51:02 PDT
Comment on attachment 102808 [details]
Patch

Clearing flags on attachment: 102808

Committed r92313: <http://trac.webkit.org/changeset/92313>
Comment 9 WebKit Review Bot 2011-08-03 14:51:06 PDT
All reviewed patches have been landed.  Closing bug.