Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17993004:
Implement PromiseResolver.prototype.{fulfill, reject} (Closed)
|
Created:
7 years, 5 months ago by yhirano Modified:
7 years, 5 months ago CC:
blink-reviews, Nils Barth (inactive), jsbell+bindings_chromium.org, eae+blinkwatch, marja+watch_chromium.org, dglazkov+blink, adamk+blink_chromium.org, Nate Chapin, do-not-use Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionImplement PromiseResolver.prototype.{fulfill, reject}
We are implementing DOM/Promises.
The first CL is https://src.chromium.org/viewvc/blink?revision=152943&view=revision .
This CL implements PromiseResolver.prototype.fulfill and PromiseResolver.prototype.reject.
Again, all methods are marked as Custom and there are many boilerplate. we will fix them in a future CL.
There are no tests. We will add tests in a future.
BUG=243345
R=abarth
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153199
Patch Set 1 #Patch Set 2 : #
Total comments: 35
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 4
Patch Set 7 : #
Messages
Total messages: 20 (0 generated)
For the global receiver problem pointed in [1], I have no idea how to call a function without receiver. Is it better to get the global object when we are about to call a function? Is there another way? [1] https://codereview.chromium.org/17874002/
On 2013/06/27 06:10:06, yhirano wrote: > For the global receiver problem pointed in [1], I have no idea how to call a > function without receiver. Can we pass v8::Undefined or an empty handle? > Is it better to get the global object when we are about to call a function? Is > there another way? > > [1] https://codereview.chromium.org/17874002/
On 2013/06/27 06:21:04, abarth wrote: > On 2013/06/27 06:10:06, yhirano wrote: > > For the global receiver problem pointed in [1], I have no idea how to call a > > function without receiver. > > Can we pass v8::Undefined or an empty handle? > > > Is it better to get the global object when we are about to call a function? Is > > there another way? > > > > [1] https://codereview.chromium.org/17874002/ We can't pass undefined because callFunction takes v8::Handle<v8::Object>. Passing an empty handle causes crash.
On 2013/06/27 06:25:59, yhirano wrote: > On 2013/06/27 06:21:04, abarth wrote: > > On 2013/06/27 06:10:06, yhirano wrote: > > > For the global receiver problem pointed in [1], I have no idea how to call a > > > function without receiver. > > > > Can we pass v8::Undefined or an empty handle? > > > > > Is it better to get the global object when we are about to call a function? > Is > > > there another way? > > > > > > [1] https://codereview.chromium.org/17874002/ > > We can't pass undefined because callFunction takes v8::Handle<v8::Object>. > Passing an empty handle causes crash. I think I can fix it. Just a second. As far as I read the code, it looks like V8 is intending to use a global object as a receiver when a receiver is undefined. https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/execution.c...
LGTM https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:63: ASSERT(!result.IsEmpty()); You can check the m_ versions of these, if you like. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:89: ASSERT(!m_result.newLocal(isolate).IsEmpty()); No need to call newLocal here. ScopedPersistent has an isEmpty() function. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:157: value = internal->GetInternalField(V8PromiseCustom::InternalFulfillCallbackIndex); You can combine these lines. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:171: call(callback, global, result, mode, isolate); Do we need to check for exceptions? https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:175: void V8PromiseCustom::rejectResolver(v8::Handle<v8::Object> resolver, v8::Handle<v8::Value> result, SynchronousMode mode, v8::Isolate* isolate) Can we share more code between this function and fulfillResolver? They seem to be doing basically the same things. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:271: double number = value.As<v8::Number>()->Value(); Why not use a v8::Integer ? We shouldn't have to use doubles here. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:288: v8::TryCatch trycatch; I see. We catch the exception and ignore it.
It would be better to pass Undefined and let V8 find the global object for us, if we can make that work.
> I think I can fix it. Just a second. > > As far as I read the code, it looks like V8 is intending to use a global object > as a receiver when a receiver is undefined. > https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/execution.c... Oh, I found Function::Call() calls Execution::Call(..., bool convert_receiver = false), which means that V8 doesn't want to convert an undefined handle to a global object implicitly. Why can't you pass the global object when you call callFunction()? I mean, you can just call V8ScriptRunner::callFunction(, context->Global(), ...).
On 2013/06/27 06:39:36, haraken wrote: > Why can't you pass the global object when you call callFunction()? I mean, you > can just call V8ScriptRunner::callFunction(, context->Global(), ...). I guess it's ok. It's just slightly error prone.
It's a bit sad you have to write a lot of ASSERT(!value.IsEmpty()); ASSERT(value->IsXXX()); v8::Handle<v8::Value> v = value.As<XXX>(); Would you define a helper method like toArray() and toFunction() in V8Binding.h and simplify your code? https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:79: ASSERT(context->isDocument()); Nit: You can move this check to the head of this method. ASSERT(context && context->isDocument()) would be better. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:91: V8ScriptRunner::callFunction(m_callback.newLocal(isolate), context, m_receiver.newLocal(isolate), 1, &result); Nit: We normally write this as follows: v8::Handle<v8::Value> args[] = { result }; V8ScriptRunner::callFunction(..., WTF_ARRAY_LENGTH(args), args); https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:249: if (value->IsNull()) What if value is undefined? https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:271: double number = value.As<v8::Number>()->Value(); You can use toInt32() or some utility methods in V8Binding.h. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:289: V8ScriptRunner::callFunction(function, getScriptExecutionContext(), receiver, 1, &result); Nit: WTF_ARRAY_LENGTH(args). https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... File Source/bindings/v8/custom/V8PromiseResolverCustom.cpp (right): https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseResolverCustom.cpp:43: v8::Local<v8::Value> result = v8::Undefined(); You don't need to set v8::Undefined(). https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseResolverCustom.cpp:44: if (args.Length() > 0 && !args[0].IsEmpty()) Do you need to check !args[0].IsEmpty()? Normally we don't have this kind of check. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseResolverCustom.cpp:48: v8SetReturnValue(args, v8::Undefined()); Remove this. v8::Undefined() is set by default. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseResolverCustom.cpp:55: v8::Local<v8::Value> result = v8::Undefined(); Ditto. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseResolverCustom.cpp:56: if (args.Length() > 0 && !args[0].IsEmpty()) Ditto. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseResolverCustom.cpp:60: v8SetReturnValue(args, v8::Undefined()); Ditto.
Does value.As<XXX>(); ASSERT already in V8?
On 2013/06/27 07:43:20, abarth wrote: > Does value.As<XXX>(); ASSERT already in V8? There seems to be, but I see no crash when I do an invalid cast in a layout test with --debug. https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&q=...
Maybe we don't have V8_ENABLE_CHECKS enabled, even in debug. (This isn't a problem we need to solve before landing this CL.)
On 2013/06/27 08:22:33, abarth wrote: > Maybe we don't have V8_ENABLE_CHECKS enabled, even in debug. (This isn't a > problem we need to solve before landing this CL.) You are right. Then, can I delete many of IsEmpty() and IsXxx() from my CL? Many of IsEmpty() can be deleted safely and IsXxx() will be covered by As<Xxx> if V8_ENABLE_CHECKS will be on.
> Then, can I delete many of IsEmpty() and IsXxx() from my CL? > Many of IsEmpty() can be deleted safely and IsXxx() will be covered by As<Xxx> > if V8_ENABLE_CHECKS will be on. Sounds reasonable to me.
https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:63: ASSERT(!result.IsEmpty()); On 2013/06/27 06:34:01, abarth wrote: > You can check the m_ versions of these, if you like. Done. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:79: ASSERT(context->isDocument()); On 2013/06/27 06:55:56, haraken wrote: > > Nit: You can move this check to the head of this method. ASSERT(context && > context->isDocument()) would be better. Done. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:89: ASSERT(!m_result.newLocal(isolate).IsEmpty()); On 2013/06/27 06:34:01, abarth wrote: > No need to call newLocal here. ScopedPersistent has an isEmpty() function. I deleted the ASSERTs. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:91: V8ScriptRunner::callFunction(m_callback.newLocal(isolate), context, m_receiver.newLocal(isolate), 1, &result); On 2013/06/27 06:55:56, haraken wrote: > > Nit: We normally write this as follows: > > v8::Handle<v8::Value> args[] = { result }; > V8ScriptRunner::callFunction(..., WTF_ARRAY_LENGTH(args), args); Done. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:157: value = internal->GetInternalField(V8PromiseCustom::InternalFulfillCallbackIndex); On 2013/06/27 06:34:01, abarth wrote: > You can combine these lines. Done. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:171: call(callback, global, result, mode, isolate); On 2013/06/27 06:34:01, abarth wrote: > Do we need to check for exceptions? V8PromiseCustom::call doesn't throws an exception, as stated in the header comment. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:175: void V8PromiseCustom::rejectResolver(v8::Handle<v8::Object> resolver, v8::Handle<v8::Value> result, SynchronousMode mode, v8::Isolate* isolate) On 2013/06/27 06:34:01, abarth wrote: > Can we share more code between this function and fulfillResolver? They seem to > be doing basically the same things. Done. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:249: if (value->IsNull()) On 2013/06/27 06:55:56, haraken wrote: > > What if value is undefined? Null was the special value that indicates the internal object is detached from the resolver. I added utility functions for readability and changed the detached mark from null to undefined. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:271: double number = value.As<v8::Number>()->Value(); On 2013/06/27 06:55:56, haraken wrote: > You can use toInt32() or some utility methods in V8Binding.h. Done. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:288: v8::TryCatch trycatch; On 2013/06/27 06:34:01, abarth wrote: > I see. We catch the exception and ignore it. I added a comment. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseCustom.cpp:289: V8ScriptRunner::callFunction(function, getScriptExecutionContext(), receiver, 1, &result); On 2013/06/27 06:55:56, haraken wrote: > > Nit: WTF_ARRAY_LENGTH(args). Done. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... File Source/bindings/v8/custom/V8PromiseResolverCustom.cpp (right): https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseResolverCustom.cpp:43: v8::Local<v8::Value> result = v8::Undefined(); On 2013/06/27 06:55:56, haraken wrote: > > You don't need to set v8::Undefined(). Done. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseResolverCustom.cpp:44: if (args.Length() > 0 && !args[0].IsEmpty()) On 2013/06/27 06:55:56, haraken wrote: > > Do you need to check !args[0].IsEmpty()? Normally we don't have this kind of > check. Done. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseResolverCustom.cpp:48: v8SetReturnValue(args, v8::Undefined()); On 2013/06/27 06:55:56, haraken wrote: > > Remove this. v8::Undefined() is set by default. Done. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseResolverCustom.cpp:55: v8::Local<v8::Value> result = v8::Undefined(); On 2013/06/27 06:55:56, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseResolverCustom.cpp:56: if (args.Length() > 0 && !args[0].IsEmpty()) On 2013/06/27 06:55:56, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/17993004/diff/3001/Source/bindings/v8/custom/... Source/bindings/v8/custom/V8PromiseResolverCustom.cpp:60: v8SetReturnValue(args, v8::Undefined()); On 2013/06/27 06:55:56, haraken wrote: > > Ditto. Done.
LGTM https://codereview.chromium.org/17993004/diff/32001/Source/bindings/v8/custom... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/17993004/diff/32001/Source/bindings/v8/custom... Source/bindings/v8/custom/V8PromiseCustom.cpp:226: return value->IsUndefined(); You might want to add ASSERT(!value.IsEmpty()) just above this line. https://codereview.chromium.org/17993004/diff/32001/Source/bindings/v8/custom... Source/bindings/v8/custom/V8PromiseCustom.cpp:263: V8ScriptRunner::callFunction(function, getScriptExecutionContext(), receiver, WTF_ARRAY_LENGTH(args), args); Are you sure that you've entered the context? If you're not sure, you can add: v8::Context::Scope scope(context_you_want_to_enter);
https://codereview.chromium.org/17993004/diff/32001/Source/bindings/v8/custom... File Source/bindings/v8/custom/V8PromiseCustom.cpp (right): https://codereview.chromium.org/17993004/diff/32001/Source/bindings/v8/custom... Source/bindings/v8/custom/V8PromiseCustom.cpp:226: return value->IsUndefined(); On 2013/06/27 10:36:29, haraken wrote: > > You might want to add ASSERT(!value.IsEmpty()) just above this line. Done. https://codereview.chromium.org/17993004/diff/32001/Source/bindings/v8/custom... Source/bindings/v8/custom/V8PromiseCustom.cpp:263: V8ScriptRunner::callFunction(function, getScriptExecutionContext(), receiver, WTF_ARRAY_LENGTH(args), args); On 2013/06/27 10:36:29, haraken wrote: > > Are you sure that you've entered the context? If you're not sure, you can add: > > v8::Context::Scope scope(context_you_want_to_enter); > Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/17993004/28006
Message was sent while issue was closed.
Change committed as 153199 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
