1150697 - Display IP address in Network tab

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

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.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 2

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'

Comment 3

[Jan Keromnes [:janx]]

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?

Comment 4

[Jan Keromnes [:janx]]

Attachment: Add IP address column to the Network Monitor.

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

Comment 5

[Jan Keromnes [:janx]]

Attachment: netmon-ip-column.png

Comment 6

[Jan Keromnes [:janx]]

Comment on attachment 8591777 [details] [diff] [review]
Add IP address column to the Network Monitor.

Victor, what do you think about this approach?

Comment 7

[Brian Grinstead [:bgrins]]

(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

[Panos Astithas (he/him) [:past] (please ni?)]

+1 on putting the address in the tooltip. The table is already cramped as it is.

Comment 9

[Jan Keromnes [:janx]]

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.

Comment 10

[Jan Keromnes [:janx]]

Attachment: 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

Comment 11

[Brian Grinstead [:bgrins]]

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

Comment 12

[Jan Keromnes [:janx]]

Attachment: 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

Comment 13

[Brian Grinstead [:bgrins]]

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.

Comment 14

[Jan Keromnes [:janx]]

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.

Comment 15

[Jan Keromnes [:janx]]

Attachment: 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

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8594035 [details] [diff] [review]
Fix confusing messages in Network Monitor tests.

Review of attachment 8594035 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks

Comment 17

[Jan Keromnes [:janx]]

Thank you! :)

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/dc8a2c8a0df3
https://hg.mozilla.org/integration/fx-team/rev/24d17cc67c60

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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

[Brian Grinstead [:bgrins]]

(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

Comment 21

[Jordan Santell [:jsantell] [@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)

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc8a2c8a0df3
https://hg.mozilla.org/mozilla-central/rev/24d17cc67c60