Add notifications for when V8Proxy creates or destroys a V8 Context.

Add notifications for when V8Proxy creates or destroys a V8 Context.

BUG=no
TEST=no

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19424

[Matt Perry, 2009-06-25 01:28:00]

I didn't realize ChromiumBridge.h was in third_party.  I need to whip up a
webkit CL to go along with this.

[darin (slow to review), 2009-06-25 05:28:51]

http://codereview.chromium.org/147124/diff/22/1007
File webkit/api/src/ChromiumBridge.cpp (right):

http://codereview.chromium.org/147124/diff/22/1007#newcode168
Line 168: return webKitClient()->didCreateScriptContext(context);
This seems like it should be a WebViewDelegate method since you can tie the
context back to the corresponding Frame.  Or, do you care about V8 contexts
created for worker threads?

It is still OK to use ChromiumBridge, but you would need to move the
implementation of these methods to glue/chromium_bridge_impl.cc so that it can
play with WebFrame and WebViewDelegate.  (That is just temporary until those
classes move into api/src.)

[Matt Perry, 2009-06-25 18:25:14]

http://codereview.chromium.org/147124/diff/22/1007
File webkit/api/src/ChromiumBridge.cpp (right):

http://codereview.chromium.org/147124/diff/22/1007#newcode168
Line 168: return webKitClient()->didCreateScriptContext(context);
On 2009/06/25 05:28:51, darin wrote:
> This seems like it should be a WebViewDelegate method since you can tie the
> context back to the corresponding Frame.  Or, do you care about V8 contexts
> created for worker threads?

This is not necessary for my purposes, because I can use
RetrieveFrameForCurrentContext to accomplish the same thing.   But these API
abstractions are new to me, so if WebViewDelegate is the better place for this,
I can move it there.  Should I?

> 
> It is still OK to use ChromiumBridge, but you would need to move the
> implementation of these methods to glue/chromium_bridge_impl.cc so that it can
> play with WebFrame and WebViewDelegate.  (That is just temporary until those
> classes move into api/src.)

This part is only true if I used WebViewDelegate, right?

[darin (slow to review), 2009-06-25 18:41:35]

http://codereview.chromium.org/147124/diff/22/1009
File webkit/port/bindings/v8/v8_proxy.cpp (right):

http://codereview.chromium.org/147124/diff/22/1009#newcode2279
Line 2279: m_frame->loader()->dispatchWindowObjectAvailable();
So, this call to dispatchWindowObjectAvailable leads to
WebViewDelegate::WindowObjectCleared() being called.  I wonder if that is a
sufficient notification for you.

Alternatively, it seems like you might want to just add notifications to
FrameLoaderClient for context destroyed and created.  That seems superior to
dumping more things into ChromiumBridge.

[Matt Perry, 2009-06-25 19:09:27]

http://codereview.chromium.org/147124/diff/22/1009
File webkit/port/bindings/v8/v8_proxy.cpp (right):

http://codereview.chromium.org/147124/diff/22/1009#newcode2279
Line 2279: m_frame->loader()->dispatchWindowObjectAvailable();
On 2009/06/25 18:41:35, darin wrote:
> So, this call to dispatchWindowObjectAvailable leads to
> WebViewDelegate::WindowObjectCleared() being called.  I wonder if that is a
> sufficient notification for you.

That gets called at other times as well.  I also need context-destroyed, which
no current notification corresponds to.

> Alternatively, it seems like you might want to just add notifications to
> FrameLoaderClient for context destroyed and created.  That seems superior to
> dumping more things into ChromiumBridge.

I can do that.  So maybe a notification like:
  didCreateScriptContext(frame)
with an API on WebFrame to retrieve that frame's v8 context?

[darin (slow to review), 2009-06-25 19:49:02]

http://codereview.chromium.org/147124/diff/22/1009
File webkit/port/bindings/v8/v8_proxy.cpp (right):

http://codereview.chromium.org/147124/diff/22/1009#newcode2279
Line 2279: m_frame->loader()->dispatchWindowObjectAvailable();
> That gets called at other times as well.  I also need context-destroyed, which
> no current notification corresponds to.

I see.


> > Alternatively, it seems like you might want to just add notifications to
> > FrameLoaderClient for context destroyed and created.  That seems superior to
> > dumping more things into ChromiumBridge.
> 
> I can do that.  So maybe a notification like:
>   didCreateScriptContext(frame)
> with an API on WebFrame to retrieve that frame's v8 context?

Yeah, sounds good to me.
-Darin

[Matt Perry, 2009-06-25 21:28:30]

OK, I rearranged it to use FrameLoaderClient.  There's a corresponding webkit
change to FrameLoaderClient.h that I'll send to you shortly.

[darin (slow to review), 2009-06-25 22:13:11]

Looks good... just a few small things:

http://codereview.chromium.org/147124/diff/29/2008
File webkit/glue/webframe.h (right):

http://codereview.chromium.org/147124/diff/29/2008#newcode36
Line 36: template <class T> class Local;
nit: this file is using google style, so 2 space indent

http://codereview.chromium.org/147124/diff/29/2008#newcode83
Line 83: virtual v8::Local<v8::Context> GetContext() = 0;
nit: GetScriptContext or GetV8Context?  GetContext seems a little too vague.

http://codereview.chromium.org/147124/diff/29/2014
File webkit/port/bindings/v8/v8_proxy.cpp (right):

http://codereview.chromium.org/147124/diff/29/2014#newcode2281
Line 2281: m_frame->loader()->client()->didCreateScriptContext();
maybe we should call didCreateScriptContext first?  also, i bet the names of the
FrameLoaderClient method should start with dispatchDid* since that seems to be
their convention.  it is meant to "dispatch" to the WebKit layer delegate where
the method name loses the dispatch prefix.

[Matt Perry, 2009-06-25 22:25:15]

http://codereview.chromium.org/147124/diff/29/2008
File webkit/glue/webframe.h (right):

http://codereview.chromium.org/147124/diff/29/2008#newcode36
Line 36: template <class T> class Local;
On 2009/06/25 22:13:11, darin wrote:
> nit: this file is using google style, so 2 space indent

Done.

http://codereview.chromium.org/147124/diff/29/2008#newcode83
Line 83: virtual v8::Local<v8::Context> GetContext() = 0;
On 2009/06/25 22:13:11, darin wrote:
> nit: GetScriptContext or GetV8Context?  GetContext seems a little too vague.

Done.

http://codereview.chromium.org/147124/diff/29/2014
File webkit/port/bindings/v8/v8_proxy.cpp (right):

http://codereview.chromium.org/147124/diff/29/2014#newcode2281
Line 2281: m_frame->loader()->client()->didCreateScriptContext();
On 2009/06/25 22:13:11, darin wrote:
> maybe we should call didCreateScriptContext first?
Done.

>  also, i bet the names of the
> FrameLoaderClient method should start with dispatchDid* since that seems to be
> their convention.  it is meant to "dispatch" to the WebKit layer delegate
where
> the method name loses the dispatch prefix.

There's no real convention that I can tell.  There are methods like
dispatchDidCommitLoad, didChangeTitle, and frameLoadCompleted.

[darin (slow to review), 2009-06-25 22:26:33]

> There's no real convention that I can tell.  There are methods like
> dispatchDidCommitLoad, didChangeTitle, and frameLoadCompleted.

Yeah, it's true... I'm happy with or without the dispatch prefix.