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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Apr 10 23:48:26 PDT 2012

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

Attachment 136625: Patch

------- 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,

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.)

More information about the webkit-reviews mailing list