Bug 80841 - [V8] Refactor V8 wrapping methods
: [V8] Refactor V8 wrapping methods
Status: RESOLVED FIXED
: WebKit
WebCore JavaScript
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 80913 80916
:
  Show dependency treegraph
 
Reported: 2012-03-12 08:23 PST by
Modified: 2012-03-12 19:27 PST (History)


Attachments
Patch (12.27 KB, patch)
2012-03-12 08:32 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff
WIP (12.19 KB, patch)
2012-03-12 17:32 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-12 08:23:44 PST
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().
------- Comment #1 From 2012-03-12 08:32:16 PST -------
Created an attachment (id=131327) [details]
Patch
------- Comment #2 From 2012-03-12 08:35:45 PST -------
(From update of attachment 131327 [details])
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 #3 From 2012-03-12 10:25:42 PST -------
(From update of attachment 131327 [details])
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 #4 From 2012-03-12 14:26:13 PST -------
(From update of attachment 131327 [details])
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?
------- Comment #5 From 2012-03-12 17:32:18 PST -------
Created an attachment (id=131470) [details]
WIP
------- Comment #6 From 2012-03-12 17:35:20 PST -------
(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.
------- Comment #7 From 2012-03-12 19:27:05 PST -------
All intended refactoring has been completed.