Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2867023002:
[ServiceWorker] waitUntil() should wait until all promises got resolved/rejected. (Closed)
|
Created:
3 years, 7 months ago by leonhsl(Using Gerrit) Modified:
3 years, 7 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, blink-reviews-w3ctests_chromium.org, nhiroki, haraken, kinuko+serviceworker, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, tzik Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ServiceWorker] waitUntil() should wait until all promises got resolved/rejected.
Spec: https://w3c.github.io/ServiceWorker/#wait-until-method
For multiple waitUntil() pending promises, even if some one got rejected
while some others are still pending at the same time, we should continue
waiting, until all pending promises completed(resolved or rejected).
BUG=719850
TEST=blink_tests
external/wpt/service-workers/service-worker/extendable-event-waituntil.https.html
TBR=kinuko@chromium.org
Review-Url: https://codereview.chromium.org/2867023002
Cr-Commit-Position: refs/heads/master@{#470828}
Committed: https://chromium.googlesource.com/chromium/src/+/06f33f6fa8d71e31838103a0e12f95e8420510d2
Patch Set 1 #
Total comments: 4
Patch Set 2 : Remove old FIXME #
Total comments: 9
Patch Set 3 : Clean up #
Total comments: 11
Patch Set 4 : More clean up #
Messages
Total messages: 60 (33 generated)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ServiceWorker] waitUntil() should wait until all promises got resolved/rejected. BUG=621440 TEST=blink_tests external/wpt/service-workers/service-worker/extendable-event-waituntil.https.html ========== to ========== [ServiceWorker] waitUntil() should wait until all promises got resolved/rejected. Spec: https://w3c.github.io/ServiceWorker/#wait-until-method For multiple waitUntil() pending promises, even if some one got rejected while some others are still pending at the same time, we should continue waiting, until all pending promises completed(resolved or rejected). BUG=621440 TEST=blink_tests external/wpt/service-workers/service-worker/extendable-event-waituntil.https.html ==========
leon.han@intel.com changed reviewers: + falken@chromium.org, shimazu@chromium.org
PTAL, Thanks! https://codereview.chromium.org/2867023002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2867023002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:147: // FIXME: Propagate error message to the client for onerror handling. ServiceWorker spec does not have such requirement, can we just remove this FIXME? https://codereview.chromium.org/2867023002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:148: NOTIMPLEMENTED(); I'm confused why NOTIMPLEMENTED() here does not crash.. IIRC NOTIMPLEMENTED() in Chromium codes does crash, while it's different in Blink codes?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I don't really understand how the code change is causing the behavior change. It looks like the original ReportError() just set a flag also. What's going on here? Also, I think this is fixing a different bug than 621440. That bug is for doing something like: waitUntil(promise) promise.then(waitUntil(another_promise)) Instead of just: waitUntil(promise) waitUntil(another_promise) The WPT for 621440 is extendable-event-async-waituntil.https.html (note the -async- part) Could you open a new bug for this change? https://codereview.chromium.org/2867023002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2867023002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:148: NOTIMPLEMENTED(); On 2017/05/08 10:33:48, leonhsl wrote: > I'm confused why NOTIMPLEMENTED() here does not crash.. IIRC NOTIMPLEMENTED() in > Chromium codes does crash, while it's different in Blink codes? I think it's fine to delete the FIXME and the NOTIMPLEMENTED, they are from ancient times.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ServiceWorker] waitUntil() should wait until all promises got resolved/rejected. Spec: https://w3c.github.io/ServiceWorker/#wait-until-method For multiple waitUntil() pending promises, even if some one got rejected while some others are still pending at the same time, we should continue waiting, until all pending promises completed(resolved or rejected). BUG=621440 TEST=blink_tests external/wpt/service-workers/service-worker/extendable-event-waituntil.https.html ========== to ========== [ServiceWorker] waitUntil() should wait until all promises got resolved/rejected. Spec: https://w3c.github.io/ServiceWorker/#wait-until-method For multiple waitUntil() pending promises, even if some one got rejected while some others are still pending at the same time, we should continue waiting, until all pending promises completed(resolved or rejected). BUG=719850 TEST=blink_tests external/wpt/service-workers/service-worker/extendable-event-waituntil.https.html ==========
Thanks a lot for review! I created a new bug and updated CL description with it. |has_error_| flag indicates that the event handler(oninstall or onactivate etc.) execution got some uncaught error, it is set by WaitUntilObserver::DidDispatchEvent(). And, once WaitUntilObserver::DecrementPendingActivity() noticed that this flag got set, it will finish the event lifetime immediately even if there are other pending promises (|pending_activity_| > 0). But, some one promise getting rejected should not be regarded as such error, so I created another flag |has_rejected_promise_| to record whether there has any promise already got rejected, then after all promises completed, if |has_rejected_promise_| was true, we finish this event with a result kWebServiceWorkerEventResultRejected. https://codereview.chromium.org/2867023002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2867023002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:148: NOTIMPLEMENTED(); On 2017/05/09 04:00:25, falken wrote: > On 2017/05/08 10:33:48, leonhsl wrote: > > I'm confused why NOTIMPLEMENTED() here does not crash.. IIRC NOTIMPLEMENTED() > in > > Chromium codes does crash, while it's different in Blink codes? > > I think it's fine to delete the FIXME and the NOTIMPLEMENTED, they are from > ancient times. Done.
Thanks for working on this and for creating the new bug. https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h (right): https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:45: void DidDispatchEvent(bool error_occurred); I see, it's not from your patch but |error_occurred| and |has_error| are a bit inaccurately named (they are from an earlier time). Can you rename them to |event_dispatch_failed|? You also will need to update the comments on callsites that are currently "// error_occurred". Now there's a big difference between |has_rejected_promise| and this flag, so I want better names. Also comments are good: // Must be called before dispatching the event. void WillDispatchEvent(); // Must be called after dispatching the event. If |event_dispatch_failed|, this WaitUntilObserver will soon report to ServiceWorkerGlobalScopeClient that the event finished, without waiting for all waitUntil promises to settle. https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:67: void ReportPromiseRejected(const ScriptValue&); Maybe just "OnPromiseRejected", as it's not really reporting to anything. The param isn't used, so it can be removed. Also a comment like would be good like: // Called when a promise passed to a waitUntil() call that is associated with this observer was rejected. https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:77: bool event_dispatched_ = false; Actually, instead of |has_error_| and |event_dispatched_|, can you change it to an enum like: kInitial, kEventDispatchCompleted, kEventDispatchFailed
https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:72: observer_->DecrementPendingActivity(); Looking at this more, it looks like things can be cleaner if we just pass an argument (probably an enum) to DecrementPendingActivity that indicates whether a promise rejected, resolved, or the event dispatch failed. Then I think we don't need ReportPromiseRejected() or set |has_error_| separately, it can all be done by DecrementPendingActivity(). WDYT?
Sorry for the constant comments. https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:72: observer_->DecrementPendingActivity(); On 2017/05/09 08:29:02, falken wrote: > Looking at this more, it looks like things can be cleaner if we just pass an > argument (probably an enum) to DecrementPendingActivity that indicates whether a > promise rejected, resolved, or the event dispatch failed. Then I think we don't > need ReportPromiseRejected() or set |has_error_| separately, it can all be done > by DecrementPendingActivity(). WDYT? Or maybe better functions like: OnPromiseResolved() OnPromiseRejected() OnEventDispatchFailed() The point is instead of calling a function that sets state that affects antoher function, we'd just need to call a single function and have it do the work.
On 2017/05/09 08:29:02, falken wrote: > https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp > (right): > > https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:72: > observer_->DecrementPendingActivity(); > Looking at this more, it looks like things can be cleaner if we just pass an > argument (probably an enum) to DecrementPendingActivity that indicates whether a > promise rejected, resolved, or the event dispatch failed. Then I think we don't > need ReportPromiseRejected() or set |has_error_| separately, it can all be done > by DecrementPendingActivity(). WDYT? I like this idea making code simpler. If we do this way, one question comes up: WaitUntilObserver::DecrementPendingActivity() is also used by RespondWithObserver, means DecrementPendingActivity() would notify: Event dispatch completed/failed waitUntil promise resolved/rejected respondWith promise resolved/rejected Now we fire didHandleXXXEvent(Rejected) if we finally found any one rejected waitUntil promise, but should we also do this if we found out that there has a rejected respondWith promise? I'm not very sure about this.
On 2017/05/09 09:02:54, falken wrote: > Sorry for the constant comments. > > https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp > (right): > > https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:72: > observer_->DecrementPendingActivity(); > On 2017/05/09 08:29:02, falken wrote: > > Looking at this more, it looks like things can be cleaner if we just pass an > > argument (probably an enum) to DecrementPendingActivity that indicates whether > a > > promise rejected, resolved, or the event dispatch failed. Then I think we > don't > > need ReportPromiseRejected() or set |has_error_| separately, it can all be > done > > by DecrementPendingActivity(). WDYT? > > Or maybe better functions like: > OnPromiseResolved() > OnPromiseRejected() > OnEventDispatchFailed() > > The point is instead of calling a function that sets state that affects antoher > function, we'd just need to call a single function and have it do the work. Thus in correspondence seems we also need to replace IncrementPendingActivity() with OnPromiseStarted() OnEventDispatchStarted() I'd like to keep {In,De}crementPendingActivity:-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/09 09:15:36, leonhsl wrote: > On 2017/05/09 08:29:02, falken wrote: > > > https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp > > (right): > > > > > https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:72: > > observer_->DecrementPendingActivity(); > > Looking at this more, it looks like things can be cleaner if we just pass an > > argument (probably an enum) to DecrementPendingActivity that indicates whether > a > > promise rejected, resolved, or the event dispatch failed. Then I think we > don't > > need ReportPromiseRejected() or set |has_error_| separately, it can all be > done > > by DecrementPendingActivity(). WDYT? > > I like this idea making code simpler. If we do this way, one question comes up: > WaitUntilObserver::DecrementPendingActivity() is also used by > RespondWithObserver, means DecrementPendingActivity() would notify: > Event dispatch completed/failed > waitUntil promise resolved/rejected > respondWith promise resolved/rejected > Now we fire didHandleXXXEvent(Rejected) if we finally found any one rejected > waitUntil promise, but should we also do this if we found out that there has a > rejected respondWith promise? I'm not very sure about this. RespondWith reports its rejection on its own. I don't think didHandleXXXEvent should care about the result of respondWith.
On 2017/05/09 14:22:05, shimazu wrote: > On 2017/05/09 09:15:36, leonhsl wrote: > > On 2017/05/09 08:29:02, falken wrote: > > > > > > https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:72: > > > observer_->DecrementPendingActivity(); > > > Looking at this more, it looks like things can be cleaner if we just pass an > > > argument (probably an enum) to DecrementPendingActivity that indicates > whether > > a > > > promise rejected, resolved, or the event dispatch failed. Then I think we > > don't > > > need ReportPromiseRejected() or set |has_error_| separately, it can all be > > done > > > by DecrementPendingActivity(). WDYT? > > > > I like this idea making code simpler. If we do this way, one question comes > up: > > WaitUntilObserver::DecrementPendingActivity() is also used by > > RespondWithObserver, means DecrementPendingActivity() would notify: > > Event dispatch completed/failed > > waitUntil promise resolved/rejected > > respondWith promise resolved/rejected > > Now we fire didHandleXXXEvent(Rejected) if we finally found any one rejected > > waitUntil promise, but should we also do this if we found out that there has a > > rejected respondWith promise? I'm not very sure about this. > > RespondWith reports its rejection on its own. I don't think didHandleXXXEvent > should care about the result of respondWith. Yeah I'd just preserve the existing behavior, so if you take my suggestion it'd be DecrementPendingActivity(OK) or something. The result of WaitUntilObserver for fetch events doesn't really matter, see for example: // false is okay because waitUntil for fetch event doesn't care about the // promise rejection or an uncaught runtime script error. wait_until_observer->DidDispatchEvent(false /* errorOccurred */);
On 2017/05/09 09:20:12, leonhsl wrote: > On 2017/05/09 09:02:54, falken wrote: > > Sorry for the constant comments. > > > > > https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp > > (right): > > > > > https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:72: > > observer_->DecrementPendingActivity(); > > On 2017/05/09 08:29:02, falken wrote: > > > Looking at this more, it looks like things can be cleaner if we just pass an > > > argument (probably an enum) to DecrementPendingActivity that indicates > whether > > a > > > promise rejected, resolved, or the event dispatch failed. Then I think we > > don't > > > need ReportPromiseRejected() or set |has_error_| separately, it can all be > > done > > > by DecrementPendingActivity(). WDYT? > > > > Or maybe better functions like: > > OnPromiseResolved() > > OnPromiseRejected() > > OnEventDispatchFailed() > > > > The point is instead of calling a function that sets state that affects > antoher > > function, we'd just need to call a single function and have it do the work. > > Thus in correspondence seems we also need to replace IncrementPendingActivity() > with > OnPromiseStarted() > OnEventDispatchStarted() > I'd like to keep {In,De}crementPendingActivity:-) OK makes sense. I'm just interested in cleaning up the state being held between the functions that doesn't appear to be needed.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Uploaded ps#3, PTAnL, Thanks. https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:72: observer_->DecrementPendingActivity(); On 2017/05/09 08:29:02, falken wrote: > Looking at this more, it looks like things can be cleaner if we just pass an > argument (probably an enum) to DecrementPendingActivity that indicates whether a > promise rejected, resolved, or the event dispatch failed. Then I think we don't > need ReportPromiseRejected() or set |has_error_| separately, it can all be done > by DecrementPendingActivity(). WDYT? Done. https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h (right): https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:45: void DidDispatchEvent(bool error_occurred); On 2017/05/09 08:07:31, falken wrote: > I see, it's not from your patch but |error_occurred| and |has_error| are a bit > inaccurately named (they are from an earlier time). Can you rename them to > |event_dispatch_failed|? You also will need to update the comments on callsites > that are currently "// error_occurred". > > Now there's a big difference between |has_rejected_promise| and this flag, so I > want better names. > > Also comments are good: > // Must be called before dispatching the event. > void WillDispatchEvent(); > > // Must be called after dispatching the event. If |event_dispatch_failed|, this > WaitUntilObserver will soon report to ServiceWorkerGlobalScopeClient that the > event finished, without waiting for all waitUntil promises to settle. Done. https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:67: void ReportPromiseRejected(const ScriptValue&); On 2017/05/09 08:07:31, falken wrote: > Maybe just "OnPromiseRejected", as it's not really reporting to anything. > > The param isn't used, so it can be removed. > > Also a comment like would be good like: > // Called when a promise passed to a waitUntil() call that is associated with > this observer was rejected. > Removed this method, DecrementPendingActivity() covers its functionality. https://codereview.chromium.org/2867023002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:77: bool event_dispatched_ = false; On 2017/05/09 08:07:31, falken wrote: > Actually, instead of |has_error_| and |event_dispatched_|, can you change it to > an enum like: kInitial, kEventDispatchCompleted, kEventDispatchFailed Done.
lgtm with nits. Please wait for falken's comments also. https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h (right): https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:77: enum EventDispatchState { kInitial, kCompleted, kFailed }; "enum class" is better here: https://www.chromium.org/blink/coding-style#TOC-Right:12 https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:78: EventDispatchState event_dispatch_state_ = kInitial; It would be better to initialize this at the constructor to align with other members.
This looks pretty good. https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:143: void WaitUntilObserver::OnPromiseCompleted(bool rejected) { OnPromiseSettled is aligned with promise terminology. To avoid the bool param, I'd rather have this use the enum ResolveType, or just split it into OnPromiseFulfilled and OnPromiseRejected. https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h (right): https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:45: // this WaitUntilObserver will soon report to ServiceWorkerGlobalScopeClient my bad, I didn't realize the DidDispatchEvent will immediately report to the SWGSC in this case. So "If |event_dispatch_failed| is true, then DidDispatchEvent() immediately reports to ServiceWorkerGlobalScopeClient that the event finished, without waiting for all waitUntil promises to settle." https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:77: enum EventDispatchState { kInitial, kCompleted, kFailed }; Can you document: // Event dispatch has not yet finished. kInitial, // Event dispatch completed. There may still be outstanding waitUntil promises that must settle before notifying ServiceWorkerGlobalScopeClient that the event finished. kCompleted, // Event dispatch failed. Any outstanding waitUntil promises are ignored. kFailed
https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h (right): https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:78: EventDispatchState event_dispatch_state_ = kInitial; On 2017/05/10 05:20:14, shimazu wrote: > It would be better to initialize this at the constructor to align with other > members. I thought so too but pending_activity_ and has_rejected_promise_ are initialized here already... I don't know whether one is better than the other anymore. Maybe type_ and event_id_ should just be assigned to something here too?
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Uploaded ps#4 to address comments, PTAnL, Thanks. https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp (right): https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:143: void WaitUntilObserver::OnPromiseCompleted(bool rejected) { On 2017/05/10 05:25:12, falken wrote: > OnPromiseSettled is aligned with promise terminology. > > To avoid the bool param, I'd rather have this use the enum ResolveType, or just > split it into OnPromiseFulfilled and OnPromiseRejected. Done. https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h (right): https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:45: // this WaitUntilObserver will soon report to ServiceWorkerGlobalScopeClient On 2017/05/10 05:25:12, falken wrote: > my bad, I didn't realize the DidDispatchEvent will immediately report to the > SWGSC in this case. So "If |event_dispatch_failed| is true, then > DidDispatchEvent() immediately reports to ServiceWorkerGlobalScopeClient that > the event finished, without waiting for all waitUntil promises to settle." Done. https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:77: enum EventDispatchState { kInitial, kCompleted, kFailed }; On 2017/05/10 05:20:14, shimazu wrote: > "enum class" is better here: > https://www.chromium.org/blink/coding-style#TOC-Right:12 Done. https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:77: enum EventDispatchState { kInitial, kCompleted, kFailed }; On 2017/05/10 05:25:12, falken wrote: > Can you document: > > // Event dispatch has not yet finished. > kInitial, > // Event dispatch completed. There may still be outstanding waitUntil promises > that must settle before notifying ServiceWorkerGlobalScopeClient that the event > finished. > kCompleted, > // Event dispatch failed. Any outstanding waitUntil promises are ignored. > kFailed Done. https://codereview.chromium.org/2867023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h:78: EventDispatchState event_dispatch_state_ = kInitial; On 2017/05/10 05:26:52, falken wrote: > On 2017/05/10 05:20:14, shimazu wrote: > > It would be better to initialize this at the constructor to align with other > > members. > > I thought so too but pending_activity_ and has_rejected_promise_ are initialized > here already... I don't know whether one is better than the other anymore. > > Maybe type_ and event_id_ should just be assigned to something here too? We set class member's default value in .h file, so I set event_dispatch_state_ together with pending_activity_ and event_dispatch_time_ etc. While type_ and event_id_ seems have no default value, they're expected to be set explicitly by constructor.
lgtm, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
leon.han@intel.com changed reviewers: + kinuko@chromium.org
Thanks all for review, and let me TBR kinuko@ for the trivial change(code comments only) in third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp. Send to CQ now.
Description was changed from ========== [ServiceWorker] waitUntil() should wait until all promises got resolved/rejected. Spec: https://w3c.github.io/ServiceWorker/#wait-until-method For multiple waitUntil() pending promises, even if some one got rejected while some others are still pending at the same time, we should continue waiting, until all pending promises completed(resolved or rejected). BUG=719850 TEST=blink_tests external/wpt/service-workers/service-worker/extendable-event-waituntil.https.html ========== to ========== [ServiceWorker] waitUntil() should wait until all promises got resolved/rejected. Spec: https://w3c.github.io/ServiceWorker/#wait-until-method For multiple waitUntil() pending promises, even if some one got rejected while some others are still pending at the same time, we should continue waiting, until all pending promises completed(resolved or rejected). BUG=719850 TEST=blink_tests external/wpt/service-workers/service-worker/extendable-event-waituntil.https.html TBR=kinuko@chromium.org ==========
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from shimazu@chromium.org Link to the patchset: https://codereview.chromium.org/2867023002/#ps60001 (title: "More clean up")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leon.han@intel.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leon.han@intel.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leon.han@intel.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494473950939140,
"parent_rev": "6156f7b21c2555d4ce52cecff7f5851ebb57652e", "commit_rev":
"06f33f6fa8d71e31838103a0e12f95e8420510d2"}
Message was sent while issue was closed.
Description was changed from ========== [ServiceWorker] waitUntil() should wait until all promises got resolved/rejected. Spec: https://w3c.github.io/ServiceWorker/#wait-until-method For multiple waitUntil() pending promises, even if some one got rejected while some others are still pending at the same time, we should continue waiting, until all pending promises completed(resolved or rejected). BUG=719850 TEST=blink_tests external/wpt/service-workers/service-worker/extendable-event-waituntil.https.html TBR=kinuko@chromium.org ========== to ========== [ServiceWorker] waitUntil() should wait until all promises got resolved/rejected. Spec: https://w3c.github.io/ServiceWorker/#wait-until-method For multiple waitUntil() pending promises, even if some one got rejected while some others are still pending at the same time, we should continue waiting, until all pending promises completed(resolved or rejected). BUG=719850 TEST=blink_tests external/wpt/service-workers/service-worker/extendable-event-waituntil.https.html TBR=kinuko@chromium.org Review-Url: https://codereview.chromium.org/2867023002 Cr-Commit-Position: refs/heads/master@{#470828} Committed: https://chromium.googlesource.com/chromium/src/+/06f33f6fa8d71e31838103a0e12f... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/06f33f6fa8d71e31838103a0e12f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
