Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2569993002:
[ServiceWorker] Mojofy PushEvent of Service Worker. (Closed)
|
Created:
4 years ago by xiaofengzhang Modified:
3 years, 12 months ago CC:
Aaron Boodman, abarth-chromium, blink-worker-reviews_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, harkness+watch_chromium.org, horo+watch_chromium.org, jam, johnme+watch_chromium.org, jsbell+serviceworker_chromium.org, kinuko+serviceworker, kinuko+watch, michaeln, mlamouri+watch-content_chromium.org, nhiroki, Peter Beverloo, qsr+mojo_chromium.org, serviceworker-reviews, shimazu+serviceworker_chromium.org, tzik, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ServiceWorker] Mojofy PushEvent of Service Worker.
This CL converts push event IPCs into mojo interface:
ServiceWorkerMsg_PushEvent
ServiceWorkerHostMsg_PushEventFinished
BUG=629701
TEST=content_unittests
Committed: https://crrev.com/35842853527f50a04662a6dfe2c3cd1b10cf5915
Cr-Commit-Position: refs/heads/master@{#439739}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Address comments from Peter and shimazu #
Total comments: 7
Patch Set 3 : Rebase and address comments from Peter and Marijn #Messages
Total messages: 51 (23 generated)
Description was changed from ========== [ServiceWorker] Mojofy PushEvent of Service Worker. This CL converts push event IPCs into mojo interface: ServiceWorkerMsg_PushEvent ServiceWorkerHostMsg_PushEventFinished BUG=629701 TEST=content_unittests ========== to ========== [ServiceWorker] Mojofy PushEvent of Service Worker. This CL converts push event IPCs into mojo interface: ServiceWorkerMsg_PushEvent ServiceWorkerHostMsg_PushEventFinished BUG=629701 TEST=content_unittests ==========
xiaofeng.zhang@intel.com changed reviewers: + leon.han@intel.com
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.
peter@chromium.org changed reviewers: + peter@chromium.org
peter@chromium.org changed required reviewers: + peter@chromium.org
Cool, thank you! Welcome to the Chromium project! :) https://codereview.chromium.org/2569993002/diff/1/AUTHORS File AUTHORS (right): https://codereview.chromium.org/2569993002/diff/1/AUTHORS#newcode730 AUTHORS:730: Xiaofeng Zhang <xiaofeng.zhang@intel.com> Note: CLA says yes. https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... File content/browser/push_messaging/push_messaging_router.cc (right): https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... content/browser/push_messaging/push_messaging_router.cc:133: base::Unretained(service_worker.get()), request_id)); Instead of using base::Unretained(), just pass |service_worker| directly. It's a scoped_refptr so that should work just fine, and that allows you to drop the comment. Ideally this entire class would stop using const& for the scoped_refptr<> arguments. That would allow us to use std::move() instead of making copies. But that's a clean-up for another time :). https://codereview.chromium.org/2569993002/diff/1/content/common/service_work... File content/common/service_worker/service_worker_event_dispatcher.typemap (right): https://codereview.chromium.org/2569993002/diff/1/content/common/service_work... content/common/service_worker/service_worker_event_dispatcher.typemap:22: "blink.mojom.ServiceWorkerEventStatus=::content::ServiceWorkerStatusCode", nit: maybe alphabetize? https://codereview.chromium.org/2569993002/diff/1/content/renderer/service_wo... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2569993002/diff/1/content/renderer/service_wo... content/renderer/service_worker/service_worker_context_client.cc:598: } fwiw, since this is now repeated four times, consider having an anonymous, templatized function that reduces the duplication. https://codereview.chromium.org/2569993002/diff/1/content/renderer/service_wo... content/renderer/service_worker/service_worker_context_client.cc:777: return; Should this have a LOG(WARN)? Maybe even a DCHECK? In which scenario would we end up acknowledging that a push event that doesn't exist anymore has finished? https://codereview.chromium.org/2569993002/diff/1/content/renderer/service_wo... content/renderer/service_worker/service_worker_context_client.cc:778: if (result == blink::WebServiceWorkerEventResultCompleted) { I notice that this reverses the logic for determining the |status|. Previously[1] we would always return |SERVICE_WORKER_OK|, and only return |SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED| when the |result| was set to |blink::WebServiceWorkerEventResultRejected|. Since there are only two entries in the |WebServiceWorkerEventResult| enum that's probably OK, but it becomes significant if a third entry ever gets added. The best option would be to use a switch() so that we get compile time verification, but that may end up being verbose. I'll defer to the OWNERS of this file. [1] https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w... https://codereview.chromium.org/2569993002/diff/1/content/renderer/service_wo... content/renderer/service_worker/service_worker_context_client.cc:783: base::Time::FromDoubleT(event_dispatch_time)); nit: prefer avoiding multiple calls to the same method. Something like the following ought to work: ServiceWorkerStatusCode status = result == blink::WebServiceWorkerEventResultCompleted ? SERVICE_WORKER_OK : SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED; callback->Run(status, ...);
Thanks Peter a lot for kindly review! https://codereview.chromium.org/2569993002/diff/1/AUTHORS File AUTHORS (right): https://codereview.chromium.org/2569993002/diff/1/AUTHORS#newcode730 AUTHORS:730: Xiaofeng Zhang <xiaofeng.zhang@intel.com> Nit: alphabetize? https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... File content/browser/push_messaging/push_messaging_router.cc (right): https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... content/browser/push_messaging/push_messaging_router.cc:133: base::Unretained(service_worker.get()), request_id)); On 2016/12/13 13:31:32, Peter Beverloo wrote: > Instead of using base::Unretained(), just pass |service_worker| directly. It's a > scoped_refptr so that should work just fine, and that allows you to drop the > comment. > I have one question which I'm not so sure: This way the bound callback would hold an additional reference on |service_worker|, then only after this callback got fired or destroyed, |service_worker| would got chance to be destroyed, this would be a little different with previous scenario of |service_worker| lifecycle? > Ideally this entire class would stop using const& for the scoped_refptr<> > arguments. That would allow us to use std::move() instead of making copies. But > that's a clean-up for another time :).
shimazu@chromium.org changed reviewers: + shimazu@chromium.org
Thanks for working on this! https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... File content/browser/push_messaging/push_messaging_router.cc (right): https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... content/browser/push_messaging/push_messaging_router.cc:129: // use nit: please fix indent
On 2016/12/13 13:31:32, Peter Beverloo wrote: Thanks a lot for your kindly review. > Cool, thank you! Welcome to the Chromium project! :) > > https://codereview.chromium.org/2569993002/diff/1/AUTHORS > File AUTHORS (right): > > https://codereview.chromium.org/2569993002/diff/1/AUTHORS#newcode730 > AUTHORS:730: Xiaofeng Zhang <mailto:xiaofeng.zhang@intel.com> > Note: CLA says yes. In the internal process. > https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... > File content/browser/push_messaging/push_messaging_router.cc (right): > > https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... > content/browser/push_messaging/push_messaging_router.cc:133: > base::Unretained(service_worker.get()), request_id)); > Instead of using base::Unretained(), just pass |service_worker| directly. It's a > scoped_refptr so that should work just fine, and that allows you to drop the > comment. > > Ideally this entire class would stop using const& for the scoped_refptr<> > arguments. That would allow us to use std::move() instead of making copies. But > that's a clean-up for another time :). > > https://codereview.chromium.org/2569993002/diff/1/content/common/service_work... > File content/common/service_worker/service_worker_event_dispatcher.typemap > (right): > > https://codereview.chromium.org/2569993002/diff/1/content/common/service_work... > content/common/service_worker/service_worker_event_dispatcher.typemap:22: > "blink.mojom.ServiceWorkerEventStatus=::content::ServiceWorkerStatusCode", > nit: maybe alphabetize? > Acknowledged. > https://codereview.chromium.org/2569993002/diff/1/content/renderer/service_wo... > File content/renderer/service_worker/service_worker_context_client.cc (right): > > https://codereview.chromium.org/2569993002/diff/1/content/renderer/service_wo... > content/renderer/service_worker/service_worker_context_client.cc:598: } > fwiw, since this is now repeated four times, consider having an anonymous, > templatized function that reduces the duplication. > Acknowledged. > https://codereview.chromium.org/2569993002/diff/1/content/renderer/service_wo... > content/renderer/service_worker/service_worker_context_client.cc:777: return; > Should this have a LOG(WARN)? Maybe even a DCHECK? In which scenario would we > end up acknowledging that a push event that doesn't exist anymore has finished? > Acknowledged. I should use DCHECK. > https://codereview.chromium.org/2569993002/diff/1/content/renderer/service_wo... > content/renderer/service_worker/service_worker_context_client.cc:778: if (result > == blink::WebServiceWorkerEventResultCompleted) { > I notice that this reverses the logic for determining the |status|. > Previously[1] we would always return |SERVICE_WORKER_OK|, and only return > |SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED| when the |result| was set to > |blink::WebServiceWorkerEventResultRejected|. > > Since there are only two entries in the |WebServiceWorkerEventResult| enum > that's probably OK, but it becomes significant if a third entry ever gets added. > The best option would be to use a switch() so that we get compile time > verification, but that may end up being verbose. I'll defer to the OWNERS of > this file. > > [1] > https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w... > > https://codereview.chromium.org/2569993002/diff/1/content/renderer/service_wo... > content/renderer/service_worker/service_worker_context_client.cc:783: > base::Time::FromDoubleT(event_dispatch_time)); > nit: prefer avoiding multiple calls to the same method. Something like the > following ought to work: > > ServiceWorkerStatusCode status = > result == blink::WebServiceWorkerEventResultCompleted > ? SERVICE_WORKER_OK > : SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED; > > callback->Run(status, ...); Acknowledged.
On 2016/12/14 03:29:18, leonhsl wrote: > Thanks Peter a lot for kindly review! > > https://codereview.chromium.org/2569993002/diff/1/AUTHORS > File AUTHORS (right): > > https://codereview.chromium.org/2569993002/diff/1/AUTHORS#newcode730 > AUTHORS:730: Xiaofeng Zhang <mailto:xiaofeng.zhang@intel.com> > Nit: alphabetize? > Acknowledged.Thanks. > https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... > File content/browser/push_messaging/push_messaging_router.cc (right): > > https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... > content/browser/push_messaging/push_messaging_router.cc:133: > base::Unretained(service_worker.get()), request_id)); > On 2016/12/13 13:31:32, Peter Beverloo wrote: > > Instead of using base::Unretained(), just pass |service_worker| directly. It's > a > > scoped_refptr so that should work just fine, and that allows you to drop the > > comment. > > > I have one question which I'm not so sure: This way the bound callback would > hold an additional reference on |service_worker|, then only after this callback > got fired or destroyed, |service_worker| would got chance to be destroyed, this > would be a little different with previous scenario of |service_worker| > lifecycle? > > > Ideally this entire class would stop using const& for the scoped_refptr<> > > arguments. That would allow us to use std::move() instead of making copies. > But > > that's a clean-up for another time :).
On 2016/12/14 04:42:34, shimazu wrote: > Thanks for working on this! > > https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... > File content/browser/push_messaging/push_messaging_router.cc (right): > > https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... > content/browser/push_messaging/push_messaging_router.cc:129: // use > nit: please fix indent Shimazu, Thanks a lot for your kindly review
On 2016/12/14 04:42:34, shimazu wrote: > Thanks for working on this! > > https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... > File content/browser/push_messaging/push_messaging_router.cc (right): > > https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... > content/browser/push_messaging/push_messaging_router.cc:129: // use > nit: please fix indent Hi, Shimazu-san, Xiaofeng is team mate targeting on contributions of service worker mojofication together with me, yoroshiku~
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.
https://codereview.chromium.org/2569993002/diff/1/AUTHORS File AUTHORS (right): https://codereview.chromium.org/2569993002/diff/1/AUTHORS#newcode730 AUTHORS:730: Xiaofeng Zhang <xiaofeng.zhang@intel.com> On 2016/12/13 13:31:32, Peter Beverloo wrote: > Note: CLA says yes. In the internal process.. https://codereview.chromium.org/2569993002/diff/1/AUTHORS#newcode730 AUTHORS:730: Xiaofeng Zhang <xiaofeng.zhang@intel.com> On 2016/12/14 03:29:18, leonhsl wrote: > Nit: alphabetize? Done. https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... File content/browser/push_messaging/push_messaging_router.cc (right): https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... content/browser/push_messaging/push_messaging_router.cc:129: // use On 2016/12/14 04:42:34, shimazu wrote: > nit: please fix indent Acknowledged. https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... content/browser/push_messaging/push_messaging_router.cc:133: base::Unretained(service_worker.get()), request_id)); On 2016/12/13 13:31:32, Peter Beverloo wrote: > Instead of using base::Unretained(), just pass |service_worker| directly. It's a > scoped_refptr so that should work just fine, and that allows you to drop the > comment. > > Ideally this entire class would stop using const& for the scoped_refptr<> > arguments. That would allow us to use std::move() instead of making copies. But > that's a clean-up for another time :). I re-submit and pass |service_worker| directly, but please also see leon's question. For "stop using const& for the scoped_refptr", yes, I will do it in another next cl. Thanks :) https://codereview.chromium.org/2569993002/diff/1/content/common/service_work... File content/common/service_worker/service_worker_event_dispatcher.typemap (right): https://codereview.chromium.org/2569993002/diff/1/content/common/service_work... content/common/service_worker/service_worker_event_dispatcher.typemap:22: "blink.mojom.ServiceWorkerEventStatus=::content::ServiceWorkerStatusCode", On 2016/12/13 13:31:32, Peter Beverloo wrote: > nit: maybe alphabetize? Done. https://codereview.chromium.org/2569993002/diff/1/content/renderer/service_wo... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2569993002/diff/1/content/renderer/service_wo... content/renderer/service_worker/service_worker_context_client.cc:777: return; On 2016/12/13 13:31:32, Peter Beverloo wrote: > Should this have a LOG(WARN)? Maybe even a DCHECK? In which scenario would we > end up acknowledging that a push event that doesn't exist anymore has finished? Done. https://codereview.chromium.org/2569993002/diff/1/content/renderer/service_wo... content/renderer/service_worker/service_worker_context_client.cc:783: base::Time::FromDoubleT(event_dispatch_time)); On 2016/12/13 13:31:32, Peter Beverloo wrote: > nit: prefer avoiding multiple calls to the same method. Something like the > following ought to work: > > ServiceWorkerStatusCode status = > result == blink::WebServiceWorkerEventResultCompleted > ? SERVICE_WORKER_OK > : SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED; > > callback->Run(status, ...); Done. Thanks :-)
https://codereview.chromium.org/2569993002/diff/1/AUTHORS File AUTHORS (right): https://codereview.chromium.org/2569993002/diff/1/AUTHORS#newcode730 AUTHORS:730: Xiaofeng Zhang <xiaofeng.zhang@intel.com> On 2016/12/15 01:30:48, xiaofeng.zhang wrote: > On 2016/12/13 13:31:32, Peter Beverloo wrote: > > Note: CLA says yes. > > In the internal process.. Sorry - I just wanted to confirm that you're able to contribute :). https://codereview.chromium.org/2569993002/diff/20001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2569993002/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:757: DCHECK(callback); nit: yay, thanks! :) This has one downside - DCHECK only works in debug builds. If |callback| is NULL in a release build, we'd crash on the next line. Instead, you could format it as such: if (!callback) { NOTREACHED(); return; } The NOTREACHED() has the same effect as the DCHECK, whereas the returns make sure that we never use the NULL |callback| value. https://codereview.chromium.org/2569993002/diff/20001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2569993002/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.h:299: for (typename T::iterator it(&x); !it.IsAtEnd(); it.Advance()) { nit: s/x/callbacks/ https://codereview.chromium.org/2569993002/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.h:303: nit: I'd move this to service_worker_context_client.cc to around line 167. (In the anonymous namespace.) It's only used by the file itself, so there's no need to define it in the header and (potentially) expose it to other users.
https://codereview.chromium.org/2569993002/diff/20001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2569993002/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:757: DCHECK(callback); On 2016/12/15 at 17:35:39, Peter Beverloo wrote: > nit: yay, thanks! :) > > This has one downside - DCHECK only works in debug builds. If |callback| is NULL in a release build, we'd crash on the next line. Instead, you could format it as such: > > if (!callback) { > NOTREACHED(); > return; > } > > The NOTREACHED() has the same effect as the DCHECK, whereas the returns make sure that we never use the NULL |callback| value. Handling a DCHECK failure (or your NOTREACHED case, which as you point out is pretty much identical) is explicitly against our styleguide (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...). So the code in the CL with DCHECK and possible crash is the only style-guide approved way to write this.
https://codereview.chromium.org/2569993002/diff/1/AUTHORS File AUTHORS (right): https://codereview.chromium.org/2569993002/diff/1/AUTHORS#newcode730 AUTHORS:730: Xiaofeng Zhang <xiaofeng.zhang@intel.com> On 2016/12/15 17:35:39, Peter Beverloo wrote: > On 2016/12/15 01:30:48, xiaofeng.zhang wrote: > > On 2016/12/13 13:31:32, Peter Beverloo wrote: > > > Note: CLA says yes. > > > > In the internal process.. > > Sorry - I just wanted to confirm that you're able to contribute :). Yes, I have confirmed, I can contribute https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... File content/browser/push_messaging/push_messaging_router.cc (right): https://codereview.chromium.org/2569993002/diff/1/content/browser/push_messag... content/browser/push_messaging/push_messaging_router.cc:133: base::Unretained(service_worker.get()), request_id)); On 2016/12/13 13:31:32, Peter Beverloo wrote: > Instead of using base::Unretained(), just pass |service_worker| directly. It's a > scoped_refptr so that should work just fine, and that allows you to drop the > comment. > > Ideally this entire class would stop using const& for the scoped_refptr<> > arguments. That would allow us to use std::move() instead of making copies. But > that's a clean-up for another time :). Any comments on Leon's question: "I have one question which I'm not so sure: This way the bound callback would hold an additional reference on |service_worker|, then only after this callback got fired or destroyed, |service_worker| would got chance to be destroyed, this would be a little different with previous scenario of |service_worker| lifecycle?"
https://codereview.chromium.org/2569993002/diff/20001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2569993002/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:757: DCHECK(callback); On 2016/12/15 17:42:36, Marijn Kruisselbrink wrote: > On 2016/12/15 at 17:35:39, Peter Beverloo wrote: > > nit: yay, thanks! :) > > > > This has one downside - DCHECK only works in debug builds. If |callback| is > NULL in a release build, we'd crash on the next line. Instead, you could format > it as such: > > > > if (!callback) { > > NOTREACHED(); > > return; > > } > > > > The NOTREACHED() has the same effect as the DCHECK, whereas the returns make > sure that we never use the NULL |callback| value. > > Handling a DCHECK failure (or your NOTREACHED case, which as you point out is > pretty much identical) is explicitly against our styleguide > (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...). > So the code in the CL with DCHECK and possible crash is the only style-guide > approved way to write this. Acknowledged. https://codereview.chromium.org/2569993002/diff/20001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2569993002/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.h:299: for (typename T::iterator it(&x); !it.IsAtEnd(); it.Advance()) { On 2016/12/15 17:35:40, Peter Beverloo wrote: > nit: s/x/callbacks/ Acknowledged. https://codereview.chromium.org/2569993002/diff/20001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.h:303: On 2016/12/15 17:35:39, Peter Beverloo wrote: > nit: I'd move this to service_worker_context_client.cc to around line 167. (In > the anonymous namespace.) It's only used by the file itself, so there's no need > to define it in the header and (potentially) expose it to other users. Acknowledged.
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.
Friendly ping Shimazu and Peter :)
shimazu@chromium.org changed reviewers: + falken@chromium.org
Non-owner lgtm, thanks:) +falken as the OWNER.
sw lgtm. Beware that we have seen ordering issues when moving from IPC to Mojo: https://bugs.chromium.org/p/chromium/issues/detail?id=671980 So be careful if the push event IPC had to come before or after some other IPC.
push lgtm - thank you! On 2016/12/19 06:21:39, falken wrote: > sw lgtm. Beware that we have seen ordering issues when moving from IPC to Mojo: > https://bugs.chromium.org/p/chromium/issues/detail?id=671980 > > So be careful if the push event IPC had to come before or after some other IPC. We're good here. Receiving a message over a push subscription requires the developer's application server to know the details, which they only receive after we resolve the subscribe() person. Thanks for the reminder!
leon.han@intel.com changed reviewers: + tsepez@chromium.org
Hi, Tom, would you PTAL for OWNER review of mojom/typemap files? Thanks.
lgtm
On 2016/12/20 00:17:25, Tom Sepez wrote: > lgtm Thanks a lot for all the reviews.
The CQ bit was checked by xiaofeng.zhang@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": 40001, "attempt_start_ts": 1482213071287690,
"parent_rev": "7269ddefcac0661e7a9b399f7c0e70d9cb8281c0", "commit_rev":
"66a48c8fc2181dabe5d5c0006151fd65a012cf90"}
Message was sent while issue was closed.
Description was changed from ========== [ServiceWorker] Mojofy PushEvent of Service Worker. This CL converts push event IPCs into mojo interface: ServiceWorkerMsg_PushEvent ServiceWorkerHostMsg_PushEventFinished BUG=629701 TEST=content_unittests ========== to ========== [ServiceWorker] Mojofy PushEvent of Service Worker. This CL converts push event IPCs into mojo interface: ServiceWorkerMsg_PushEvent ServiceWorkerHostMsg_PushEventFinished BUG=629701 TEST=content_unittests Review-Url: https://codereview.chromium.org/2569993002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [ServiceWorker] Mojofy PushEvent of Service Worker. This CL converts push event IPCs into mojo interface: ServiceWorkerMsg_PushEvent ServiceWorkerHostMsg_PushEventFinished BUG=629701 TEST=content_unittests Review-Url: https://codereview.chromium.org/2569993002 ========== to ========== [ServiceWorker] Mojofy PushEvent of Service Worker. This CL converts push event IPCs into mojo interface: ServiceWorkerMsg_PushEvent ServiceWorkerHostMsg_PushEventFinished BUG=629701 TEST=content_unittests Committed: https://crrev.com/35842853527f50a04662a6dfe2c3cd1b10cf5915 Cr-Commit-Position: refs/heads/master@{#439739} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/35842853527f50a04662a6dfe2c3cd1b10cf5915 Cr-Commit-Position: refs/heads/master@{#439739}
Message was sent while issue was closed.
On 2016/12/20 07:55:02, commit-bot: I haz the power wrote: > Patchset 3 (id:??) landed as > https://crrev.com/35842853527f50a04662a6dfe2c3cd1b10cf5915 > Cr-Commit-Position: refs/heads/master@{#439739} I think this is causing crashes on canary. I'm inclined to revert to try to keep canary stable over the holiday season. I opened https://bugs.chromium.org/p/chromium/issues/detail?id=676984 but as a crash bug it's restricted and unfortunately I can't cc xiaofeng.zhang on it (but did cc leonhsl).
Message was sent while issue was closed.
On 2016/12/26 02:20:26, falken wrote: > On 2016/12/20 07:55:02, commit-bot: I haz the power wrote: > > Patchset 3 (id:??) landed as > > https://crrev.com/35842853527f50a04662a6dfe2c3cd1b10cf5915 > > Cr-Commit-Position: refs/heads/master@{#439739} > > I think this is causing crashes on canary. I'm inclined to revert to try to keep > canary stable over the holiday season. I opened > https://bugs.chromium.org/p/chromium/issues/detail?id=676984 but as a crash bug > it's restricted and unfortunately I can't cc xiaofeng.zhang on it (but did cc > leonhsl). @falken I don't have the access to see the issue #676984, could you provide more details about the crash?
Message was sent while issue was closed.
On 2016/12/26 02:25:35, xiaofeng.zhang wrote: > On 2016/12/26 02:20:26, falken wrote: > > On 2016/12/20 07:55:02, commit-bot: I haz the power wrote: > > > Patchset 3 (id:??) landed as > > > https://crrev.com/35842853527f50a04662a6dfe2c3cd1b10cf5915 > > > Cr-Commit-Position: refs/heads/master@{#439739} > > > > I think this is causing crashes on canary. I'm inclined to revert to try to > keep > > canary stable over the holiday season. I opened > > https://bugs.chromium.org/p/chromium/issues/detail?id=676984 but as a crash > bug > > it's restricted and unfortunately I can't cc xiaofeng.zhang on it (but did cc > > leonhsl). > > @falken > I don't have the access to see the issue #676984, could you provide more details > about the crash? Ah didn't realize we are in the same time zone and working :) Yes I couldn't cc you because you're not yet in the system. Here is the crash stack: Thread 14 CRASHED [EXCEPTION_ACCESS_VIOLATION_READ @ 0x00000000 ] MAGIC SIGNATURE THREAD Stack Quality100%Show frame trust levels 0x5eb4a4fa (chrome.dll + 0x000fa4fa ) base::Callback<void ,1,1>::Callback<void ,1,1>(base::Callback<void ,1,1> const &) 0x6003ec7d (chrome.dll -service_worker_version.cc:1095 ) content::ServiceWorkerVersion::OnSimpleEventFinished(int,content::ServiceWorkerStatusCode,base::Time) 0x5ff64f33 (chrome.dll -bind_internal.h:214 ) base::internal::FunctorTraits<void ( content::ServiceWorkerVersion::*)(int,content::ServiceWorkerStatusCode,base::Time),void>::Invoke<scoped_refptr<content::ServiceWorkerVersion> const &,int const &,content::ServiceWorkerStatusCode,base::Time>(void ( content::ServiceWorkerVersion::*)(int,content::ServiceWorkerStatusCode,base::Time),scoped_refptr<content::ServiceWorkerVersion> const &,int const &,content::ServiceWorkerStatusCode &&,base::Time &&) 0x5ff50241 (chrome.dll -bind_internal.h:285 ) base::internal::InvokeHelper<0,void>::MakeItSo<void ( content::ServiceWorkerVersion::*const &)(int,blink::WebServiceWorkerEventResult,base::Time),scoped_refptr<content::ServiceWorkerVersion> const &,int,blink::WebServiceWorkerEventResult,base::Time>(void ( content::ServiceWorkerVersion::*const &)(int,blink::WebServiceWorkerEventResult,base::Time),scoped_refptr<content::ServiceWorkerVersion> const &,int &&,blink::WebServiceWorkerEventResult &&,base::Time &&) 0x5ff655a6 (chrome.dll -bind_internal.h:339 ) base::internal::Invoker<base::internal::BindState<void ( content::ServiceWorkerVersion::*)(int,content::ServiceWorkerStatusCode,base::Time),scoped_refptr<content::ServiceWorkerVersion>,int>,void >::Run(base::internal::BindStateBase *,content::ServiceWorkerStatusCode &&,base::Time &&) 0x5fbe1b09 (chrome.dll -callback.h:85 ) base::internal::RunMixin<base::Callback<void ,1,1> >::Run(filesystem::mojom::FileError,__int64) 0x5fba0984 (chrome.dll -service_worker_event_dispatcher.mojom.cc:225 ) content::mojom::ServiceWorkerEventDispatcher_DispatchPushEvent_ForwardToCallback::Accept(mojo::Message *) 0x6037605b (chrome.dll -interface_endpoint_client.cc:336 ) mojo::InterfaceEndpointClient::HandleValidatedMessage(mojo::Message *) Feel free to ping me on IRC for chatting (going to lunch soon though).
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2600863002/ by falken@chromium.org. The reason for reverting is: Causes crashes on canary. See https://crbug.com/676984 0x5eb4a4fa (chrome.dll + 0x000fa4fa ) base::Callback<void ,1,1>::Callback<void ,1,1>(base::Callback<void ,1,1> const &) 0x6003ec7d (chrome.dll -service_worker_version.cc:1095 ) content::ServiceWorkerVersion::OnSimpleEventFinished(int,content::ServiceWorkerStatusCode,base::Time) 0x5ff64f33 (chrome.dll -bind_internal.h:214 ) base::internal::FunctorTraits<void ( content::ServiceWorkerVersion::*)(int,content::ServiceWorkerStatusCode,base::Time),void>::Invoke<scoped_refptr<content::ServiceWorkerVersion> const &,int const &,content::ServiceWorkerStatusCode,base::Time>(void ( content::ServiceWorkerVersion::*)(int,content::ServiceWorkerStatusCode,base::Time),scoped_refptr<content::ServiceWorkerVersion> const &,int const &,content::ServiceWorkerStatusCode &&,base::Time &&) 0x5ff50241 (chrome.dll -bind_internal.h:285 ) base::internal::InvokeHelper<0,void>::MakeItSo<void ( content::ServiceWorkerVersion::*const &)(int,blink::WebServiceWorkerEventResult,base::Time),scoped_refptr<content::ServiceWorkerVersion> const &,int,blink::WebServiceWorkerEventResult,base::Time>(void ( content::ServiceWorkerVersion::*const &)(int,blink::WebServiceWorkerEventResult,base::Time),scoped_refptr<content::ServiceWorkerVersion> const &,int &&,blink::WebServiceWorkerEventResult &&,base::Time &&) 0x5ff655a6 (chrome.dll -bind_internal.h:339 ) base::internal::Invoker<base::internal::BindState<void ( content::ServiceWorkerVersion::*)(int,content::ServiceWorkerStatusCode,base::Time),scoped_refptr<content::ServiceWorkerVersion>,int>,void >::Run(base::internal::BindStateBase *,content::ServiceWorkerStatusCode &&,base::Time &&) 0x5fbe1b09 (chrome.dll -callback.h:85 ) base::internal::RunMixin<base::Callback<void ,1,1> >::Run(filesystem::mojom::FileError,__int64) 0x5fba0984 (chrome.dll -service_worker_event_dispatcher.mojom.cc:225 ) content::mojom::ServiceWorkerEventDispatcher_DispatchPushEvent_ForwardToCallback::Accept(mojo::Message *) 0x6037605b (chrome.dll -interface_endpoint_client.cc:336 ) mojo::InterfaceEndpointClient::HandleValidatedMessage(mojo::Message *) . |
