[webkit-reviews] review granted: [Bug 204879] Do not use DOMGuarded for maplike : [Attachment 384995] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 204879: Do not use DOMGuarded for maplike
https://bugs.webkit.org/show_bug.cgi?id=204879

Attachment 384995: Patch

https://bugs.webkit.org/attachment.cgi?id=384995&action=review




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 384995
  --> https://bugs.webkit.org/attachment.cgi?id=384995
Patch

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

> Source/WebCore/bindings/js/JSDOMMapLike.cpp:39
> +    if (backingMap.isUndefined()) {

I would like this better with an early exit:

    if (!undefined)
	// less code

> Source/WebCore/bindings/js/JSDOMMapLike.cpp:52
> +void clearBackingMap(JSC::JSGlobalObject& lexicalGlobalObject,
JSC::JSObject& backingMap)

Could we refactor this so clearBackingMap and setBackingMap share more code? I
think we could make a helper function that takes a property name and a argument
buffer and does all the work.

> Source/WebCore/bindings/js/JSDOMMapLike.cpp:56
> +    ASSERT(!function.isUndefined());

What makes this a safe assertion? I don’t see how we can guarantee this. But
also, I don’t think we need to assert it since getCallData can handle
undefined.

> Source/WebCore/bindings/js/JSDOMMapLike.cpp:60
> +    ASSERT(callType != JSC::CallType::None);

What makes this a safe assertion? I don’t see how we can guarantee this.

> Source/WebCore/bindings/js/JSDOMMapLike.cpp:69
> +    ASSERT(!function.isUndefined());

What makes this a safe assertion? I don’t see how we can guarantee this. But
also, I don’t think we need to assert it since getCallData can handle
undefined.

> Source/WebCore/bindings/js/JSDOMMapLike.cpp:73
> +    ASSERT(callType != JSC::CallType::None);

What makes this a safe assertion? I don’t see how we can guarantee this.

> Source/WebCore/bindings/js/JSDOMMapLike.h:56
> +    template<class IDLKeyType, class IDLValueType> void set(typename
IDLKeyType::ParameterType, typename IDLValueType::ParameterType);

I personally prefer typename to class for the template arguments.

> Source/WebCore/bindings/js/JSDOMMapLike.h:70
> +template<class IDLKeyType, class IDLValueType>

Ditto.

> Source/WebCore/bindings/js/JSDOMMapLike.h:71
> +inline void DOMMapAdapter::set(typename IDLKeyType::ParameterType key,
typename IDLValueType::ParameterType value)

I don’t think we need inline here. It probably does not affect whether this is
inlined or not, and outside of that the main function of "inline" is to make it
safe to have something in the header, which is already the case for a function
template.

> Source/WebCore/bindings/js/JSDOMMapLike.h:79
> +inline void DOMMapAdapter::clear()

Why should this be inline and in the header?

> Source/WebCore/bindings/js/JSDOMMapLike.h:84
> +template<typename WrapperClass> inline JSC::JSObject&
getAndInitializeBackingMap(JSC::JSGlobalObject& lexicalGlobalObject,
WrapperClass& mapLike)

Don’t think we need inline here (same reason as above>.

> Source/WebCore/bindings/js/JSDOMMapLike.h:94
>  template<typename WrapperClass> inline JSC::JSValue
forwardSizeToMapLike(JSC::JSGlobalObject& lexicalGlobalObject, WrapperClass&
mapLike)

Don’t think we need inline here (same reason as above>. Same for the other
functions.