Closed Bug 1316291 Opened 3 years ago Closed 3 years ago

Rename the "requests-menu" CSS classes in netmonitor.css


(DevTools :: Netmonitor, defect, P1)



(firefox54 verified)

Firefox 54
54.3 - Mar 6
Tracking Status
firefox54 --- verified


(Reporter: jsnajdr, Assigned: gasolin)


(Blocks 2 open bugs)


(Whiteboard: [netmonitor-reserve])


(3 files, 2 obsolete files)

As a part of the netmonitor.html refactoring, the CSS classes in the netmonitor.css should be given some reasonable names that are compatible with component names.

"requests-menu-*" should really be "request-list-*"

Remove elements with fixed IDs (like #network-table) in favor of CSS classes.
Depends on: 1309866
Whiteboard: [netmonitor][triage]
Flags: qe-verify?
Yes please, using "requests-menu" has been always so confusing.
Thanks for the report Jarda.

Flags: qe-verify? → qe-verify+
Whiteboard: [netmonitor][triage] → [netmonitor]
Priority: -- → P2
QA Contact: ciprian.georgiu
See also, which I ran into some troubles when replacing IDs with classes for summary button.
Assignee: nobody → gasolin
Iteration: --- → 54.1 - Feb 6
Priority: P2 → P1
Iteration: 54.1 - Feb 6 → 54.2 - Feb 20
The first patch start from replace `request-menu*` to `request-list*`.
Would make sure test all green then add other patches upon it.
Attachment #8836580 - Attachment is obsolete: true
Attachment #8836580 - Flags: review?(odvarko)
Attachment #8836593 - Attachment is obsolete: true
Attachment #8836593 - Flags: review?(odvarko)
test all green.

I think we should land the base PR as soon as review granted and put other id->class issues in separate bug, or any request-list related change will conflict with this PR.

I'd suggest to postpone to work on this patch since this code cleanup seems not in high priority, and moreover, there are many CSS changes in current de-XUL MVPs (bug 1309183, bug 1336384 and bug 1309826). Thus, it would introduce lots of conflicts in this time.

IMO, this patch merely change "menu" -> "list" which seems not that meaningful for me. Reduce the usage of id -> class is another main goal in this issue. So, I'd recommend that we could fix both of them (request-menu-* -> request-list-* and id -> class) if possible.

On the other hand, CSS renaming work can be done more easily if we can wait for bug 1309826, and furthermore, it would be safe to fix id -> class in one sweep as well.
Yeah, I'd add dependencies and we can handle this after all dependencies are resolved
Attachment #8834766 - Flags: review?(odvarko)
Iteration: 54.2 - Feb 20 → ---
Priority: P1 → P2
Priority: P2 → P3
Whiteboard: [netmonitor] → [netmonitor-reserve]
Iteration: --- → 54.2 - Feb 20
Priority: P3 → P1
rebased and test all green
Blocks: 1336382
Comment on attachment 8834766 [details]
Bug 1316291 - PART 1:Rename the requests-menu CSS classes in netmonitor.css;


Attachment #8834766 - Flags: review?(odvarko) → review+
Comment on attachment 8837926 [details]
Bug 1316291 - PART 2:remove request-list elements with fixed ID;


Attachment #8837926 - Flags: review?(odvarko) → review+
Comment on attachment 8837946 [details]
Bug 1316291 - PART 3:Remove toolbar elements with fixed IDs;


Also R+, assuming tests are green.

Attachment #8837946 - Flags: review?(odvarko) → review+
Keywords: checkin-needed
Pushed by
Rename the requests-menu CSS classes in netmonitor.css;r=Honza
remove request-list elements with fixed ID;r=Honza
Remove toolbar elements with fixed IDs;r=Honza
Keywords: checkin-needed
Backed out for failing mochitest devtools/client/netmonitor/test/browser_net_copy_headers.js:

Push with failures:
Failure log:

[task 2017-02-17T16:17:39.789489Z] 16:17:39     INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_copy_headers.js | Clipboard has the given value - 
[task 2017-02-17T16:17:39.790143Z] 16:17:39     INFO - Clipboard contains the currently selected item's request headers.
[task 2017-02-17T16:17:39.790797Z] 16:17:39     INFO - Buffered messages finished
[task 2017-02-17T16:17:39.791645Z] 16:17:39     INFO - TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_copy_headers.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_copy_headers.js:71 - TypeError: monitor.toolbox.doc.querySelector(...) is null
[task 2017-02-17T16:17:39.792245Z] 16:17:39     INFO - Stack trace:
[task 2017-02-17T16:17:39.793631Z] 16:17:39     INFO -     setup@chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_copy_headers.js:71:5
[task 2017-02-17T16:17:39.794165Z] 16:17:39     INFO -     SimpleTest.waitForClipboard/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1028:12
[task 2017-02-17T16:17:39.794853Z] 16:17:39     INFO -     wait@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1015:13
[task 2017-02-17T16:17:39.795456Z] 16:17:39     INFO -     SimpleTest.waitForClipboard@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1025:5
[task 2017-02-17T16:17:39.795977Z] 16:17:39     INFO -     waitForClipboardPromise/<@chrome://mochitests/content/browser/devtools/client/framework/test/shared-head.js:519:5
[task 2017-02-17T16:17:39.796612Z] 16:17:39     INFO -     waitForClipboardPromise@chrome://mochitests/content/browser/devtools/client/framework/test/shared-head.js:518:10
[task 2017-02-17T16:17:39.797174Z] 16:17:39     INFO -     @chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_copy_headers.js:68:9
[task 2017-02-17T16:17:39.797818Z] 16:17:39     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
[task 2017-02-17T16:17:39.798408Z] 16:17:39     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
[task 2017-02-17T16:17:39.799014Z] 16:17:39     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59
[task 2017-02-17T16:17:39.799558Z] 16:17:39     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
[task 2017-02-17T16:17:39.800130Z] 16:17:39     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
[task 2017-02-17T16:17:39.800682Z] 16:17:39     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59
[task 2017-02-17T16:17:39.801463Z] 16:17:39     INFO - Leaving test bound 
[task 2017-02-17T16:17:39.802192Z] 16:17:39     INFO - JavaScript error: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/netmonitor/components/request-list-header.js, line 72: TypeError: this.refs.header is undefined
[task 2017-02-17T16:17:39.803036Z] 16:17:39     INFO - Console message: [JavaScript Error: "TypeError: this.refs.header is undefined" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/netmonitor/components/request-list-header.js" line: 72}]
Flags: needinfo?(gasolin)
Blocks: 1339558
Iteration: 54.2 - Feb 20 → 54.3 - Mar 6
try green, thanks
Flags: needinfo?(gasolin)
Keywords: checkin-needed
Pushed by
PART 1:Rename the requests-menu CSS classes in netmonitor.css;r=Honza
PART 2:remove request-list elements with fixed ID;r=Honza
PART 3:Remove toolbar elements with fixed IDs;r=Honza
Keywords: checkin-needed
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Fred, empty list UI is broken since there was an additional "s" added by mistake (
Flags: needinfo?(gasolin)
Blocks: 1341975
Thanks for report, have patch in following bug 1341975
Flags: needinfo?(gasolin)
This issue is verified fixed on latest Nightly 54.0a1 (2017-03-06) using Windows 10 x64, Mac OS 10.11.6 and Ubuntu 16.04 x64 LTS. Marking here accordingly.
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.