[webkit-reviews] review denied: [Bug 83472] Implement Dictionary.h on mac : [Attachment 136625] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Jon Lee <jonlee at apple.com>'s
request for review:
Bug 83472: Implement Dictionary.h on mac
https://bugs.webkit.org/show_bug.cgi?id=83472

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
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.)