Can't call ExecuteFuncton on a CefV8Value outside of an Execute callback.

[GoogleCodeExporter]

Attached is the proposed patch to fix this.

Added a CefV8Context object to keep track of a V8 Context. Clients can create 
them on demand via CefV8Context::CreateForCurrentContext() and 
CefV8Context::CreateForEnteredContext(). (See WebFrame.h for meaning of the 
two.)

Added CefV8Value::ExecuteFunctionWithContext to use the CefV8Context that is 
given. (Old ExecuteFunction now calls the new one with an empty CefV8Context 
which defaults to using the current context.)

Added CefBrowser::BrowserForCurrentContext() and 
CefBrowser::BrowserForEnteredContext() static methods.

The browser static methods are really useful in the Execute() call to determine 
which CefBrowser the caller is in.

Minor fix to libcef_dll/wrapper/libcef_dll_wrapper.cc to include the new 
objects in this patch and web_urlrequest/web_urlrequest_client.

Expanded the v8_unittest.cc to include testing of this.

Original issue reported on code.google.com by gusver...@gmail.com on 14 Feb 2011 at 8:41

Attachments:

• v8_context_support.patch

[GoogleCodeExporter]

Is it valid for a CefV8Value to be used across multiple V8 contexts? Maybe we 
should instead require all CefV8Value objects to be associated with a context 
by changing the static CefV8Value methods to require a CefV8Context as the 
first argument. If the argument is empty then the CefV8Value would be 
associated with the current context. v8::Context::Scope could then be used for 
all V8-related calls and it wouldn't be necessary to pass the context 
explicitly to ExecuteFunction. What do you think?

Original comment by magreenb...@gmail.com on 15 Feb 2011 at 6:38

[GoogleCodeExporter]

I believe that the V8 context only matters for function types that are executed 
by V8. Every other CefV8Value type, even cpp functions, don't need a context 
and can therefore run in practically any V8 context.

My thinking on not automatically sending the V8 context in the Execute call or 
attaching the context to function objects was due to the probability that not 
every Execute handler was going to need it. (and then have to pick which one to 
attach)

Letting clients decide when and what context to save left open the possibility 
that a particular design needed to preserve the entered context rather than the 
current -- though I have yet to determine a possible use-case.

Original comment by gusver...@gmail.com on 15 Feb 2011 at 9:34

[GoogleCodeExporter]

OK, if the context is only needed for ExecuteFunction then the way you 
implemented it makes sense. A few more comments...

1. Lets use "GetCurrentContext" and "GetEnteredContext" instead of 
"CreateFor...".

2. What do you think about adding a non-static GetBrowser() method to 
CefV8Context instead of static methods to CefBrowser?  So 
"CefV8Context::GetCurrentContext()->GetBrowser()" would replace 
"CefBrowser::BrowserForCurrentV8Context()". This has the advantage of allowing 
retrieval of the associated browser object at a later time even if the current 
context has changed and may make the usage a bit clearer.

3. The comments in WebFrame.h seem to indicate that the context can be 
associated with any WebFrame, not just the main frame. So perhaps it should be 
a GetFrame() method instead of a GetBrowser() method?

Original comment by magreenb...@gmail.com on 15 Feb 2011 at 11:48

[GoogleCodeExporter]

1,2 - Yes! way way better. Good points. Agreed.

3 - It is very helpful to get at that CefBrowser object. I don't see a way to 
get it from the CefFrame even though it owns one. Even if we add a method to 
CefFrame to get at the CefBrowser, I think we should still expose a 
GetBrowser() method here as this is more than likely an object you will want. 
(Getting the CefFrame involves finding the CefBrowser and asking it for the 
frame anyways so the code is already here.)

Let me know if you want me to throw in the CefFrame::GetBrowser() method.

Original comment by gusver...@gmail.com on 16 Feb 2011 at 1:54

[GoogleCodeExporter]

In FindBrowserForFrame you're comparing the frame for the context to the 
browser main frame. Will the context frame always be the main frame? The 
comment in WebFrame.h seems to suggest otherwise.

Original comment by magreenb...@gmail.com on 16 Feb 2011 at 2:22

[GoogleCodeExporter]

To find the CefBrowser I am getting the current frame for the v8 context via 
frameForCurrentContext() and then looking at that frame's top() frame, which is 
the main frame. With that I can find the CefBrowser by looking for the one with 
this as its main frame. From the CefBrowser instance, I can then call it's 
UIT_GetCefFrame() method with the frame I got from frameForCurrentContext() 
which, as you mention, is not necessarily the main frame. I need to add a test 
case for this.

Original comment by gusver...@gmail.com on 16 Feb 2011 at 3:52

[GoogleCodeExporter]

Thanks, I missed the call to top().

I think at a minimum we need CefV8Context::GetFrame() and 
CefFrame::GetBrowser(). I'm not sure we need CefV8Context::GetBrowser() but I'm 
OK with it.

Original comment by magreenb...@gmail.com on 17 Feb 2011 at 1:07

[GoogleCodeExporter]

Attached is a new patch that addresses the issues noted above. Thanks for the 
great suggestions. (updated to r187)

1. Renamed CreateForXXXXX to GetXXXXX in CefV8Context.
2. Moved GetBrowser() and GetFrame() to CefV8Context rather than as static 
methods on  CefBrowser.
3. Added a GetBrowser() method to CefFrame.
4. Extended unittest to include a test with different frames for the entered 
and current contexts.

Original comment by gusver...@gmail.com on 17 Feb 2011 at 3:10

Attachments:

• v8_context_support_2.patch

[GoogleCodeExporter]

I have another patch coming that adds a GetGlobal() method to CefV8Context to 
return the global object for the context...

Original comment by gusver...@gmail.com on 17 Feb 2011 at 2:25

[GoogleCodeExporter]

Attached is a patch that adds support for getting the global object from a 
CefV8Context.

I also made a change to the ExecuteFunction and ExecuteFunctionWithContext in 
CefV8Value to let clients call it with an empty "object". When "object" is 
empty, the receiver (aka "this") in the call will default to the global object 
for the context.

Updated unit test.

Original comment by gusver...@gmail.com on 17 Feb 2011 at 4:36

Attachments:

• v8_context_support_3.patch

[GoogleCodeExporter]

It's probably not safe to create and/or delete (Dispose) V8 objects on threads 
other than the UI thread. Now that we're allowing CefV8 objects to be created 
in other locations we should guarantee this.

For the CefV8Value constructors (CefV8Value::Create*) we can simply require 
that they be called on the UI thread using the CefThread::CurrentlyOn approach.

For the CefV8ContextImpl destructor I think we can use the same approach as 
CefV8ValueImpl::Detach() to guarantee that v8_context_.Dispose() is called on 
the UI thread.

See http://create.tpsitulsa.com/blog/2009/02/02/on-v8-and-multithreading/ for 
more information on V8 thread safety.

Original comment by magreenb...@gmail.com on 18 Feb 2011 at 8:43

[GoogleCodeExporter]

In fact, we should probably change the CefV8ValueImpl implementation to require 
that all methods are called on the UI thread. This will also eliminate the need 
for locking in CefV8ValueImpl.

Original comment by magreenb...@gmail.com on 18 Feb 2011 at 8:46

[GoogleCodeExporter]

Strike what I said in Comment #11 about CefV8ValueImpl::Detach() being OK. It's 
probably not safe to call MakeWeak() on other threads either. So instead we 
need to guarantee that CefV8 objects are destroyed on the UI thread. See 
web_urlrequest_impl.cc for an example of how this can be done.

Original comment by magreenb...@gmail.com on 18 Feb 2011 at 8:52

[GoogleCodeExporter]

Thanks, I'll take a look.

Original comment by gusver...@gmail.com on 18 Feb 2011 at 9:43

[GoogleCodeExporter]

Attached is a patch with the suggested changes to make sure calls to Dispose 
and MakeWeak occur in the UI thread as well as enforcing the rule that CefV8* 
methods are get called in the UI thread. Lock/Unlock usage has been removed. I 
also added NOTREACHED() checks to alert developers of the miss-use of some 
methods in debug builds.

Original comment by gusver...@gmail.com on 21 Feb 2011 at 3:39

Attachments:

• v8_context_support_4.patch

[GoogleCodeExporter]

Thanks, committed with minor style and documentation changes as revision 188.

Original comment by magreenb...@gmail.com on 21 Feb 2011 at 10:46

• Changed state: Fixed