Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
311733004:
Introduce KeepAliveWhilePending to ScriptPromiseResolverWithContext. (Closed)
|
Created:
6 years, 6 months ago by yhirano Modified:
6 years, 6 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, sof, eae+blinkwatch, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@refactor-webmidi-initialization Visibility:
Public. |
DescriptionIntroduce KeepAliveWhilePending to ScriptPromiseResolverWithContext.
Some modules, for example WebMIDI and WebCrypto, needs "Async Initializer".
The initializer should stay alive until the initialization succeeds (i.e. the
associated Promise is resolved), the initialization fails
(i.e. rejected), or the associated ExecutionContext is stopped.
This CL introduces the the constructor mode ScriptPromiseResolverWithContext.
If KeepAliveWhilePending is specified, the created resolver stays alive while
one of the above conditions meets.
Each initializer can stay alive by inheriting ScriptPromiseResolverWithContext.
This CL rewrites WebMIDI and WebCrypto async operations with it.
BUG=361041
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176381
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176403
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : rebase #Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : #
Total comments: 8
Patch Set 8 : #
Total comments: 21
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #
Total comments: 10
Patch Set 14 : #Patch Set 15 : #
Total comments: 6
Patch Set 16 : #
Total comments: 10
Patch Set 17 : #Patch Set 18 : #
Total comments: 10
Patch Set 19 : #Patch Set 20 : rebase #Patch Set 21 : #Patch Set 22 : relaxAdoptionRequirement is gone #
Total comments: 2
Patch Set 23 : #Messages
Total messages: 51 (0 generated)
kouhei: Can you take a look at this CL? This CL is still WIP but I would appreciate if you could give me some comments.
cc:haraken I'm still looking
Kouhei told me that we can use Persistent to avoid the destruct-in-iteration problem. PS4 doesn't touch ExecutionContext at all.
https://codereview.chromium.org/311733004/diff/20001/Source/core/dom/AsyncIni... File Source/core/dom/AsyncInitializerResolver.h (right): https://codereview.chromium.org/311733004/diff/20001/Source/core/dom/AsyncIni... Source/core/dom/AsyncInitializerResolver.h:39: static WeakPtr<AsyncInitializerResolver<T> > create(ScriptState* scriptState) Is this used? https://codereview.chromium.org/311733004/diff/60001/Source/core/dom/AsyncIni... File Source/core/dom/AsyncInitializerResolver.h (right): https://codereview.chromium.org/311733004/diff/60001/Source/core/dom/AsyncIni... Source/core/dom/AsyncInitializerResolver.h:111: Persistent<Holder> m_protect; Why don't we make AsyncInitializerResolver GarbageCollectedFinalized. Then we can remove Holder. :)
https://codereview.chromium.org/311733004/diff/60001/Source/core/dom/AsyncIni... File Source/core/dom/AsyncInitializerResolver.h (right): https://codereview.chromium.org/311733004/diff/60001/Source/core/dom/AsyncIni... Source/core/dom/AsyncInitializerResolver.h:111: Persistent<Holder> m_protect; On 2014/06/05 07:54:47, kouhei wrote: > Why don't we make AsyncInitializerResolver GarbageCollectedFinalized. Then we > can remove Holder. :) My compiler complains at having Persistent in GarbageCollectedFinalized, like this: ../../third_party/WebKit/Source/core/dom/AsyncInitializerResolver.h:33:1: error: [blink-gc] Class 'AsyncInitializerResolver<WebCore::MIDIAccessInitializer>' contains GC root in field 'm_protect'. class AsyncInitializerResolver FINAL : public GarbageCollectedFinalized<AsyncInitializerResolver<T> >, public ContextLifecycleObserver { ^ ../../third_party/WebKit/Source/core/dom/AsyncInitializerResolver.h:103:5: note: [blink-gc] Field 'm_protect' defining a GC root declared here: Persistent<AsyncInitializerResolver<Initializer> > m_protect;
Now it's ready for review. PTAL. https://codereview.chromium.org/311733004/diff/20001/Source/core/dom/AsyncIni... File Source/core/dom/AsyncInitializerResolver.h (right): https://codereview.chromium.org/311733004/diff/20001/Source/core/dom/AsyncIni... Source/core/dom/AsyncInitializerResolver.h:39: static WeakPtr<AsyncInitializerResolver<T> > create(ScriptState* scriptState) On 2014/06/05 07:54:47, kouhei wrote: > Is this used? Removed.
Eric, I once commented that I would provide some helper mechanism for a lifecycle issue of PromiseResolver at [1]. This class is intended to solve the problem. If this CL lands, I will use it in crypto, like [2]. Does it look reasonable to you? 1: https://codereview.chromium.org/263163006/ 2: https://codereview.chromium.org/312653005/
Thanks yhirano! The use in WebCrypto looks good, and is a nice improvement over what I previously had. Thanks for following up! My only other comment is that I found the name AsyncInitializerResolver difficult to understand.
On 2014/06/06 19:02:39, eroman wrote: > Thanks yhirano! The use in WebCrypto looks good, and is a nice improvement over > what I previously had. Thanks for following up! > > My only other comment is that I found the name AsyncInitializerResolver > difficult to understand. Thanks, I am not a fan of the name, too, but it is the best I could think of... If you have a good idea, please let me know.
kouhei, haraken: PTAL
This is a very general question, but why do we need to limit the usage of AsyncInitializerResolver to initializers? Don't we want to make a more general class that works for more methods, not limited to initializers? https://codereview.chromium.org/311733004/diff/80001/Source/core/dom/AsyncIni... File Source/core/dom/AsyncInitializerResolver.h (right): https://codereview.chromium.org/311733004/diff/80001/Source/core/dom/AsyncIni... Source/core/dom/AsyncInitializerResolver.h:92: Persistent<Holder> m_protect; I wonder why you need oilpan to keep the InitializerResolver object alive. Can't we just use an OwnPtr to the InitializerResolver object?
https://codereview.chromium.org/311733004/diff/80001/Source/core/dom/AsyncIni... File Source/core/dom/AsyncInitializerResolver.h (right): https://codereview.chromium.org/311733004/diff/80001/Source/core/dom/AsyncIni... Source/core/dom/AsyncInitializerResolver.h:92: Persistent<Holder> m_protect; On 2014/06/10 10:54:59, haraken wrote: > > I wonder why you need oilpan to keep the InitializerResolver object alive. Can't > we just use an OwnPtr to the InitializerResolver object? We can't destroy a ContextLifecycleObserver in another ContextLifecycleObserver's contextDestroyed due to a release assert. See https://codereview.chromium.org/263163006/ for the past discussion. Because ScriptPromiseResolverWithContext is an ActiveDOMObject, we can't use OwnPtr for protecting InitializerResolver.
I discussed with haraken offline and changed the design. I added ScriptPromiseResolverWithContext::keepObjectWhilePending. And now this CL also includes the implementation for WebCrypto. PTAL. haraken: overall design and bindings/ eroman: modules/crypto toyoshim: modules/webmidi rafaelw: ScriptPromiseResolverWithContext::unregisterKeptObject (please see my comment) https://codereview.chromium.org/311733004/diff/120001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right): https://codereview.chromium.org/311733004/diff/120001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:116: v8Promise->DeleteHiddenValue(name); Rafael, I heard that deleting a property is generally bad for performance (for example, your comments at [1]). Is the rule also applied to hidden properties? 1. https://codereview.chromium.org/250883002/#msg8
https://codereview.chromium.org/311733004/diff/140001/Source/modules/webmidi/... File Source/modules/webmidi/MIDIAccessInitializer.cpp (right): https://codereview.chromium.org/311733004/diff/140001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:26: m_resolver->keepObjectWhilePending(navigator); I'm fine with this approach overall. Another (probably better or worse) idea would be: - Make the Navigator an ActiveDOMObject. - Make Navigator::hasPendingActivity() return true as long as the ResolutionState of the ScriptPromiseResolver is Pending. What do you think?
drive-by comment. https://codereview.chromium.org/311733004/diff/140001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right): https://codereview.chromium.org/311733004/diff/140001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:114: ScriptPromise promise = this->promise(); The comment for promise() states "an empty ScriptPromise will be returned after resolve or reject is called." On lines 72 & 76 this method is called after resolve/reject, so will the isEmpty() call below always return true?
https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/C... File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.h:39: ExceptionCode webCryptoErrorToExceptionCode(blink::WebCryptoErrorType); Since this file has been essentially renamed to CrossThreadCryptoResult, can you move this last function out and delete it alltogether? https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... File Source/modules/crypto/SubtleCrypto.cpp (right): https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:51: class SubtleCrypto::PendingResult : public CryptoResult { This doesn't need to inherit from CryptoResult, it could inherit RefCounted directly (which would be more efficient since CryptoResult is thread-safe refcounted, and also doesn't need any virtual methods). That said, this doesn't need to be refcounted at all. * unregisterThis() could directly do the "delete this" which I think would be clearer than indirecting through the hashmap for ownership. *~SubtleCrypto() would have to also now delete any existing PendingResults In fact, I will probably have to switch to that model anyway to support cancellation. Since in a future change I would like to expose some method like: CrossThreadCryptoResult::wasCancelled() As part of http://crbug.com/375430 (which can be called from any thread) In this new design I imagine I would do that by having PendingResult also hold a pointer to the CrossThreadCryptoResult, so when ~SubtleCrypto() runs the outstanding CrossThreadCryptoResult can be marked as cancelled. https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:90: ScriptPromise promise() { return m_resolver->promise(); } extra space https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:100: m_resolver->keepObjectWhilePending(owner); Could there be any issues with keeping SubtleCrypto alive for longer? Or is it already the case that once created, SubtleCrypto lives until the ExecutionContext is destroyed anyway? https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:104: void unregisterThis() Please document that this will likely also "delete this" https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:106: if (m_owner->m_pendingResults.contains(this)) No need for the "contains()" -- remove() is a no-op if it it doesn't contain it. The contains() should probably be an assertion instead. https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:110: RefPtr<ScriptPromiseResolverWithContext> m_resolver; The ownership model here has become quite complicated! https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:159: RefPtr<PendingResult> pending = PendingResult::create(scriptState, this); Can you extract the creation of CrossThreadCryptoResult to a member function since this is done in several places? RefPtr<CrossThreadCryptoResult> SubtleCrypto::createResult();
https://codereview.chromium.org/311733004/diff/120001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right): https://codereview.chromium.org/311733004/diff/120001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:116: v8Promise->DeleteHiddenValue(name); Haraken is probably more authoritative on this than I, but it looks to me like hidden properties aren't stored in the object's map, but in a referenced hash map (https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects.cc&...). I wouldn't expect deleting a hidden property to slow the object down On 2014/06/12 11:02:51, yhirano wrote: > Rafael, I heard that deleting a property is generally bad for performance (for > example, your comments at [1]). Is the rule also applied to hidden properties? > > 1. https://codereview.chromium.org/250883002/#msg8
https://codereview.chromium.org/311733004/diff/120001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right): https://codereview.chromium.org/311733004/diff/120001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:116: v8Promise->DeleteHiddenValue(name); > Haraken is probably more authoritative on this than I, but it looks to me like > hidden properties aren't stored in the object's map, but in a referenced hash > map > (https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects.cc&...). > I wouldn't expect deleting a hidden property to slow the object down Agreed, I don't think deleting a hidden property affects performance. However, an object that has hidden properties is heavier (in terms of memory) than an object that doesn't have hidden properties. Also hidden property access is much slower than normal property access. I'm not sure if these performance & memory issues become a real problem here though.
Thank you for the review. I made SubtleCrypto::PendingResult GarbageCollectedFinalized. I hope the code get easier to understand than before. https://codereview.chromium.org/311733004/diff/120001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right): https://codereview.chromium.org/311733004/diff/120001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:116: v8Promise->DeleteHiddenValue(name); On 2014/06/13 02:36:17, rafaelw wrote: > Haraken is probably more authoritative on this than I, but it looks to me like > hidden properties aren't stored in the object's map, but in a referenced hash > map > (https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects.cc&...). > I wouldn't expect deleting a hidden property to slow the object down > > > On 2014/06/12 11:02:51, yhirano wrote: > > Rafael, I heard that deleting a property is generally bad for performance (for > > example, your comments at [1]). Is the rule also applied to hidden properties? > > > > 1. https://codereview.chromium.org/250883002/#msg8 > Thanks! https://codereview.chromium.org/311733004/diff/120001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:116: v8Promise->DeleteHiddenValue(name); On 2014/06/13 03:36:53, haraken wrote: > > Haraken is probably more authoritative on this than I, but it looks to me like > > hidden properties aren't stored in the object's map, but in a referenced hash > > map > > > (https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects.cc&...). > > I wouldn't expect deleting a hidden property to slow the object down > > Agreed, I don't think deleting a hidden property affects performance. > > However, an object that has hidden properties is heavier (in terms of memory) > than an object that doesn't have hidden properties. Also hidden property access > is much slower than normal property access. > > I'm not sure if these performance & memory issues become a real problem here > though. Then is it better to have another boolean member variable showing whether keepObjectWhilePending is called, for performance? https://codereview.chromium.org/311733004/diff/140001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right): https://codereview.chromium.org/311733004/diff/140001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:114: ScriptPromise promise = this->promise(); On 2014/06/12 20:10:24, jrummell wrote: > The comment for promise() states "an empty ScriptPromise will be returned after > resolve or reject is called." On lines 72 & 76 this method is called after > resolve/reject, so will the isEmpty() call below always return true? You are right, thanks for pointing it out. Fixed. https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/C... File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.h:39: ExceptionCode webCryptoErrorToExceptionCode(blink::WebCryptoErrorType); On 2014/06/13 01:52:52, eroman wrote: > Since this file has been essentially renamed to CrossThreadCryptoResult, can you > move this last function out and delete it alltogether? Done. https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... File Source/modules/crypto/SubtleCrypto.cpp (right): https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:51: class SubtleCrypto::PendingResult : public CryptoResult { On 2014/06/13 01:52:52, eroman wrote: > This doesn't need to inherit from CryptoResult, it could inherit RefCounted > directly (which would be more efficient since CryptoResult is thread-safe > refcounted, and also doesn't need any virtual methods). > > That said, this doesn't need to be refcounted at all. > * unregisterThis() could directly do the "delete this" which I think would be > clearer than indirecting through the hashmap for ownership. > > *~SubtleCrypto() would have to also now delete any existing PendingResults > > In fact, I will probably have to switch to that model anyway to support > cancellation. Since in a future change I would like to expose some method like: > CrossThreadCryptoResult::wasCancelled() > As part of http://crbug.com/375430 (which can be called from any thread) > > In this new design I imagine I would do that by having PendingResult also hold a > pointer to the CrossThreadCryptoResult, so when ~SubtleCrypto() runs the > outstanding CrossThreadCryptoResult can be marked as cancelled. I added decouplsed CompleteWith* functions from CryptoResult to CryptoResultBase. Now PendingResult is an Oilpan class. What do you think of this? https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:90: ScriptPromise promise() { return m_resolver->promise(); } On 2014/06/13 01:52:52, eroman wrote: > extra space Done. https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:100: m_resolver->keepObjectWhilePending(owner); On 2014/06/13 01:52:52, eroman wrote: > Could there be any issues with keeping SubtleCrypto alive for longer? Or is it > already the case that once created, SubtleCrypto lives until the > ExecutionContext is destroyed anyway? SubtleCrypto lives while one of the following meets: 1. Its wrapper is referred by JavaScript 2. One of *pending* promises which it returned is alive. When all Promises are resolved or rejected and the wrapper is not reachable from the root set, the SubtleCrypto will be garbage collected. When the ExecutionContext is stopped, all Promises will become not Pending. https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:104: void unregisterThis() On 2014/06/13 01:52:52, eroman wrote: > Please document that this will likely also "delete this" Now PendingResult is an oilpan class and |this| will be garbage collected later. https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:106: if (m_owner->m_pendingResults.contains(this)) On 2014/06/13 01:52:52, eroman wrote: > No need for the "contains()" -- remove() is a no-op if it it doesn't contain it. > > The contains() should probably be an assertion instead. Done. I don't want to put the assertion in order to allow multiple calls. https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:110: RefPtr<ScriptPromiseResolverWithContext> m_resolver; On 2014/06/13 01:52:52, eroman wrote: > The ownership model here has become quite complicated! It becomes simpler now, I hope. https://codereview.chromium.org/311733004/diff/140001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:159: RefPtr<PendingResult> pending = PendingResult::create(scriptState, this); On 2014/06/13 01:52:52, eroman wrote: > Can you extract the creation of CrossThreadCryptoResult to a member function > since this is done in several places? > > RefPtr<CrossThreadCryptoResult> SubtleCrypto::createResult(); Done. https://codereview.chromium.org/311733004/diff/140001/Source/modules/webmidi/... File Source/modules/webmidi/MIDIAccessInitializer.cpp (right): https://codereview.chromium.org/311733004/diff/140001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:26: m_resolver->keepObjectWhilePending(navigator); On 2014/06/12 15:58:52, haraken wrote: > > I'm fine with this approach overall. > > Another (probably better or worse) idea would be: > > - Make the Navigator an ActiveDOMObject. > - Make Navigator::hasPendingActivity() return true as long as the > ResolutionState of the ScriptPromiseResolver is Pending. > > What do you think? In general, an object (such as a Navigator) issues multiple Promises, so it requires another list to be introduced in the object. That may be hard to understand for developers, I think. One idea is having such list in ActiveDOMObject and returns appropriately in ActiveDOMObject::hasPending. But currently some classes (see WebSocket for example) don't respect ActiveDOMObject::hasPending in its own hasPending.
https://codereview.chromium.org/311733004/diff/120001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right): https://codereview.chromium.org/311733004/diff/120001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:103: v8Promise->SetHiddenValue(name, value); Consider using helper methods in V8HiddenValue.h instead of directly using Get/Set/DeleteHiddenValue. (We're planning to replace the implementation of V8HiddenValue so that it uses private names, which is faster than Get/Set/DeleteHiddenValue.) https://codereview.chromium.org/311733004/diff/120001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:116: v8Promise->DeleteHiddenValue(name); On 2014/06/13 04:19:30, yhirano wrote: > On 2014/06/13 03:36:53, haraken wrote: > > > Haraken is probably more authoritative on this than I, but it looks to me > like > > > hidden properties aren't stored in the object's map, but in a referenced > hash > > > map > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects.cc&...). > > > I wouldn't expect deleting a hidden property to slow the object down > > > > Agreed, I don't think deleting a hidden property affects performance. > > > > However, an object that has hidden properties is heavier (in terms of memory) > > than an object that doesn't have hidden properties. Also hidden property > access > > is much slower than normal property access. > > > > I'm not sure if these performance & memory issues become a real problem here > > though. > > Then is it better to have another boolean member variable showing whether > keepObjectWhilePending is called, for performance? Only if this code is really performance-sensitive (and I don't think so). https://codereview.chromium.org/311733004/diff/140001/Source/modules/webmidi/... File Source/modules/webmidi/MIDIAccessInitializer.cpp (right): https://codereview.chromium.org/311733004/diff/140001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:26: m_resolver->keepObjectWhilePending(navigator); On 2014/06/13 04:19:30, yhirano wrote: > On 2014/06/12 15:58:52, haraken wrote: > > > > I'm fine with this approach overall. > > > > Another (probably better or worse) idea would be: > > > > - Make the Navigator an ActiveDOMObject. > > - Make Navigator::hasPendingActivity() return true as long as the > > ResolutionState of the ScriptPromiseResolver is Pending. > > > > What do you think? > In general, an object (such as a Navigator) issues multiple Promises, so it > requires another list to be introduced in the object. That may be hard to > understand for developers, I think. > One idea is having such list in ActiveDOMObject and returns appropriately in > ActiveDOMObject::hasPending. But currently some classes (see WebSocket for > example) don't respect ActiveDOMObject::hasPending in its own hasPending. Makes sense. The approach of this CL is fine with me. I'm just not really happy that we currently have two approaches to keep wrappers alive and their distinction is ambuigous: ActiveDOMObject::hasPendingActivity() and hidden properties. At the moment, it's not clear which we should use when.
https://codereview.chromium.org/311733004/diff/120001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right): https://codereview.chromium.org/311733004/diff/120001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:103: v8Promise->SetHiddenValue(name, value); On 2014/06/13 05:03:51, haraken wrote: > > Consider using helper methods in V8HiddenValue.h instead of directly using > Get/Set/DeleteHiddenValue. (We're planning to replace the implementation of > V8HiddenValue so that it uses private names, which is faster than > Get/Set/DeleteHiddenValue.) Done.
I'm sorry about iterative comments! https://codereview.chromium.org/311733004/diff/240001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right): https://codereview.chromium.org/311733004/diff/240001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:14: const char hiddenPropertyName[] = "blink::keepObjectWhilePending"; Can you use V8HiddenPropertyName::pendingObject? You need to add it to V8HiddenPropertyName.h. https://codereview.chromium.org/311733004/diff/240001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:66: unregisterKeptObject(); I think you need to enter ScriptState::Scope before calling this. As far as I see Promise related code, it looks error-prone about when we should enter ScriptState::Scope. Can we invent a better programming rule about it so that we can be confident about we're always in a correct ScriptState? You can address this issue in a separate CL. https://codereview.chromium.org/311733004/diff/240001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.h (right): https://codereview.chromium.org/311733004/diff/240001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.h:73: // This function registers |value| to the associated Promise as a hidden This function registers a wrapper of |value| to the ... https://codereview.chromium.org/311733004/diff/240001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.h:83: ScriptState::Scope scope(m_scriptState.get()); Probably it's better to move this ScriptState::Scope into toV8Value(). https://codereview.chromium.org/311733004/diff/240001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.h:100: return ToV8Value<ScriptPromiseResolverWithContext, v8::Handle<v8::Object> >::toV8Value(value, m_scriptState->context()->Global(), m_scriptState->isolate()); I mean, you can add ScriptState::Scope(scriptState()) here and remove the scopes from caller sites. https://codereview.chromium.org/311733004/diff/240001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.h:124: void unregisterKeptObject(); keepObjectWhilePending => registerPendingObject keepObjectWhilePendingInternal => registerPendingWrapper unregisterKeptObject => unregisterPendingObject or clearPendingObject ? (It's up to you.) https://codereview.chromium.org/311733004/diff/240001/Source/modules/crypto/S... File Source/modules/crypto/SubtleCrypto.cpp (right): https://codereview.chromium.org/311733004/diff/240001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:52: PendingResult(ScriptState* scriptState, RawPtr<SubtleCrypto> crypto) RawPtr<SubtleCrypto> => SubtleCrypto* https://codereview.chromium.org/311733004/diff/240001/Source/modules/crypto/S... Source/modules/crypto/SubtleCrypto.cpp:380: RawPtr<PendingResult> pending = new PendingResult(scriptState, this); RawPtr<PendingResult> => PendingResult* https://codereview.chromium.org/311733004/diff/240001/Source/modules/webmidi/... File Source/modules/webmidi/MIDIAccessInitializer.h (right): https://codereview.chromium.org/311733004/diff/240001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.h:74: explicit MIDIAccessInitializer(ScriptState*, const MIDIOptions&, Navigator*, Client*); Drop explicit. https://codereview.chromium.org/311733004/diff/240001/Source/modules/webmidi/... File Source/modules/webmidi/NavigatorWebMIDI.h (right): https://codereview.chromium.org/311733004/diff/240001/Source/modules/webmidi/... Source/modules/webmidi/NavigatorWebMIDI.h:64: HashSet<OwnPtr<MIDIAccessInitializer> > m_initializers; I'm sorry for asking design questions a lot of times, but let me ask one more thing: - It seems that MIDIAccessInitializer is kept alive as long as you want to keep the Navigator object alive. - Then why don't you just make the MIDIAccessInitializer hold a RefPtr to the Navigator? - I want to understand the lifetime of the MIDIAccessInitializer, the Navigator and the Promise returned back to JS.
For the design comment: > - It seems that MIDIAccessInitializer is kept alive as long as you want to keep > the Navigator object alive. > > - Then why don't you just make the MIDIAccessInitializer hold a RefPtr to the > Navigator? > > - I want to understand the lifetime of the MIDIAccessInitializer, the Navigator > and the Promise returned back to JS. Generally I don't like a RefPointer loop because it makes the ownership relation hard to understand. The problem this CL tries to solve is a general one and it is shame if all modules that encounter this pattern implement their own RefPointer loops. And, it won't work with GarbageCollected. I'm trying to provide an interface to module developers implementing an initialization context such as MIDIAccessInitializer. The interface should alleviate initializer's lifetime management. From this point of view, I like the AsyncInitializer interface the best: All you have to do is to call resolve or reject and you don't have to worry about initializer's lifetime. By default there is no relation between the initialization context (MIDIAccessInitializer) and the method owner (Navigator) and it is easy for developers to give part of method owner's resource to the initialization context. Note: I implemented AsyncInitializer with a Persistent loop, but there are other ways to implement. Another approach is to provide AsyncInitializerBase which is a ScriptWrapperble ActiveDOMObject and stays alive while the associated Promise is pending. Each AsyncInitializer will inherit it and no lifetime management will be needed. The current approach is acceptable for me: make the method owner have the initialization context is not so unnatural. It erases the RefPointer loop though the initialization context should clear the reference manually. What do you think?
On 2014/06/13 15:18:13, yhirano wrote: > For the design comment: > > - It seems that MIDIAccessInitializer is kept alive as long as you want to > keep > > the Navigator object alive. > > > > - Then why don't you just make the MIDIAccessInitializer hold a RefPtr to the > > Navigator? > > > > - I want to understand the lifetime of the MIDIAccessInitializer, the > Navigator > > and the Promise returned back to JS. > > Generally I don't like a RefPointer loop because it makes the ownership relation > hard to understand. > The problem this CL tries to solve is a general one and it is shame if all > modules that encounter this pattern implement their own RefPointer loops. > And, it won't work with GarbageCollected. > > I'm trying to provide an interface to module developers implementing an > initialization context such as MIDIAccessInitializer. > The interface should alleviate initializer's lifetime management. > > From this point of view, I like the AsyncInitializer interface the best: All you > have to do is to call resolve or reject and you don't have to worry about > initializer's lifetime. > By default there is no relation between the initialization context > (MIDIAccessInitializer) and the method owner (Navigator) and it is easy for > developers to give part of method owner's resource to the initialization > context. > Note: I implemented AsyncInitializer with a Persistent loop, but there are other > ways to implement. > > Another approach is to provide AsyncInitializerBase which is a ScriptWrapperble > ActiveDOMObject and stays alive while the associated Promise is pending. > Each AsyncInitializer will inherit it and no lifetime management will be needed. > > The current approach is acceptable for me: make the method owner have the > initialization context is not so unnatural. > It erases the RefPointer loop though the initialization context should clear the > reference manually. > > What do you think? Thanks for the details; I'd be happy if we could chat offline again on Monday. I'm sorry that I'm making you going back & forth :(
haraken: PTAL?
This looks much simpler! LGTM for overall design and bindings/. https://codereview.chromium.org/311733004/diff/280001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right): https://codereview.chromium.org/311733004/diff/280001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:84: RefPtr<ScriptPromiseResolverWithContext> protect(this); Why is this protect needed? https://codereview.chromium.org/311733004/diff/280001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:91: deref(); Just help me understand: What's the relationship between this deref() and the deref() you introduced in this CL? https://codereview.chromium.org/311733004/diff/280001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.h (right): https://codereview.chromium.org/311733004/diff/280001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.h:36: KeepSelfWhilePending, KeepAliveWhilePending ?
https://codereview.chromium.org/311733004/diff/280001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right): https://codereview.chromium.org/311733004/diff/280001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:84: RefPtr<ScriptPromiseResolverWithContext> protect(this); On 2014/06/16 10:54:04, haraken wrote: > > Why is this protect needed? It was needed because I checked m_mode after calling deref. Now it is not needed because I exchanged two deref blocks. https://codereview.chromium.org/311733004/diff/280001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:91: deref(); On 2014/06/16 10:54:04, haraken wrote: > > Just help me understand: What's the relationship between this deref() and the > deref() you introduced in this CL? ref() in constructor is for keeping the resolver while the promise is pending. ref() in resolveOrReject is for keeping the resolver while the resolver is resolving or rejecting the Promise and the execution context is suspended. https://codereview.chromium.org/311733004/diff/280001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.h (right): https://codereview.chromium.org/311733004/diff/280001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.h:36: KeepSelfWhilePending, On 2014/06/16 10:54:04, haraken wrote: > > KeepAliveWhilePending ? Done.
toyoshim, eroman, can you take a look at the CL if you have time? Thanks in advance.
LGTM for webcrypto side! https://codereview.chromium.org/311733004/diff/300001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right): https://codereview.chromium.org/311733004/diff/300001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:89: deref(); Is it possible for the state to be Pending here? If so, then this line may "delete this" and it would be strange to execute the following line. https://codereview.chromium.org/311733004/diff/300001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.h (right): https://codereview.chromium.org/311733004/diff/300001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.h:41: // to stay alive while the promise is pending and the accosiated typo: accosiated --> associated https://codereview.chromium.org/311733004/diff/300001/Source/modules/modules.... File Source/modules/modules.gypi (right): https://codereview.chromium.org/311733004/diff/300001/Source/modules/modules.... Source/modules/modules.gypi:302: 'crypto/CryptoResultImpl.cpp', Why re-arrange these? Doing a lexicographic sort I get '.' (0x2e) sorting above 'R' (0x52).
https://codereview.chromium.org/311733004/diff/300001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right): https://codereview.chromium.org/311733004/diff/300001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:23: ref(); When I run this patch in debug mode I am hitting this assertion: ../../third_party/WebKit/Source/wtf/RefCounted.h:59 59 ASSERT(!m_adoptionIsRequired); I think either the ref() needs to be moved out of the constructor and into the create() method, or maybe you can call relaxAdoptionRequirement() to turn off that assertion.
https://codereview.chromium.org/311733004/diff/300001/Source/modules/crypto/C... File Source/modules/crypto/CryptoResultImpl.cpp (right): https://codereview.chromium.org/311733004/diff/300001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.cpp:54: static WeakPtr<ScriptPromiseResolverWithContext> create(ScriptState* scriptState) This additionally needs to call suspendIfNeeded()
https://codereview.chromium.org/311733004/diff/300001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.cpp (right): https://codereview.chromium.org/311733004/diff/300001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:23: ref(); On 2014/06/16 20:05:50, eroman wrote: > When I run this patch in debug mode I am hitting this assertion: > > ../../third_party/WebKit/Source/wtf/RefCounted.h:59 > 59 ASSERT(!m_adoptionIsRequired); > > I think either the ref() needs to be moved out of the constructor and into the > create() method, or maybe you can call relaxAdoptionRequirement() to turn off > that assertion. Thanks for pointing it out. I would like to call relaxAdoptionRequirement here, because this class can be subclassed. https://codereview.chromium.org/311733004/diff/300001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:89: deref(); On 2014/06/16 19:36:35, eroman wrote: > Is it possible for the state to be Pending here? > > If so, then this line may "delete this" and it would be strange to execute the > following line. I added a comment. Is it clear now? https://codereview.chromium.org/311733004/diff/300001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.h (right): https://codereview.chromium.org/311733004/diff/300001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.h:41: // to stay alive while the promise is pending and the accosiated On 2014/06/16 19:36:35, eroman wrote: > typo: accosiated --> associated Done. https://codereview.chromium.org/311733004/diff/300001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.h:41: // to stay alive while the promise is pending and the accosiated On 2014/06/16 19:36:35, eroman wrote: > typo: accosiated --> associated Done. https://codereview.chromium.org/311733004/diff/300001/Source/modules/crypto/C... File Source/modules/crypto/CryptoResultImpl.cpp (right): https://codereview.chromium.org/311733004/diff/300001/Source/modules/crypto/C... Source/modules/crypto/CryptoResultImpl.cpp:54: static WeakPtr<ScriptPromiseResolverWithContext> create(ScriptState* scriptState) On 2014/06/16 20:57:08, eroman wrote: > This additionally needs to call suspendIfNeeded() Done.
LGTM with nits https://codereview.chromium.org/311733004/diff/340001/Source/modules/webmidi/... File Source/modules/webmidi/MIDIAccessInitializer.cpp (right): https://codereview.chromium.org/311733004/diff/340001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:33: ASSERT(controller); On this ASSERT(), See a comment below. https://codereview.chromium.org/311733004/diff/340001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:49: if (controller) { Here, we assume that MIDIController can be NULL, but we do not expect it in the descriptor. It looks curious. Maybe we want to remove the previous ASSERT(). If a blink embedder do not need to support sysex messages, the embedder may miss MIDIController implementation. https://codereview.chromium.org/311733004/diff/340001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:58: { How about addding an ASSERT() to check |m_accessor| is valid. It will be useful in the future to detect a error for switching client implementations currently done in MIDIAccess constructor. https://codereview.chromium.org/311733004/diff/340001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:63: { ditto https://codereview.chromium.org/311733004/diff/340001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:68: { ditto
https://codereview.chromium.org/311733004/diff/340001/Source/modules/webmidi/... File Source/modules/webmidi/MIDIAccessInitializer.cpp (right): https://codereview.chromium.org/311733004/diff/340001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:33: ASSERT(controller); On 2014/06/17 05:10:08, Takashi Toyoshima (chromium) wrote: > On this ASSERT(), See a comment below. Done. https://codereview.chromium.org/311733004/diff/340001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:49: if (controller) { On 2014/06/17 05:10:08, Takashi Toyoshima (chromium) wrote: > Here, we assume that MIDIController can be NULL, but we do not expect it in the > descriptor. It looks curious. Maybe we want to remove the previous ASSERT(). If > a blink embedder do not need to support sysex messages, the embedder may miss > MIDIController implementation. Done. https://codereview.chromium.org/311733004/diff/340001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:58: { On 2014/06/17 05:10:08, Takashi Toyoshima (chromium) wrote: > How about addding an ASSERT() to check |m_accessor| is valid. > It will be useful in the future to detect a error for switching client > implementations currently done in MIDIAccess constructor. Done. https://codereview.chromium.org/311733004/diff/340001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:63: { On 2014/06/17 05:10:08, Takashi Toyoshima (chromium) wrote: > ditto Done. https://codereview.chromium.org/311733004/diff/340001/Source/modules/webmidi/... Source/modules/webmidi/MIDIAccessInitializer.cpp:68: { On 2014/06/17 05:10:08, Takashi Toyoshima (chromium) wrote: > ditto Done.
LGTM!
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/311733004/380001
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/311733004/400001
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/311733004/420001
Message was sent while issue was closed.
Change committed as 176381
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/339443004/ by yhirano@chromium.org. The reason for reverting is: This CL breaks the build..
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/338373005/ by jamesr@chromium.org. The reason for reverting is: FAILED: /mnt/data/b/build/goma/gomacc c++ -MMD -MF obj/third_party/WebKit/Source/bindings/v8/webcore_generated.ScriptPromiseResolverWithContext.o.d -DV8_DEPRECATION_WARNINGS -DBLINK_SCALE_FILTERS_AT_RECORD_TIME -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DTOOLKIT_VIEWS=1 -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_ASH=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_LIBJPEG_TURBO=1 -DUSE_X11=1 -DUSE_CLIPBOARD_AURAX11=1 -DENABLE_ONE_CLICK_SIGNIN -DUSE_XI2_MT=2 -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DUSE_UDEV -DENABLE_EGLIMAGE=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 -DENABLE_FULL_PRINTING=1 -DENABLE_PRINTING=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_MANAGED_USERS=1 -DENABLE_MDNS=1 -DENABLE_SERVICE_DISCOVERY=1 -DGL_GLEXT_PROTOTYPES -DBLINK_IMPLEMENTATION=1 -DINSIDE_BLINK -DENABLE_CUSTOM_SCHEME_HANDLER=0 -DENABLE_SVG_FONTS=1 -DWTF_USE_CONCATENATED_IMPULSE_RESPONSES=1 -DENABLE_INPUT_MULTIPLE_FIELDS_UI=1 -DENABLE_MEDIA_CAPTURE=0 -DENABLE_WEB_AUDIO=1 -DWTF_USE_WEBAUDIO_FFMPEG=1 -DENABLE_OPENTYPE_VERTICAL=1 -DWTF_USE_DEFAULT_RENDER_THEME=1 -DU_USING_ICU_NAMESPACE=0 -DU_STATIC_IMPLEMENTATION -DSK_ENABLE_INST_COUNT=0 -DSK_SUPPORT_GPU=1 '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"' -DSK_ENABLE_LEGACY_API_ALIASING=1 -DSK_ATTR_DEPRECATED=SK_NOTHING_ARG1 -DGR_GL_IGNORE_ES3_MSAA=0 -DSK_WILL_NEVER_DRAW_PERSPECTIVE_TEXT -DSK_SUPPORT_LEGACY_GETTOPDEVICE -DSK_SUPPORT_LEGACY_DEVICE_VIRTUAL_ISOPAQUE -DSK_SUPPORT_LEGACY_N32_NAME -DSK_SUPPORT_LEGACY_SETCONFIG -DSK_IGNORE_ETC1_SUPPORT -DSK_SUPPORT_LEGACY_GETTOTALCLIP -DSK_USE_POSIX_THREADS -DSK_DEFERRED_CANVAS_USES_FACTORIES=1 -DCHROME_PNG_WRITE_SUPPORT -DPNG_USER_CONFIG -DLIBXML_STATIC -DLIBXSLT_STATIC -DUSE_NSS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -D_FORTIFY_SOURCE=2 -Igen -I../../third_party/WebKit/Source -Igen/blink/core -Igen/blink/modules -Igen/blink/platform -Igen/blink/bindings/core/v8 -Igen/blink/bindings/modules/v8 -Igen/blink -I../.. -I../../skia/config -I../../third_party/khronos -I../../gpu -I../../third_party/angle/include -I../../third_party/ffmpeg -I../../third_party/WebKit -I../../third_party/ots/include -I../../third_party/zlib -I../../third_party/icu/source/i18n -I../../third_party/icu/source/common -I../../third_party/skia/src/core -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../skia/ext -I../../third_party/iccjpeg -I../../third_party/libpng -I../../third_party/libwebp -I../../third_party/libxml/linux/include -I../../third_party/libxml/src/include -I../../third_party/libxslt -I../../third_party/npapi -I../../third_party/npapi/bindings -I../../third_party/qcms/src -I../../third_party/sqlite -I../../v8/include -fstack-protector --param=ssp-buffer-size=4 -Werror -pthread -fno-exceptions -fno-strict-aliasing -Wall -Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe -fPIC -B/mnt/data/b/build/slave/WebKit_Linux/build/src/third_party/binutils/Linux_x64/Release/bin -fno-strict-aliasing -pthread -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -m64 -march=x86-64 -O2 -fno-ident -fdata-sections -ffunction-sections -funwind-tables -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -Wsign-compare -Wno-c++0x-compat -c ../../third_party/WebKit/Source/bindings/v8/ScriptPromiseResolverWithContext.cpp -o obj/third_party/WebKit/Source/bindings/v8/webcore_generated.ScriptPromiseResolverWithContext.o ../../third_party/WebKit/Source/bindings/v8/ScriptPromiseResolverWithContext.cpp: In constructor 'WebCore::ScriptPromiseResolverWithContext::ScriptPromiseResolverWithContext(WebCore::ScriptState*, WebCore::ScriptPromiseResolverWithContext::Mode)': ../../third_party/WebKit/Source/bindings/v8/ScriptPromiseResolverWithContext.cpp:23:34:error: 'relaxAdoptionRequirement' was not declared in this scope ninja: build stopped: subcommand failed. .
reopen. relaxAdoptionRequirement is removed.
I added ScriptPromiseResolverWithContext::keepAliveWhilePending and removed the constructor parameter, because relaxAdoptionRequirement was removed in a recent change. haraken: can you take a look, if you have time?
LGTM for ScriptPromiseResolverWithContext. https://codereview.chromium.org/311733004/diff/440001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.h (right): https://codereview.chromium.org/311733004/diff/440001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.h:89: enum LifeMode { LifetimeMode ?
https://codereview.chromium.org/311733004/diff/440001/Source/bindings/v8/Scri... File Source/bindings/v8/ScriptPromiseResolverWithContext.h (right): https://codereview.chromium.org/311733004/diff/440001/Source/bindings/v8/Scri... Source/bindings/v8/ScriptPromiseResolverWithContext.h:89: enum LifeMode { On 2014/06/18 06:56:01, haraken wrote: > > LifetimeMode ? Done.
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/311733004/460001
Message was sent while issue was closed.
Change committed as 176403 |
