Closed Bug 1316291 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox54 verified)

VERIFIED FIXED
Firefox 54
Iteration:
54.3 - Mar 6
Tracking Status
firefox54 --- verified

People

(Reporter: jsnajdr, Assigned: gasolin)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [netmonitor-reserve])

Attachments

(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.

Honza
Flags: qe-verify? → qe-verify+
Whiteboard: [netmonitor][triage] → [netmonitor]
Priority: -- → P2
QA Contact: ciprian.georgiu
See also https://bugzilla.mozilla.org/show_bug.cgi?id=1308426#c14, which I ran into some troubles when replacing IDs with classes for summary button.
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
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.
Fred, 

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)
Status: ASSIGNED → NEW
Iteration: 54.2 - Feb 20 → ---
Priority: P1 → P2
Priority: P2 → P3
Whiteboard: [netmonitor] → [netmonitor-reserve]
Status: NEW → ASSIGNED
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;

https://reviewboard.mozilla.org/r/110608/#review114810

LGTM

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

https://reviewboard.mozilla.org/r/112920/#review114812

LGTM

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

https://reviewboard.mozilla.org/r/112948/#review114814

LGTM

Also R+, assuming tests are green.

Honza
Attachment #8837946 - Flags: review?(odvarko) → review+
thanks
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/65d7f3924ba6
Rename the requests-menu CSS classes in netmonitor.css;r=Honza
https://hg.mozilla.org/integration/autoland/rev/94ecf75560ac
remove request-list elements with fixed ID;r=Honza
https://hg.mozilla.org/integration/autoland/rev/0b49523fc19d
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:

https://hg.mozilla.org/integration/autoland/rev/554a4f2a15231564a40f5775f302f3a4f1f24133
https://hg.mozilla.org/integration/autoland/rev/aaad269f3230f913ca2054b9783defe3645dade6
https://hg.mozilla.org/integration/autoland/rev/25ecf724e39b9237b0f6c163ecfabaead25bcede

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0b49523fc19ddc22f6d31b7ba1de4ae496aa23e6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=78337506&repo=autoland

[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 rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/331a956158b2
PART 1:Rename the requests-menu CSS classes in netmonitor.css;r=Honza
https://hg.mozilla.org/integration/autoland/rev/7ffc683fb8af
PART 2:remove request-list elements with fixed ID;r=Honza
https://hg.mozilla.org/integration/autoland/rev/93ab9c15d211
PART 3:Remove toolbar elements with fixed IDs;r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/331a956158b2
https://hg.mozilla.org/mozilla-central/rev/7ffc683fb8af
https://hg.mozilla.org/mozilla-central/rev/93ab9c15d211
Status: ASSIGNED → RESOLVED
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 (https://hg.mozilla.org/mozilla-central/diff/7ffc683fb8af/devtools/client/netmonitor/components/request-list-empty.js#l1.14)
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.