Closed Bug 1350215 Opened 7 years ago Closed 7 years ago

Move MC code into src/ and left panel.js, index.xhtml in top level

Categories

(DevTools :: Netmonitor, enhancement, P1)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Iteration:
55.2 - Apr 3
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: gasolin, Assigned: rickychien)

References

(Depends on 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(7 files)

Bug 1350215 - Merge shared/components to components
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1350215 - Move store.js into utils/store.js
59 bytes, text/x-review-board-request
Honza
: review+
gasolin
: review+
Details
Bug 1350215 - Move top level source files to src folder
59 bytes, text/x-review-board-request
gasolin
: review+
Honza
: review+
Details
Bug 1350215 - Merge toolbox launcher in index.html
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1350215 - Fix mochitest failures
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1350215 - Move netmonitor.css to netmonitor folder
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1350215 - Rename components from camelcase to hyphens
59 bytes, text/x-review-board-request
Honza
: review+
Details 
We plan to run netmonitor on both toolbar and browser tab.

The New directory structure would looks like

* devtools/client/netmonitor (Required for running on the toolbox)
- panel.js
- index.xhtml
- netmonitor.js
- src/
- README.md

* devtools/client/netmonitor (Required for running on the launchpad)
- package.json
- webpack.config.js
- index.js or main.js
src/

We'll focus on the first part in this bug. (refactor current code structure to adapt future changes)

Some related change may apply:

* move store.js into utils/store.js
* move shared/components into components
Priority: -- → P1
Whiteboard: [netmonitor]
Blocks: 1350217
Priority: P1 → P2
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
After finishing my netmonitor.html experiment for firefox panel, I'm confident and got clear picture in my mind about how to restructure netmonitor folder. Most of my experience come from my experiment repo, we can leverage it and provide a better environment for porting firefox panel and launchpad.

Fred, would you mind if I take over this bug?

I'd like to do more cleanup in this kick off patch. In order to speed up progress and avoid blocking other dependencies, I'll separate this task into several patches for review. Thanks!
Flags: needinfo?(gasolin)
sure, thanks
Assignee: gasolin → rchien
Flags: needinfo?(gasolin)
Comment on attachment 8851456 [details]
Bug 1350215 - Move store.js into utils/store.js

https://reviewboard.mozilla.org/r/123750/#review126154

loos good
Attachment #8851456 - Flags: review?(gasolin) → review+
Flags: qe-verify?
Priority: P2 → P1
Iteration: --- → 55.2 - Apr 3
Comment on attachment 8851458 [details]
Bug 1350215 - Merge toolbox launcher in index.html

https://reviewboard.mozilla.org/r/123754/#review126570

I don't think we need any change to merge the netmonitor.js into index.html, we can keep it as it is.
Attachment #8851458 - Flags: review?(gasolin) → review-
I saw several test fail during move netmonitor/ to netmonitor/src/ . 
Refer to inspector's repo, they didn't move the source to src but also able to run on browser tab through webpack.config. 
Maybe we could take similar approach so we can go faster without solving the test failure or the rebase hell for other working on bugs. We can polish the file structure later when we think its a good time to do so.

Ricky, how do you think?
Flags: needinfo?(rchien)
Fred,

Do you think that is there any good benefit to keep netmonitor.js as a separate file? You can leave you opinions or talk to me directly (We can speed up our progress and land phase II quickly).

IMO, merging all firefox-panel stuff into one place netmonitor/index.html can get a clear entry structure, and which also tells you there are merely two additional files panels.js and index.html in charge of connecting with firefox-panel (toolbox). SO everything in src is our core source code which is responsible for netmonitor business model.
Flags: needinfo?(rchien)
Per offline discussion we agree to keep netmonitor.js as it is now and maybe change the name to distinguish it from the other to bootstrap launchpad. (ex: index_firefox.js & index_browser.js)

& Since try is green, we can land the rest if no big issues there.
Comment on attachment 8851455 [details]
Bug 1350215 - Merge shared/components to components

https://reviewboard.mozilla.org/r/123748/#review126616

::: devtools/client/netmonitor/components/App.js:15
(Diff revision 3)
> +  PropTypes,
> +} = require("devtools/client/shared/vendor/react");
> +const { connect } = require("devtools/client/shared/vendor/react-redux");
> +
> +// Components
> +const MonitorPanel = createFactory(require("./MonitorPanel"));

Though CamelCase is common for react component convention, I think we should still keep the same naming convention before we move out of mozilla-central.

::: devtools/client/netmonitor/components/HeadersPanel.js:19
(Diff revision 3)
> +const { L10N } = require("../utils/l10n");
> +const {
> +  getHeadersURL,
> +  getHTTPStatusCodeURL,
> +} = require("../utils/mdn-utils");
> +const { writeHeaderText } = require("../utils/request-utils");

nice to see requires in order
Attachment #8851455 - Flags: review-
(In reply to Fred Lin [:gasolin] from comment #33)
> Per offline discussion we agree to keep netmonitor.js as it is now and maybe
> change the name to distinguish it from the other to bootstrap launchpad.
> (ex: index_firefox.js & index_browser.js)
> 
> & Since try is green, we can land the rest if no big issues there.

Yes.

Correction: In the future we can have two entry points in netmonitor folder

netmonitor/
  firefox-panel.html  ->  for launching netmonitor in firefox devtools
  launchpad-panel.js  ->  for running netmonitor through devtools-launchpad, webpack's entry target as well.
Comment on attachment 8851455 [details]
Bug 1350215 - Merge shared/components to components

https://reviewboard.mozilla.org/r/123748/#review126616

> Though CamelCase is common for react component convention, I think we should still keep the same naming convention before we move out of mozilla-central.

I'd prefer to drop this issue unless there exists file name convention tell us using xxx-xxx instead of CamelCase. CamelCase file name is used widely by all over web / frontend community and it's also popular in almost every developers who know React. This can be a good refinement for our code quality for growing web community.
Comment on attachment 8851455 [details]
Bug 1350215 - Merge shared/components to components

https://reviewboard.mozilla.org/r/123748/#review126654

::: devtools/client/netmonitor/components/App.js:15
(Diff revision 3)
> +  PropTypes,
> +} = require("devtools/client/shared/vendor/react");
> +const { connect } = require("devtools/client/shared/vendor/react-redux");
> +
> +// Components
> +const MonitorPanel = createFactory(require("./MonitorPanel"));

I also like using camelCase for react component file names (also because it's standard in react world), but I think that we should wait till there is an agreement for this in the (DevTools) team.

Honza
Attachment #8851455 - Flags: review?(odvarko) → review-
Comment on attachment 8851456 [details]
Bug 1350215 - Move store.js into utils/store.js

https://reviewboard.mozilla.org/r/123750/#review126658
Attachment #8851456 - Flags: review?(odvarko) → review+
Ok. I'll rename all components back. Thanks
Flags: qe-verify? → qe-verify-
Comment on attachment 8851457 [details]
Bug 1350215 - Move top level source files to src folder

https://reviewboard.mozilla.org/r/123752/#review126690

Not sure why devtools/client/netmonitor/src/har/har-builder.js is displayed as a new file, it's been just moved, correct?

I like having all files within src dir.

R+ assuming try is green.

Honza
Attachment #8851457 - Flags: review?(odvarko) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #40)
> Not sure why devtools/client/netmonitor/src/har/har-builder.js is displayed
> as a new file, it's been just moved, correct?

Hmm.. I think it probably is a bug in mozreview board.
Comment on attachment 8851458 [details]
Bug 1350215 - Merge toolbox launcher in index.html

https://reviewboard.mozilla.org/r/123754/#review126740

Also agree that we can have two entry points in netmonitor folder (one for the Toolbox and one for Launchpad)

Honza
Attachment #8851458 - Flags: review?(odvarko) → review-
Comment on attachment 8851459 [details]
Bug 1350215 - Fix mochitest failures

https://reviewboard.mozilla.org/r/123756/#review126742

R+ assuming try is green

Honza
Attachment #8851459 - Flags: review?(odvarko) → review+
Comment on attachment 8851502 [details]
Bug 1350215 - Move netmonitor.css to netmonitor folder

https://reviewboard.mozilla.org/r/123828/#review126744

This is great!

Honza
Attachment #8851502 - Flags: review?(odvarko) → review+
Comment on attachment 8851455 [details]
Bug 1350215 - Merge shared/components to components

https://reviewboard.mozilla.org/r/123748/#review126746

I am still seeing some files with CamelCase file name.
(e.g. CookiesPanel.js, CustomRequestPanel.js, etc.)

Honza
Attachment #8851455 - Flags: review?(odvarko) → review-
There are some mistakes when apply git commit --fixup, so many changes reverted. I'd like to make this renaming patch as a separate patch so that maybe we can revert it back easily in someday.
Comment on attachment 8851455 [details]
Bug 1350215 - Merge shared/components to components

https://reviewboard.mozilla.org/r/123748/#review126824
Attachment #8851455 - Flags: review?(odvarko) → review+
Comment on attachment 8851959 [details]
Bug 1350215 - Rename components from camelcase to hyphens

https://reviewboard.mozilla.org/r/124210/#review126828
Attachment #8851959 - Flags: review?(odvarko) → review+
Comment on attachment 8851458 [details]
Bug 1350215 - Merge toolbox launcher in index.html

https://reviewboard.mozilla.org/r/123754/#review126832

I'd prefer to have two entry points and not mix JS with HTML 
(see also  Comment 48)

Honza
Attachment #8851458 - Flags: review?(odvarko) → review-
(In reply to Jan Honza Odvarko [:Honza] from comment #64)
> Comment on attachment 8851458 [details]
> Bug 1350215 - Merge toolbox launcher in index.html
> 
> https://reviewboard.mozilla.org/r/123754/#review126832
> 
> I'd prefer to have two entry points and not mix JS with HTML 
> (see also  Comment 48)

Sorry for no giving detail explanation about this. I though we should move it quickly and this part wouldn't cause too many issues. However, I was wrong.

Some reasons to prefer to use mix JS with HTML

* In order to make us more close to Github production (no matter in mono-repo or standalone repo), we can mimics structure of debugger.html and convert xhtml to html.
* There are some issues when loading netmonitor.js in index.html, but it works in index.xhtml. I have no idea about the root cause of this, only noticed that <script src="./index.js"></script> cannot execute index.js correctly and return an empty object.
* And debugger.html also use mix JS and HTML [1].
* So that let me prefer to merge them into one HTML file for clear our code structure.

Now the call path from firefox devtools would be:

* devtools toolbox loads index.html
* execute panel.js

the call path from browser tab would be:

* load webpack.config.js
* execute index-browser.js or index.js or main.js (this file will be introduced in next step)

[1] https://github.com/devtools-html/debugger.html/blob/master/assets/panel/index.html#L22-L29
Flags: needinfo?(odvarko)
BTW, in the future if we're going to support bundle netmonitor on firefox-panel (toolbox) and devtools-launchpad (browser tab), we can replace index.html script section [1] to load bundle.js. There is no necessary to get rid of the useless index-firefox.js file.

As a result, that's why I'd prefer to merge all firefox panel relevant logic into one file.

[1] https://github.com/devtools-html/debugger.html/blob/master/assets/panel/index.html#L23-L28
Comment on attachment 8851458 [details]
Bug 1350215 - Merge toolbox launcher in index.html

https://reviewboard.mozilla.org/r/123754/#review126842
Attachment #8851458 - Flags: review- → review+
Comment on attachment 8851458 [details]
Bug 1350215 - Merge toolbox launcher in index.html

Note bug 1349415 is about to land which modified netmonitor.js, please rebase before landing this
Attachment #8851458 - Flags: review?(gasolin) → review+
Comment on attachment 8851457 [details]
Bug 1350215 - Move top level source files to src folder

https://reviewboard.mozilla.org/r/123752/#review127052
Attachment #8851457 - Flags: review?(gasolin) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e6d27888cb6f
Merge shared/components to components r=Honza
https://hg.mozilla.org/integration/autoland/rev/e676e004d08e
Move store.js into utils/store.js r=gasolin,Honza
https://hg.mozilla.org/integration/autoland/rev/f812f7a98a9c
Move top level source files to src folder r=gasolin,Honza
https://hg.mozilla.org/integration/autoland/rev/e219f3f6e8e7
Merge toolbox launcher in index.html r=Honza
https://hg.mozilla.org/integration/autoland/rev/59d2a3f98f6d
Fix mochitest failures r=Honza
https://hg.mozilla.org/integration/autoland/rev/b8358b21928c
Move netmonitor.css to netmonitor folder r=Honza
https://hg.mozilla.org/integration/autoland/rev/435557f30204
Rename components from camelcase to hyphens r=Honza
Thanks. It seems like a unreferenced file which has introduced by rebasing the patch.
Flags: needinfo?(rchien)
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d2ad134761f4
Merge shared/components to components r=Honza
https://hg.mozilla.org/integration/autoland/rev/29e9f5f6c3af
Move store.js into utils/store.js r=gasolin,Honza
https://hg.mozilla.org/integration/autoland/rev/2942893f5c85
Move top level source files to src folder r=gasolin,Honza
https://hg.mozilla.org/integration/autoland/rev/b49b6e0c36a3
Merge toolbox launcher in index.html r=Honza
https://hg.mozilla.org/integration/autoland/rev/8ca3b7b0c645
Fix mochitest failures r=Honza
https://hg.mozilla.org/integration/autoland/rev/f737432916d8
Move netmonitor.css to netmonitor folder r=Honza
https://hg.mozilla.org/integration/autoland/rev/4787ae58a7d2
Rename components from camelcase to hyphens r=Honza
Blocks: 1350229
Just removing NI
Honza
Flags: needinfo?(odvarko)
No longer blocks: 1350229
Comment on attachment 8851458 [details]
Bug 1350215 - Merge toolbox launcher in index.html

Clean r?gasolin flag.

Fred has approved this part at https://bugzilla.mozilla.org/show_bug.cgi?id=1350215#c68 but bugzilla or mozreview doesn't update it accordingly.
Attachment #8851458 - Flags: review?(gasolin)
Why did you add a version parameter to a script element in index.html? We should not add any new versioned scripts. See bug 1342144 for details.
Flags: needinfo?(rchien)
Blocks: 1352288
Thanks for reporting this. Follow-up fix will be addressed in bug 1352288.
Flags: needinfo?(rchien)
Depends on: 1355620
Depends on: 1307800
Depends on: 1356786
Depends on: 1373594
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: