Bug 157721 – Use more references in JS wrappers related code

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Use more references in JS wrappers related code. Also avoid some refcounting churn when using toJSNewlyCreated().

[Chris Dumez]

Created attachment 278968 [details]
Patch

[Chris Dumez]

Created attachment 278969 [details]
Patch

[Build Bot]

Comment on attachment 278969 [details]
Patch

Attachment 278969 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1326082

New failing tests:
http/tests/performance/performance-resource-timing-cached-entries.html

[Build Bot]

Created attachment 278970 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Chris Dumez]

(In reply to comment #3)
> Comment on attachment 278969 [details]
> Patch
> 
> Attachment 278969 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: http://webkit-queues.webkit.org/results/1326082
> 
> New failing tests:
> http/tests/performance/performance-resource-timing-cached-entries.html

I don't think this is related. It did the same thing on https://bugs.webkit.org/show_bug.cgi?id=157659#c13

[Chris Dumez]

Created attachment 278972 [details]
Patch

[Chris Dumez]

Attempt to fix the Window build.

[Darin Adler]

Comment on attachment 278969 [details]
Patch

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

I made some comments in this older version of the patch. I will continue reviewing the newer one.

> Source/WebCore/bindings/js/JSAudioContextCustom.cpp:111
> +    return JSValue::encode(CREATE_DOM_WRAPPER_FROM_REF(jsConstructor->globalObject(), AudioContext, audioContext.releaseNonNull()));

Unfortunate that this requires a different macro. As you know, I prefer to use overloading when we can instead. Also, do we really need to abbreviate reference as REF?

> Source/WebCore/bindings/js/JSDocumentCustom.cpp:94
> +    auto* windowGlobalObject = toJSDOMWindow(toJS(&state, *window));
> +    // Creating a wrapper for domWindow might have created a wrapper for document as well.
> +    return getCachedWrapper(windowGlobalObject->world(), document);

Not sure this code benefits from using a local variable.

> Source/WebCore/bindings/js/JSDocumentCustom.cpp:100
> +    for (Node* node = &document; node; node = NodeTraversal::next(*node))

Could use children iterator instead of calling NodeTraversal directly.

> Source/WebCore/bindings/js/JSDocumentCustom.h:41
> +JSC::JSObject* cachedDocumentWrapper(JSC::ExecState&, JSDOMGlobalObject&, Document&);

Type can’t be more specific than JSObject*?

> Source/WebCore/bindings/js/JSElementCustom.cpp:54
> +    if (is<HTMLElement>(element.get()))

Should we overload is<> for all the smart pointer and reference types so we don’t need to do the .get() in cases like this? Probably not, but just wondered.

[Darin Adler]

Comment on attachment 278972 [details]
Patch

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

I put a few of my comments on an earlier version of this patch.

There are a few more get() calls than I expected in this patch. Maybe some of them could have been omitted?

All those new CustomToJSObject seem a little unfortunate.

> Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:61
> +    // Make sure the document is kept around by the window object, and works right with the back/forward cache.

I find the original of this comment confusing, and it’s just as confusing here. I don’t see the connection between calling reportMemoryForFramelessDocument and ensuring those two things.

Also seems like reportMemoryForFramelessDocument could include the frame null check given its name. Would make it harder to “call wrong”. If we need to inline the frame null check we could still do that.

Maybe the comment should go in reportMemoryForFramelessDocument instead of here. If we made these changes then this would be a slightly tighter function because we wouldn’t need if statements or blank lines.

> Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:73
> +    if (auto* wrapper = cachedDocumentWrapper(*state, *globalObject, document))
> +        return wrapper;
> +
> +    return createNewHTMLDocumentWrapper(*state, *globalObject, document);

Would read slightly nicer without the blank line.

> Source/WebCore/bindings/js/JSHTMLTemplateElementCustom.cpp:48
> +    DocumentFragment& content = wrapped().content();

Maybe auto& instead?

> Source/WebCore/bindings/js/JSStyleSheetCustom.cpp:46
> -    if (JSC::JSObject* wrapper = getCachedWrapper(globalObject->world(), &styleSheet))
> +    if (auto* wrapper = getCachedWrapper(globalObject->world(), styleSheet))
>          return wrapper;
>  
>      if (styleSheet.isCSSStyleSheet())
> -        return CREATE_DOM_WRAPPER(globalObject, CSSStyleSheet, &styleSheet);
> +        return CREATE_DOM_WRAPPER(globalObject, CSSStyleSheet, styleSheet);
>  
> -    return CREATE_DOM_WRAPPER(globalObject, StyleSheet, &styleSheet);
> +    return CREATE_DOM_WRAPPER(globalObject, StyleSheet, styleSheet);

Not sure you’d agree, but I think this would read better without the blank lines.

> Source/WebCore/bindings/js/JSTextCustom.cpp:47
> +    if (auto* wrapper = getCachedWrapper(globalObject->world(), text))
> +        return wrapper;
> +
> +    return createNewTextWrapper(*globalObject, text);

Ditto.

> Source/WebCore/bindings/js/JSXMLDocumentCustom.cpp:38
> +static inline JSValue createNewXMLDocumentWrapper(ExecState& state, JSDOMGlobalObject& globalObject, Ref<XMLDocument>&& passedDocument)

Sure would be nice to be able to generate more of this instead of having so many copies written out.

> Source/WebCore/bindings/js/JSXMLDocumentCustom.cpp:49
> +    // Make sure the document is kept around by the window object, and works right with the back/forward cache.
> +    if (!document.frame())
> +        reportMemoryForFramelessDocument(state, document);

Same comment as earlier.

> Source/WebCore/bindings/js/JSXMLDocumentCustom.cpp:59
> +    if (auto* wrapper = cachedDocumentWrapper(*state, *globalObject, document))
> +        return wrapper;
> +
> +    return createNewXMLDocumentWrapper(*state, *globalObject, document);

Ditto.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1491
> +        push(@headerContent, "inline JSC::JSValue toJSNewlyCreated(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, RefPtr<$implType>&& impl) { return impl ? toJSNewlyCreated(exec, globalObject, impl.releaseNonNull()) : JSC::jsNull(); }\n");

Why did you change the name from state to exec here? I think I changed it in the other direction earlier. Are you perhaps reserving the name state for only references, not pointers?

[Chris Dumez]

(In reply to comment #8)
> Comment on attachment 278969 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278969&action=review
> 
> I made some comments in this older version of the patch. I will continue
> reviewing the newer one.
> 
> > Source/WebCore/bindings/js/JSAudioContextCustom.cpp:111
> > +    return JSValue::encode(CREATE_DOM_WRAPPER_FROM_REF(jsConstructor->globalObject(), AudioContext, audioContext.releaseNonNull()));
> 
> Unfortunate that this requires a different macro. As you know, I prefer to
> use overloading when we can instead. Also, do we really need to abbreviate
> reference as REF?

I used the REF because I thought it made it clearer it is to be used with a Ref<> (i.e. rather than a C++ reference).

I do agree that it'd be nice to not have 2 versions of this macro. I tried to use only one but could not get it working. I'll give it another shot.

[Chris Dumez]

(In reply to comment #9)
> Comment on attachment 278972 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278972&action=review
> 
> I put a few of my comments on an earlier version of this patch.
> 
> There are a few more get() calls than I expected in this patch. Maybe some
> of them could have been omitted?

You're probably referring to the calls to getCachedWrapper(), the compiler was complaining without because it is declared like so:
template<typename DOMClass> inline JSC::JSObject* getCachedWrapper(DOMWrapperWorld& world, DOMClass& domObject);

And so DOMClass would be resolved as Ref<X> instead of X.

> All those new CustomToJSObject seem a little unfortunate.
> > Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:61
> > +    // Make sure the document is kept around by the window object, and works right with the back/forward cache.
> 
> I find the original of this comment confusing, and it’s just as confusing
> here. I don’t see the connection between calling
> reportMemoryForFramelessDocument and ensuring those two things.
> 
> Also seems like reportMemoryForFramelessDocument could include the frame
> null check given its name. Would make it harder to “call wrong”. If we need
> to inline the frame null check we could still do that.
> 
> Maybe the comment should go in reportMemoryForFramelessDocument instead of
> here. If we made these changes then this would be a slightly tighter
> function because we wouldn’t need if statements or blank lines.
> 
> > Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:73
> > +    if (auto* wrapper = cachedDocumentWrapper(*state, *globalObject, document))
> > +        return wrapper;
> > +
> > +    return createNewHTMLDocumentWrapper(*state, *globalObject, document);
> 
> Would read slightly nicer without the blank line.
> 
> > Source/WebCore/bindings/js/JSHTMLTemplateElementCustom.cpp:48
> > +    DocumentFragment& content = wrapped().content();
> 
> Maybe auto& instead?
> 
> > Source/WebCore/bindings/js/JSStyleSheetCustom.cpp:46
> > -    if (JSC::JSObject* wrapper = getCachedWrapper(globalObject->world(), &styleSheet))
> > +    if (auto* wrapper = getCachedWrapper(globalObject->world(), styleSheet))
> >          return wrapper;
> >  
> >      if (styleSheet.isCSSStyleSheet())
> > -        return CREATE_DOM_WRAPPER(globalObject, CSSStyleSheet, &styleSheet);
> > +        return CREATE_DOM_WRAPPER(globalObject, CSSStyleSheet, styleSheet);
> >  
> > -    return CREATE_DOM_WRAPPER(globalObject, StyleSheet, &styleSheet);
> > +    return CREATE_DOM_WRAPPER(globalObject, StyleSheet, styleSheet);
> 
> Not sure you’d agree, but I think this would read better without the blank
> lines.
> 
> > Source/WebCore/bindings/js/JSTextCustom.cpp:47
> > +    if (auto* wrapper = getCachedWrapper(globalObject->world(), text))
> > +        return wrapper;
> > +
> > +    return createNewTextWrapper(*globalObject, text);
> 
> Ditto.
> 
> > Source/WebCore/bindings/js/JSXMLDocumentCustom.cpp:38
> > +static inline JSValue createNewXMLDocumentWrapper(ExecState& state, JSDOMGlobalObject& globalObject, Ref<XMLDocument>&& passedDocument)
> 
> Sure would be nice to be able to generate more of this instead of having so
> many copies written out.
> 
> > Source/WebCore/bindings/js/JSXMLDocumentCustom.cpp:49
> > +    // Make sure the document is kept around by the window object, and works right with the back/forward cache.
> > +    if (!document.frame())
> > +        reportMemoryForFramelessDocument(state, document);
> 
> Same comment as earlier.
> 
> > Source/WebCore/bindings/js/JSXMLDocumentCustom.cpp:59
> > +    if (auto* wrapper = cachedDocumentWrapper(*state, *globalObject, document))
> > +        return wrapper;
> > +
> > +    return createNewXMLDocumentWrapper(*state, *globalObject, document);
> 
> Ditto.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1491
> > +        push(@headerContent, "inline JSC::JSValue toJSNewlyCreated(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, RefPtr<$implType>&& impl) { return impl ? toJSNewlyCreated(exec, globalObject, impl.releaseNonNull()) : JSC::jsNull(); }\n");
> 
> Why did you change the name from state to exec here? I think I changed it in
> the other direction earlier. Are you perhaps reserving the name state for
> only references, not pointers?

Just an old habit, I'll use state.

[Chris Dumez]

(In reply to comment #8)
> Comment on attachment 278969 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278969&action=review
> 
> I made some comments in this older version of the patch. I will continue
> reviewing the newer one.
> 
> > Source/WebCore/bindings/js/JSAudioContextCustom.cpp:111
> > +    return JSValue::encode(CREATE_DOM_WRAPPER_FROM_REF(jsConstructor->globalObject(), AudioContext, audioContext.releaseNonNull()));
> 
> Unfortunate that this requires a different macro. As you know, I prefer to
> use overloading when we can instead. Also, do we really need to abbreviate
> reference as REF?
> 
> > Source/WebCore/bindings/js/JSDocumentCustom.cpp:94
> > +    auto* windowGlobalObject = toJSDOMWindow(toJS(&state, *window));
> > +    // Creating a wrapper for domWindow might have created a wrapper for document as well.
> > +    return getCachedWrapper(windowGlobalObject->world(), document);
> 
> Not sure this code benefits from using a local variable.
> 
> > Source/WebCore/bindings/js/JSDocumentCustom.cpp:100
> > +    for (Node* node = &document; node; node = NodeTraversal::next(*node))
> 
> Could use children iterator instead of calling NodeTraversal directly.

Aren't those only for element types?

[Darin Adler]

(In reply to comment #12)
> (In reply to comment #8)
> > > Source/WebCore/bindings/js/JSDocumentCustom.cpp:100
> > > +    for (Node* node = &document; node; node = NodeTraversal::next(*node))
> > 
> > Could use children iterator instead of calling NodeTraversal directly.
> 
> Aren't those only for element types?

Yes, I think you’re right.

[Chris Dumez]

Created attachment 278982 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 278982 [details]
Patch

Clearing flags on attachment: 278982

Committed r200934: <http://trac.webkit.org/changeset/200934>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.