Summary: | [meta][V8] Remove V8Proxy | ||
---|---|---|---|
Product: | WebKit | Reporter: | Kentaro Hara <haraken> |
Component: | WebCore JavaScript | Assignee: | 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 |
Description
Kentaro Hara
2012-05-01 17:53:56 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). 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. Here's where it all started: http://codereview.chromium.org/21370 I actually think we should keep notHandledByInterceptor. It is a lot clearer than returning an empty v8::Value. (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? (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. (In reply to comment #6) > The return throwError(...) case is clearly the winner. Sure, I'll unify them into 'return throwError(...)'. V8Proxy has been removed. |