Daniel Morante Reporter
	Description • 7 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170125094131

Steps to reproduce:

Inspect the current page using the context menu item "Inspect Element", click on the network tab, then reload the page.


Actual results:

I see columns for several items including domain.  I do not see a column for IP address.
The IP address is shown only as a tooltip.  I am unable to sort columns by IP address.

See attachment "firefox.png"


Expected results:

A column should appear for the IP address.  Allowing you to sort and/or be able to identify loaded resources by IP address.  This is critical for debugging DNS related issues.

See attachment 'firebug.png"

	Ruturaj Vartak Assignee
	Comment 2 • 7 years ago

Is the IP is already available ? without writing any code on C / C++.

	Ruturaj Vartak Assignee
	Comment 3 • 7 years ago

- Its available, hover (tooltip / title) of the domain name shows the IP Address [1]
- Should the RemoteIP column be listed next to the domain column before the "Cause" column ?

[1] - https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/components/request-list-item.js#284

	Ruturaj Vartak Assignee
	Comment 4 • 7 years ago

I was willing to take this, needed few clarifications regarding the UI, If we add the RemoteIP column

- the exact position of the column (comment#3)
- would we completely remove the tooltop (title) from the domain ?
- Any tooltip associated with this list-item.
- Any other rendering / beautification / color associated to the item, or plain old "<IP>:<Port>" should suffice?

	Tim Nguyen :ntim
	Comment 5 • 7 years ago

(In reply to Ruturaj Vartak from comment #3)
> - Its available, hover (tooltip / title) of the domain name shows the IP
> Address [1]
> - Should the RemoteIP column be listed next to the domain column before the
> "Cause" column ?
Makes sense to have it right after Domain.

> - would we completely remove the tooltop (title) from the domain ?
It would make sense with the new column.

> - Any tooltip associated with this list-item.
Not sure what useful info we could add there.

> - Any other rendering / beautification / color associated to the item, or plain old "<IP>:<Port>" should suffice?
<IP>:<Port> should be fine.


Though I don't know whether we want a new column. We have more columns than Firebug, so we might not have space for a new IP column.

Honza ?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 6 • 7 years ago

(In reply to Tim Nguyen :ntim from comment #5)
> Though I don't know whether we want a new column. We have more columns than
> Firebug, so we might not have space for a new IP column.
Yes, the problem is with the horizontal space. I am happy to have new columns but, we need a way tho show/hide them. See bug 862855. 

Honza

	Tim Nguyen :ntim
	Comment 7 • 7 years ago

Note that I'm currently working on bug 862855, so this bug should be actionable once that's done.

	Tim Nguyen :ntim
	Comment 8 • 7 years ago

Ruturaj, are you interested in working on this now that bug 862855 is done ?

	Ruturaj Vartak Assignee
	Comment 9 • 7 years ago

Yes, ill pick it up.

	Ruturaj Vartak Assignee
	Comment 10 • 7 years ago

Hey Tim,

As of 10.30AM IST Apr 5 (5.00AM UTC) I don't see any UI feature for bug#862855 - I'm on master@bceaacd2b

I don't see any right click interaction with Headers of the Network list that'll provide for Add / remove cols.

I'm not sure if your patch has been pushed in master branch, I see bug#1350228 as ur last commit.

	Tim Nguyen :ntim
	Comment 11 • 7 years ago

(In reply to Ruturaj Vartak from comment #10)
> Hey Tim,
> 
> As of 10.30AM IST Apr 5 (5.00AM UTC) I don't see any UI feature for
> bug#862855 - I'm on master@bceaacd2b
> 
> I don't see any right click interaction with Headers of the Network list
> that'll provide for Add / remove cols.
> 
> I'm not sure if your patch has been pushed in master branch, I see
> bug#1350228 as ur last commit.

Ah, bug 862855 is cuurently only on autoland, it should arrive in mozilla-central (master) anytime today. You should be able to apply the patch manually though, and work on top of it.

	Ruturaj Vartak Assignee
	Comment 12 • 7 years ago

Nice! that worked, thanks.

	Ruturaj Vartak Assignee
	Comment 13 • 7 years ago

This is a WIP patch, for verification if I'm in the right direction.

- This patch currently fails 2 tests
- I'll be adding few more tests for sort and filtering

	Ruturaj Vartak Assignee
	Comment 14 • 7 years ago

- Is it OK to not have remoteIP is certain requests?
- Also "RemoteIP" label has been selected from Firebug
- Currently the port is not being shown, since we already have a Green padlock for domain, should I also show the port?

	Tim Nguyen :ntim
	Comment 15 • 7 years ago

Comment on attachment 8856074 [details] [diff] [review]
WIP-fix-1344523.patch

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

Thanks for working on this! This is going in the right direction, here are a few minor comments.

::: devtools/client/locales/en-US/netmonitor.properties
@@ +411,5 @@
>  # in the network table toolbar, above the "domain" column.
>  netmonitor.toolbar.domain=Domain
>  
> +# LOCALIZATION NOTE (netmonitor.toolbar.domain): This is the label displayed
> +# in the network table toolbar, above the "domain" column.

nit: fix this comment.

@@ +412,5 @@
>  netmonitor.toolbar.domain=Domain
>  
> +# LOCALIZATION NOTE (netmonitor.toolbar.domain): This is the label displayed
> +# in the network table toolbar, above the "domain" column.
> +netmonitor.toolbar.remoteip=RemoteIP

Should be "Remote IP" (with a space). Firebug also uses a space here.

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css
@@ +1307,5 @@
>    }
>  
>    .requests-list-status {
>      max-width: none;
> +    width: 8vw;

hmm, width: 8wv seems like duplicated code from above, let's just remove the width declaration.

::: devtools/client/netmonitor/src/reducers/ui.js
@@ +22,5 @@
>    status: true,
>    method: true,
>    file: true,
>    domain: true,
> +  remoteip: true,

We probably don't want this enabled by default atm to avoid taking up more space initially. We can potentially re-evaluate later which columns to show by default though.

So, let's keep this to false for now.

You might want to adjust the following files for that:
- devtools/client/preferences/devtools.js
- devtools/client/netmonitor/src/middleware/prefs.js

::: devtools/client/netmonitor/src/utils/filter-predicates.js
@@ +103,5 @@
>  }
>  
> +function isFreetextMatch({ url, remoteAddress = "" }, text) {
> +  // This ensures search is for URL and RemoteIP
> +  let lowerCaseUrl = url.toLowerCase() + remoteAddress.toLowerCase();

I would rather fix this with bug 1041895, and have it as an operator. Let's leave this aside for now.

	Tim Nguyen :ntim
	Comment 16 • 7 years ago

(In reply to Ruturaj Vartak from comment #14)
> Created attachment 8856075 [details]
> Preview of RemoteIP implementation
> 
> - Is it OK to not have remoteIP is certain requests?
Hmm, I'm surprised some requests don't have a Remote IP. Anyway, that's fine for now.

> - Also "RemoteIP" label has been selected from Firebug
Firebug has a space between Remote and IP, so let's add that as well.

> - Currently the port is not being shown, since we already have a Green
> padlock for domain, should I also show the port?

The green padlock does not necessarily represent the port. I think we should also show the port in this case.

	Tim Nguyen :ntim
	Comment 17 • 7 years ago

Btw, I've added more tests that test column showing/hiding in bug 1353380. So you might want to have that locally to make sure that you don't regress anything (no need to add extra testcases though).

	Ruturaj Vartak Assignee
	Comment 18 • 7 years ago

Hey Tim,
Thanks for the review, I'll work on the changes.

It seems a lot is going on with the search :) personally i think `status-code:` / gmail kinda search would be over-engineering. There ain't a big worksheet with multiple rows in the network tab that I'd need such complex tools like specific filtering, etc..

	Tim Nguyen :ntim
	Comment 19 • 7 years ago

(In reply to Ruturaj Vartak from comment #18)
> It seems a lot is going on with the search :) personally i think
> `status-code:` / gmail kinda search would be over-engineering. There ain't a
> big worksheet with multiple rows in the network tab that I'd need such
> complex tools like specific filtering, etc..

I want to bring search to be in parity with Chrome (in fact I've made sure the flag names match those from chrome).

	Ruturaj Vartak Assignee
	Comment 20 • 7 years ago

I forgot to ask - I had set the width of RemoteIP col to 8vw and other cols' width I had reduced from 10vw to 8vw so that the total would be 100%. But if the remoteIP col is going to be disabled by default, what should I set its width?

	Tim Nguyen :ntim
	Comment 21 • 7 years ago

(In reply to Ruturaj Vartak from comment #20)
> I forgot to ask - I had set the width of RemoteIP col to 8vw and other cols'
> width I had reduced from 10vw to 8vw so that the total would be 100%. But if
> the remoteIP col is going to be disabled by default, what should I set its
> width?

Same as domain sounds good.

	Ruturaj Vartak Assignee
	Comment 22 • 7 years ago

- using "Remote IP" as label
- using "IP:Port" as the item value
- Adding "remoteip" as default hidden column
- Ignoring filter/search, as its gonna be reworked in another bug

	Tim Nguyen :ntim
	Comment 23 • 7 years ago

Comment on attachment 8856206 [details] [diff] [review]
fix-1344523-1.patch

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

The code looks mostly fine, I will do some manual testing before r+ ing this.

Do you have access to the try server? https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server

(Please request access if you don't, I can vouch you)

::: devtools/client/netmonitor/src/middleware/prefs.js
@@ +35,5 @@
>            .map(([column, shown]) => column);
>          break;
>  
>        case RESET_COLUMNS:
> +        Prefs.hiddenColumns = ["remoteip"];

I would prefer removing this bit of code, and replace:

case TOGGLE_COLUMN:

with:

case TOGGLE_COLUMN:
case RESET_COLUMNS:

It should just work, because the column state is being changed in both cases.

	Tim Nguyen :ntim
	Comment 24 • 7 years ago

Comment on attachment 8856206 [details] [diff] [review]
fix-1344523-1.patch

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

So sorting doesn't work as expected.
103.**.**.**::**
should go *after*
87.**.**.**::**

when sorting in ascending order. Feel free to fix this in a follow up bug.

	Tim Nguyen :ntim
	Comment 25 • 7 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=175aaa4d5f45d8c8020d0280aea3e798363a9dcd

	Ruturaj Vartak Assignee
	Comment 26 • 7 years ago

Hey Tim,
- Would we also get IPV6 address in `remoteAddress` ? Fx supports it? - I need to take care of it in the sort-predicates.js
- Regarding the `TOGGLE_COLUMN`.. you wanted something like the following ?
```
      case TOGGLE_COLUMN:
      case RESET_COLUMNS:
        Prefs.hiddenColumns = [...store.getState().ui.columns]
          .filter(([column, shown]) => !shown)
          .map(([column, shown]) => column);
        Prefs.hiddenColumns = ["remoteip"];
        break;
```

	Tim Nguyen :ntim
	Comment 27 • 7 years ago

(In reply to Ruturaj Vartak from comment #26)
> Hey Tim,
> - Would we also get IPV6 address in `remoteAddress` ? Fx supports it? - I
> need to take care of it in the sort-predicates.js
I think it already works with ipv6, not sure, you can test with https://ipv6.google.com/

> - Regarding the `TOGGLE_COLUMN`.. you wanted something like the following ?
> ```
>       case TOGGLE_COLUMN:
>       case RESET_COLUMNS:
>         Prefs.hiddenColumns = [...store.getState().ui.columns]
>           .filter(([column, shown]) => !shown)
>           .map(([column, shown]) => column);
>         Prefs.hiddenColumns = ["remoteip"];
>         break;
> ```

No, this is what I'm suggesting:
```
       case TOGGLE_COLUMN:
       case RESET_COLUMNS:
         Prefs.hiddenColumns = [...store.getState().ui.columns]
           .filter(([column, shown]) => !shown)
           .map(([column, shown]) => column);
         break;
 ```

	Tim Nguyen :ntim
	Comment 28 • 7 years ago

bug 1041895 has landed (it's on master), you might want to add the operator yourself.

	Tim Nguyen :ntim
	Comment 29 • 7 years ago

Something like:

remote-ip:**.**.**.** to search by IP.

	Ruturaj Vartak Assignee
	Comment 30 • 7 years ago

(In reply to Tim Nguyen :ntim from comment #29)
> Something like:
> 
> remote-ip:**.**.**.** to search by IP.

aah !! nice.. let me try that.

I forgot to answer your question about try, no I don't have access to try. I think try needs hg / or git-cinnabar. Last time I tried installing cinnabar, I lost half of the day and I gave up :P

	Tim Nguyen :ntim
	Comment 31 • 7 years ago

(In reply to Ruturaj Vartak from comment #30)
> (In reply to Tim Nguyen :ntim from comment #29)
> > Something like:
> > 
> > remote-ip:**.**.**.** to search by IP.
> 
> aah !! nice.. let me try that.
To be clear, I meant that you can add the operator yourself (it's not implemented yet for IP):

In devtools/client/netmonitor/src/constants.js, register your column with canFilter: true (and optionally a filter key, if you don't register a filter key, it will use the column name).

Then in devtools/client/netmonitor/src/utils/filter-text-utils.js, add a new case statement for your new column.
 
> I forgot to answer your question about try, no I don't have access to try. I
> think try needs hg / or git-cinnabar. Last time I tried installing cinnabar,
> I lost half of the day and I gave up :P
It's worth it if you get the chance to do it (ask in #developers if you have trouble doing this) since you can use mozreview as well.

	Ruturaj Vartak Assignee
	Comment 32 • 7 years ago

- Implemented after merging latest master
- All tests are passing
- however, Sort of RemoteIP isn't working as expected. I wanted another eye to go over it.

	Tim Nguyen :ntim
	Comment 33 • 7 years ago

Comment on attachment 8856977 [details] [diff] [review]
fix-1344523-2.patch

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

::: devtools/client/netmonitor/src/utils/filter-text-utils.js
@@ +125,5 @@
>      case "domain":
>        match = item.urlDetails.host.toLowerCase().includes(value);
>        break;
> +    case "remoteip":
> +      match = (item.remoteAddress + item.remotePort + "").toLowerCase().includes(value);

shouldn't it be:

`${item.remoteAddress}:${item.remotePort}`.toLowerCase().includes(value); ?

(you were missing ":")

::: devtools/client/netmonitor/src/utils/sort-predicates.js
@@ +28,4 @@
>    return first > second ? 1 : -1;
>  }
>  
> +function ip2long(ip) {

I would put this function after compareValues() in sort-predicates.js

also: ipToLong

@@ +29,5 @@
>  }
>  
> +function ip2long(ip) {
> +  if (!ip) {
> +    return 0;

I would put -1, so 0.0.0.0 appears after no IP.

@@ +33,5 @@
> +    return 0;
> +  }
> +  let base;
> +  let octets = ip.split(".");
> +  if (octets.length === 4) {

Add comment indicator it's IPv4.

@@ +39,5 @@
> +  } else {
> +    // Probably IPv6
> +    octets = ip.split(":");
> +    if (octets.length >= 6) {
> +      // Some IPv6s might have ::

I'm guessing this comment applies to the greater or equal to 6

In this case, I prefer doing:

octets = ip.replace(/\:+/g, ":").split(":");
if (octets.length === 6)

@@ +41,5 @@
> +    octets = ip.split(":");
> +    if (octets.length >= 6) {
> +      // Some IPv6s might have ::
> +      base = 16;
> +    } else {

else {
  // Invalid IP

@@ +42,5 @@
> +    if (octets.length >= 6) {
> +      // Some IPv6s might have ::
> +      base = 16;
> +    } else {
> +      return 0;

Same comment here. (-1)

@@ +83,5 @@
>  }
>  
> +function remoteip(first, second) {
> +  const firstIP = ip2long(first.remoteAddress);
> +  const secondIP = ip2long(second.rempteAddress);

typo: remoteAddress not rempteAdress

(might be why sorting wasn't working well)

	Ruturaj Vartak Assignee
	Comment 34 • 7 years ago

- the remoteAddress typo was silly, yes, things worked post that
- returning -1 for invalid IP

	Tim Nguyen :ntim
	Comment 35 • 7 years ago

Comment on attachment 8857015 [details] [diff] [review]
fix-1344523-3.patch

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

Thanks!

::: devtools/client/netmonitor/src/utils/sort-predicates.js
@@ +39,5 @@
> +    base = 10;
> +  } else {
> +    // Probably IPv6
> +    octets = ip.replace(/\:+/g, ":").split(":");
> +    if (octets.length >= 6) {

This condition is incorrect.

"::1" is a totally valid IPv6 address (it means " 0000:0000:0000:0000:0000:0000:0000:0001")

I think the condition can be omitted and the else above can be changed to:

else if (ip.includes(":")) {

@@ +40,5 @@
> +  } else {
> +    // Probably IPv6
> +    octets = ip.replace(/\:+/g, ":").split(":");
> +    if (octets.length >= 6) {
> +      // Some IPv6s might have ::

I read up about IPv6, and the algorithm is actually a bit more complicated:

- :: represents series of zeros,
- leading zeros can be omitted. (though we don't need to handle this for this function)

So the right algorithm would be:

```
// Process IPv6 short notation
// See https://en.wikipedia.org/wiki/IPv6#Address_representation
let numberOfZeroSections = 8 - ip.replace(/^:+|:+$/g, "").split(/:+/g).length;
octets = ip
  .replace("::", `:${"0:".repeat(numberOfZeroSections)}`)
  .replace(/^:|:$/g, "")
  .split(":");

```

I apologize for not reading this earlier :)

	Ruturaj Vartak Assignee
	Comment 36 • 7 years ago

- used your IPv6 validation, thanks for that!!
- moved ipToLong function to request-utils.js; I think you meant that in one of your comments

	Tim Nguyen :ntim
	Comment 37 • 7 years ago

Comment on attachment 8857333 [details] [diff] [review]
fix-1344523-4.patch

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

Awesome

::: package.json
@@ +1,2 @@
>  {
> +  "name": "mozilla-eslint-setup",

unrelated change.

	Tim Nguyen :ntim
	Comment 38 • 7 years ago

I've fixed a bug related to column showing.
I've also done some minor code style changes.

	Pulsebot
	Comment 39 • 7 years ago

Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8322b71c7df2
Add IP Address in Network Monitor. r=ntim.

	Ruturaj Vartak Assignee
	Comment 40 • 7 years ago

Thanks a lot Tim. Could you please explain what was the change in `netmonitor/src/utils/create-store.js` ?

	Tim Nguyen :ntim
	Comment 41 • 7 years ago

(In reply to Ruturaj Vartak from comment #40)
> Thanks a lot Tim. Could you please explain what was the change in
> `netmonitor/src/utils/create-store.js` ?

There was a bug where if you enabled the Remote IP column then closed the toolbox and restarted the toolbox, the Remote IP column disappeared (the column state should persist across devtools sessions).

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 42 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8322b71c7df2

	Sebastian Zartner [:sebo]
	Comment 43 • 7 years ago

Mentioned this feature at https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Network_request_fields and https://developer.mozilla.org/en-US/Firefox/Releases/55#Developer_Tools.

Sebastian

	Ruturaj Vartak Assignee
	Comment 44 • 7 years ago

Hey Sebastian,

Thanks, Remote IP is disabled by default, users have to right click on Column header and then select the columns. If we could mention that somewhere as well.

	Sebastian Zartner [:sebo]
	Comment 45 • 7 years ago

Thank you for the note, Ruturaj! I have further clarified the description for how to toggle the hidden columns and added a note to each initially hidden column.

Sebastian

	Sajedul Islam
	Comment 46 • 7 years ago

I have reproduced this bug with Nightly 54.0a1 (2017-03-04) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly !

Build ID 	20170503030212
User Agent 	Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170503]