85330 2012-05-01 17:53:56 -0700 [meta][V8] Remove V8Proxy 2013-09-12 22:33:36 -0700 1 1 1 Unclassified WebKit WebCore JavaScript 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 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 93095 1 haraken haraken abarth andersca arv dglazkov oldest_to_newest 613837 0 haraken 2012-05-01 17:53:56 -0700 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. 627327 1 haraken 2012-05-17 22:01:07 -0700 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). 627331 2 dglazkov 2012-05-17 22:06:25 -0700 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. 627336 3 dglazkov 2012-05-17 22:26:02 -0700 Here's where it all started: http://codereview.chromium.org/21370 627784 4 arv 2012-05-18 10:13:41 -0700 I actually think we should keep notHandledByInterceptor. It is a lot clearer than returning an empty v8::Value. 628669 5 haraken 2012-05-20 17:23:41 -0700 (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? 629166 6 arv 2012-05-21 09:16:44 -0700 (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. 629745 7 haraken 2012-05-21 17:19:22 -0700 (In reply to comment #6) > The return throwError(...) case is clearly the winner. Sure, I'll unify them into 'return throwError(...)'. 928646 8 andersca 2013-09-12 22:33:36 -0700 V8Proxy has been removed.