Implement V8 interceptors

[GoogleCodeExporter]

CEF3 has an implementation for V8 accessors, but it doesn't support 
interceptors. How should API for this feature look? Probably one should create 
a new CefV8Interceptor interface with four functions: Get/SetNamedProperty, 
Get/SetIndexedProperty. A CefRefPtr<CefV8Interceptor> could be passed to 
CefV8Value::CreateObject as a new argument.
I could be a volunteer, although I'm quite new to programming. That would be a 
practical exercise for me.

Original issue reported on code.google.com by kuba.trz...@gmail.com on 18 Dec 2013 at 6:15

[GoogleCodeExporter]

I've added support for interceptors. Tell what you think. Actually, the patch 
also contains .gitignore apropriate for CEF project. I think that it could be 
added to SVN repo.

Original comment by kuba.trz...@gmail.com on 28 Dec 2013 at 10:41

Attachments:

• interceptors.patch

[GoogleCodeExporter]

Thanks for the patch -- unit tests are especially appreciated :) Overall it 
looks good. Some comments:

1. CefV8Interceptor methods should return a bool similar to CefV8Handler and 
CefV8Accessor.

2. How does v8::ObjectTemplate::New() behave from a memory usage standpoint? We 
should add a performance test for it. Hopefully it doesn't incrementally 
increase memory usage like v8::FunctionTemplate 
(https://code.google.com/p/v8/issues/detail?id=2221).

-  // Create the new V8 object.
-  v8::Local<v8::Object> obj = v8::Object::New();
+  // Create the new V8 object. If an interceptor is passed, create object form
+  // template and set property handlers.
+  v8::Local<v8::Object> obj;
+  if(interceptor.get()) {
+    v8::Local<v8::ObjectTemplate> tmpl = v8::ObjectTemplate::New();
+    tmpl->SetNamedPropertyHandler(
+        InterceptorGetterCallbackImpl<v8::Local<v8::String> >,
+        InterceptorSetterCallbackImpl<v8::Local<v8::String> >);
+    tmpl->SetIndexedPropertyHandler(InterceptorGetterCallbackImpl<uint32_t>,
+                                    InterceptorSetterCallbackImpl<uint32_t>);
+    obj = tmpl->NewInstance();
+  } else {
+    obj = v8::Object::New();
+  }

3. Use spaces instead of tabs for indentation.

+// Two helper functions for V8 Interceptor callbacks
+CefString PropertyToIndex(v8::Local<v8::String> str) {
+   CefString name;
+  GetCefString(str, name);
+   return name;
+}
+
+int PropertyToIndex(uint32_t index) {
+   return static_cast<int>(index);
+}

 CefRefPtr<CefV8Value> CefV8Value::CreateObject(
-    CefRefPtr<CefV8Accessor> accessor) {
+    CefRefPtr<CefV8Accessor> accessor,
+   CefRefPtr<CefV8Interceptor> interceptor) {

4. Spelling error in "form"

+  // Create the new V8 object. If an interceptor is passed, create object form
+  // template and set property handlers.

5. Wrap lines at 80 characters. This is a problem elsewhere in v8_unittest.cc 
as well.

+    static const char* kGetByNameExceptionMsg = "Uncaught Error: My get_byname 
exception";
+    static const char* kGetByIndexExceptionMsg = "Uncaught Error: My 
get_byindex exception";
+    static const char* kSetByNameExceptionMsg = "Uncaught Error: My set_byname 
exception";
+    static const char* kSetByIndexExceptionMsg = "Uncaught Error: My 
set_byindex exception";

6. Don't include auto-generated files. They make the patch file harder to read.

7. Don't include unrelated files like .gitignore. I'll add that as a separate 
commit.

Original comment by magreenb...@gmail.com on 10 Jan 2014 at 6:53

• Changed state: Accepted
• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

Issue 599 has been merged into this issue.

Original comment by magreenb...@gmail.com on 10 Jan 2014 at 6:56

[GoogleCodeExporter]

Thank you for all your comments. Sorry for all the mistakes, it's my first real 
contribution to a big project.

1. That would be consistent. And I did it like that initially, but it was 
confusing for many reasons.

a) Why do we need that boolean at all? Setting the retval (or not setting it) 
is the obvious way of returning/not returning a value.
b) In CefV8Accessor::Set we confuse the user, suggesting that it's important if 
they return true/false. Actually, the return value isn't even checked (!).
c)  After calling CefV8Accessor::Get we don't check for the exception if the 
"retrieval wasn't handled" (boolean == false). But we always check for the 
exception after calling Set, even when (boolean == false).
d) In CefV8Accessor::Get we silently allow the following scenario: boolean == 
true && exception.empty() && !retval.get(). And it has the same effect as 
(boolean == false).

If you consider these arguments not strong enough, I can make getters return 
booleans. But interceptor's setters technically cannot be unhandled, so they 
really should return void IMO.

2. I have no idea. :( I'll have to check that.

3-5. I'll fix that.

6-7. Got it.

Original comment by kuba.trz...@gmail.com on 11 Jan 2014 at 3:07

[GoogleCodeExporter]

@comment#4: Thanks for bringing up these inconsistencies. There's still one 
case that's unclear without a bool return value: What does it mean if the user 
doesn't set any of the out values?

This is how I think it should behave for all handler methods:

A. Return false if the method isn't handled. All out values are ignored. 
Appropriate default handling occurs (behave the same as if no handler was 
passed to the Create method).
B. Return true if the method is handled. If an exception is specified use that, 
otherwise use the retval. If both exception and retval are empty then set 
retval to Undefined.

Original comment by magreenb...@gmail.com on 13 Jan 2014 at 6:12

[GoogleCodeExporter]

@comment#5:
0. You convinced me, we can stay with bools.

1. "What does it mean if the user doesn't set any of the out values?"
It means:
a) There is no value to return
b) No real error occured
It's the same as returning false in boolean version.

2. Does A. mean that CefV8Accessor::Set caller is broken and it should check 
for return value and than not check for exception if (boolean == false)? Maybe, 
then, place some asserts in a few places?
a) Not to let user pass outvalues when (boolean == false).
b) Not to let user leave retval unset when (boolean == true), instead of making 
something implicit like setting Undefined.

Original comment by kuba.trz...@gmail.com on 13 Jan 2014 at 6:46

[GoogleCodeExporter]

@comment#6: If we proceed with the usage from comment#4 then we should verify 
and fix any methods that don't conform, including CefV8Accessor::Set. Non-fatal 
usage-related assertions in libcef usually aren't helpful because most users 
don't know what they mean. Consequently they just result in additional 
confusion and invalid bug reports. Instead, we should try to be consistent, 
document everything clearly, and provide reasonable default behaviors.

Original comment by magreenb...@gmail.com on 13 Jan 2014 at 6:55

[GoogleCodeExporter]

@comment#7
Ok, I'll make a new patchset. Maybe I'll find some time tommorow.

Original comment by kuba.trz...@gmail.com on 13 Jan 2014 at 7:23

[GoogleCodeExporter]

Hi Everyone,
I am following this project for a long time. I don't see the support for 
Interceptors (CefV8Interceptor) in current cef. If these changes are not 
incorporated, could you please allow me to submit the required change for 
Interceptor support.
This will be my first contribution on any big open-source project.

Thanks ,
-Anand

Original comment by LinkinAn...@gmail.com on 16 Jun 2014 at 8:49

[GoogleCodeExporter]

CEF is transitioning from Google Code to Bitbucket project hosting. If you 
would like to continue receiving notifications on this issue please add 
yourself as a Watcher at the new location: 
https://bitbucket.org/chromiumembedded/cef/issue/1159

Original comment by magreenb...@gmail.com on 14 Mar 2015 at 3:29