Bug 83472 – Implement Dictionary.h on mac

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Jon Lee]

As opposed to JSDictionary used for generated bindings, Dictionary exposes JS dictionaries to WebCore objects. The interface differs between the two, so we can't have JSDictionary work for both without having a confusing interface, and also exposing unneeded information to the WebCore layer.

[Radar WebKit Bug Importer]

<rdar://problem/11209766>

[Jon Lee]

Created attachment 136625 [details]
Patch

[Build Bot]

Comment on attachment 136625 [details]
Patch

Attachment 136625 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12384652

[Kentaro Hara]

Comment on attachment 136625 [details]
Patch

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

> Source/WebCore/bindings/js/Dictionary.h:42
> +    Dictionary()

Is this constructor needed?

> Source/WebCore/bindings/js/Dictionary.h:45
> +    { }

Nit: '{' and '}' for each line, please

> Source/WebCore/bindings/js/Dictionary.h:50
> +    { }

Nit: Ditto.

> Source/WebCore/bindings/js/Dictionary.h:68
> +    if (!m_isValid)
> +        return false;

Maybe we want to move this check to JSDictionary.h, like this:

bool JSDictionary::tryGetProperty(...) {
    if (!m_initializerObject)
        return false;
    ...;
}

Then you can remove m_isValid from Dictionary.h.

> Source/WebCore/bindings/js/Dictionary.h:71
> +    bool noExceptions = m_dictionary.tryGetProperty(propertyName.ascii().data(), result, getPropertyResult);

Why do we need both 'getPropertyResult' and 'noExceptions'? Isn't it possible to have tryGetProperty return 'getPropertyResult' as a return value? (If the return value is 'ExceptionThrown', it means that there is an exception.)

[Jon Lee]

Comment on attachment 136625 [details]
Patch

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

>> Source/WebCore/bindings/js/Dictionary.h:42
>> +    Dictionary()
> 
> Is this constructor needed?

The existence of the default constructor in the v8 implementation led me to believe this was required. Obviously nobody is using Dictionary right now except IDB, so this could be removed until needed.

>> Source/WebCore/bindings/js/Dictionary.h:50
>> +    { }
> 
> Nit: Ditto.

I didn't think inline empty functions or constructors required this. The indenting for initializing the variables is wrong though.

>> Source/WebCore/bindings/js/Dictionary.h:68
>> +        return false;
> 
> Maybe we want to move this check to JSDictionary.h, like this:
> 
> bool JSDictionary::tryGetProperty(...) {
>     if (!m_initializerObject)
>         return false;
>     ...;
> }
> 
> Then you can remove m_isValid from Dictionary.h.

I think I might have to expose the exec state now that i'm trying to use it with notifications, so it's possible i can do the check that way and avoid adding this variable.

>> Source/WebCore/bindings/js/Dictionary.h:71
>> +    bool noExceptions = m_dictionary.tryGetProperty(propertyName.ascii().data(), result, getPropertyResult);
> 
> Why do we need both 'getPropertyResult' and 'noExceptions'? Isn't it possible to have tryGetProperty return 'getPropertyResult' as a return value? (If the return value is 'ExceptionThrown', it means that there is an exception.)

I didn't want to change the existing signatures in JSDictionary because the bindings generation code relies on it, and I didn't want this bug to include having to completely overhaul that. I was afraid about making the API for JSDictionary inconsistent and confusing, but I think I found a way to make it work.

This patch is also missing new expected results for the bindings tests, which I will include in a forthcoming patch.

[Kentaro Hara]

(In reply to comment #5)
> >> Source/WebCore/bindings/js/Dictionary.h:50
> >> +    { }
> > 
> > Nit: Ditto.
> 
> I didn't think inline empty functions or constructors required this. The indenting for initializing the variables is wrong though.

Nit: Please look at the coding style guide ("Other punctuations" 1.): http://www.webkit.org/coding/coding-style.html

I think whether inline or not does not matter.

[Jon Lee]

Created attachment 136852 [details]
Patch

[Kentaro Hara]

Comment on attachment 136852 [details]
Patch

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

> Source/WebCore/bindings/js/JSDictionary.h:120
> +bool JSDictionary::valueForProperty(const char* propertyName, Result& finalResult) const

Nit: Maybe this can be renamed to JSDictionary::get(), for naming consistency with Dictionary::get(). It's up to you.

[Jon Lee]

Committed r114001: <http://trac.webkit.org/changeset/114001>