Closed Bug 1352049 Opened 7 years ago Closed 7 years ago

Network panel documentation

Categories

(DevTools :: Netmonitor, enhancement, P2)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox55 fixed)

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

People

(Reporter: Honza, Assigned: gasolin)

Details

Attachments

(1 file)

Bug 1352049 - Network panel documentation;
59 bytes, text/x-review-board-request
Honza
: review+
rickychien
: review+
Details 
We should put together a documentation about new architecture of the Network panel. It should be added in m-c in the following directory:

m-c/devtools/docs/tools/network-panel.md

We can get some inspiration from existing docs for other panels so, the docs is consistent.

Honza
Priority: -- → P2
Here is the work-in-progress doc about new Netmonitor structure, welcome for any suggestion https://gist.github.com/gasolin/84a9dfd05b2c180e56ac18c10d9335ae
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
I actually hit netmonitor's standalone setup doc over here - https://github.com/mozilla/gecko-dev/tree/master/devtools/client/netmonitor 's README.md

If the above doc is the same, then I'll love some sort of a sym-link in `/devtools/client/netmonitor` as well
Ruturaj, comment 1 is a draft version of updated README for a quicker feedback. Will merge to README or somewhere after review process.
Comment on attachment 8856378 [details]
Bug 1352049 - Network panel documentation;

https://reviewboard.mozilla.org/r/128298/#review130820

Looks good! There are some comments please fix it.

::: devtools/client/netmonitor/README.md:3
(Diff revision 1)
> -# netmonitor
> +# Network Monitor
>  
> -The Network Monitor shows you all the network requests Firefox makes (for example, when it loads a page, or due to XMLHttpRequests), how long each request takes, and details of each request.
> +The Network Monitor(netmonitor) shows you all the network requests Firefox makes (for example, when it loads a page, or due to XMLHttpRequests), how long each request takes, and details of each request. Read [MDN](https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor) to learn more about how to use the Network Monitor.

nit: remove /en-US to make MDN redirect the language depends on user location.

::: devtools/client/netmonitor/README.md:16
(Diff revision 1)
> +Once `node`(`npm` included) is installed, use the following command to install `yarn`.
> +
> +```
> +$ npm install -g yarn

Good suggestion here. I'd prefer to provide the offical yarn docs https://yarnpkg.com/en/docs/install here instead of telling user to use npm install. Because there are a bunch of ways to install without using npm.

::: devtools/client/netmonitor/README.md:52
(Diff revision 1)
> +
> +Go to the Web Developer menu in Firefox and select [Developer Toolbar](https://developer.mozilla.org/en-US/docs/Tools/GCLI). Run the command
> +
> +`listen 6080 mozilla-rdp`
> +
> +The command will make your Firefox act as a remote debugging server. 

nit: remove tailing space

::: devtools/client/netmonitor/README.md:60
(Diff revision 1)
> +
> +`yarn start`
> +
> +### How it works
> +
> +Network Monitor use [webpack](https://webpack.js.org/) and several packages from [devtools-core](https://github.com/devtools-html/devtools-core) to run Network Monitor as a normal web page. Network Monitor use [Mozilla remote debugging protocol](http://searchfox.org/mozilla-central/source/devtools/docs/backend/protocol.md) to fetch result and execute commands from Firefox browser.

nit: uses

::: devtools/client/netmonitor/README.md:87
(Diff revision 1)
> +* `webpack.config.js` the webpack config file, including plenty of module alias map to shims and polifills
> +* `package.json` declare every required packages and available commands
> +
> +### UI
> +
> +Network Monitor's UI is constructed by [React](http://searchfox.org/mozilla-central/source/devtools/docs/frontend/react.md) components(in `src/components/`). 

nit: remove tailing space

::: devtools/client/netmonitor/README.md:95
(Diff revision 1)
> +* Three major container components are
> +  - **Toolbar** Panel related functions.
> +  - **RequestList** Show each request information.
> +  - **NetworkDetailsPanel** Show detailed infomation per request.
> +* `src/assets` Styles that affect Network Monitor panel.
> +  

nit: remove whitespace.
Attachment #8856378 - Flags: review?(rchien) → review+
The README should at least help QA able to run Network Monitor on a browser tab.

Current README patch only describe file structures in high level, we may need document a deeper version in `devtools/docs/tools/network-panel.md` to describe more details.
Comment on attachment 8856378 [details]
Bug 1352049 - Network panel documentation;

https://reviewboard.mozilla.org/r/128298/#review131012

The new docs are great, definitively helps someone new to the source code.

::: devtools/client/netmonitor/README.md:3
(Diff revision 2)
> -# netmonitor
> +# Network Monitor
>  
> -The Network Monitor shows you all the network requests Firefox makes (for example, when it loads a page, or due to XMLHttpRequests), how long each request takes, and details of each request.
> +The Network Monitor(netmonitor) shows you all the network requests Firefox makes (for example, when it loads a page, or due to XMLHttpRequests), how long each request takes, and details of each request. Read [MDN](https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor) to learn more about how to use the Network Monitor.

nit: space before "(netmonitor)"

What about: (for example, when a page is loaded or when an XMLHttpRequest is performed) ?


Also, in the last sentence, I would use "how to use the tool." to avoid repetition.

::: devtools/client/netmonitor/README.md:7
(Diff revision 2)
>  
> -The Network Monitor shows you all the network requests Firefox makes (for example, when it loads a page, or due to XMLHttpRequests), how long each request takes, and details of each request.
> +The Network Monitor(netmonitor) shows you all the network requests Firefox makes (for example, when it loads a page, or due to XMLHttpRequests), how long each request takes, and details of each request. Read [MDN](https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor) to learn more about how to use the Network Monitor.
>  
>  ## Prerequisite
>  
> +If you want to build Network Monitor inside of the Firefox Devtools Panel, follow [simple Firefox build](https://developer.mozilla.org/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build) document in MDN. Start your compiled firefox and open the Firefox developer tool, you can see Network Monitor inside.

nit: the Network monitor

Also, we commonly use "toolbox" instead of "Firefox DevTools panel".

nit: the [simple Firefox build]

::: devtools/client/netmonitor/README.md:9
(Diff revision 2)
>  
>  ## Prerequisite
>  
> +If you want to build Network Monitor inside of the Firefox Devtools Panel, follow [simple Firefox build](https://developer.mozilla.org/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build) document in MDN. Start your compiled firefox and open the Firefox developer tool, you can see Network Monitor inside.
> +
> +If you want to run Network Monitor in the browser tab (experimental), you need following packages:

How about "If you would like to run" to avoid repetition?

::: devtools/client/netmonitor/README.md:16
(Diff revision 2)
>  * [node](https://nodejs.org/) >= 6.9.x
>  * [npm](https://docs.npmjs.com/getting-started/installing-node) >= 3.x
>  * [yarn](https://yarnpkg.com/docs/install) >= 0.21.x
> +* [Firefox](https://www.mozilla.org/firefox/new/) either the released version or build from the source code.
> +
> +Once `node`(`npm` included) is installed, use the following command to install `yarn`.

nit: space before (

::: devtools/client/netmonitor/README.md:24
(Diff revision 2)
> +$ npm install -g yarn
> +```
>  
>  ## Quick Setup
>  
> -Assume you are in netmonitor folder
> +Assume you are in `netmonitor` folder. Run following commands:

It would probably be more clear like this:

"Navigate in the `netmonitor` folder with your terminal, then run the following commands:"

::: devtools/client/netmonitor/README.md:41
(Diff revision 2)
>  
> -Then open localhost:8000 in any browser to see all tabs in Firefox.
> +Then open `localhost:8000` in any browser to see all tabs in Firefox.
> +
> +### More detailed setup
> +
> +If you have a opened Firefox browser, you can manually config Firefox as well.

nit: an opened

nit: you can manually configure

::: devtools/client/netmonitor/README.md:43
(Diff revision 2)
> +
> +### More detailed setup
> +
> +If you have a opened Firefox browser, you can manually config Firefox as well.
> +
> +Type `about:config` in Firefox URL field, grant the warning to access preferences. We need to config 2 preferences:

We need to set two preferences:

::: devtools/client/netmonitor/README.md:45
(Diff revision 2)
> +
> +If you have a opened Firefox browser, you can manually config Firefox as well.
> +
> +Type `about:config` in Firefox URL field, grant the warning to access preferences. We need to config 2 preferences:
> +
> +* disable `devtools.debugger.prompt-connection` preference to enable connection without prompt.

nit: disable `...` to remove the connection prompt

::: devtools/client/netmonitor/README.md:46
(Diff revision 2)
> +If you have a opened Firefox browser, you can manually config Firefox as well.
> +
> +Type `about:config` in Firefox URL field, grant the warning to access preferences. We need to config 2 preferences:
> +
> +* disable `devtools.debugger.prompt-connection` preference to enable connection without prompt.
> +* enable `devtools.debugger.remote-enabled` preference to allow running mozilla remote debugging protocol(rdp)

nit: space before (rdp)

Also, the wording could be improved:

enable `...` to allow remotely debugging a browser tab via the Firefox remote debugging protocol (RDP)

::: devtools/client/netmonitor/README.md:60
(Diff revision 2)
> +
> +`yarn start`
> +
> +### How it works
> +
> +Network Monitor uses [webpack](https://webpack.js.org/) and several packages from [devtools-core](https://github.com/devtools-html/devtools-core) to run Network Monitor as a normal web page. Network Monitor use [Mozilla remote debugging protocol](http://searchfox.org/mozilla-central/source/devtools/docs/backend/protocol.md) to fetch result and execute commands from Firefox browser.

nit: uses (before [mozilla remote debugging protocol])

nit: The network monitor

::: devtools/client/netmonitor/README.md:62
(Diff revision 2)
> +
> +### How it works
> +
> +Network Monitor uses [webpack](https://webpack.js.org/) and several packages from [devtools-core](https://github.com/devtools-html/devtools-core) to run Network Monitor as a normal web page. Network Monitor use [Mozilla remote debugging protocol](http://searchfox.org/mozilla-central/source/devtools/docs/backend/protocol.md) to fetch result and execute commands from Firefox browser.
> +
> +Open `localhost:8000` in any browser to see the [launchpad](https://github.com/devtools-html/devtools-core/tree/master/packages/devtools-launchpad) interface. Devtools Launchpad will communicate with your Firefox(the remote debugging server) and list all opened tabs from your Firefox browser. Click one of the browser tab entry, now you can see Network Monitor runs in a browser tab.

"your Firefox" and "your Firefox browser" seem heavy, how about just "Firefox" ?

nit: space before "(the remote debugging server)"

::: devtools/client/netmonitor/README.md:66
(Diff revision 2)
> +
> +Open `localhost:8000` in any browser to see the [launchpad](https://github.com/devtools-html/devtools-core/tree/master/packages/devtools-launchpad) interface. Devtools Launchpad will communicate with your Firefox(the remote debugging server) and list all opened tabs from your Firefox browser. Click one of the browser tab entry, now you can see Network Monitor runs in a browser tab.
> +
> +## Code Structure
> +
> +Files located on the top level of code base are used to launch Network Monitor inside of the Firefox Devtools Panel or run in the browser tab(experimental). Network Monitor source is mainly located in `src/` folder, the same code base is used to run in both environments.

nit: "Top level files are used to launch [...]" instead of "Files located on the top level [...]"

nit: "the Network Monitor"

nit: space before (experimental)

nit: "the `src` folder"

::: devtools/client/netmonitor/README.md:68
(Diff revision 2)
> +
> +## Code Structure
> +
> +Files located on the top level of code base are used to launch Network Monitor inside of the Firefox Devtools Panel or run in the browser tab(experimental). Network Monitor source is mainly located in `src/` folder, the same code base is used to run in both environments.
> +
> +### Run inside of the Firefox Devtools Panel

nit: Run inside the DevTools toolbox

::: devtools/client/netmonitor/README.md:70
(Diff revision 2)
> +
> +Files located on the top level of code base are used to launch Network Monitor inside of the Firefox Devtools Panel or run in the browser tab(experimental). Network Monitor source is mainly located in `src/` folder, the same code base is used to run in both environments.
> +
> +### Run inside of the Firefox Devtools Panel
> +
> +Files use to run Network Monitor inside of the Firefox Devtools panel.

nit: Files used

nit: we use toolbox instead of Firefox Devtools panel

::: devtools/client/netmonitor/README.md:77
(Diff revision 2)
> +* `panel.js` called by devtools toolbox to launch the Network Monitor panel
> +* `index.html` panel UI and launch scripts
> +
> +### Run in the browser tab (experimental)
> +
> +Files use to run Network Monitor in the browser tab

nit: Files used

::: devtools/client/netmonitor/README.md:82
(Diff revision 2)
> +Files use to run Network Monitor in the browser tab
> +
> +* `bin/` files to launch test server
> +* `configs/` dev configs
> +* `index.js` the entry point, equivalent to `index.html`
> +* `webpack.config.js` the webpack config file, including plenty of module alias map to shims and polifills

nit: polyfills

::: devtools/client/netmonitor/README.md:87
(Diff revision 2)
> +* `webpack.config.js` the webpack config file, including plenty of module alias map to shims and polifills
> +* `package.json` declare every required packages and available commands
> +
> +### UI
> +
> +Network Monitor's UI is constructed by [React](http://searchfox.org/mozilla-central/source/devtools/docs/frontend/react.md) components(in `src/components/`).

nit: "The Network Monitor UI" (the network monitor is not a person, so we don't use 's)

How about "is built using" instead of "is constructed  by" ?

nit: space before (in `src...

::: devtools/client/netmonitor/README.md:96
(Diff revision 2)
> +  - **Toolbar** Panel related functions.
> +  - **RequestList** Show each request information.
> +  - **NetworkDetailsPanel** Show detailed infomation per request.
> +* `src/assets` Styles that affect Network Monitor panel.
> +
> +We prefer stateless component(define by function) instead of stateful component(define by class) unless the component has to maintain its internal state.

nit: stateless components (define by ...)

nit: stateful components (define by ...)

::: devtools/client/netmonitor/README.md:100
(Diff revision 2)
> +
> +We prefer stateless component(define by function) instead of stateful component(define by class) unless the component has to maintain its internal state.
> +
> +### State
> +
> +Besides the UI, Network Monitor manage states via [Redux](http://searchfox.org/mozilla-central/source/devtools/docs/frontend/redux.md). Here are places to check when you want to change state.

nit: manages the app state


Also: "Here are places to check when you want to change state." may be misleading because sometimes you want to add a button that changes the app state using an action that's already defined, in this case you don't necessarily touch those 4 locations.

So, maybe "The following locations define the app state:" might be more clear ?

::: devtools/client/netmonitor/README.md:102
(Diff revision 2)
> +
> +### State
> +
> +Besides the UI, Network Monitor manage states via [Redux](http://searchfox.org/mozilla-central/source/devtools/docs/frontend/redux.md). Here are places to check when you want to change state.
> +
> +* `src/constants.js` site wide constants includes action and event name.

global constants used across the tool including action and event names ?

::: devtools/client/netmonitor/README.md:105
(Diff revision 2)
> +Besides the UI, Network Monitor manage states via [Redux](http://searchfox.org/mozilla-central/source/devtools/docs/frontend/redux.md). Here are places to check when you want to change state.
> +
> +* `src/constants.js` site wide constants includes action and event name.
> +* `src/actions/` for all actions that change the state.
> +* `src/reducers/` for all reducers that change the state.
> +* `src/selectors/` all selectors to help select a network request.

That's actually misleading in my opinion, there are selectors that don't necessarily select a network request.

I previously did not know what this directory was about. But it turns out, selectors are just functions that return a formatted version of parts of the app state. For example, we have a selector that returns all filter types as an array. I think that might actually be a good definition.

::: devtools/client/netmonitor/README.md:107
(Diff revision 2)
> +* `src/constants.js` site wide constants includes action and event name.
> +* `src/actions/` for all actions that change the state.
> +* `src/reducers/` for all reducers that change the state.
> +* `src/selectors/` all selectors to help select a network request.
> +
> +We use [immutable.js](https://github.com/facebook/immutable-js) and [reselect](https://github.com/reactjs/reselect) libraries to return new state object efficiently.

nit: a new state object

Btw, I would add a small sentence linking to the immutable docs, because you need to get your head around some immutable bottlenecks (some nice things to know: records are immutable, you can't have a field named `size` on a record, ...) if you work on the app state.
> Navigate in the `netmonitor` folder with your terminal

Whoops, "to the" not "in the"
> (some nice things to know: records are immutable

meant iterable, not immutable (they're immutable duh)
Comment on attachment 8856378 [details]
Bug 1352049 - Network panel documentation;

https://reviewboard.mozilla.org/r/128298/#review131012

> That's actually misleading in my opinion, there are selectors that don't necessarily select a network request.
> 
> I previously did not know what this directory was about. But it turns out, selectors are just functions that return a formatted version of parts of the app state. For example, we have a selector that returns all filter types as an array. I think that might actually be a good definition.

Yeah it's more clear and I forgot the reducer/ also includes some request filter functions

> nit: a new state object
> 
> Btw, I would add a small sentence linking to the immutable docs, because you need to get your head around some immutable bottlenecks (some nice things to know: records are immutable, you can't have a field named `size` on a record, ...) if you work on the app state.

I've change the link to immutable doc, but I'd not add the particular sentance now since it's might be worth to put into `docs/frontend/tools/network-panel`.
Add a new section to talk about how to develop with related modules (devtools-core modules and devtools-reps).

Add an issue for devtools-reps (merge reps into devtools-core/packages)
https://github.com/devtools-html/reps/issues/124
Comment on attachment 8856378 [details]
Bug 1352049 - Network panel documentation;

https://reviewboard.mozilla.org/r/128298/#review131374

Looks great to me!

Please create yet another netmonitor-panel.md file in devtools/docs/tools/ directory and put there a note about the README.md file.

Something like:
You can find the Netmonitor documentation in `devtools/client/netmonitor/README.md` file.

(can be separate patch attached to this bug)

Honza
Attachment #8856378 - Flags: review?(odvarko) → review+
create network-panel.md in devtools/docs/tools/ with follow sentence:

```
You can find the Network Monitor documentation on `devtools/client/netmonitor/README.md` file.
```

* use `yarn link` instead of `npm link` in doc
* add `src/utils/firefox/` explaination under `Run inside of the DevTools toolbox` section

Since we can change document without regression, I'll just land it and welcome for any followup modification.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/02fa8fa9a9a0
Network panel documentation;r=Honza,rickychien
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/02fa8fa9a9a0
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: