Closed Bug 1344160 Opened 7 years ago Closed 7 years ago

Refactor netmonitor-controller.js

Categories

(DevTools :: Netmonitor, enhancement, P1)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: rickychien, Assigned: rickychien)

References

(Depends on 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(2 files)

Bug 1344160 - Refactor netmonitor-controller.js
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1344160 - Fix mochitest failures
59 bytes, text/x-review-board-request
Honza
: review+
Details 
Tasks should be done in netmonitor-controller.js refactoring:

* Rename to NetworkEventsHandler
* Create constructor parameters for Controller (toolbox, _target) in order to get rid of properties injection in netmonitor.js
* Remove this._target
* Remove this.toolbox
* Remove window.gStore but pass to controller instance directly.
* Remove get client(), get webConsoleClient(), get timelineFront()
* Move these bootstrap functions such as startupNetMonitor, connect, shutdownNetMonitor and disconnect to netmonitor.js
Flags: qe-verify?
Flags: qe-verify? → qe-verify-
Priority: -- → P3
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
Blocks: netmonitor-phaseII
No longer blocks: netmonitor-html
Priority: P3 → P2
Whiteboard: [netmonitor-reserve] → [netmonitor]
Depends on: 1356957
Depends on: 1358054
Depends on: 1358715
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Priority: P2 → P1
The concept of netmonitor-controller refactoring is derived from https://github.com/rickychien/netmonitor.html experiment. This is a huge refactor for entire controller. So please review it kindly.

Some breaking changes are:

* Moved netmonitor-controller to connector/ folder
* Create firefox-connector to take over the responsibility of netmonitor-controller
* Moved utils/client.js APIs into connector
* Create connector/index.js for public connector interface APIs. It's designed for adapting Firefox debugging protocol and Chrome debugging protocol.

In order to support Chrome debugging protocol, we can create a connector/chrome-connector.js and implement interface which are defined in connector folder in the future.
Comment on attachment 8862713 [details]
Bug 1344160 - Refactor netmonitor-controller.js

https://reviewboard.mozilla.org/r/134572/#review137594

This concept of "connector" seems wrong to me. Instead of having one connector for each browser, it would be better to support an adapter between chrome's protocol and Firefox's protocol, so you don't have to write 2 different connectors in the client.

This is the approach that has been taken for Valence in the other tools, I think that would be a better way to go.
Hey ntim,

Could you provide the example how Valence implements the adapter between Chrome's protocol and Firefox's protocol? Or at least describe your idea of how the adapter should look like?

No matter what ultimate architecture of adapter/connector would look like. My idea here is simple and merely want to propose a concept which allows us to have connector / adapter implementation to connect CDP or FDP.

Feel free to add suggestions. This refactoring will also affect the Netmonitor Outreachy program [1] starting at the end of May. I'd like to help our future intern work on it easily and let he can focus on implementing Chrome debugger protocol (CDP).


[1] https://wiki.mozilla.org/Outreachy#Improve_DevTools_Network_Monitor
Flags: needinfo?(jryans)
Valence works by creating separate actors that implement the Firefox RDP so that our tools can connect, but behind the scenes it talks to the target browser over the Chrome RDP, so it functions as a two-way translation layer converting packets between Firefox and Chrome RDP.

My opinion is that the Valence approach hasn't been very successful for several reasons, including:

1. It forces you to be an expert in both RDPs simultaneously so can convert correctly between both, which typically means we keep getting things wrong (since most people aren't experts in both)
2. There are typically edge cases in each protocol that can be hard to represent properly when you're forced to create an actor hierarchy from one protocol that doesn't match the other

As Jim has said several times, a translation layer like Valence sounds "simple" and great at the start, but there's quite a lot of places for an impedance mismatch that makes things hard.

As I understand it, the new debugger has separate client paths for Firefox and Chrome RDP and converts them both to a unified data structure it wants to use.  This sounds similar to what Ricky is proposing here for network monitor.  It's also beneficial in case we decide to convert more of the Firefox RDP server to the Chrome RDP, since the tools can still function without a packet translation layer in the middle.

So, I think Ricky's approach sounds reasonable to me.  :jlast might have more to say about how this has worked out for the debugger so far.
Flags: needinfo?(jryans) → needinfo?(jlaster)
Oh, forgot to add, here's a link to a Valence actor example:

https://github.com/mozilla/valence/blob/master/lib/chromium/walker.js

to get an idea of how that worked.
Iteration: 55.4 - May 1 → ---
The connector logic looks sound. It is similar to what we are doing in the debugger, with a common interface for separate browser clients (firefox, chrome).

I've added links below for how we use our client in the redux actions. Ultimately, it is relatively easy to isolate the client API from the internals of the redux/react app so the two are interchangeable. 

https://github.com/devtools-html/debugger.html/blob/master/src/client/firefox/commands.js#L56-L60
https://github.com/devtools-html/debugger.html/blob/master/src/actions/breakpoints.js#L72-L76
Flags: needinfo?(jlaster)
Comment on attachment 8862713 [details]
Bug 1344160 - Refactor netmonitor-controller.js

https://reviewboard.mozilla.org/r/134572/#review138906

::: devtools/client/netmonitor/src/connector/firefox-connector.js:398
(Diff revision 2)
> +    if (type === ACTIVITY_TYPE.RELOAD.WITH_CACHE_DEFAULT) {
> +      return reconfigureTabAndWaitForNavigation({}).then(standBy);
> +    }
> +    if (type === ACTIVITY_TYPE.RELOAD.WITH_CACHE_ENABLED) {
> +      this.currentActivity = ACTIVITY_TYPE.ENABLE_CACHE;
> +      this.tabTarget.once("will-navigate", () => {
> +        this.currentActivity = type;
> +      });
> +      return reconfigureTabAndWaitForNavigation({
> +        cacheDisabled: false,
> +        performReload: true
> +      }).then(standBy);
> +    }
> +    if (type === ACTIVITY_TYPE.RELOAD.WITH_CACHE_DISABLED) {
> +      this.currentActivity = ACTIVITY_TYPE.DISABLE_CACHE;
> +      this.tabTarget.once("will-navigate", () => {
> +        this.currentActivity = type;
> +      });
> +      return reconfigureTabAndWaitForNavigation({
> +        cacheDisabled: true,
> +        performReload: true
> +      }).then(standBy);
> +    }
> +    if (type === ACTIVITY_TYPE.ENABLE_CACHE) {
> +      this.currentActivity = type;
> +      return reconfigureTab({
> +        cacheDisabled: false,
> +        performReload: false
> +      }).then(standBy);
> +    }
> +    if (type === ACTIVITY_TYPE.DISABLE_CACHE) {
> +      this.currentActivity = type;
> +      return reconfigureTab({
> +        cacheDisabled: true,
> +        performReload: false
> +      }).then(standBy);
> +    }

This could probably be a switch statement
Comment on attachment 8862713 [details]
Bug 1344160 - Refactor netmonitor-controller.js

https://reviewboard.mozilla.org/r/134572/#review139126

I really like the patch!

Please see my inline comments

Thanks Ricky,
Honza

::: devtools/client/netmonitor/index.html:38
(Diff revision 2)
>        // Inject EventEmitter into global window.
>        EventEmitter.decorate(window);
> +      // Inject to global window for testing
> +      window.actions = actions;
> +      window.store = store;
>  

Is the plan to get rid of those globals too or tests are an exception?

::: devtools/client/netmonitor/index.html:54
(Diff revision 2)
> +          render(Provider({ store }, App()), this.mount);
> +          return onFirefoxConnect(connection, actions, store.getState);
>          },
>  
>          destroy() {
>            unmountComponentAtNode(this.mount);

Is `destroy` called when the Net monitor is loaded in Launchpad?

::: devtools/client/netmonitor/index.js:42
(Diff revision 2)
>  pref("devtools.netmonitor.har.pageLoadedTimeout", 1500);
>  pref("devtools.netmonitor.har.enableAutoExportToFile", false);
>  pref("devtools.webconsole.persistlog", false);
>  
>  const App = require("./src/components/app");
> -const store = window.gStore = configureStore();
> +const store = window.store = configureStore();

Is this `window.store` global for tests? If yes a comment would be useful.

::: devtools/client/netmonitor/src/connector/firefox-connector.js:398
(Diff revision 2)
> +    if (type === ACTIVITY_TYPE.RELOAD.WITH_CACHE_DEFAULT) {
> +      return reconfigureTabAndWaitForNavigation({}).then(standBy);
> +    }
> +    if (type === ACTIVITY_TYPE.RELOAD.WITH_CACHE_ENABLED) {
> +      this.currentActivity = ACTIVITY_TYPE.ENABLE_CACHE;
> +      this.tabTarget.once("will-navigate", () => {
> +        this.currentActivity = type;
> +      });
> +      return reconfigureTabAndWaitForNavigation({
> +        cacheDisabled: false,
> +        performReload: true
> +      }).then(standBy);
> +    }
> +    if (type === ACTIVITY_TYPE.RELOAD.WITH_CACHE_DISABLED) {
> +      this.currentActivity = ACTIVITY_TYPE.DISABLE_CACHE;
> +      this.tabTarget.once("will-navigate", () => {
> +        this.currentActivity = type;
> +      });
> +      return reconfigureTabAndWaitForNavigation({
> +        cacheDisabled: true,
> +        performReload: true
> +      }).then(standBy);
> +    }
> +    if (type === ACTIVITY_TYPE.ENABLE_CACHE) {
> +      this.currentActivity = type;
> +      return reconfigureTab({
> +        cacheDisabled: false,
> +        performReload: false
> +      }).then(standBy);
> +    }
> +    if (type === ACTIVITY_TYPE.DISABLE_CACHE) {
> +      this.currentActivity = type;
> +      return reconfigureTab({
> +        cacheDisabled: true,
> +        performReload: false
> +      }).then(standBy);
> +    }

Yep, I like the idea of using `switch` here.

::: devtools/client/netmonitor/src/request-list-header-context-menu.js:24
(Diff revision 2)
>    }
>  
>    get columns() {
> -    return window.gStore.getState().ui.columns;
> +    // FIXME: Convert request-list-header-context-menu as a react component and
> +    // access getSelectedRequest through react-redux connect() method
> +    return window.store.getState().ui.columns;

Should we have bug filed for this?
Attachment #8862713 - Flags: review?(odvarko)
Comment on attachment 8862714 [details]
Bug 1344160 - Fix mochitest failures

https://reviewboard.mozilla.org/r/134574/#review139140

Looks reasonable!

R+ assuming try is green

Honza
Attachment #8862714 - Flags: review?(odvarko) → review+
One more thing, rebase is needed.

patching file devtools/client/webconsole/new-console-output/new-console-output-wrapper.js
Hunk #1 FAILED at 82
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/webconsole/new-console-output/new-console-output-wrapper.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh net-controller1


patching file devtools/client/netmonitor/test/browser_net_filter-flags.js
Hunk #2 FAILED at 180
1 out of 3 hunks FAILED -- saving rejects to file devtools/client/netmonitor/test/browser_net_filter-flags.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh net-controller2
Blocks: 1362054
No longer blocks: 1362054
Comment on attachment 8862713 [details]
Bug 1344160 - Refactor netmonitor-controller.js

https://reviewboard.mozilla.org/r/134572/#review139126

> Should we have bug filed for this?

Comment updated.
Comment on attachment 8862713 [details]
Bug 1344160 - Refactor netmonitor-controller.js

https://reviewboard.mozilla.org/r/134572/#review139156

::: devtools/client/netmonitor/index.html:38
(Diff revision 2)
>        // Inject EventEmitter into global window.
>        EventEmitter.decorate(window);
> +      // Inject to global window for testing
> +      window.actions = actions;
> +      window.store = store;
>  

There are still too many mochitest accessing store and dispatch actions directly, it's hard to get rid of them completely. But window.actions it's possible to be removed (and patch has updated!).

Now request-list-context-menu and request-list-header-context-menu are using window.gStore for accessing latest state from store as well. They also are removable after rewritting both of them as react component and then hook them to react-redux's connect().

Afterward, we only need to export window.store to global for testing but it's no longer to export window.actions anymore.

::: devtools/client/netmonitor/index.html:54
(Diff revision 2)
> +          render(Provider({ store }, App()), this.mount);
> +          return onFirefoxConnect(connection, actions, store.getState);
>          },
>  
>          destroy() {
>            unmountComponentAtNode(this.mount);

I think the answer is no. Only way to shutdown netmonitor in launchpad is through closing the tab. I haven't seen debugger [1] setting its destroy method for launchpad as well.

Once launchpad support destroy method like panel.js (probably support multiple devtools panels), we can add it back.

[1] https://github.com/devtools-html/debugger.html/blob/master/src/main.js#L44-L54
Comment on attachment 8862713 [details]
Bug 1344160 - Refactor netmonitor-controller.js

https://reviewboard.mozilla.org/r/134572/#review139506

LGTM, r+

Thanks Ricky!

Honza
Attachment #8862713 - Flags: review?(odvarko) → review+
https://hg.mozilla.org/mozilla-central/rev/f3f5430112ec
https://hg.mozilla.org/mozilla-central/rev/a9339d769d2b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: