Refactor JS dictionary code into helper class and covert geolocation code to use it

[Sam Weinig]

Refactor JS dictionary code into helper class and covert geolocation code to use it

[Sam Weinig]

Created attachment 105657 [details]
Patch

[Darin Adler]

Comment on attachment 105657 [details]
Patch

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

r=me, but I think this can be done even better

> Source/WebCore/bindings/js/JSDictionary.h:33
> +class JSDictionaryObject {

Why JSDictionaryObject instead of JSDictionary?

> Source/WebCore/bindings/js/JSDictionary.h:37
> +        , m_initializerObject(initializerObject)

What does “initializer object” mean here? I don’t quite follow.

> Source/WebCore/bindings/js/JSDictionary.h:42
> +    template <typename Result>
> +    bool tryGetProperty(const char* propertyName, Result& result);

I don’t think the argument name is needed here given the type name you chose.

> Source/WebCore/bindings/js/JSDictionary.h:78
> +template <typename Result>
> +bool JSDictionaryObject::tryGetProperty(const char* propertyName, Result& finalResult)
> +{
> +    JSC::Identifier identifier(m_exec, propertyName);
> +    JSC::PropertySlot slot(m_initializerObject);
> +    if (m_initializerObject->getPropertySlot(m_exec, identifier, slot)) {
> +        if (m_exec->hadException())
> +            return false;
> +
> +        JSC::JSValue value = slot.getValue(m_exec, identifier);
> +        if (m_exec->hadException())
> +            return false;
> +
> +        Result result;
> +        convertValue(m_exec, value, result);
> +
> +        if (m_exec->hadException())
> +            return false;
> +        
> +        finalResult = result;
> +    }
> +
> +    return true;
> +}

It’s a shame that this entire function is a template when only the call to convertValue is really type-specific. I suggest making a helper function to keep as much code as possible outside the template.

> Source/WebCore/bindings/js/JSDictionary.h:103
> +template <typename T, typename Result>
> +bool JSDictionaryObject::tryGetProperty(const char* propertyName, T* context, void (*setter)(T* context, const Result&))
> +{
> +    JSC::Identifier identifier(m_exec, propertyName);
> +    JSC::PropertySlot slot(m_initializerObject);
> +    if (m_initializerObject->getPropertySlot(m_exec, identifier, slot)) {
> +        if (m_exec->hadException())
> +            return false;
> +
> +        JSC::JSValue value = slot.getValue(m_exec, identifier);
> +        if (m_exec->hadException())
> +            return false;
> +
> +        Result result;
> +        convertValue(m_exec, value, result);
> +
> +        if (m_exec->hadException())
> +            return false;
> +        
> +        setter(context, result);
> +    }
> +
> +    return true;
> +}

Same comment here. It could probably even be the same helper function.

[Sam Weinig]

Thanks for the review, I agree with all your comments.  I was really just throwing this up here to make sure it built (while I got lunch), but that plan seems to have failed.  Will upload a new one with better everything.

[Sam Weinig]

Created attachment 105680 [details]
Patch

[Sam Weinig]

Committed r94119: <http://trac.webkit.org/changeset/94119>