Bug 85330

Summary: [meta][V8] Remove V8Proxy
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: WebCore JavaScriptAssignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andersca, arv, dglazkov
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 86831, 87079, 87083, 87102, 87106, 93101, 93103, 93105, 93106, 93115, 93209, 93605, 93610, 93679, 93682, 93792, 93830, 93834, 94437, 94438, 94443, 94444, 94445, 94446, 94450, 94453, 94455, 94456, 94459, 94460, 94561, 94563, 94593, 94596, 94597, 94598, 94701, 94706, 94713, 94715, 94768, 94770, 94773, 94777, 94794    
Bug Blocks: 93095    

Kentaro Hara
Reported 2012-05-01 17:53:56 PDT
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.
Attachments
Kentaro Hara
Comment 1 2012-05-17 22:01:07 PDT
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)
Comment 2 2012-05-17 22:06:25 PDT
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)
Comment 3 2012-05-17 22:26:02 PDT
Here's where it all started: http://codereview.chromium.org/21370
Erik Arvidsson
Comment 4 2012-05-18 10:13:41 PDT
I actually think we should keep notHandledByInterceptor. It is a lot clearer than returning an empty v8::Value.
Kentaro Hara
Comment 5 2012-05-20 17:23:41 PDT
(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
Comment 6 2012-05-21 09:16:44 PDT
(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
Comment 7 2012-05-21 17:19:22 PDT
(In reply to comment #6) > The return throwError(...) case is clearly the winner. Sure, I'll unify them into 'return throwError(...)'.
Anders Carlsson
Comment 8 2013-09-12 22:33:36 PDT
V8Proxy has been removed.
Note You need to log in before you can comment on or make changes to this bug.