[webkit-reviews] review granted: [Bug 67213] Refactor JS dictionary code into helper class and covert geolocation code to use it : [Attachment 105657] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 67213: Refactor JS dictionary code into helper class and covert geolocation
code to use it
https://bugs.webkit.org/show_bug.cgi?id=67213

Attachment 105657: Patch
https://bugs.webkit.org/attachment.cgi?id=105657&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.