[webkit-reviews] review granted: [Bug 200427] WebRTC: got incorrect `this` in negotiationneeded event : [Attachment 375509] 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 200427: WebRTC: got incorrect `this` in negotiationneeded event
https://bugs.webkit.org/show_bug.cgi?id=200427

Attachment 375509: Patch

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




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

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

> Source/WebCore/bindings/js/JSDOMBuiltinConstructor.h:95
> +typename JSC::JSObject&
createJSObjectFromWrapper(JSDOMBuiltinConstructor<JSClass>& constructor,
Ref<typename JSClass::DOMWrapped>&& domObject)

Not a big fan of names with "dom" in them. I’d call this "wrappedObject"
instead of "domObject" if I was writing the code.

> Source/WebCore/bindings/js/JSDOMBuiltinConstructor.h:101
> +    auto* domObjectPtr = domObject.ptr();
> +    auto wrapper =
JSClass::create(getDOMStructure<JSClass>(globalObject.vm(), globalObject),
&globalObject, WTFMove(domObject));
> +    cacheWrapper(globalObject.world(), domObjectPtr, wrapper);
> +    return *wrapper;

This looks so identical to createWrapper from JSDOMWrapperCache.h. There’s got
to be a way to factor so they share most of their code.

> Source/WebCore/bindings/js/JSDOMBuiltinConstructor.h:104
> +template<typename JSClass> inline

Seems unnecessary that all of these function templates say "inline". I can’t
imagine this has any effect one way or another on whether they are inlined nor
on whether they are safe to include in a header.