Closed Bug 1314921 Opened 8 years ago Closed 7 years ago

Reduce number of top-level files in devtools/client/netmonitor/

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox54 fixed)

RESOLVED FIXED
Firefox 54
Iteration:
54.2 - Feb 20
Tracking Status
firefox54 --- fixed

People

(Reporter: ntim, Assigned: gasolin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(2 files)

      No description provided.
Priority: -- → P4
Whiteboard: [netmonitor][triage]
No longer depends on: netmonitor-html
Flags: qe-verify?
Priority: P4 → --
Flags: qe-verify? → qe-verify-
Whiteboard: [netmonitor][triage] → [netmonitor]
Priority: P3 → P2
Priority: P2 → P3
Whiteboard: [netmonitor] → [netmonitor-reserve]
I did some glance around top level files and like to propose a treatment below:

* .eslintrc -> not move
* constants -> not move
* custom-request-view -> [component]
* events -> merge into constants
* filter-predicates -> move to shared/utils (used in details panel)
* l10n.js -> move to shared/utils (used in details panel)
* prefs -> not move
* RequestListContextMenu -> [component]
* request-utils -> move to shared/utils since its also heavily used by sidebar panels
* request-menu-view -> [component]
* sidebar-view -> [component]
* sort-predicates -> move to utils (used in selectors/requests.js)
* store -> not move
* waterfullBackground -> move to utils (used in request-list-header.js)

One more thing
* shared/components/headers-mdn.js -> shared/utils/headers-mdn.js (The file just in the wrong place)

File marked as [component] means not move at this moment. These view related files will be moved to components/ once its refactored


Does that looks reasonable to you?
Flags: needinfo?(rchien)
Flags: needinfo?(odvarko)
Thanks for picking up this. It's time to do some code clean up and then move forward.

IMO, we can leave custom-request-view (will be removed soon), request-menu-view, sidebar-view (will be removed soon) and waterfullBackground in top level folder because they will be removed soon and they don't belong to components as well (component folder is for react component).

I'm still thinking that it's very hard to distinguish which part of code should be shareable. My feelings is every component can share for any purposes if we need, for example: It's fine with me to put network-details-panel.js in component but not in shared/component

In webconsole component

require("devtools/client/netmonitor/component/network-details-panel"); looks good to me.

For those component which is built for generic purpose like tabs, tree-view and reps...etc, it makes sense move them to "devtools/client/shared/".


The list in my mind would be:

* .eslintrc -> not move
* constants -> not move
* custom-request-view -> not move
* events -> merge into constants
* filter-predicates -> move to utils
* l10n.js -> move to utils
* prefs -> move to utils
* RequestListContextMenu -> [component]
* request-utils -> move to utils since
* request-menu-view -> not move
* sidebar-view -> not move
* sort-predicates -> move to utils
* store -> not move
* waterfullBackground -> not move

* shared/components/headers-mdn.js -> merge into headers-panel.js because it's very simple and could treat it as top level component method in headers-pane.js.
Flags: needinfo?(rchien)
(In reply to Ricky Chien [:rickychien] from comment #2)

> IMO, we can leave custom-request-view (will be removed soon),
> request-menu-view, sidebar-view (will be removed soon) and
> waterfullBackground in top level folder because they will be removed soon
> and they don't belong to components as well (component folder is for react
> component).
I think it's what Fred is suggesting: "File marked as [component] means not move at this moment. These view related files will be moved to components/ once its refactored"

Anyway, I agree that the components dir should contain only real React components.


> I'm still thinking that it's very hard to distinguish which part of code
> should be shareable. My feelings is every component can share for any
> purposes if we need, for example: It's fine with me to put
> network-details-panel.js in component but not in shared/component
Yes, I have the same feeling. At some point we could merge `shared/components` into `components` dir.
But, I would give it some more time before we do it, though. E.g. when we integrate with the Console panel again.

It's fine for me to move all utility files into `netmonitor/utils` and *not* introduce new `netmonitor/shared/utils` dir now.

> require("devtools/client/netmonitor/component/network-details-panel"); looks
> good to me.
Agree

> For those component which is built for generic purpose like tabs, tree-view
> and reps...etc, it makes sense move them to "devtools/client/shared/".
Yes

> The list in my mind would be:
> 
> * .eslintrc -> not move
> * constants -> not move
> * custom-request-view -> not move
> * events -> merge into constants
> * filter-predicates -> move to utils
> * l10n.js -> move to utils
> * prefs -> move to utils
> * RequestListContextMenu -> [component]
> * request-utils -> move to utils since
> * request-menu-view -> not move
> * sidebar-view -> not move
> * sort-predicates -> move to utils
> * store -> not move
> * waterfullBackground -> not move
Looks good to me.


> * shared/components/headers-mdn.js -> merge into headers-panel.js because
> it's very simple and could treat it as top level component method in
> headers-pane.js.
Agree

Honza
Flags: needinfo?(odvarko)
Anyway, the day of removal XUL is approaching, I'd suggest that we can start to work on this as soon as bug 1309826 is resolved.
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Iteration: --- → 54.2 - Feb 20
Priority: P3 → P1
Whiteboard: [netmonitor-reserve] → [netmonitor]
Blocks: 1316291
Depends on: 1339686
The PR moved top-level files to utils
- filter-predicates
- l10n 
- prefs
- request-utils
- sort-predicates
events -> merge into constants
Comment on attachment 8837963 [details]
Bug 1314921 - move top-level files into utils;

https://reviewboard.mozilla.org/r/112966/#review114588

Looks good,

R+ but please merge events into constants (comment #7)

Honza
Attachment #8837963 - Flags: review?(odvarko) → review+
Here's PART 2 - move events.js into constants
Comment on attachment 8838368 [details]
Bug 1314921 - merge events.js into constants.js;

https://reviewboard.mozilla.org/r/113306/#review114774

LGTM r+
Attachment #8838368 - Flags: review?(rchien) → review+
thanks
Keywords: checkin-needed
Autoland couldn't rebase this for landing.
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> Autoland couldn't rebase this for landing.
Flags: needinfo?(gasolin)
Thanks for notice. The patch conflict to recent landed MDNLink and HAR-dechrome patch.
Fixed and local test passed. Will add check-in flag once try green.
Flags: needinfo?(gasolin)
try green, thanks
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/39b9391c2abc
move top-level files into utils;r=Honza
https://hg.mozilla.org/integration/autoland/rev/a60b8bb664be
merge events.js into constants.js;r=rickychien
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/39b9391c2abc
https://hg.mozilla.org/mozilla-central/rev/a60b8bb664be
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.