1356872 - Hard to distinguish IP from Port in IPv6 address

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement)

Description

[Tim Nguyen :ntim]

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

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

We should display it like this:

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

[IP]:Port

Comment 1

[Vangelis Katsikaros]

Attachment: remote_ip.png

Comment 2

[Vangelis Katsikaros]

Attachment: Bug 1356872 - Hard to distinguish IP from Port in IPv6 address.

Review commit: https://reviewboard.mozilla.org/r/130758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130758/

Comment 3

[Tim Nguyen :ntim]

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).

Comment 4

[Vangelis Katsikaros]

Attachment: ip_tooltip_n_header.png

Comment 5

[Vangelis Katsikaros]

Comment on attachment 8858725 [details]
Bug 1356872 - Hard to distinguish IP from Port in IPv6 address.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130758/diff/1-2/

Comment 6

[Vangelis Katsikaros]

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 7

[Tim Nguyen :ntim]

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.

Comment 8

[Vangelis Katsikaros]

Comment on attachment 8858725 [details]
Bug 1356872 - Hard to distinguish IP from Port in IPv6 address.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130758/diff/2-3/

Comment 9

[Ricky Chien [:rickychien] (inactive)]

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

Comment 10

[Tim Nguyen :ntim]

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

Comment 11

[Vangelis Katsikaros]

>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)?

Comment 12

[Tim Nguyen :ntim]

(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.

Comment 13

[Vangelis Katsikaros]

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)?

Comment 14

[Tim Nguyen :ntim]

You should be able to use requestItem.remotePort from here:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/test/head.js#406

Comment 15

[Vangelis Katsikaros]

Comment on attachment 8858725 [details]
Bug 1356872 - Hard to distinguish IP from Port in IPv6 address.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130758/diff/3-4/

Comment 16

[Vangelis Katsikaros]

Comment on attachment 8858725 [details]
Bug 1356872 - Hard to distinguish IP from Port in IPv6 address.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130758/diff/4-5/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07e5bbf67548
Hard to distinguish IP from Port in IPv6 address. r=ntim,rickychien

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/07e5bbf67548