Bug 65744 – Remove LegacyDefaultOptionalArguments flag from storage IDL files

• 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 all remaining IndexedDB and WebSQLDatabase-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. ONE EXCEPTION: it changes IDBTransaction.idl to make the objectStore() method require its single argument, as required by WebIDL and consistent with all the other changes we've made in the recent past to make IndexedDB as strict as WebIDL requires.

All existing tests pass.

[Mark Pilgrim (Google)]

Created attachment 103032 [details]
Patch

[Adam Barth]

Comment on attachment 103032 [details]
Patch

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

> Source/WebCore/storage/StorageInfo.idl:41
> -        [CallWith=ScriptExecutionContext] void queryUsageAndQuota(in unsigned short storageType, in [Callback, Optional] StorageInfoUsageCallback usageCallback, in [Callback, Optional] StorageInfoErrorCallback errorCallback);
> -        [CallWith=ScriptExecutionContext] void requestQuota(in unsigned short storageType, in unsigned long long newQuotaInBytes, in [Callback, Optional] StorageInfoQuotaCallback quotaCallback, in [Callback, Optional] StorageInfoErrorCallback errorCallback);
> +        [CallWith=ScriptExecutionContext] void queryUsageAndQuota(in [Optional=CallWithDefaultValue] unsigned short storageType, 
> +                                                                  in [Callback, Optional] StorageInfoUsageCallback usageCallback, 
> +                                                                  in [Callback, Optional] StorageInfoErrorCallback errorCallback);
> +        [CallWith=ScriptExecutionContext] void requestQuota(in [Optional=CallWithDefaultValue] unsigned short storageType, 
> +                                                            in [Optional=CallWithDefaultValue] unsigned long long newQuotaInBytes, 
> +                                                            in [Callback, Optional] StorageInfoQuotaCallback quotaCallback, 
> +                                                            in [Callback, Optional] StorageInfoErrorCallback errorCallback);

QUOTA is very new.  I wouldn't add the optional attributes.

[Erik Arvidsson]

Comment on attachment 103032 [details]
Patch

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

> Source/WebCore/storage/Storage.idl:42
> +        [DontEnum, ConvertNullStringTo=Null] DOMString key(in [Optional=CallWithDefaultValue] unsigned long index);
> +        [DontEnum, ConvertNullStringTo=Null] DOMString getItem(in [Optional=CallWithDefaultValue] DOMString key);
> +        [DontEnum] void setItem(in [Optional=CallWithDefaultValue] DOMString key, 
> +                                in [Optional=CallWithDefaultValue] DOMString data) 
>              raises(DOMException);
> -        [DontEnum] void removeItem(in DOMString key);
> +        [DontEnum] void removeItem(in [Optional=CallWithDefaultValue] DOMString key);

I understand that this doesn't change the semantics but do we really want these to be optional?

[Adam Barth]

> I understand that this doesn't change the semantics but do we really want these to be optional?

Yeah, they make much more sense as non-optional.  The only question is the compatibility risk.

Do other browsers require these arguments?

[Erik Arvidsson]

Created attachment 113388 [details]
Patch

[Erik Arvidsson]

(In reply to comment #4)
> Do other browsers require these arguments?

I checked and all these arguments are required in Firefox. I can test IE9 too if you want.

[Adam Barth]

Comment on attachment 113388 [details]
Patch

Can we add those tests as LayoutTests?

[Erik Arvidsson]

IE9 allows skipping the arguments in Storage.idl even though it makes little sense in doing

localStorage.setItem()

I'm fine changing this to match IE9 but I feel like we are better of not allowing this.

I will add some tests.

[Erik Arvidsson]

Created attachment 113391 [details]
Patch

[Adam Barth]

Comment on attachment 113391 [details]
Patch

What about StorageInfo.idl ?  Would you be willing to add tests for those changes too.  Note: Quota is very new, so there's not much risk of breaking stuff.

[Erik Arvidsson]

Committed r99130: <http://trac.webkit.org/changeset/99130>