Andreas Tolfsen ❲:ato❳ Assignee
	Description • 7 years ago

When Marionette starts and shuts down it emits the remote-active
system notification.  On starting the data is set to true (a boolean),
but on shutdown it is left undefined which according to [1] defaults
to null.

We should use false (boolean) explicitly.

  [1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIObserverService#notifyObservers()

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 3 • 7 years ago
mozreview-review

Comment on attachment 8934256 [details]
Bug 1422915 - Send remote-active after socket has ceased listening.

https://reviewboard.mozilla.org/r/205172/#review211180

::: testing/marionette/server.js:391
(Diff revision 1)
> -    Services.obs.notifyObservers(this, NOTIFY_RUNNING);
> -
>      // Shutdown server socket, and no longer listen for new connections
>      this.acceptConnections = false;
>  
> +    Services.obs.notifyObservers(this, NOTIFY_RUNNING);

Maybe it would be better to move it closer to the code which enables/disables the TCP server. So why can't we have it in `acceptConnections` directly?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 4 • 7 years ago
mozreview-review-reply

Comment on attachment 8934256 [details]
Bug 1422915 - Send remote-active after socket has ceased listening.

https://reviewboard.mozilla.org/r/205172/#review211180

> Maybe it would be better to move it closer to the code which enables/disables the TCP server. So why can't we have it in `acceptConnections` directly?

We want to fire this at the outer bounds of when Marionette is
initialised and torn down.  The Marionette component’s lifetime
ends when this stop() function is called – it is the only place we
can be absolutely sure everything has come to a halt.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 5 • 7 years ago
mozreview-review-reply

Comment on attachment 8934256 [details]
Bug 1422915 - Send remote-active after socket has ceased listening.

https://reviewboard.mozilla.org/r/205172/#review211180

> We want to fire this at the outer bounds of when Marionette is
> initialised and torn down.  The Marionette component’s lifetime
> ends when this stop() function is called – it is the only place we
> can be absolutely sure everything has come to a halt.

Ok, that makes sense.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8934256 [details]
Bug 1422915 - Send remote-active after socket has ceased listening.

https://reviewboard.mozilla.org/r/205172/#review211406

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8934257 [details]
Bug 1422915 - Be explicit about remote-active data when shutting down.

https://reviewboard.mozilla.org/r/205174/#review211408

	Pulsebot
	Comment 8 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a30ad544bf0f
Send remote-active after socket has ceased listening. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/d61481e31a93
Be explicit about remote-active data when shutting down. r=whimboo

	Tiberius Oros[:tiberius_oros]
	Comment 9 • 7 years ago

Backed out 2 changesets (bug 1422915)for eslint failure on /builds/worker/checkouts/gecko/testing/marionette/server.js:391:5 r=backout

https://hg.mozilla.org/integration/autoland/rev/0a040f4441d0a284b1b51daa8567a8ed7feb1b6f

https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=150148480

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d61481e31a93819cb54e07525294c8b42b8f7e90

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 10 • 7 years ago

OK, fine I’ll drop this if eslint complains.  Will still land the first commit.

	Pulsebot
	Comment 11 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d3eb0f3887b
Send remote-active after socket has ceased listening. r=whimboo

	Dorel Luca [:dluca]
	Comment 12 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7d3eb0f3887b