Closed Bug 1356872 Opened 8 years ago Closed 8 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
Status: ASSIGNED → RESOLVED
Closed: 8 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.

Attachment

General

Created:
Updated:
Size: