Issue 2879 - Restructure existing process script, split it up into multiple modules

This is a preparation step, merely restructuring existing code.

[Wladimir Palant, 2016-11-17 08:42:58]

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/actor.js
File lib/child/actor.js (left):

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/actor.js#...
lib/child/actor.js:37: let nodes = new Map();
I copied the same file (chrome/content/processScript.js) to three new files -
each of them kept some pieces of it while removing all the others.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/bootstrap.js
File lib/child/bootstrap.js (right):

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/bootstrap...
lib/child/bootstrap.js:7: "use strict";
This is very close to lib/child/bootstrap.js we use in Adblock Plus. The
differences are:

* "use strict"
* messageManager is declared explicitly (we cannot just use `this` within a
function because with "use strict" it won't be defined).
* Function shutdown protects against reentry, won't do anything if we have
onShutdown.done.
* Not expecting the script URL as parameter for shutdown message - I don't
remember what issue this was working around in Adblock Plus but it was working
fine without in EHH.
* Shutdown message is called ElemHideHelper:Shutdown rather than
AdblockPlus:Shutdown.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/main.js
File lib/child/main.js (right):

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/main.js#n...
lib/child/main.js:10: require("./nodeInfo");
Note that I require this module explicitly here even though it is used by
./actor. At this moment it isn't purely a utility module, it listens to messages
as well. This will change with the next commit.

[saroyanm, 2016-11-22 14:58:32]

Looks good in general, just have couple of comments/questions

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/actor.js
File lib/child/actor.js (right):

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/actor.js#...
lib/child/actor.js:9: let console;
Why do we load console ? Are we outputing/planing to output smth there ?

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/actor.js#...
lib/child/actor.js:56: function Actor(connection, tabActor)
Detail: Why do we need this empty constructor ?
At least we are not using the parameters inside.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/actor.js#...
lib/child/actor.js:56: function Actor(connection, tabActor)
Detail: Maybe it's personal preference, but what about moving the constructor
and prototype on top of the the actor variable declaration for readability
reasons[I do not feel strong about suggestion].

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/actor.js#...
lib/child/actor.js:65: return getNodeInfo(nodeActor ? nodeActor.rawNode: null);
Detail: missing space after first expression.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/bootstrap.js
File lib/child/bootstrap.js (right):

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/bootstrap...
lib/child/bootstrap.js:7: "use strict";
On 2016/11/17 08:42:58, Wladimir Palant wrote:
> This is very close to lib/child/bootstrap.js we use in Adblock Plus. The
> differences are:
> 
> * "use strict"
> * messageManager is declared explicitly (we cannot just use `this` within a
> function because with "use strict" it won't be defined).
> * Function shutdown protects against reentry, won't do anything if we have
> onShutdown.done.
> * Not expecting the script URL as parameter for shutdown message - I don't
> remember what issue this was working around in Adblock Plus but it was working
> fine without in EHH.
> * Shutdown message is called ElemHideHelper:Shutdown rather than
> AdblockPlus:Shutdown.

Acknowledged.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/bootstrap...
lib/child/bootstrap.js:54: info, messageManager
In AdblockPlus we are defining the modules:
modules: {"info": info, "messageManager": this}

Should this be defined like:
modules: {"info": info, "messageManager": messageManager} ?

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/bootstrap...
lib/child/bootstrap.js:63: function shutdown(message)
message parameter is not being used, probably can be omitted.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/main.js
File lib/child/main.js (right):

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/main.js#n...
lib/child/main.js:10: require("./nodeInfo");
On 2016/11/17 08:42:58, Wladimir Palant wrote:
> Note that I require this module explicitly here even though it is used by
> ./actor. At this moment it isn't purely a utility module, it listens to
messages
> as well. This will change with the next commit.

Acknowledged.

[Wladimir Palant, 2016-11-24 14:16:29]

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/actor.js
File lib/child/actor.js (right):

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/actor.js#...
lib/child/actor.js:9: let console;
On 2016/11/22 14:58:31, saroyanm wrote:
> Why do we load console ? Are we outputing/planing to output smth there ?

We are not, this is a debugging helper. In fact, I am considering changing
module scopes so that we would generally have a console objects - would be more
consistent than using Cu.reportError().

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/actor.js#...
lib/child/actor.js:56: function Actor(connection, tabActor)
On 2016/11/22 14:58:31, saroyanm wrote:
> Detail: Why do we need this empty constructor ?
> At least we are not using the parameters inside.

Well, DebuggerServer.addTabActor() requires a constructor - it will create
objects are necessary. The parameters aren't used but they are being passed in
by the developer tools nevertheless.

On 2016/11/22 14:58:31, saroyanm wrote:
> Detail: Maybe it's personal preference, but what about moving the constructor
> and prototype on top of the the actor variable declaration for readability
> reasons[I do not feel strong about suggestion].

Done.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/actor.js#...
lib/child/actor.js:65: return getNodeInfo(nodeActor ? nodeActor.rawNode: null);
On 2016/11/22 14:58:31, saroyanm wrote:
> Detail: missing space after first expression.

Done.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/bootstrap.js
File lib/child/bootstrap.js (right):

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/bootstrap...
lib/child/bootstrap.js:54: info, messageManager
On 2016/11/22 14:58:31, saroyanm wrote:
> Should this be defined like:
> modules: {"info": info, "messageManager": messageManager} ?

Why? This short notation is available starting with Firefox 34 and we are
starting to use it. The code in Adblock Plus is simply older.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/bootstrap...
lib/child/bootstrap.js:63: function shutdown(message)
On 2016/11/22 14:58:31, saroyanm wrote:
> message parameter is not being used, probably can be omitted.

It sure can be but it is still being passed in so I'd rather not omit it.

[saroyanm, 2016-11-24 17:46:11]

Just some nits:
LGTM

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/actor.js
File lib/child/actor.js (right):

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/actor.js#...
lib/child/actor.js:9: let console;
On 2016/11/24 14:16:29, Wladimir Palant wrote:
> On 2016/11/22 14:58:31, saroyanm wrote:
> > Why do we load console ? Are we outputing/planing to output smth there ?
> 
> We are not, this is a debugging helper. In fact, I am considering changing
> module scopes so that we would generally have a console objects - would be
more
> consistent than using Cu.reportError().

Acknowledged.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/actor.js#...
lib/child/actor.js:56: function Actor(connection, tabActor)
On 2016/11/24 14:16:29, Wladimir Palant wrote:
> On 2016/11/22 14:58:31, saroyanm wrote:
> > Detail: Why do we need this empty constructor ?
> > At least we are not using the parameters inside.
> 
> Well, DebuggerServer.addTabActor() requires a constructor - it will create
> objects are necessary. The parameters aren't used but they are being passed in
> by the developer tools nevertheless.
Acknowledged.
> On 2016/11/22 14:58:31, saroyanm wrote:
> > Detail: Maybe it's personal preference, but what about moving the
constructor
> > and prototype on top of the the actor variable declaration for readability
> > reasons[I do not feel strong about suggestion].
> 
> Done.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/bootstrap.js
File lib/child/bootstrap.js (right):

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/bootstrap...
lib/child/bootstrap.js:16: let {Loader, main, unload} =
Cu.import("resource://gre/modules/commonjs/toolkit/loader.js", {});
Nit: Exceeding 80 chars.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/bootstrap...
lib/child/bootstrap.js:54: info, messageManager
On 2016/11/24 14:16:29, Wladimir Palant wrote:
> On 2016/11/22 14:58:31, saroyanm wrote:
> > Should this be defined like:
> > modules: {"info": info, "messageManager": messageManager} ?
> 
> Why? This short notation is available starting with Firefox 34 and we are
> starting to use it. The code in Adblock Plus is simply older.

Oh, I was referring actually to your initial comment regarding the differences
in bootstrap file. 
Acknowledged.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/nodeInfo.js
File lib/child/nodeInfo.js (right):

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/nodeInfo....
lib/child/nodeInfo.js:20:
messageManager.removeMessageListener("ElemHideHelper:GetNodeInfo",
onGetNodeInfo);
Nit: Exceeding 80 chars.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/nodeInfo....
lib/child/nodeInfo.js:30:
messageManager.sendAsyncMessage("ElemHideHelper:GetNodeInfo:Response",
nodeInfo);
Nit: Exceeding 80 chars.

[Wladimir Palant, 2016-12-01 09:40:49]

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/bootstrap.js
File lib/child/bootstrap.js (right):

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/bootstrap...
lib/child/bootstrap.js:16: let {Loader, main, unload} =
Cu.import("resource://gre/modules/commonjs/toolkit/loader.js", {});
On 2016/11/24 17:46:10, saroyanm wrote:
> Nit: Exceeding 80 chars.

Done.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/nodeInfo.js
File lib/child/nodeInfo.js (right):

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/nodeInfo....
lib/child/nodeInfo.js:20:
messageManager.removeMessageListener("ElemHideHelper:GetNodeInfo",
onGetNodeInfo);
On 2016/11/24 17:46:11, saroyanm wrote:
> Nit: Exceeding 80 chars.

Done.

https://codereview.adblockplus.org/29362609/diff/29362610/lib/child/nodeInfo....
lib/child/nodeInfo.js:30:
messageManager.sendAsyncMessage("ElemHideHelper:GetNodeInfo:Response",
nodeInfo);
On 2016/11/24 17:46:11, saroyanm wrote:
> Nit: Exceeding 80 chars.

Done.