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)
DevTools
Netmonitor
Tracking
(firefox54 fixed)
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: ntim, Assigned: gasolin)
References
(Blocks 1 open bug)
Details
(Whiteboard: [netmonitor])
Attachments
(2 files)
No description provided.
Reporter | ||
Updated•8 years ago
|
Priority: -- → P4
Updated•8 years ago
|
Whiteboard: [netmonitor][triage]
Updated•8 years ago
|
Updated•8 years ago
|
Flags: qe-verify? → qe-verify-
Updated•8 years ago
|
Priority: -- → P3
Updated•8 years ago
|
Whiteboard: [netmonitor][triage] → [netmonitor]
Updated•8 years ago
|
Priority: P3 → P2
Updated•7 years ago
|
Priority: P2 → P3
Whiteboard: [netmonitor] → [netmonitor-reserve]
Assignee | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
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)
Comment 3•7 years ago
|
||
(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)
Comment 4•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Updated•7 years ago
|
Iteration: --- → 54.2 - Feb 20
Priority: P3 → P1
Whiteboard: [netmonitor-reserve] → [netmonitor]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
The PR moved top-level files to utils - filter-predicates - l10n - prefs - request-utils - sort-predicates
Comment 7•7 years ago
|
||
events -> merge into constants
Comment 8•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
Here's PART 2 - move events.js into constants
Comment 11•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #15) > Autoland couldn't rebase this for landing.
Flags: needinfo?(gasolin)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•7 years ago
|
||
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)
Comment 23•7 years ago
|
||
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
Comment 24•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/39b9391c2abc https://hg.mozilla.org/mozilla-central/rev/a60b8bb664be
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•