Use more references in JS wrappers related code. Also avoid some refcounting churn when using toJSNewlyCreated().
Created attachment 278968 [details] Patch
Created attachment 278969 [details] Patch
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
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
(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
Created attachment 278972 [details] Patch
Attempt to fix the Window build.
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.
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?
(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.
(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.
(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?
(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.
Created attachment 278982 [details] Patch
Comment on attachment 278982 [details] Patch Clearing flags on attachment: 278982 Committed r200934: <http://trac.webkit.org/changeset/200934>
All reviewed patches have been landed. Closing bug.