Closed Bug 1356872 Opened 4 years ago Closed 4 years ago

Hard to distinguish IP from Port in IPv6 address

Categories

(DevTools :: Netmonitor, enhancement)

enhancement
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: ntim, Assigned: vkatsikaros)

Details

Attachments

(2 files, 1 obsolete file)

**:**:**:***:**

There's no way to see where's the port (or if there's one).

We should display it like this:

[**:**:**:**]:**

[IP]:Port
Assignee: nobody → vkatsikaros
Attached image remote_ip.png (obsolete) —
Comment on attachment 8858725 [details]
Bug 1356872 - Hard to distinguish IP from Port in IPv6 address.

https://reviewboard.mozilla.org/r/130758/#review133306

Thanks!

::: devtools/client/netmonitor/src/components/request-list-column-remote-ip.js:28
(Diff revision 1)
> -    let remoteSummary = remoteAddress ? `${remoteAddress}:${remotePort}` : "";
> +    let remoteSummary;
> +    if (remoteAddress) {
> +      remoteSummary = remoteAddress.match(/:+/) ? `[${remoteAddress}]:${remotePort}` :
> +        `${remoteAddress}:${remotePort}`;
> +    } else {
> +      remoteSummary = "";
> +    }

This works nicely for the ip column, but the bug still affects two other places:

https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools%2Fclient%2Fnetmonitor%2Fsrc%2F+remoteAddress%7D&redirect=false


(You might want to create a formatting function in format-utils: getFormattedIPAndPort).
Attachment #8858725 - Flags: review?(ntim.bugs)
Attached image ip_tooltip_n_header.png
Attachment #8858718 - Attachment is obsolete: true
Comment on attachment 8858725 [details]
Bug 1356872 - Hard to distinguish IP from Port in IPv6 address.

https://reviewboard.mozilla.org/r/130758/#review133358

::: devtools/client/netmonitor/src/utils/filter-text-utils.js:136
(Diff revision 2)
>      case "domain":
>        match = item.urlDetails.host.toLowerCase().includes(value);
>        break;
>      case "remote-ip":
> -      match = `${item.remoteAddress}:${item.remotePort}`.toLowerCase().includes(value);
> +      let ipAndPort = getFormattedIPAndPort(item.remoteAddress, item.remotePort);
> +      match = ipAndPort.toLowerCase().includes(value);

Without the temporary variable `ipAndPort` the line would either be olnger than 90 chars, or eslint would complain about breaking the dot to another line

::: devtools/client/netmonitor/src/utils/filter-text-utils.js:226
(Diff revision 2)
> + *
> + * @param {string} ip - IP address
> + * @param {string} port
> + * @return {string} the formatted IP + port
> + */
> +

The function is short, and I don't see documentation in the other ones of the file, but I am not sure what is considered standard.
Comment on attachment 8858725 [details]
Bug 1356872 - Hard to distinguish IP from Port in IPv6 address.

https://reviewboard.mozilla.org/r/130758/#review133380

Thanks, you're almost there :)

::: devtools/client/netmonitor/src/utils/filter-text-utils.js:136
(Diff revision 2)
>      case "domain":
>        match = item.urlDetails.host.toLowerCase().includes(value);
>        break;
>      case "remote-ip":
> -      match = `${item.remoteAddress}:${item.remotePort}`.toLowerCase().includes(value);
> +      let ipAndPort = getFormattedIPAndPort(item.remoteAddress, item.remotePort);
> +      match = ipAndPort.toLowerCase().includes(value);

That's fine, it's a common practice to do this to fit the line length.

::: devtools/client/netmonitor/src/utils/filter-text-utils.js:227
(Diff revision 2)
> + * @param {string} ip - IP address
> + * @param {string} port
> + * @return {string} the formatted IP + port
> + */
> +
> +function getFormattedIPAndPort(ip, port) {

This should be in devtools/client/netmonitor/src/utils/format-utils.js as it's a formatting function.
Attachment #8858725 - Flags: review?(ntim.bugs)
Comment on attachment 8858725 [details]
Bug 1356872 - Hard to distinguish IP from Port in IPv6 address.

https://reviewboard.mozilla.org/r/130758/#review133648

LGTM. Please see my inline comment and there are some code styling issues. :)

::: devtools/client/netmonitor/src/components/request-list-column-remote-ip.js:7
(Diff revision 3)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +const { getFormattedIPAndPort } = require("../utils/format-utils");

nit: move this line after `} = require("devtools/client/shared/vendor/react");`

::: devtools/client/netmonitor/src/utils/format-utils.js:87
(Diff revision 3)
> + *
> + * @param {string} ip - IP address
> + * @param {string} port
> + * @return {string} the formatted IP + port
> + */
> +

nit: remove empty line.

::: devtools/client/netmonitor/src/utils/format-utils.js:100
(Diff revision 3)
>  module.exports = {
>    getFormattedSize,
>    getFormattedTime,
>    getSizeWithDecimals,
>    getTimeWithDecimals,
> +  getFormattedIPAndPort,

nit: alphabetical order
Attachment #8858725 - Flags: review+
Comment on attachment 8858725 [details]
Bug 1356872 - Hard to distinguish IP from Port in IPv6 address.

https://reviewboard.mozilla.org/r/130758/#review133422

r+ with Ricky's comments addressed.

::: commit-message-c697e:1
(Diff revision 3)
> +Bug 1356872 - Hard to distinguish IP from Port in IPv6 address. r?ntim

r=ntim, rickychien
Attachment #8858725 - Flags: review?(ntim.bugs) → review+
Status: NEW → ASSIGNED
>Browser Chrome Test Summary
>	Passed: 9843
>	Failed: 422
>	Todo: 0
>	Mode: e10s
> [...]
>INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_api-calls.js | The tooltip domain is correct. - Got example.com (127.0.0.1:8888), expected example.com (127.0.0.1)

The tests are complaining that I changed the tooltip in the "Domain" column. Should I revert the change for this column or tweak the tests (my guess is the first one)?
Flags: needinfo?(ntim.bugs)
(In reply to Vangelis Katsikaros from comment #11)
> >Browser Chrome Test Summary
> >	Passed: 9843
> >	Failed: 422
> >	Todo: 0
> >	Mode: e10s
> > [...]
> >INFO TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_api-calls.js | The tooltip domain is correct. - Got example.com (127.0.0.1:8888), expected example.com (127.0.0.1)
> 
> The tests are complaining that I changed the tooltip in the "Domain" column.
> Should I revert the change for this column or tweak the tests (my guess is
> the first one)?

I think having the port is useful, so I would tweak the tests.
Flags: needinfo?(ntim.bugs)
From the output of the test when it starts I understand that the remote port of the test server is configurable via `_SERVER_PORT` (in my case with a default value of 8888)
> MochitestServer : launching [u'/home/vag/projects/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/xpcshell', '-g', u'/home/vag/projects/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin', '-v', '170', '-f', u'/home/vag/projects/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/components/httpd.js', '-e', "const _PROFILE_PATH = '/tmp/tmpsO_u7t.mozrunner'; const _SERVER_PORT = '8888'; const _SERVER_ADDR = '127.0.0.1'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;", '-f', '/home/vag/projects/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/testing/mochitest/server.js']

Is there an easy way to access this value from within the test (as this is part of the string that needs to be verified)?
Flags: needinfo?(ntim.bugs)
You should be able to use requestItem.remotePort from here:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/test/head.js#406
Flags: needinfo?(ntim.bugs)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/07e5bbf67548
Hard to distinguish IP from Port in IPv6 address. r=ntim,rickychien
https://hg.mozilla.org/mozilla-central/rev/07e5bbf67548
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.