[V8] Simplify V8DOMWindowShell::getEntered
Created attachment 174033 [details] Patch
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.)
> 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.
This patch looks great, and I'd really like to build on it. Kentaro, any chance you want to take another look?
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.)
Created attachment 175060 [details] Patch
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.
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.
LGTM too:)
Comment on attachment 175060 [details] Patch Clearing flags on attachment: 175060 Committed r135208: <http://trac.webkit.org/changeset/135208>
All reviewed patches have been landed. Closing bug.