Summary: | Remove LegacyDefaultOptionalArguments flag from DOMWindow.idl | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Pilgrim (Google) <pilgrim> | ||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Mark Pilgrim (Google)
2011-07-28 18:51:58 PDT
Created attachment 102322 [details]
Patch
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? (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. > (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 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. Created attachment 102808 [details]
Patch
Skipped FileSystem and animationFrame APIs. Refactored code generators as per Darin's comment. Comment on attachment 102808 [details] Patch Clearing flags on attachment: 102808 Committed r92313: <http://trac.webkit.org/changeset/92313> All reviewed patches have been landed. Closing bug. |