Display IP address in Network tab

RESOLVED FIXED in Firefox 40

Status

()

Firefox
Developer Tools: Netmonitor
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: canuckistani, Assigned: janx)

Tracking

36 Branch
Firefox 40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(2 attachments, 3 obsolete attachments)

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]
Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
(Assignee)

Updated

3 years ago
Assignee: nobody → janx
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 3

3 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

3 years ago
Created attachment 8591777 [details] [diff] [review]
Add IP address column to the Network Monitor.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11ff48a45cb4
(Assignee)

Comment 5

3 years ago
Created attachment 8591779 [details]
netmon-ip-column.png
(Assignee)

Comment 6

3 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)
(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.
+1 on putting the address in the tooltip. The table is already cramped as it is.
(Assignee)

Comment 9

3 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

3 years ago
Created attachment 8593384 [details] [diff] [review]
Add IP address to the Network Monitor domain tooltip.

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

3 years ago
Attachment #8593384 - Flags: review?(bgrinstead)
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

3 years ago
Created attachment 8593890 [details] [diff] [review]
Add IP address to the Network Monitor domain tooltip.

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

3 years ago
Attachment #8593890 - Flags: review?(bgrinstead)
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

3 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

3 years ago
Created attachment 8594035 [details] [diff] [review]
Fix confusing messages in Network Monitor tests.

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

3 years ago
Attachment #8594035 - Flags: review?(bgrinstead)
(Assignee)

Updated

3 years ago
Attachment #8591779 - Attachment is obsolete: true
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+
(Assignee)

Comment 17

3 years ago
Thank you! :)
Keywords: checkin-needed
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).
(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)
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)
https://hg.mozilla.org/mozilla-central/rev/dc8a2c8a0df3
https://hg.mozilla.org/mozilla-central/rev/24d17cc67c60
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=easy][fixed-in-fx-team] → [devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 40
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
You need to log in before you can comment on or make changes to this bug.