Closed Bug 1150697 Opened 5 years ago Closed 5 years ago

Display IP address in Network tab

Categories

(DevTools :: Netmonitor, defect, P1)

36 Branch
defect

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)

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: nobody → janx
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
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
Attached image netmon-ip-column.png (obsolete) —
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.
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)
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
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)
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
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+
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.
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
Attachment #8594035 - Flags: review?(bgrinstead)
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+
Thank you! :)
Keywords: checkin-needed
(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
Closed: 5 years ago
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]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.