1340464 - Handle NetMonitorController connection state in util/client

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[[e-mail]]

As the first step to refactor NetMonitorController, separate netmonitor-controller.js to 
- netmonitor-controller.js which contains NetMonitorController
- netmonitor-target-events.js which contains TargetEventsHandler
- netmonitor-network-events.js which contains NetworkEventsHandler

Comment 1

[Ricky Chien [:rickychien] (inactive)]

We should take a look how debugger.html deal with its controller part. They create a uitls/client.js to handle `navigate` events [1] like we did in TargetEventsHandler. And further, they implement actions.willNavigate and actions.navigated as callback, so it makes sense for us to move _onTabNavigated out of TargetEventsHandler modules and use action instead.

Separate NetMonitorController into three modules might be useless if we're going to adopt this approach. Maybe we can implement one controller module to register all listeners and move these listener callbacks into actions.

Fred, Honza, how do you think?


[1] https://github.com/devtools-html/debugger.html/blob/0fb0d7cc56240851536874e17ef166ac21a56a11/src/utils/client.js#L16-L17

Comment 2

[[e-mail]]

Yes, it make sense to refer how debugger.html organize their controllers and reflect it back to netmonitor

Comment 3

[[e-mail]]

We can change the title once I figured out if there is a better way to organize netmonitor controller

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Also, agree. Supporting the same architecture as debugger.html makes total sense. Especially if we can share code/modules...

Honza

Comment 5

[[e-mail]]

learned from debugger.html panel.js

* wrap init process as bootstrap
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/debugger/new/panel.js#L24

Then we will have similar interface to reuse `devtools-launchpad`

* L10N as global variable
https://github.com/mozilla/gecko-dev/blob/master/devtools/client/debugger/new/panel.js#L14
Therefore we can remove L10N dependency from components

Comment 6

[[e-mail]]

* connection state is handled in util/client::onFirefoxConnect

https://github.com/devtools-html/debugger.html/blob/master/src/utils/client.js#L16

And debugger.html didn't handle disconnect (target.off) states. 

* activeConsole sham is not enough for netmonitor/webconsole
https://github.com/devtools-html/devtools-core/blob/master/packages/devtools-client-adapters/src/firefox/types.js#L229

We don't have to deal with it until we are moving to github

* gNetwork.getString is defined in NetMonitorController.NetworkEventsHandler

We can remove global `gNetwork` by define `getString` in NetMonitorController directly, then we can move NetworkEventsHandler out of NetMonitorController.

Comment 7

[[e-mail]]

Attachment: Bug 1340464 - PART 1: wrap init process into bootstrap function;

Review commit: https://reviewboard.mozilla.org/r/114434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114434/

Comment 8

[[e-mail]]

This is part 1 patch, the part 2 will convert TargetEventsHandler to util/client

And I plan to create some following bugs:

* L10N as global variable
  - dup webconsole related string into netmonitor strings, and remove webconsole strings dependency
  - turn L10N as global variable and remove utils/l10n


* Move NetworkEventsHandler to utils/network-events-handler
 - Move getString out of NetworkEventsHandler
 - Move NetworkEventsHandler to utils/network-events-handler and is called by utils/client


Still in concern:

* wrap activeConsole(webconsoleClient) related operations into a module
 - it might more make sense to use `activeConsole` instead of webconsoleClient)

Comment 9

[[e-mail]]

Attachment: Bug 1340464 - PART 2: handle connection state in util/client;

Review commit: https://reviewboard.mozilla.org/r/114862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114862/

Comment 10

[[e-mail]]

Comment on attachment 8839850 [details]
Bug 1340464 - PART 1: wrap init process into bootstrap function;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114434/diff/1-2/

Comment 11

[[e-mail]]

Comment on attachment 8840356 [details]
Bug 1340464 - PART 2: handle connection state in util/client;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114862/diff/1-2/

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8839850 [details]
Bug 1340464 - PART 1: wrap init process into bootstrap function;

https://reviewboard.mozilla.org/r/114434/#review116338

Looks good but, please resolve my comments/question.

R+ assuming try is green.

Honza

::: devtools/client/netmonitor/netmonitor.js:25
(Diff revision 2)
> +
> +    this.hook = document.querySelector(".root");
>  
>      render(Provider(
>        { store: window.gStore },
>        NetworkMonitor({ toolbox: window.NetMonitorController._toolbox }),

Why `toolbox` prop is passed into `NetworkMonitor` component?

Also, I am seeing typo in NetworkMonitor component file.

Line 15: MonitoPanel -> MonitorPanel
(and also line 26)

Please fix it as part of this patch

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8840356 [details]
Bug 1340464 - PART 2: handle connection state in util/client;

https://reviewboard.mozilla.org/r/114862/#review116358

One question before R+

Thanks,
Honza

::: devtools/client/netmonitor/netmonitor-controller.js:107
(Diff revision 2)
>          return undefined;
>        };
>        await connectTimeline();
>  
> -      this.TargetEventsHandler.connect();
> +      onFirefoxConnect(this._target);
> +      this._target.on("close", this._onTabDetached);

Shouldn't handling of the `close` event be part of utils/client module?
(just like it was part of the TargetEventsHandler)

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Fred Lin [:gasolin] from comment #8)
> This is part 1 patch, the part 2 will convert TargetEventsHandler to
> util/client
> 
> And I plan to create some following bugs:
> 
> * L10N as global variable
>   - dup webconsole related string into netmonitor strings, and remove
> webconsole strings dependency
>   - turn L10N as global variable and remove utils/l10n
Yep, I like that.

> 
> 
> * Move NetworkEventsHandler to utils/network-events-handler
>  - Move getString out of NetworkEventsHandler
>  - Move NetworkEventsHandler to utils/network-events-handler and is called
> by utils/client
Also agree


> Still in concern:
> 
> * wrap activeConsole(webconsoleClient) related operations into a module
>  - it might more make sense to use `activeConsole` instead of
> webconsoleClient)
Can you please provide more info for this? What's the outcome here?

Honza

Comment 15

[[e-mail]]

> > +      this._target.on("close", this._onTabDetached);
> 
> Shouldn't handling of the `close` event be part of utils/client module?
> (just like it was part of the TargetEventsHandler)

The onTabDetached called shutdown NetworkController function, which is defined in NetworkController. 
Define onClose into utils/client will create a cyclic require condition.

Since this is a first step to refactor NetworkController, we should more carefully add things into utils/client than keep things in NetworkController. I can see we add onClose into utils/client until NetworkController is refactored

Comment 16

[[e-mail]]

> 
> > Still in concern:
> > 
> > * wrap activeConsole(webconsoleClient) related operations into a module
> >  - it might more make sense to use `activeConsole` instead of
> > webconsoleClient)
> Can you please provide more info for this? What's the outcome here?
> 

That's not much value if its just a renaming. 
We have use gNetwork variable (NetworkEventHandler in NetworkController) to getString from activeconsole, I think we can re-purpose `gNetwork` to expose everything NetworkController exposed to netmonitor.
During that process we can re-evaluate if that function can be provided without global variable.

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8840356 [details]
Bug 1340464 - PART 2: handle connection state in util/client;

https://reviewboard.mozilla.org/r/114862/#review116728

Thanks for the explanation Fred!

R+ assuming try is green.

Honza

Comment 18

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8840356 [details]
Bug 1340464 - PART 2: handle connection state in util/client;

https://reviewboard.mozilla.org/r/114862/#review116934

::: devtools/client/netmonitor/netmonitor-controller.js:62
(Diff revision 2)
>        return this._shutdown;
>      }
>      this._shutdown = new Promise(async (resolve) => {
>        gStore.dispatch(Actions.batchReset());
> -      this.TargetEventsHandler.disconnect();
> +      onFirefoxDisconnect(this._target);
> +      this._target.on("close", this._onTabDetached);

listen _target.on again in shutdownNetMonitor is a mistake. It should be _target.off().

::: devtools/client/netmonitor/netmonitor-controller.js:107
(Diff revision 2)
>          return undefined;
>        };
>        await connectTimeline();
>  
> -      this.TargetEventsHandler.connect();
> +      onFirefoxConnect(this._target);
> +      this._target.on("close", this._onTabDetached);

nit: remove _onTabDetached and use this.shutdownNetMonitor directly.

this._target.on("close", this.shutdownNetMonitor);

::: devtools/client/netmonitor/utils/client.js:17
(Diff revision 2)
> + * Called for each location change in the monitored tab.
> + *
> + * @param {String} type Packet type.
> + * @param {Object} packet Packet received from the server.
> + */
> +function _onTabNavigated(type) {

Can we separate _onTabNavigated into two methods here?

It's unnecessary to use swtich case in this case. And also remove _ prefix.

We can make function name as simple as its event name like:

tabTarget.on("navigate", navigate);
tabTarget.on("will-navigate", willNavigate);

::: devtools/client/netmonitor/utils/client.js:21
(Diff revision 2)
> + */
> +function _onTabNavigated(type) {
> +  switch (type) {
> +    case "will-navigate": {
> +      // Reset UI.
> +      if (!Services.prefs.getBoolPref("devtools.webconsole.persistlog")) {

Do you think we can export these Services.prefs from utils/prefs.js?

As a result, we can merge all preference setting into prefs.js and let PrefsHelper access settings all at once.

This could be done in another patch and get rid of Services step by step while refactoring NetmonitorController.

Comment 19

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8839850 [details]
Bug 1340464 - PART 1: wrap init process into bootstrap function;

https://reviewboard.mozilla.org/r/114434/#review116936

::: devtools/client/netmonitor/netmonitor.js:21
(Diff revision 2)
>  
> -    this.networkMonitor = document.querySelector(".root");
> +    window.NetMonitorController = require("./netmonitor-controller").NetMonitorController;
> +    window.NetMonitorController._toolbox = toolbox;
> +    window.NetMonitorController._target = tabTarget;
> +
> +    this.hook = document.querySelector(".root");

nit: this.root to align with class name.

::: devtools/client/netmonitor/netmonitor.js:25
(Diff revision 2)
> +
> +    this.hook = document.querySelector(".root");
>  
>      render(Provider(
>        { store: window.gStore },
>        NetworkMonitor({ toolbox: window.NetMonitorController._toolbox }),

nit: toolbox prop is unused anymore, we can remove it.

Comment 20

[[e-mail]]

Comment on attachment 8840356 [details]
Bug 1340464 - PART 2: handle connection state in util/client;

https://reviewboard.mozilla.org/r/114862/#review116934

> listen _target.on again in shutdownNetMonitor is a mistake. It should be _target.off().

good catch!

> nit: remove _onTabDetached and use this.shutdownNetMonitor directly.
> 
> this._target.on("close", this.shutdownNetMonitor);

there are plenty of `this` reference in shutdown function, a binding is safer for this case

> Do you think we can export these Services.prefs from utils/prefs.js?
> 
> As a result, we can merge all preference setting into prefs.js and let PrefsHelper access settings all at once.
> 
> This could be done in another patch and get rid of Services step by step while refactoring NetmonitorController.

The pref here take separate domain (devtools.webconsole) from utils/prefs.js(devtools.netmonitor), and currently utils/prefs.js is not  for multiple PrefsHelper.

And there's no need to remove "services" require since there's already a shim for it `require("./client/shared/shim/Services");`

Comment 21

[[e-mail]]

Comment on attachment 8839850 [details]
Bug 1340464 - PART 1: wrap init process into bootstrap function;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114434/diff/2-3/

Comment 22

[[e-mail]]

Comment on attachment 8840356 [details]
Bug 1340464 - PART 2: handle connection state in util/client;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114862/diff/2-3/

Comment 23

[[e-mail]]

Comment on attachment 8839850 [details]
Bug 1340464 - PART 1: wrap init process into bootstrap function;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114434/diff/3-4/

Comment 24

[[e-mail]]

Comment on attachment 8840356 [details]
Bug 1340464 - PART 2: handle connection state in util/client;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114862/diff/3-4/

Comment 25

[Ricky Chien [:rickychien] (inactive)]

(In reply to Fred Lin [:gasolin] from comment #20)
> The pref here take separate domain (devtools.webconsole) from
> utils/prefs.js(devtools.netmonitor), and currently utils/prefs.js is not 
> for multiple PrefsHelper.
> 
> And there's no need to remove "services" require since there's already a
> shim for it `require("./client/shared/shim/Services");`

I mean we can get rid of Services if it's accessing prefs APIs, but not mean to remove all Services in codebase since there is a shim already existed.

IMO, prefs.js is created for managing all prefs relevant intents, so we should move all prefs relevant calls to prefs itself. 

For separate domain like devtools.webconsole prefs, it won't be a problem. The case is pretty much like export WEBCONSOLE_L10N in utils/l10n.js, we can do it as well to export a WebconsolePrefs for managing web console domain.

Comment 26

[[e-mail]]

Try green, thanks


> IMO, prefs.js is created for managing all prefs relevant intents, so we
> should move all prefs relevant calls to prefs itself. 
> 
> For separate domain like devtools.webconsole prefs, it won't be a problem.
> The case is pretty much like export WEBCONSOLE_L10N in utils/l10n.js, we can
> do it as well to export a WebconsolePrefs for managing web console domain.


Yeah, we can do so in follow up refactor patch

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44fc24be2928
PART 1: wrap init process into bootstrap function;r=Honza
https://hg.mozilla.org/integration/autoland/rev/9703c14de853
PART 2: handle connection state in util/client;r=Honza

Comment 28

[[e-mail]]

waitForAllRequestsFinished is only used in talo, we can remove it from NetworkController

http://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js#120

And some functions (such as supportsTransferredResponseSize) not used any where, we can safely remove them from NetworkController as well.

Comment 29

[Ricky Chien [:rickychien] (inactive)]

Things we can do for refactoring netmonitor-controller.js
* Remove isConnected()
* Remove unused getCurrentActivity()
* Remove unused supportsTransferredResponseSize()
* Remove supportsPerfStats()
* Move implementation of waitForAllRequestsFinished() out of controller to damp.js

Comment 30

[[e-mail]]

Thanks for find out redundant functions!

`supportsPerfStats` is used in request-list-context-menu.js but others are removable

Comment 31

[Ricky Chien [:rickychien] (inactive)]

supportsPerfStats() is safe to remove since we don't detect it in summary button but only in request-list-context-menu.js...

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/44fc24be2928
https://hg.mozilla.org/mozilla-central/rev/9703c14de853