Bug 102156 – [V8] Simplify V8DOMWindowShell::getEntered

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

[V8] Simplify V8DOMWindowShell::getEntered

[Adam Barth]

Created attachment 174033 [details]
Patch

[Kentaro Hara]

Comment on attachment 174033 [details]
Patch

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

The change looks reasonable, but is a bit confusing. Sometimes you use 'V8DOMWindowShell::isolated(v8::Context::GetEntered())', sometimes you use 'v8::Context::InContext() && V8DOMWindowShell::isolated(v8::Context::GetEntered())', and sometimes you use 'DOMWrapperWorld::isolatedWorldsExist()) && v8::Context::InContext()'. To make it clear what kind of check should be used in each place, shall we make a well-named helper function to do the checks?

> Source/WebCore/bindings/v8/DOMDataStore.cpp:60
> +    V8DOMWindowShell* context = V8DOMWindowShell::isolated(v8::Context::GetEntered());

Nit: context => shell.

> Source/WebCore/bindings/v8/WorldContextHandle.cpp:-51
> -        if (!context.IsEmpty()) {

Is it OK to remove this check? (I don't fully understand WorldContextHandle.)

[Adam Barth]

> The change looks reasonable, but is a bit confusing. Sometimes you use 'V8DOMWindowShell::isolated(v8::Context::GetEntered())', sometimes you use 'v8::Context::InContext() && V8DOMWindowShell::isolated(v8::Context::GetEntered())',

Yeah, I'm skipping the v8::Context::InContext calls in locations where we know that V8 is on the stack.

> and sometimes you use 'DOMWrapperWorld::isolatedWorldsExist()) && v8::Context::InContext()'.

isolatedWorldsExist is just a performance optimization.  It's only needed in extremely performance sensitive code.

> To make it clear what kind of check should be used in each place, shall we make a well-named helper function to do the checks?

Sure.  Do you have thoughts on what to name these functions?

> > Source/WebCore/bindings/v8/DOMDataStore.cpp:60
> > +    V8DOMWindowShell* context = V8DOMWindowShell::isolated(v8::Context::GetEntered());
> 
> Nit: context => shell.
> 
> > Source/WebCore/bindings/v8/WorldContextHandle.cpp:-51
> > -        if (!context.IsEmpty()) {
> 
> Is it OK to remove this check? (I don't fully understand WorldContextHandle.)

Yes.  If we're in a context, it's impossible for the current context to be empty.

[Dan Carney]

This patch looks great, and I'd really like to build on it. Kentaro, any chance you want to take another look?

[Kentaro Hara]

Sorry for the delayed review.

(In reply to comment #3)
> > and sometimes you use 'DOMWrapperWorld::isolatedWorldsExist()) && v8::Context::InContext()'.
> 
> isolatedWorldsExist is just a performance optimization.  It's only needed in extremely performance sensitive code.
> 
> > To make it clear what kind of check should be used in each place, shall we make a well-named helper function to do the checks?
> 
> Sure.  Do you have thoughts on what to name these functions?

(1) How about implementing:

  V8IsolatedWorld* getEnteredIsolatedWorld() {
    if (!v8::Context::InContext())
      return 0;
    V8DOMWindowShell* shell = V8DOMWindowShell::isolated(v8::Context::GetEntered());
    if (!shell)
      return 0;
    return shell->world();
  }

and use getEnteredIsolatedWorld() as much as possible.

(2) At a performance-sensitive place, you can use 'DOMWrapperWorld::isolatedWorldsExist()) && v8::Context::InContext()' instead of getEnteredIsolatedWorld(). It would be better to add a comment about that.

(Context and IsolatedWorld are one of the most difficult concepts to understand. So I want to make what each code is doing clearer.)

[Adam Barth]

Created attachment 175060 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 175060 [details]
Patch

Still looks fine.  Appears you've answered haraken's concerns.  Your judgement as to if you feel you need to wait for him to see it again.

[Adam Barth]

I think we've gotten enough data from him that it's ok to land this and unblock bug 102158 and Dan's work.  Thanks for the review.

@haraken: If there's more you'd like me to change here, I'm happy to do more work in a followup patch.

[Kentaro Hara]

LGTM too:)

[Adam Barth]

Comment on attachment 175060 [details]
Patch

Clearing flags on attachment: 175060

Committed r135208: <http://trac.webkit.org/changeset/135208>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.