Chromium Code Reviews
|
|
|
Created:
4 years, 10 months ago by jochen (gone - plz use gerrit) Modified:
4 years, 10 months ago CC:
arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, Inactive, vivekg_samsung, vivekg Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionReturn null when no promise/reason is given in PromiseRejectionEvent
BUG=495801
R=haraken@chromium.org
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197475
Patch Set 1 #
Total comments: 8
Patch Set 2 : updates #
Total comments: 13
Patch Set 3 : updates #
Total comments: 1
Patch Set 4 : updates #Patch Set 5 : updates #Patch Set 6 : updates #
Total comments: 2
Patch Set 7 : updates #Messages
Total messages: 30 (4 generated)
https://codereview.chromium.org/1181353005/diff/1/Source/bindings/core/v8/cus... File Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp (right): https://codereview.chromium.org/1181353005/diff/1/Source/bindings/core/v8/cus... Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:11: #include "bindings/core/v8/V8History.h" This wouldn't be needed. https://codereview.chromium.org/1181353005/diff/1/Source/bindings/core/v8/cus... Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:12: #include "core/events/PopStateEvent.h" Ditto. https://codereview.chromium.org/1181353005/diff/1/Source/bindings/core/v8/cus... Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:13: #include "core/frame/History.h" Ditto. https://codereview.chromium.org/1181353005/diff/1/Source/bindings/core/v8/cus... Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:20: v8::Local<v8::Value> result = V8HiddenValue::getHiddenValue(isolate, info.Holder(), V8HiddenValue::promise(isolate)); What is the point of setting a hidden value here? The hidden value makes sense in CustomEventCustom etc to avoid an unnecessary serialization/deserialization for CustomEvent::m_serializedDetail. My proposal would be to introduce PromiseRejectionEvent::m_serializedPromise and use the same pattern here. It will solve the cross-world-wrapper-leakage problem. https://codereview.chromium.org/1181353005/diff/1/Source/bindings/core/v8/cus... Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:40: v8::Local<v8::Value> result = V8HiddenValue::getHiddenValue(isolate, info.Holder(), V8HiddenValue::reason(isolate)); Ditto. https://codereview.chromium.org/1181353005/diff/1/Source/core/events/PromiseR... File Source/core/events/PromiseRejectionEvent.h (right): https://codereview.chromium.org/1181353005/diff/1/Source/core/events/PromiseR... Source/core/events/PromiseRejectionEvent.h:28: v8::Local<v8::Value> promise(v8::Isolate*) const; Can we keep returning ScriptValue and ScriptPromise? We don't want use raw V8 APIs in core/ (and we're reducing them).
https://codereview.chromium.org/1181353005/diff/1/Source/bindings/core/v8/cus... File Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp (right): https://codereview.chromium.org/1181353005/diff/1/Source/bindings/core/v8/cus... Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:20: v8::Local<v8::Value> result = V8HiddenValue::getHiddenValue(isolate, info.Holder(), V8HiddenValue::promise(isolate)); On 2015/06/18 at 17:52:08, haraken wrote: > What is the point of setting a hidden value here? The hidden value makes sense in CustomEventCustom etc to avoid an unnecessary serialization/deserialization for CustomEvent::m_serializedDetail. > > My proposal would be to introduce PromiseRejectionEvent::m_serializedPromise and use the same pattern here. It will solve the cross-world-wrapper-leakage problem. i should probably just return null if it's from a different world. we can't serialize promises
https://codereview.chromium.org/1181353005/diff/1/Source/bindings/core/v8/cus... File Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp (right): https://codereview.chromium.org/1181353005/diff/1/Source/bindings/core/v8/cus... Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:20: v8::Local<v8::Value> result = V8HiddenValue::getHiddenValue(isolate, info.Holder(), V8HiddenValue::promise(isolate)); On 2015/06/18 18:53:14, jochen wrote: > On 2015/06/18 at 17:52:08, haraken wrote: > > What is the point of setting a hidden value here? The hidden value makes sense > in CustomEventCustom etc to avoid an unnecessary serialization/deserialization > for CustomEvent::m_serializedDetail. > > > > My proposal would be to introduce PromiseRejectionEvent::m_serializedPromise > and use the same pattern here. It will solve the cross-world-wrapper-leakage > problem. > > i should probably just return null if it's from a different world. > > we can't serialize promises Ah, right! Maybe it would be better to have a comment about this -- we don't want to share the promise among worlds. However, doesn't it imply that PromiseRejectionEvent doesn't need to hold the promise? I guess the hidden value would be enough. Also the hidden value on the event wrapper will keep the promise alive.
ptal
we can't have hidden values until the event has a wrapper. I could force wrapper creation, however, that's just another hack :-/
Mostly looks good. https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... File Source/bindings/core/v8/RejectedPromises.cpp (right): https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/RejectedPromises.cpp:152: ScriptState* m_scriptState; yhirano@: Why is this not a RefPtr<ScriptState>? How is it guaranteed that the ScriptState doesn't become a stale pointer? https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... File Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp (right): https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:7: Add more includes. V8Bindings.h, ScriptPromise.h, PromiseRejectionEvent.h, DOMWrapperWorld.h and ScriptState.h. https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:15: if (promise.isEmpty() || promise.scriptValue().scriptState()->world().worldId() != ScriptState::current(isolate)->world().worldId()) { Let's add some comment. // Return null when the promise is accessed by a different world than the world that created the promise. https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:28: if (reason.isEmpty() || reason.scriptState()->world().worldId() != ScriptState::current(isolate)->world().worldId()) { Ditto. https://codereview.chromium.org/1181353005/diff/20001/Source/core/events/Prom... File Source/core/events/PromiseRejectionEvent.cpp (right): https://codereview.chromium.org/1181353005/diff/20001/Source/core/events/Prom... Source/core/events/PromiseRejectionEvent.cpp:35: if (!m_scriptState || !m_scriptState->contextIsValid()) When is it possible that m_scriptState becomes 0? https://codereview.chromium.org/1181353005/diff/20001/Source/core/events/Prom... Source/core/events/PromiseRejectionEvent.cpp:62: void PromiseRejectionEvent::didCollectReason(const v8::WeakCallbackInfo<PromiseRejectionEvent>& data) (Nit: It's not really nice we have to write V8 things in core/, but we need to solve the ScriptValue-leak problem to remove the code...)
haraken@chromium.org changed reviewers: + yhirano@chromium.org
+yhirano
ptal https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... File Source/bindings/core/v8/RejectedPromises.cpp (right): https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/RejectedPromises.cpp:152: ScriptState* m_scriptState; On 2015/06/19 at 11:38:02, haraken wrote: > yhirano@: Why is this not a RefPtr<ScriptState>? How is it guaranteed that the ScriptState doesn't become a stale pointer? the scriptstate can't become invalid before the promise was collected at which point m_collected = true and we no longer access the message. https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... File Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp (right): https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:7: On 2015/06/19 at 11:38:02, haraken wrote: > Add more includes. V8Bindings.h, ScriptPromise.h, PromiseRejectionEvent.h, DOMWrapperWorld.h and ScriptState.h. done https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:15: if (promise.isEmpty() || promise.scriptValue().scriptState()->world().worldId() != ScriptState::current(isolate)->world().worldId()) { On 2015/06/19 at 11:38:03, haraken wrote: > Let's add some comment. > > // Return null when the promise is accessed by a different world than the world that created the promise. done https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:28: if (reason.isEmpty() || reason.scriptState()->world().worldId() != ScriptState::current(isolate)->world().worldId()) { On 2015/06/19 at 11:38:02, haraken wrote: > Ditto. done https://codereview.chromium.org/1181353005/diff/20001/Source/core/events/Prom... File Source/core/events/PromiseRejectionEvent.cpp (right): https://codereview.chromium.org/1181353005/diff/20001/Source/core/events/Prom... Source/core/events/PromiseRejectionEvent.cpp:35: if (!m_scriptState || !m_scriptState->contextIsValid()) On 2015/06/19 at 11:38:03, haraken wrote: > When is it possible that m_scriptState becomes 0? The ctor that doesn't take parameters doesn't initialize m_scriptState - it doesn't become null in that case, it always is null
https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... File Source/bindings/core/v8/RejectedPromises.cpp (right): https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/RejectedPromises.cpp:152: ScriptState* m_scriptState; On 2015/06/19 11:38:02, haraken wrote: > > yhirano@: Why is this not a RefPtr<ScriptState>? How is it guaranteed that the > ScriptState doesn't become a stale pointer? Hm, I don't know, I've never seen RejectedPromises files.
https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... File Source/bindings/core/v8/RejectedPromises.cpp (right): https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/RejectedPromises.cpp:152: ScriptState* m_scriptState; On 2015/06/19 12:06:21, jochen wrote: > On 2015/06/19 at 11:38:02, haraken wrote: > > yhirano@: Why is this not a RefPtr<ScriptState>? How is it guaranteed that the > ScriptState doesn't become a stale pointer? > > the scriptstate can't become invalid before the promise was collected at which > point m_collected = true and we no longer access the message. You mean, the ScriptState can't become invalid before the promise is collected because the promise has a strong reference to the creation context, and the creation context has a strong reference to the ScriptState? That sounds reasonable, but a common pattern is just to use RefPtr<ScriptState>. (Not related to this CL though.) https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... File Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp (right): https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:21: if (promise.isEmpty() || promise.scriptValue().scriptState()->world().worldId() != ScriptState::current(isolate)->world().worldId()) { Sorry for an iterative comment... By moving this check into PromiseRejectionEvent::promise(), can we get rid of the custom binding? In other words, if PromiseRejectionEvent::promise() is implemented in a way that returns a valid ScriptPromise only for the same world, we won't need this custom binding.
On 2015/06/19 at 12:17:33, haraken wrote: > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > File Source/bindings/core/v8/RejectedPromises.cpp (right): > > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > Source/bindings/core/v8/RejectedPromises.cpp:152: ScriptState* m_scriptState; > On 2015/06/19 12:06:21, jochen wrote: > > On 2015/06/19 at 11:38:02, haraken wrote: > > > yhirano@: Why is this not a RefPtr<ScriptState>? How is it guaranteed that the > > ScriptState doesn't become a stale pointer? > > > > the scriptstate can't become invalid before the promise was collected at which > > point m_collected = true and we no longer access the message. > > You mean, the ScriptState can't become invalid before the promise is collected because the promise has a strong reference to the creation context, and the creation context has a strong reference to the ScriptState? > > That sounds reasonable, but a common pattern is just to use RefPtr<ScriptState>. (Not related to this CL though.) > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > File Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp (right): > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:21: if (promise.isEmpty() || promise.scriptValue().scriptState()->world().worldId() != ScriptState::current(isolate)->world().worldId()) { > > Sorry for an iterative comment... > > By moving this check into PromiseRejectionEvent::promise(), can we get rid of the custom binding? In other words, if PromiseRejectionEvent::promise() is implemented in a way that returns a valid ScriptPromise only for the same world, we won't need this custom binding. we need the custom bindings mainly to be able to return null instead of undefined
it looks like custom bindings don't depend on idl generation, an so incremental builds are broken? :-/
On 2015/06/19 12:32:05, jochen wrote: > On 2015/06/19 at 12:17:33, haraken wrote: > > > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > > File Source/bindings/core/v8/RejectedPromises.cpp (right): > > > > > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > > Source/bindings/core/v8/RejectedPromises.cpp:152: ScriptState* m_scriptState; > > On 2015/06/19 12:06:21, jochen wrote: > > > On 2015/06/19 at 11:38:02, haraken wrote: > > > > yhirano@: Why is this not a RefPtr<ScriptState>? How is it guaranteed that > the > > > ScriptState doesn't become a stale pointer? > > > > > > the scriptstate can't become invalid before the promise was collected at > which > > > point m_collected = true and we no longer access the message. > > > > You mean, the ScriptState can't become invalid before the promise is collected > because the promise has a strong reference to the creation context, and the > creation context has a strong reference to the ScriptState? > > > > That sounds reasonable, but a common pattern is just to use > RefPtr<ScriptState>. (Not related to this CL though.) > > > > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > > File Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp (right): > > > > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > > Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:21: if > (promise.isEmpty() || promise.scriptValue().scriptState()->world().worldId() != > ScriptState::current(isolate)->world().worldId()) { > > > > Sorry for an iterative comment... > > > > By moving this check into PromiseRejectionEvent::promise(), can we get rid of > the custom binding? In other words, if PromiseRejectionEvent::promise() is > implemented in a way that returns a valid ScriptPromise only for the same world, > we won't need this custom binding. > > we need the custom bindings mainly to be able to return null instead of > undefined I think you can use [TreatUndefinedAs=Null], but we're deprecating the IDL extended attribute. If you have a strong reason to use it, you can use it, but otherwise it would be better to use undefined in this kind of situation.
On 2015/06/19 at 12:52:56, haraken wrote: > On 2015/06/19 12:32:05, jochen wrote: > > On 2015/06/19 at 12:17:33, haraken wrote: > > > > > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > > > File Source/bindings/core/v8/RejectedPromises.cpp (right): > > > > > > > > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > > > Source/bindings/core/v8/RejectedPromises.cpp:152: ScriptState* m_scriptState; > > > On 2015/06/19 12:06:21, jochen wrote: > > > > On 2015/06/19 at 11:38:02, haraken wrote: > > > > > yhirano@: Why is this not a RefPtr<ScriptState>? How is it guaranteed that > > the > > > > ScriptState doesn't become a stale pointer? > > > > > > > > the scriptstate can't become invalid before the promise was collected at > > which > > > > point m_collected = true and we no longer access the message. > > > > > > You mean, the ScriptState can't become invalid before the promise is collected > > because the promise has a strong reference to the creation context, and the > > creation context has a strong reference to the ScriptState? > > > > > > That sounds reasonable, but a common pattern is just to use > > RefPtr<ScriptState>. (Not related to this CL though.) > > > > > > > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > > > File Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp (right): > > > > > > > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > > > Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:21: if > > (promise.isEmpty() || promise.scriptValue().scriptState()->world().worldId() != > > ScriptState::current(isolate)->world().worldId()) { > > > > > > Sorry for an iterative comment... > > > > > > By moving this check into PromiseRejectionEvent::promise(), can we get rid of > > the custom binding? In other words, if PromiseRejectionEvent::promise() is > > implemented in a way that returns a valid ScriptPromise only for the same world, > > we won't need this custom binding. > > > > we need the custom bindings mainly to be able to return null instead of > > undefined > > I think you can use [TreatUndefinedAs=Null], but we're deprecating the IDL extended attribute. If you have a strong reason to use it, you can use it, but otherwise it would be better to use undefined in this kind of situation. that's how DOM events are defined in general (returning null if a value is not present) I can't return undefined as ScriptPromise
On 2015/06/19 12:55:40, jochen wrote: > On 2015/06/19 at 12:52:56, haraken wrote: > > On 2015/06/19 12:32:05, jochen wrote: > > > On 2015/06/19 at 12:17:33, haraken wrote: > > > > > > > > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > > > > File Source/bindings/core/v8/RejectedPromises.cpp (right): > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > > > > Source/bindings/core/v8/RejectedPromises.cpp:152: ScriptState* > m_scriptState; > > > > On 2015/06/19 12:06:21, jochen wrote: > > > > > On 2015/06/19 at 11:38:02, haraken wrote: > > > > > > yhirano@: Why is this not a RefPtr<ScriptState>? How is it guaranteed > that > > > the > > > > > ScriptState doesn't become a stale pointer? > > > > > > > > > > the scriptstate can't become invalid before the promise was collected at > > > which > > > > > point m_collected = true and we no longer access the message. > > > > > > > > You mean, the ScriptState can't become invalid before the promise is > collected > > > because the promise has a strong reference to the creation context, and the > > > creation context has a strong reference to the ScriptState? > > > > > > > > That sounds reasonable, but a common pattern is just to use > > > RefPtr<ScriptState>. (Not related to this CL though.) > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > > > > File Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp > (right): > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > > > > Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:21: if > > > (promise.isEmpty() || promise.scriptValue().scriptState()->world().worldId() > != > > > ScriptState::current(isolate)->world().worldId()) { > > > > > > > > Sorry for an iterative comment... > > > > > > > > By moving this check into PromiseRejectionEvent::promise(), can we get rid > of > > > the custom binding? In other words, if PromiseRejectionEvent::promise() is > > > implemented in a way that returns a valid ScriptPromise only for the same > world, > > > we won't need this custom binding. > > > > > > we need the custom bindings mainly to be able to return null instead of > > > undefined > > > > I think you can use [TreatUndefinedAs=Null], but we're deprecating the IDL > extended attribute. If you have a strong reason to use it, you can use it, but > otherwise it would be better to use undefined in this kind of situation. > > that's how DOM events are defined in general (returning null if a value is not > present) > > I can't return undefined as ScriptPromise Maybe that's a bug of the IDL compiler. Shall we add a rule that ScriptPromise's empty value should be converted to null? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2015/06/19 at 12:59:47, haraken wrote: > On 2015/06/19 12:55:40, jochen wrote: > > On 2015/06/19 at 12:52:56, haraken wrote: > > > On 2015/06/19 12:32:05, jochen wrote: > > > > On 2015/06/19 at 12:17:33, haraken wrote: > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > > > > > File Source/bindings/core/v8/RejectedPromises.cpp (right): > > > > > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > > > > > Source/bindings/core/v8/RejectedPromises.cpp:152: ScriptState* > > m_scriptState; > > > > > On 2015/06/19 12:06:21, jochen wrote: > > > > > > On 2015/06/19 at 11:38:02, haraken wrote: > > > > > > > yhirano@: Why is this not a RefPtr<ScriptState>? How is it guaranteed > > that > > > > the > > > > > > ScriptState doesn't become a stale pointer? > > > > > > > > > > > > the scriptstate can't become invalid before the promise was collected at > > > > which > > > > > > point m_collected = true and we no longer access the message. > > > > > > > > > > You mean, the ScriptState can't become invalid before the promise is > > collected > > > > because the promise has a strong reference to the creation context, and the > > > > creation context has a strong reference to the ScriptState? > > > > > > > > > > That sounds reasonable, but a common pattern is just to use > > > > RefPtr<ScriptState>. (Not related to this CL though.) > > > > > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > > > > > File Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp > > (right): > > > > > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > > > > > Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:21: if > > > > (promise.isEmpty() || promise.scriptValue().scriptState()->world().worldId() > > != > > > > ScriptState::current(isolate)->world().worldId()) { > > > > > > > > > > Sorry for an iterative comment... > > > > > > > > > > By moving this check into PromiseRejectionEvent::promise(), can we get rid > > of > > > > the custom binding? In other words, if PromiseRejectionEvent::promise() is > > > > implemented in a way that returns a valid ScriptPromise only for the same > > world, > > > > we won't need this custom binding. > > > > > > > > we need the custom bindings mainly to be able to return null instead of > > > > undefined > > > > > > I think you can use [TreatUndefinedAs=Null], but we're deprecating the IDL > > extended attribute. If you have a strong reason to use it, you can use it, but > > otherwise it would be better to use undefined in this kind of situation. > > > > that's how DOM events are defined in general (returning null if a value is not > > present) > > > > I can't return undefined as ScriptPromise > > Maybe that's a bug of the IDL compiler. Shall we add a rule that ScriptPromise's empty value should be converted to null? > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... that would also affect all other code that returns ScriptPromises, no?
On 2015/06/19 13:02:03, jochen wrote: > On 2015/06/19 at 12:59:47, haraken wrote: > > On 2015/06/19 12:55:40, jochen wrote: > > > On 2015/06/19 at 12:52:56, haraken wrote: > > > > On 2015/06/19 12:32:05, jochen wrote: > > > > > On 2015/06/19 at 12:17:33, haraken wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > > > > > > File Source/bindings/core/v8/RejectedPromises.cpp (right): > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > > > > > > Source/bindings/core/v8/RejectedPromises.cpp:152: ScriptState* > > > m_scriptState; > > > > > > On 2015/06/19 12:06:21, jochen wrote: > > > > > > > On 2015/06/19 at 11:38:02, haraken wrote: > > > > > > > > yhirano@: Why is this not a RefPtr<ScriptState>? How is it > guaranteed > > > that > > > > > the > > > > > > > ScriptState doesn't become a stale pointer? > > > > > > > > > > > > > > the scriptstate can't become invalid before the promise was > collected at > > > > > which > > > > > > > point m_collected = true and we no longer access the message. > > > > > > > > > > > > You mean, the ScriptState can't become invalid before the promise is > > > collected > > > > > because the promise has a strong reference to the creation context, and > the > > > > > creation context has a strong reference to the ScriptState? > > > > > > > > > > > > That sounds reasonable, but a common pattern is just to use > > > > > RefPtr<ScriptState>. (Not related to this CL though.) > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > > > > > > File Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp > > > (right): > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > > > > > > Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:21: > if > > > > > (promise.isEmpty() || > promise.scriptValue().scriptState()->world().worldId() > > > != > > > > > ScriptState::current(isolate)->world().worldId()) { > > > > > > > > > > > > Sorry for an iterative comment... > > > > > > > > > > > > By moving this check into PromiseRejectionEvent::promise(), can we get > rid > > > of > > > > > the custom binding? In other words, if PromiseRejectionEvent::promise() > is > > > > > implemented in a way that returns a valid ScriptPromise only for the > same > > > world, > > > > > we won't need this custom binding. > > > > > > > > > > we need the custom bindings mainly to be able to return null instead of > > > > > undefined > > > > > > > > I think you can use [TreatUndefinedAs=Null], but we're deprecating the IDL > > > extended attribute. If you have a strong reason to use it, you can use it, > but > > > otherwise it would be better to use undefined in this kind of situation. > > > > > > that's how DOM events are defined in general (returning null if a value is > not > > > present) > > > > > > I can't return undefined as ScriptPromise > > > > Maybe that's a bug of the IDL compiler. Shall we add a rule that > ScriptPromise's empty value should be converted to null? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > that would also affect all other code that returns ScriptPromises, no? You mean, you want to return undefined for normal ScriptPromises but null only for ScriptPromises held by Event? (I think it depends on the spec.) In case of CustomEvent.detail, it returns null because the default value of CustomEventInit.detail is speced as null: http://www.w3.org/TR/dom/#customevent
On 2015/06/19 at 13:13:36, haraken wrote: > On 2015/06/19 13:02:03, jochen wrote: > > On 2015/06/19 at 12:59:47, haraken wrote: > > > On 2015/06/19 12:55:40, jochen wrote: > > > > On 2015/06/19 at 12:52:56, haraken wrote: > > > > > On 2015/06/19 12:32:05, jochen wrote: > > > > > > On 2015/06/19 at 12:17:33, haraken wrote: > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > > > > > > > File Source/bindings/core/v8/RejectedPromises.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > > > > > > > Source/bindings/core/v8/RejectedPromises.cpp:152: ScriptState* > > > > m_scriptState; > > > > > > > On 2015/06/19 12:06:21, jochen wrote: > > > > > > > > On 2015/06/19 at 11:38:02, haraken wrote: > > > > > > > > > yhirano@: Why is this not a RefPtr<ScriptState>? How is it > > guaranteed > > > > that > > > > > > the > > > > > > > > ScriptState doesn't become a stale pointer? > > > > > > > > > > > > > > > > the scriptstate can't become invalid before the promise was > > collected at > > > > > > which > > > > > > > > point m_collected = true and we no longer access the message. > > > > > > > > > > > > > > You mean, the ScriptState can't become invalid before the promise is > > > > collected > > > > > > because the promise has a strong reference to the creation context, and > > the > > > > > > creation context has a strong reference to the ScriptState? > > > > > > > > > > > > > > That sounds reasonable, but a common pattern is just to use > > > > > > RefPtr<ScriptState>. (Not related to this CL though.) > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > > > > > > > File Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > > > > > > > Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:21: > > if > > > > > > (promise.isEmpty() || > > promise.scriptValue().scriptState()->world().worldId() > > > > != > > > > > > ScriptState::current(isolate)->world().worldId()) { > > > > > > > > > > > > > > Sorry for an iterative comment... > > > > > > > > > > > > > > By moving this check into PromiseRejectionEvent::promise(), can we get > > rid > > > > of > > > > > > the custom binding? In other words, if PromiseRejectionEvent::promise() > > is > > > > > > implemented in a way that returns a valid ScriptPromise only for the > > same > > > > world, > > > > > > we won't need this custom binding. > > > > > > > > > > > > we need the custom bindings mainly to be able to return null instead of > > > > > > undefined > > > > > > > > > > I think you can use [TreatUndefinedAs=Null], but we're deprecating the IDL > > > > extended attribute. If you have a strong reason to use it, you can use it, > > but > > > > otherwise it would be better to use undefined in this kind of situation. > > > > > > > > that's how DOM events are defined in general (returning null if a value is > > not > > > > present) > > > > > > > > I can't return undefined as ScriptPromise > > > > > > Maybe that's a bug of the IDL compiler. Shall we add a rule that > > ScriptPromise's empty value should be converted to null? > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > that would also affect all other code that returns ScriptPromises, no? > > You mean, you want to return undefined for normal ScriptPromises but null only for ScriptPromises held by Event? (I think it depends on the spec.) > > In case of CustomEvent.detail, it returns null because the default value of CustomEventInit.detail is speced as null: http://www.w3.org/TR/dom/#customevent yes, it's also spec'd for the promise rejection event. And for ErrorEvent.error and PopStateEvent.data (that's all I checked :) )
On 2015/06/19 13:15:12, jochen wrote: > On 2015/06/19 at 13:13:36, haraken wrote: > > On 2015/06/19 13:02:03, jochen wrote: > > > On 2015/06/19 at 12:59:47, haraken wrote: > > > > On 2015/06/19 12:55:40, jochen wrote: > > > > > On 2015/06/19 at 12:52:56, haraken wrote: > > > > > > On 2015/06/19 12:32:05, jochen wrote: > > > > > > > On 2015/06/19 at 12:17:33, haraken wrote: > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > > > > > > > > File Source/bindings/core/v8/RejectedPromises.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/20001/Source/bindings/core/v8... > > > > > > > > Source/bindings/core/v8/RejectedPromises.cpp:152: ScriptState* > > > > > m_scriptState; > > > > > > > > On 2015/06/19 12:06:21, jochen wrote: > > > > > > > > > On 2015/06/19 at 11:38:02, haraken wrote: > > > > > > > > > > yhirano@: Why is this not a RefPtr<ScriptState>? How is it > > > guaranteed > > > > > that > > > > > > > the > > > > > > > > > ScriptState doesn't become a stale pointer? > > > > > > > > > > > > > > > > > > the scriptstate can't become invalid before the promise was > > > collected at > > > > > > > which > > > > > > > > > point m_collected = true and we no longer access the message. > > > > > > > > > > > > > > > > You mean, the ScriptState can't become invalid before the promise > is > > > > > collected > > > > > > > because the promise has a strong reference to the creation context, > and > > > the > > > > > > > creation context has a strong reference to the ScriptState? > > > > > > > > > > > > > > > > That sounds reasonable, but a common pattern is just to use > > > > > > > RefPtr<ScriptState>. (Not related to this CL though.) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > > > > > > > > File > Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1181353005/diff/40001/Source/bindings/core/v8... > > > > > > > > > Source/bindings/core/v8/custom/V8PromiseRejectionEventCustom.cpp:21: > > > if > > > > > > > (promise.isEmpty() || > > > promise.scriptValue().scriptState()->world().worldId() > > > > > != > > > > > > > ScriptState::current(isolate)->world().worldId()) { > > > > > > > > > > > > > > > > Sorry for an iterative comment... > > > > > > > > > > > > > > > > By moving this check into PromiseRejectionEvent::promise(), can we > get > > > rid > > > > > of > > > > > > > the custom binding? In other words, if > PromiseRejectionEvent::promise() > > > is > > > > > > > implemented in a way that returns a valid ScriptPromise only for the > > > same > > > > > world, > > > > > > > we won't need this custom binding. > > > > > > > > > > > > > > we need the custom bindings mainly to be able to return null instead > of > > > > > > > undefined > > > > > > > > > > > > I think you can use [TreatUndefinedAs=Null], but we're deprecating the > IDL > > > > > extended attribute. If you have a strong reason to use it, you can use > it, > > > but > > > > > otherwise it would be better to use undefined in this kind of situation. > > > > > > > > > > that's how DOM events are defined in general (returning null if a value > is > > > not > > > > > present) > > > > > > > > > > I can't return undefined as ScriptPromise > > > > > > > > Maybe that's a bug of the IDL compiler. Shall we add a rule that > > > ScriptPromise's empty value should be converted to null? > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > that would also affect all other code that returns ScriptPromises, no? > > > > You mean, you want to return undefined for normal ScriptPromises but null only > for ScriptPromises held by Event? (I think it depends on the spec.) > > > > In case of CustomEvent.detail, it returns null because the default value of > CustomEventInit.detail is speced as null: http://www.w3.org/TR/dom/#customevent > > yes, it's also spec'd for the promise rejection event. And for ErrorEvent.error > and PopStateEvent.data (that's all I checked :) ) OK, then let's try [TreatUndefinedAs=Null]. If it doesn't work, I'm fine with the custom binding (but the world check could be moved into the PromiseRejectionEvent::promise() -- it would be better to embed the check mechanism in the getter itself).
haraken@chromium.org changed reviewers: + bashi@chromium.org
+bashi, an expert of EventInits In short, I'm wondering whether the default value of PromiseRejectionEvent::promise should be null or undefined. Please just catch up the discussion in comment #15 - #19 :)
moved the world check to the event. undefined as null doesn't work because it's valid to reject a promise with undefined as reason, and in that case, I actually want to return undefined.
LGTM. Thanks for a lot of iterations. https://codereview.chromium.org/1181353005/diff/100001/Source/core/events/Pro... File Source/core/events/PromiseRejectionEvent.cpp (right): https://codereview.chromium.org/1181353005/diff/100001/Source/core/events/Pro... Source/core/events/PromiseRejectionEvent.cpp:39: return ScriptPromise(); One idea would be to add an ability to create a null ScriptPromise. Then we won't need the custom binding. https://codereview.chromium.org/1181353005/diff/100001/Source/core/events/Pro... Source/core/events/PromiseRejectionEvent.cpp:47: return ScriptValue(); If we return ScriptValue(v8::Null()), we won't need the custom binding. That said, bashi@ would know better about this, so I'll defer the decision to bashi@. Jochen: Feel free to land this CL.
The CQ bit was checked by jochen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1181353005/#ps120001 (title: "updates")
moved reason() to non-custom bindings
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181353005/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197475 |
