Bug 85330 – [meta][V8] Remove V8Proxy

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

V8Proxy is one-to-one with ScriptController. The methods in V8Proxy should be moved to ScriptController or other appropriate classes. The objective is to remove V8Proxy.

[Kentaro Hara]

How should we refactor V8Proxy::notHandledByInterceptor()?

V8Proxy::notHandledByInterceptor() is used to return "early" from the custom binding callbacks. V8Proxy::notHandledByInterceptor() just returns v8::Local<v8::Object>(). However, as far as I look over all custom bindings, the following three patterns are mixed to accomplish the early return:

- Use V8Proxy::notHandledByInterceptor().
- Hard-code 'return v8::Handle<v8::Value>()'.
- Hard code 'return v8::Handle<v8::Object>()'.

Possible refactoring approaches:

(A) Remove V8Proxy::notHandledByInterceptor(). Unify all of them into 'return v8::Handle<v8::Value>()'.

(B) Replace hard-coded 'return v8::Handle<v8::Value>()' and 'return v8::Handle<v8::Object>()' with V8Proxy::notHandledByInterceptor().

I would prefer (A), but WDYT? (The reason why I do not like notHandledByInterceptor() is that notHandledByInterceptor() is not general in a sense that it can be used for callbacks that return v8::Handle<v8::Value>() or v8::Local<v8::Object>(). We cannot use notHandledByInterceptor() for callbacks that return v8::Local<v8::Boolean>() and thus need to hard-code it anyway).

[Dimitri Glazkov (Google)]

I invented notHandledByInterceptor a while back as a way to clearly indicate that we are falling through the interceptor (that's named property handlers). I now see that it has been cargo-culted all over the place. I am ok with just removing it.

[Dimitri Glazkov (Google)]

Here's where it all started: http://codereview.chromium.org/21370

[Erik Arvidsson]

I actually think we should keep notHandledByInterceptor. It is a lot clearer than returning an empty v8::Value.

[Kentaro Hara]

(In reply to comment #4)
> I actually think we should keep notHandledByInterceptor. It is a lot clearer than returning an empty v8::Value.

arv: I've not yet landed the patch (https://bugs.webkit.org/show_bug.cgi?id=86831). Waiting for your opinion.

In most cases, notHandledByInterceptor() had been used in this pattern:

    throwError(...);
    return notHandledByInterceptor();

The patch replaces it with:

    throwError(...);
    return v8::Handle<v8::Value>();

Then I will land a follow-up patch that replaces it with:

    return throwError(...);

Some custom bindings are already using 'return throwError(...)'. This pattern seems to be cleanest. WDYH?

[Erik Arvidsson]

(In reply to comment #5)
> (In reply to comment #4)
> > I actually think we should keep notHandledByInterceptor. It is a lot clearer than returning an empty v8::Value.
> 
> arv: I've not yet landed the patch (https://bugs.webkit.org/show_bug.cgi?id=86831). Waiting for your opinion.
> 
> In most cases, notHandledByInterceptor() had been used in this pattern:
> 
>     throwError(...);
>     return notHandledByInterceptor();
> 
> The patch replaces it with:
> 
>     throwError(...);
>     return v8::Handle<v8::Value>();
> 
> Then I will land a follow-up patch that replaces it with:
> 
>     return throwError(...);
> 
> Some custom bindings are already using 'return throwError(...)'. This pattern seems to be cleanest. WDYH?

The return throwError(...) case is clearly the winner.

I looked at the patch and there seems to be a lot of cases where removing notHandledByInterceptor makes the code more obscure.

[Kentaro Hara]

(In reply to comment #6)
> The return throwError(...) case is clearly the winner.

Sure, I'll unify them into 'return throwError(...)'.

[Anders Carlsson]

V8Proxy has been removed.