Closed
Bug 1356872
Opened 8 years ago
Closed 8 years ago
Hard to distinguish IP from Port in IPv6 address
Categories
(DevTools :: Netmonitor, enhancement)
DevTools
Netmonitor
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 | ||
Updated•8 years ago
|
Assignee: nobody → vkatsikaros
Assignee | ||
Comment 1•8 years ago
|
||
Comment hidden (mozreview-request) |
Reporter | ||
Comment 3•8 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8858718 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
mozreview-review |
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.
Reporter | ||
Comment 7•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 9•8 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 10•8 years ago
|
||
mozreview-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+
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 11•8 years ago
|
||
>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)
Reporter | ||
Comment 12•8 years ago
|
||
(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)
Assignee | ||
Comment 13•8 years ago
|
||
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)
Reporter | ||
Comment 14•8 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•8 years ago
|
||
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
Comment 18•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•