V8DOMWrapper.{h,cpp} has getWrapper(), getWrapperSlow(), getExistingWrapper(), getExistingWrapperSlow() and getExistingWrapperInline(), which are confusing to track call-paths. We can refactor them and unify them into getWrapper().
Created attachment 131327 [details] Patch
Comment on attachment 131327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131327&action=review > Source/WebCore/bindings/v8/V8IsolatedContext.h:-89 > - return reinterpret_cast<V8IsolatedContext*>(getGlobalObject(v8::Context::GetEntered())->GetPointerFromInternalField(V8DOMWindow::enteredIsolatedWorldIndex)); Actually I didn't intend to make getEntered() non-inline. I had to made it non-inline because V8IsolatedContext.h cannot include "V8DOMWindow.h" to avoid recursive header including and thus cannot use V8DOMWindow::enteredIsolatedWorldIndex. Is there any solution to keep getEntered() inline?
Comment on attachment 131327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131327&action=review >> Source/WebCore/bindings/v8/V8IsolatedContext.h:-89 >> - return reinterpret_cast<V8IsolatedContext*>(getGlobalObject(v8::Context::GetEntered())->GetPointerFromInternalField(V8DOMWindow::enteredIsolatedWorldIndex)); > > Actually I didn't intend to make getEntered() non-inline. I had to made it non-inline because V8IsolatedContext.h cannot include "V8DOMWindow.h" to avoid recursive header including and thus cannot use V8DOMWindow::enteredIsolatedWorldIndex. Is there any solution to keep getEntered() inline? You could split it into 2 parts so that at least the common case is inlined
Comment on attachment 131327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131327&action=review > Source/WebCore/ChangeLog:13 > + No tests. No change in behavior. Have you run the performance tests? I need to read this carefully, but I'm distracted by gardening. Other folks should feel free to review if they like. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:438 > ALWAYS_INLINE v8::Handle<v8::Object> ${className}::existingWrapper(${nativeType}* impl) > { > END > - my $getWrapper = IsNodeSubType($dataNode) ? "V8DOMWrapper::getExistingWrapper(impl)" : "${domMapFunction}.get(impl)"; > + my $getWrapper = IsNodeSubType($dataNode) ? "V8DOMWrapper::getWrapper(impl)" : "${domMapFunction}.get(impl)"; Doesn't this change semantics?
Created attachment 131470 [details] WIP
(In reply to comment #4) > > Source/WebCore/ChangeLog:13 > > + No tests. No change in behavior. > > Have you run the performance tests? I confirmed no performance regression on DOM core tests in DROMAEO. > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:438 > > ALWAYS_INLINE v8::Handle<v8::Object> ${className}::existingWrapper(${nativeType}* impl) > > { > > END > > - my $getWrapper = IsNodeSubType($dataNode) ? "V8DOMWrapper::getExistingWrapper(impl)" : "${domMapFunction}.get(impl)"; > > + my $getWrapper = IsNodeSubType($dataNode) ? "V8DOMWrapper::getWrapper(impl)" : "${domMapFunction}.get(impl)"; > > Doesn't this change semantics? No. This patch just refactors code so that it does not change any call path. > I need to read this carefully, but I'm distracted by gardening. Other folks should feel free to review if they like. Maybe this patch is doing too many things at a breath, and it is not clear if the patch does not change the semantics. Let me land this patch step by step.
All intended refactoring has been completed.