1113627 - Refactor lifecycle event dispatching and handling

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Refactor lifecycle event dispatching and handling

install and activate events are handled by common code that then uses a ContinueLifecycleTask to finish the installation on the main thread based on success/error.

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8539238 [details] [diff] [review]
Refactor lifecycle event dispatching and handling

Review of attachment 8539238 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerManager.cpp
@@ +151,5 @@
> +  virtual ~ContinueLifecycleTask()
> +  { }
> +
> +public:
> +  virtual void ContinueAfterWorkerEvent(bool aSuccess, bool aActivateImmediately)

what about:

virtual void ContinueAfterWorkerEvent(bool aSuccess, bool aActivateImmediately) = 0;

@@ +234,1 @@
>        : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount),

This indentation is different than what you have in the other classes.
Be consistent with the comma.

@@ +910,5 @@
>      WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
>      MOZ_ASSERT(workerPrivate);
>      workerPrivate->AssertIsOnWorkerThread();
>  
> +    nsRefPtr<ContinueLifecycleRunnable> r = new ContinueLifecycleRunnable(mTask, true /* success */, mActivateImmediately);

80chars

@@ +921,5 @@
> +    WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
> +    MOZ_ASSERT(workerPrivate);
> +    workerPrivate->AssertIsOnWorkerThread();
> +
> +    nsRefPtr<ContinueLifecycleRunnable> r = new ContinueLifecycleRunnable(mTask, false /* success */, mActivateImmediately);

80chars

@@ +985,5 @@
>  
> +  // activateimmediately is only relevant to "install" event.
> +  bool activateImmediately = false;
> +  InstallEvent* downCast = static_cast<InstallEvent*>(event.get());
> +  if (downCast) {

You don't need to do this check. if event exists, downCast will exist.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Andrea Marchesini (:baku) from comment #2)
> 
> @@ +985,5 @@
> >  
> > +  // activateimmediately is only relevant to "install" event.
> > +  bool activateImmediately = false;
> > +  InstallEvent* downCast = static_cast<InstallEvent*>(event.get());
> > +  if (downCast) {
> 
> You don't need to do this check. if event exists, downCast will exist.

event is an ExtendableEvent, which may not be an InstallEvent.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ff80c877212

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #3)
> (In reply to Andrea Marchesini (:baku) from comment #2)
> > 
> > @@ +985,5 @@
> > >  
> > > +  // activateimmediately is only relevant to "install" event.
> > > +  bool activateImmediately = false;
> > > +  InstallEvent* downCast = static_cast<InstallEvent*>(event.get());
> > > +  if (downCast) {
> > 
> > You don't need to do this check. if event exists, downCast will exist.
> 
> event is an ExtendableEvent, which may not be an InstallEvent.

Right, i found out that static_cast<> doesn't really work the way i thought, the hard way
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5846471&repo=mozilla-inbound. Will fix on monday

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bb9a4a4133af for asan mochitest-4 bustage: 

https://treeherder.mozilla.org/logviewer.html#?job_id=5846451&repo=mozilla-inbound

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: fix InstallEvent downcast

Boris, is this what you meant by adding a As*Event method?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8554846 [details] [diff] [review]
fix InstallEvent downcast

Yes, except you can put the version that returns nullptr on Event, right after the place where it #includes mozilla/dom/GeneratedEventList.h.

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53a300073430

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/53a300073430