Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
970403003:
bindings: Use V8 MaybeLocal APIs in V8DeviceMotionEventCustom (Closed)
|
Created:
5 years, 9 months ago by bashi Modified:
5 years, 9 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, vivekg_samsung, arv+blink, vivekg, kinuko+serviceworker Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Descriptionbindings: Use V8 MaybeLocal APIs in V8DeviceMotionEventCustom
The V8 team is deprecating ToXXX() methods which takes v8::Isolate* for
better exception handling. The new APIs return Maybe{Local}<T>, which
requires explicit empty checks. Use it instead of APIs which return
Local<T>
BUG=462402
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191599
Patch Set 1 #Patch Set 2 : #
Total comments: 3
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 3
Patch Set 5 : #
Total comments: 11
Patch Set 6 : #Patch Set 7 : #Messages
Total messages: 32 (6 generated)
bashi@chromium.org changed reviewers: + haraken@chromium.org
PTAL? https://codereview.chromium.org/970403003/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8MaybeLocalToLocal.h (right): https://codereview.chromium.org/970403003/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/V8MaybeLocalToLocal.h:35: #define MAYBE_LOCAL_TO_LOCAL_VOID(context, inValue, outValue) do { \ I'm planning to add MAYBE_LOCAL_TO_LOCAL_DEFAULT(context, inValue, outValue, failureReturnValue) which returns failureReturnValue on failure. The name could be changed. https://codereview.chromium.org/970403003/diff/20001/Source/bindings/modules/... File Source/bindings/modules/v8/custom/V8SQLTransactionCustom.cpp (right): https://codereview.chromium.org/970403003/diff/20001/Source/bindings/modules/... Source/bindings/modules/v8/custom/V8SQLTransactionCustom.cpp:68: v8::Local<v8::Object> sqlArgsObject; It seems that vivekg@ is going to remove this custom bindings. I'll use another call site to test MAYBE_LOCAL_TO_LOCAL_VOID() if this CL looks reasonable.
yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org
https://codereview.chromium.org/970403003/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8MaybeLocalToLocal.h (right): https://codereview.chromium.org/970403003/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/V8MaybeLocalToLocal.h:40: } while (false); You don't want the last semi-colon.
I'm not quite sure if MAYBE_LOCAL_TO_LOCAL_VOID is a good abstraction. ToXXX() won't be the only V8 APIs that return MaybeLocal and there will be a bunch of other V8 APIs. I guess something like CALL_V8 would be better in order not to increase the number of macros. Local<String> result; CALL_V8_WITH_CHECK(isolate, result, toString(str), returnValue);
Thanks for reviews. I changed the implementation based on your inputs (both online/offline). Could you take another look?
jl@opera.com changed reviewers: + jl@opera.com
https://codereview.chromium.org/970403003/diff/40001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8BindingMacros.h (right): https://codereview.chromium.org/970403003/diff/40001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:83: if (!(callExpression).ToLocal(&outValue) || outValue.IsEmpty()) \ Should ToLocal() failing and the resulting local being empty necessarily be treated the same? I'd interpret ToLocal() failing as "V8 is being shut down, abort, abort!" whereas the resulting local being empty is a "normal failure". In the first case, we should presumably try to do as little as possible; just unwind the stack. In the second case, we might want to extract an exception from a TryCatch and store it in an ExceptionState, or create and return a rejected promise, or something like that. Things that might themselves fail if ToLocal() failed, perhaps?
https://codereview.chromium.org/970403003/diff/40001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8BindingMacros.h (right): https://codereview.chromium.org/970403003/diff/40001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:81: #define CALL_V8_WITH_CHECK_MAYBELOCAL(type, outValue, callExpression, failureExpression) \ From the point of view of users of this macro, I'd prefer outVariable or varName, etc. to outValue. It's variable rather than value. https://codereview.chromium.org/970403003/diff/40001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:84: failureExpression do { failureExpression; } while (0) would be better if you'd like to support cases like MACRO(..., foo(); bar(); return);
https://codereview.chromium.org/970403003/diff/40001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8BindingMacros.h (right): https://codereview.chromium.org/970403003/diff/40001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:81: #define CALL_V8_WITH_CHECK_MAYBELOCAL(type, outValue, callExpression, failureExpression) \ On 2015/03/04 09:04:22, Yuki wrote: > From the point of view of users of this macro, I'd prefer outVariable or > varName, etc. to outValue. It's variable rather than value. Done. https://codereview.chromium.org/970403003/diff/40001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:83: if (!(callExpression).ToLocal(&outValue) || outValue.IsEmpty()) \ On 2015/03/04 09:01:44, Jens Widell wrote: > Should ToLocal() failing and the resulting local being empty necessarily be > treated the same? > > I'd interpret ToLocal() failing as "V8 is being shut down, abort, abort!" > whereas the resulting local being empty is a "normal failure". > > In the first case, we should presumably try to do as little as possible; just > unwind the stack. In the second case, we might want to extract an exception from > a TryCatch and store it in an ExceptionState, or create and return a rejected > promise, or something like that. Things that might themselves fail if ToLocal() > failed, perhaps? If I read ToLocal() correctly, ToLocal() just fails if the |val_| is null, so there is no implication of abnormal termination when ToLocal() fails. We can check v8::IsExecutionTerminating(), but in this case, I couldn't find a strong reason to use ExceptionState (we would need to add an ExceptionState instance in V8DeviceMotionEventCustom.cpp, then need to call throwIfNeeded()). We could add macros which use ExceptionState when needed. Instead, I added TryCatch to make sure we rethrow it. https://codereview.chromium.org/970403003/diff/40001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:84: failureExpression On 2015/03/04 09:04:22, Yuki wrote: > do { failureExpression; } while (0) > would be better if you'd like to support cases like > MACRO(..., foo(); bar(); return); I changed to take returnValue as we should return as soon as possible. https://codereview.chromium.org/970403003/diff/60001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8BindingMacros.h (right): https://codereview.chromium.org/970403003/diff/60001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:85: if (!(callExpression).ToLocal(&outVariable)) { \ Dropped outVariable.IsEmpty() check because when this condition is true, outVariable.IsEmpty() is always true.
https://codereview.chromium.org/970403003/diff/60001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8BindingMacros.h (right): https://codereview.chromium.org/970403003/diff/60001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:84: v8::TryCatch block; \ This TryCatch serves no purpose. Exceptions are propagated by default so there's no need to catch and rethrow them if you're not interested in seeing the exception, and don't mean to use block.HasCaught() to detect failure in the first place.
https://codereview.chromium.org/970403003/diff/60001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8BindingMacros.h (right): https://codereview.chromium.org/970403003/diff/60001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:81: #define CALL_V8_WITH_CHECK_MAYBELOCAL_DEFAULT(type, outVariable, callExpression, returnValue) \ This macro reminds me quite a lot about the growing set of macros we recently removed from this file. We'll need different macro variants in much the same cases, I think: - Should an ExceptionState be used? - Should a local variable be declared or should a previously declared one be assigned? - Should a value be returned? And at the end of the day, the macro doesn't do a lot. (And macros that do a lot should be functions anyway.) Are we worried about having ToLocal() calls spelled out in the code? Does a macro named CALL_V8_WITH_CHECK_MAYBELOCAL_DEFAULT() make the code more obvious and easy to read? :-)
On 2015/03/04 10:33:05, Jens Widell wrote: > https://codereview.chromium.org/970403003/diff/60001/Source/bindings/core/v8/... > File Source/bindings/core/v8/V8BindingMacros.h (right): > > https://codereview.chromium.org/970403003/diff/60001/Source/bindings/core/v8/... > Source/bindings/core/v8/V8BindingMacros.h:81: #define > CALL_V8_WITH_CHECK_MAYBELOCAL_DEFAULT(type, outVariable, callExpression, > returnValue) \ > This macro reminds me quite a lot about the growing set of macros we recently > removed from this file. > > We'll need different macro variants in much the same cases, I think: > > - Should an ExceptionState be used? > > - Should a local variable be declared or should a previously declared one be > assigned? > > - Should a value be returned? > > And at the end of the day, the macro doesn't do a lot. (And macros that do a lot > should be functions anyway.) > > Are we worried about having ToLocal() calls spelled out in the code? > > Does a macro named CALL_V8_WITH_CHECK_MAYBELOCAL_DEFAULT() make the code more > obvious and easy to read? :-) Yeah, while writing the CL, I was wondering whether adding macros make code simple, but some folks asked me to add generic macros when I was working on https://codereview.chromium.org/964553003/. I'd appreciate if you could have better ideas to avoid having many macros. It seems that just replacing the deprecated APIs and adding checks would be the option.
On 2015/03/05 01:53:25, bashi1 wrote: > On 2015/03/04 10:33:05, Jens Widell wrote: > > > https://codereview.chromium.org/970403003/diff/60001/Source/bindings/core/v8/... > > File Source/bindings/core/v8/V8BindingMacros.h (right): > > > > > https://codereview.chromium.org/970403003/diff/60001/Source/bindings/core/v8/... > > Source/bindings/core/v8/V8BindingMacros.h:81: #define > > CALL_V8_WITH_CHECK_MAYBELOCAL_DEFAULT(type, outVariable, callExpression, > > returnValue) \ > > This macro reminds me quite a lot about the growing set of macros we recently > > removed from this file. > > > > We'll need different macro variants in much the same cases, I think: > > > > - Should an ExceptionState be used? > > > > - Should a local variable be declared or should a previously declared one be > > assigned? > > > > - Should a value be returned? > > > > And at the end of the day, the macro doesn't do a lot. (And macros that do a > lot > > should be functions anyway.) > > > > Are we worried about having ToLocal() calls spelled out in the code? > > > > Does a macro named CALL_V8_WITH_CHECK_MAYBELOCAL_DEFAULT() make the code more > > obvious and easy to read? :-) > > Yeah, while writing the CL, I was wondering whether adding macros make code > simple, but some folks asked me to add generic macros when I was working on > https://codereview.chromium.org/964553003/. I'd appreciate if you could have > better ideas to avoid having many macros. It seems that just replacing the > deprecated APIs and adding checks would be the option. IMHO, macros are fine as long as they are under our control. In other words, if the number of CALL_V8_* macros is very limited, I think it's fine.
On 2015/03/05 02:02:27, haraken wrote: > On 2015/03/05 01:53:25, bashi1 wrote: > > On 2015/03/04 10:33:05, Jens Widell wrote: > > > > > > https://codereview.chromium.org/970403003/diff/60001/Source/bindings/core/v8/... > > > File Source/bindings/core/v8/V8BindingMacros.h (right): > > > > > > > > > https://codereview.chromium.org/970403003/diff/60001/Source/bindings/core/v8/... > > > Source/bindings/core/v8/V8BindingMacros.h:81: #define > > > CALL_V8_WITH_CHECK_MAYBELOCAL_DEFAULT(type, outVariable, callExpression, > > > returnValue) \ > > > This macro reminds me quite a lot about the growing set of macros we > recently > > > removed from this file. > > > > > > We'll need different macro variants in much the same cases, I think: > > > > > > - Should an ExceptionState be used? > > > > > > - Should a local variable be declared or should a previously declared one be > > > assigned? > > > > > > - Should a value be returned? > > > > > > And at the end of the day, the macro doesn't do a lot. (And macros that do a > > lot > > > should be functions anyway.) > > > > > > Are we worried about having ToLocal() calls spelled out in the code? > > > > > > Does a macro named CALL_V8_WITH_CHECK_MAYBELOCAL_DEFAULT() make the code > more > > > obvious and easy to read? :-) > > > > Yeah, while writing the CL, I was wondering whether adding macros make code > > simple, but some folks asked me to add generic macros when I was working on > > https://codereview.chromium.org/964553003/. I'd appreciate if you could have > > better ideas to avoid having many macros. It seems that just replacing the > > deprecated APIs and adding checks would be the option. > > IMHO, macros are fine as long as they are under our control. In other words, if > the number of CALL_V8_* macros is very limited, I think it's fine. Modified the macro. Could you take another look? - Simplified the name. It seems _WITH_CHECK is redundant. - It doesn't declare the output variable. Caller need to declare it before the macro. - It requires ExceptionState. This way, we could limit the number of macros, I hope. We would need at least following variants: - CALL_V8_MAYBELOCAL_VOID() - CALL_V8_MAYBE() - CALL_V8_MAYBE_VOID()
https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8BindingMacros.h (right): https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:81: #define CALL_V8_MAYBELOCAL(outVariable, isolate, callExpression, exceptionState, failureReturnValue) \ Nit: I'd prefer calling this CALL_V8, since we don't need to expose a concept of MaybeLocal to bindings/. https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:81: #define CALL_V8_MAYBELOCAL(outVariable, isolate, callExpression, exceptionState, failureReturnValue) \ Do we need the isolate parameter? https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:83: v8::TryCatch block; \ Why do we need this TryCatch block? - It's heavy so we need to avoid it as much as possible. - If we just want to rethrow, we don't need the TryCatch block?
https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8BindingMacros.h (right): https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:81: #define CALL_V8_MAYBELOCAL(outVariable, isolate, callExpression, exceptionState, failureReturnValue) \ On 2015/03/06 02:44:07, haraken wrote: > > Do we need the isolate parameter? It seems that v8::V8::IsExecutionTerminating() is deprecated (since it uses Isolate::GetCurrent()). https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:81: #define CALL_V8_MAYBELOCAL(outVariable, isolate, callExpression, exceptionState, failureReturnValue) \ On 2015/03/06 02:44:07, haraken wrote: > > Nit: I'd prefer calling this CALL_V8, since we don't need to expose a concept of > MaybeLocal to bindings/. I'm fine with dropping _MAYBELOCAL, but what the name should be for Maybe<T>? https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&l=... https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:83: v8::TryCatch block; \ On 2015/03/06 02:44:07, haraken wrote: > > Why do we need this TryCatch block? > > - It's heavy so we need to avoid it as much as possible. Yeah, this is my concern, but... > - If we just want to rethrow, we don't need the TryCatch block? How can we detect a JavaScript exception without TryCatch? IIUC, ExceptionState does nothing with detecting JS exceptions (I might be wrong here, though).
https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8BindingMacros.h (right): https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:83: v8::TryCatch block; \ On 2015/03/06 02:54:44, bashi1 wrote: > On 2015/03/06 02:44:07, haraken wrote: > > > > Why do we need this TryCatch block? > > > > - It's heavy so we need to avoid it as much as possible. > Yeah, this is my concern, but... > > - If we just want to rethrow, we don't need the TryCatch block? > How can we detect a JavaScript exception without TryCatch? IIUC, ExceptionState > does nothing with detecting JS exceptions (I might be wrong here, though). Maybe, we'd better ask V8 team to make ToLocal() return false whenever an exception is thrown. https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:84: if ((callExpression).ToLocal(&outVariable)) { \ Did you mean?: if (!(...)) I thought that ToLocal returns true on success and false on failure.
Patchset #6 (id:100001) has been deleted
Chatted offline with haraken-san, and we are going to simply replace APIs for now. Thanks for the inputs guys! https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8BindingMacros.h (right): https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:83: v8::TryCatch block; \ On 2015/03/06 04:30:03, Yuki wrote: > On 2015/03/06 02:54:44, bashi1 wrote: > > On 2015/03/06 02:44:07, haraken wrote: > > > > > > Why do we need this TryCatch block? > > > > > > - It's heavy so we need to avoid it as much as possible. > > Yeah, this is my concern, but... > > > - If we just want to rethrow, we don't need the TryCatch block? > > How can we detect a JavaScript exception without TryCatch? IIUC, > ExceptionState > > does nothing with detecting JS exceptions (I might be wrong here, though). > > Maybe, we'd better ask V8 team to make ToLocal() return false whenever an > exception is thrown. @dcarney, WDYT? https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:84: if ((callExpression).ToLocal(&outVariable)) { \ On 2015/03/06 04:30:03, Yuki wrote: > Did you mean?: > if (!(...)) > > I thought that ToLocal returns true on success and false on failure. ToLocal() returns true when the MaybeLocal<T> is empty. That's said, I thought exactly the same as you, and AFAIK, it did return true on success... I'm not sure the reason of this change.
https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8BindingMacros.h (right): https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... Source/bindings/core/v8/V8BindingMacros.h:83: v8::TryCatch block; \ On 2015/03/06 05:11:37, bashi1 wrote: > On 2015/03/06 04:30:03, Yuki wrote: > > On 2015/03/06 02:54:44, bashi1 wrote: > > > On 2015/03/06 02:44:07, haraken wrote: > > > > > > > > Why do we need this TryCatch block? > > > > > > > > - It's heavy so we need to avoid it as much as possible. > > > Yeah, this is my concern, but... > > > > - If we just want to rethrow, we don't need the TryCatch block? > > > How can we detect a JavaScript exception without TryCatch? IIUC, > > ExceptionState > > > does nothing with detecting JS exceptions (I might be wrong here, though). > > > > Maybe, we'd better ask V8 team to make ToLocal() return false whenever an > > exception is thrown. > > @dcarney, WDYT? I'm confused. ToLocal() already returns false when an exception is thrown, because MaybeLocal is an empty handle when an exception is thrown, isn't it?
On 2015/03/06 05:30:52, haraken wrote: > https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... > File Source/bindings/core/v8/V8BindingMacros.h (right): > > https://codereview.chromium.org/970403003/diff/80001/Source/bindings/core/v8/... > Source/bindings/core/v8/V8BindingMacros.h:83: v8::TryCatch block; > \ > On 2015/03/06 05:11:37, bashi1 wrote: > > On 2015/03/06 04:30:03, Yuki wrote: > > > On 2015/03/06 02:54:44, bashi1 wrote: > > > > On 2015/03/06 02:44:07, haraken wrote: > > > > > > > > > > Why do we need this TryCatch block? > > > > > > > > > > - It's heavy so we need to avoid it as much as possible. > > > > Yeah, this is my concern, but... > > > > > - If we just want to rethrow, we don't need the TryCatch block? > > > > How can we detect a JavaScript exception without TryCatch? IIUC, > > > ExceptionState > > > > does nothing with detecting JS exceptions (I might be wrong here, though). > > > > > > Maybe, we'd better ask V8 team to make ToLocal() return false whenever an > > > exception is thrown. > > > > @dcarney, WDYT? > > I'm confused. ToLocal() already returns false when an exception is thrown, > because MaybeLocal is an empty handle when an exception is thrown, isn't it? Ah, right. (ToLocal returns true, though :)
In conclusion, the patch set 6 LGTM.
On 2015/03/06 05:36:14, haraken wrote: > In conclusion, the patch set 6 LGTM. I'll wait @dcarney's comment to make sure that ToLocal() returning IsEmpty() is intentional.
On 2015/03/06 05:38:31, bashi1 wrote: > On 2015/03/06 05:36:14, haraken wrote: > > In conclusion, the patch set 6 LGTM. > > I'll wait @dcarney's comment to make sure that ToLocal() returning IsEmpty() is > intentional. Ah, got it; yes, we want to flip the true/false :)
On 2015/03/06 05:42:58, haraken wrote: > On 2015/03/06 05:38:31, bashi1 wrote: > > On 2015/03/06 05:36:14, haraken wrote: > > > In conclusion, the patch set 6 LGTM. > > > > I'll wait @dcarney's comment to make sure that ToLocal() returning IsEmpty() > is > > intentional. > > Ah, got it; yes, we want to flip the true/false :) i found the bug yesterday and landed the fix. sorry about that
LGTM
Reversed the conditions as the V8 fix is rolled.
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, jl@opera.com Link to the patchset: https://codereview.chromium.org/970403003/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/970403003/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=191599 |
