To kill V8Proxy: - We can move matchesCurrentContext() from V8Proxy to ScriptController. - We can remove V8Proxy::isolatedWorldContext() since it is not used by anybody. - We can remove V8Proxy::finishedWithEvent() since it is empty.
Created attachment 159681 [details] Patch
Comment on attachment 159681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159681&action=review > Source/WebCore/bindings/v8/ScriptController.cpp:392 > +bool ScriptController::matchesCurrentContext() How is this different from v8::Context::GetCurrent() == contextForCurrentWorld() or I guess v8::Context::GetCurrent() == m_proxy->context() since the other patch hasn't landed yet?
Sorry to be asking all these questions, but we're getting to the really yucky functions in V8Proxy. We can move them around without changing them, but this is a good opportunity to understand them and clean them up.
Comment on attachment 159681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159681&action=review >> Source/WebCore/bindings/v8/ScriptController.cpp:392 >> +bool ScriptController::matchesCurrentContext() > > How is this different from v8::Context::GetCurrent() == contextForCurrentWorld() or I guess v8::Context::GetCurrent() == m_proxy->context() since the other patch hasn't landed yet? This is the code I wrote:) I introduced it in https://bugs.webkit.org/show_bug.cgi?id=82201 for perf optimization. Let me take a detailed look tomorrow. Thanks for review!
> This is the code I wrote:) Sorry, I didn't mean to impune your code! I just meant that it's using a pattern I introduced sometime around http://trac.webkit.org/changeset/46523 that I'm not proud of.
Comment on attachment 159681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159681&action=review >>> Source/WebCore/bindings/v8/ScriptController.cpp:392 >>> +bool ScriptController::matchesCurrentContext() >> >> How is this different from v8::Context::GetCurrent() == contextForCurrentWorld() or I guess v8::Context::GetCurrent() == m_proxy->context() since the other patch hasn't landed yet? > > This is the code I wrote:) I introduced it in https://bugs.webkit.org/show_bug.cgi?id=82201 for perf optimization. Let me take a detailed look tomorrow. Thanks for review! v8::Context::GetCurrent() == contextForCurrentWorld() works correctly, but it will be slow because contextForCurrentWorld() creates a new local context. As discussed in bug 82201, this is a hot call path of creating DOM object constructor. So it might make sense to leave the current matchesCurrentContext() as is. I'll add a comment that explains this.
Created attachment 159855 [details] Patch
Comment on attachment 159855 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159855&action=review > Source/WebCore/bindings/v8/ScriptController.cpp:397 > + // This method is equivalent to 'return v8::Context::GetCurrent() == contextForCurrentWorld()', > + // but is written without using contextForCurrentWorld(). > + // Given that this method is used by a hot call path of DOM object constructor, > + // we want to avoid the overhead of contextForCurrentWorld() creating Local<Context> every time. Ok. One option is to split contextForCurrentWorld() into two pieces: a private piece that returns the underlying persistent handle and a second pieces that returns a new local handle to the object. This function could then share the first piece with currentWorldContext. We don't need to do that in this patch if you'd rather get this one landed first. nit: contextForCurrentWorld -> currentWorldContext > Source/WebCore/bindings/v8/ScriptController.cpp:407 > + return context == context->GetCurrent(); Can we change context->GetCurrent() to v8::Context::GetCurrent()? (It's a static function right?)
(In reply to comment #8) > Ok. One option is to split contextForCurrentWorld() into two pieces: a private piece that returns the underlying persistent handle and a second pieces that returns a new local handle to the object. This function could then share the first piece with currentWorldContext. We don't need to do that in this patch if you'd rather get this one landed first. OK, let me fix it in a later patch. > nit: contextForCurrentWorld -> currentWorldContext Done. > > Source/WebCore/bindings/v8/ScriptController.cpp:407 > > + return context == context->GetCurrent(); > > Can we change context->GetCurrent() to v8::Context::GetCurrent()? (It's a static function right?) Done.
Committed r126370: <http://trac.webkit.org/changeset/126370>