Closed
Bug 1150697
Opened 9 years ago
Closed 9 years ago
Display IP address in Network tab
Categories
(DevTools :: Netmonitor, defect, P1)
Tracking
(firefox40 fixed)
RESOLVED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: canuckistani, Assigned: janx)
Details
(Whiteboard: [polish-backlog][difficulty=easy])
Attachments
(2 files, 3 obsolete files)
4.28 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
6.94 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
See uservoice: https://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/6059723-display-ip-address-in-network-tab Assuming the platform has this info, could be shallow.
Was recently added to the Headers sub-panel for a selected request, but it's not yet in the main table view. So, at least the data is accessible, it's just some UI work to get it in the table.
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
Reporter | ||
Comment 2•9 years ago
|
||
Assigning P1 priority for some devedition-40 bugs. Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → janx
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 3•9 years ago
|
||
Renaming to the original uservoice idea, because we already "show IP Address for the current request": When you click on a request, you can see its remote IP address in "Headers > Remote address". The question for this bug is rather, do we want to show the IP address in the table view? Or in a tooltip?
Summary: Show IP Address for the current request in the netmonitor → Display IP address in Network tab
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11ff48a45cb4
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8591777 [details] [diff] [review] Add IP address column to the Network Monitor. Victor, what do you think about this approach?
Attachment #8591777 -
Flags: review?(vporof)
Comment 7•9 years ago
|
||
(In reply to Jan Keromnes [:janx] from comment #3) > Renaming to the original uservoice idea, because we already "show IP Address > for the current request": When you click on a request, you can see its > remote IP address in "Headers > Remote address". > > The question for this bug is rather, do we want to show the IP address in > the table view? Or in a tooltip? Yes, this feature request is ambiguous. I don't think we should add a new column in the netmonitor table for this - it takes away space from all of the other columns. My preference would be as a tooltip (maybe on the domain column?), but interested in thoughts from others about this.
Comment 8•9 years ago
|
||
+1 on putting the address in the tooltip. The table is already cramped as it is.
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8591777 [details] [diff] [review] Add IP address column to the Network Monitor. Ok, thanks for the feedback! I'll change it to a tooltip on the domain column.
Attachment #8591777 -
Flags: review?(vporof)
Assignee | ||
Comment 10•9 years ago
|
||
Changed to exposing the IP address in the domain tooltip, which now looks like "full.domain.com (ip.addr)". Brian, please have a look. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7d224d44e14
Attachment #8591777 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8593384 -
Flags: review?(bgrinstead)
Comment 11•9 years ago
|
||
Comment on attachment 8593384 [details] [diff] [review] Add IP address to the Network Monitor domain tooltip. Review of attachment 8593384 [details] [diff] [review]: ----------------------------------------------------------------- You will need to update verifyRequestItemTarget in head.js to check for the updated tooltiptext: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/test/head.js#304
Attachment #8593384 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 12•9 years ago
|
||
Right, thanks for pointing at the location in the code. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ae16d7d6590
Attachment #8593384 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8593890 -
Flags: review?(bgrinstead)
Comment 13•9 years ago
|
||
Comment on attachment 8593890 [details] [diff] [review] Add IP address to the Network Monitor domain tooltip. Review of attachment 8593890 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me ::: browser/devtools/netmonitor/test/head.js @@ +307,2 @@ > is(target.querySelector(".requests-menu-domain").getAttribute("tooltiptext"), > + domainTooltip, "The tooltip domain is incorrect."); Why do all of these messages say "The X is incorrect"? I think they are all checking to make sure that things are right. Oh well, let's ship it as is since at least it's consistent.
Attachment #8593890 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 14•9 years ago
|
||
I had the same thought, but wanted to stay consistent. I'll consider your remark a nit and fix all the messages in a follow-up patch.
Assignee | ||
Comment 15•9 years ago
|
||
Easier to review here: https://github.com/jankeromnes/gecko-dev/commit/ab5575daa90e9d2f56eec66c9aacb7593c20d108 (It's 2015 and we're still using line-based diffs instead of char-based.) Try for both patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25b2546281a9
Assignee | ||
Updated•9 years ago
|
Attachment #8594035 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•9 years ago
|
Attachment #8591779 -
Attachment is obsolete: true
Comment 16•9 years ago
|
||
Comment on attachment 8594035 [details] [diff] [review] Fix confusing messages in Network Monitor tests. Review of attachment 8594035 [details] [diff] [review]: ----------------------------------------------------------------- Thanks
Attachment #8594035 -
Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/integration/fx-team/rev/dc8a2c8a0df3 https://hg.mozilla.org/integration/fx-team/rev/24d17cc67c60
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
(In reply to Brian Grinstead [:bgrins] from comment #13) > Comment on attachment 8593890 [details] [diff] [review] > Add IP address to the Network Monitor domain tooltip. > > Review of attachment 8593890 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good to me > > ::: browser/devtools/netmonitor/test/head.js > @@ +307,2 @@ > > is(target.querySelector(".requests-menu-domain").getAttribute("tooltiptext"), > > + domainTooltip, "The tooltip domain is incorrect."); > > Why do all of these messages say "The X is incorrect"? I think they are > all checking to make sure that things are right. Oh well, let's ship it as > is since at least it's consistent. I believe Victor's style for test assertion messages is to use the negative, since the messages are only printed when the test fails, in which case it *would* be incorrect. Seems like an unresolved style difference, since different people use different policies in different parts of the code base (inside and outside DevTools).
Comment 20•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] from comment #19) > (In reply to Brian Grinstead [:bgrins] from comment #13) > > Why do all of these messages say "The X is incorrect"? I think they are > > all checking to make sure that things are right. Oh well, let's ship it as > > is since at least it's consistent. > > I believe Victor's style for test assertion messages is to use the negative, > since the messages are only printed when the test fails, in which case it > *would* be incorrect. > > Seems like an unresolved style difference, since different people use > different policies in different parts of the code base (inside and outside > DevTools). Hm, thanks for the heads up. I haven't seen it done like that elsewhere. The negation seemed inconsistent even in head.js including lines like this [0]. is(button.hasAttribute("checked"), true, "The " + button.id + " button should have a 'checked' attribute.") The performance tool also seems use positive assertion messages [1]: ok(!button.hasAttribute("checked"), "The record button should not be checked yet."); It's a good thing that janx split attachment 8594035 [details] [diff] [review] into a separate patch, in case we want to back it out and not step on any toes :). Jordan, do you want attachment 8594035 [details] [diff] [review] to be backed out for style reasons? [0]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/test/head.js#414 [1]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/performance/test/head.js#332
Flags: needinfo?(jsantell)
Comment 21•9 years ago
|
||
I too was confused by the negative assertions -- I don't believe other tools that I've worked on (where VP wrote the tests) have this style. No need to back out, we should probably rewrite them in another bug (but after the refactoring lands as that will require a huge manual rebase)
Flags: needinfo?(jsantell)
Comment 22•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dc8a2c8a0df3 https://hg.mozilla.org/mozilla-central/rev/24d17cc67c60
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=easy][fixed-in-fx-team] → [devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 40
Reporter | ||
Updated•9 years ago
|
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•