Closed Bug 1316291 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox54 verified)

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

People

(Reporter: jsnajdr, Assigned: gasolin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor-reserve])

Attachments

(3 files, 2 obsolete files)

Bug 1316291 - PART 1:Rename the requests-menu CSS classes in netmonitor.css;
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1316291 - PART 2:remove request-list elements with fixed ID;
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1316291 - PART 3:Remove toolbar elements with fixed IDs;
59 bytes, text/x-review-board-request
Honza
: review+
Details
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.
Blocks: netmonitor-html
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
Depends on: 1309183, 1336384, 1309826, 1314921
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: 8 years ago
status-firefox54: --- → fixed
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
status-firefox54: fixed → verified
Flags: qe-verify+
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: