Closed Bug 1356867 Opened 3 years ago Closed 3 years ago

Add scheme column to netmonitor

Categories

(DevTools :: Netmonitor, enhancement)

enhancement
Not set

Tracking

(firefox55 verified)

VERIFIED FIXED
Firefox 55
Tracking Status
firefox55 --- verified

People

(Reporter: ntim, Assigned: vkatsikaros)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, good-first-bug)

Attachments

(4 files)

Scheme: https/http/ftp/...
Keywords: good-first-bug
In http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/devtools/client/netmonitor/src/utils/filter-text-utils.js#175 I see that the scheme is available via `new URL(url).protocol.replace(":", "").toLowerCase()`. I could go ahead and add the column according to this. However, it looks like an overkill to use this all over the place? Would it make sense to make this available in `item`? If so, what would this involve?
Flags: needinfo?(ntim.bugs)
(In reply to Vangelis Katsikaros from comment #1)
> In
> http://searchfox.org/mozilla-central/rev/
> 4bd7a206dea5382c97a8a0c30beef668cc449f5b/devtools/client/netmonitor/src/
> utils/filter-text-utils.js#175 I see that the scheme is available via `new
> URL(url).protocol.replace(":", "").toLowerCase()`. I could go ahead and add
> the column according to this. However, it looks like an overkill to use this
> all over the place? 
> Would it make sense to make this available in `item`? If
> so, what would this involve?

The `item` is added in netmonitor-controller.js:

http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/netmonitor-controller.js#414


However, I'm not sure we want to add it there, perhaps we want to create an utils function that does that in utils/request-utils.js

Ricky, what would the preferred approach be? Should we add the protocol to every item, use an utils function or something else ?
Flags: needinfo?(ntim.bugs) → needinfo?(rchien)
There is a urlDetails property in `item` instance. urlDetails is assigned at request reducer at requests.js#129 [1], so request-utils.js#182-208 [2] is your entry point to add new scheme object.

[1] http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/reducers/requests.js#129
[2] http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/utils/request-utils.js#182-208
Flags: needinfo?(rchien)
Assignee: nobody → vkatsikaros
Status: NEW → ASSIGNED
Comment on attachment 8863330 [details]
Bug 1356867 - Add scheme column to netmonitor.

https://reviewboard.mozilla.org/r/135112/#review138042

There seems to be an ordering issue. See screenshot.

::: devtools/client/netmonitor/src/components/request-list-item.js:144
(Diff revision 1)
>          columns.get("domain") && RequestListColumnDomain({ item, onSecurityIconClick }),
>          columns.get("remoteip") && RequestListColumnRemoteIP({ item }),
>          columns.get("cause") && RequestListColumnCause({ item, onCauseBadgeClick }),
>          columns.get("type") && RequestListColumnType({ item }),
>          columns.get("cookies") && RequestListColumnCookies({ item }),
> +        columns.get("scheme") && RequestListColumnScheme({ item }),

Can you place this column before domain ?

::: devtools/client/netmonitor/src/constants.js:134
(Diff revision 1)
>    {
>      name: "cause",
>      canFilter: true,
>    },
>    {
> +    name: "scheme",

The order in this file needs to match the one in request-list-item, otherwise the headers will appear in different order than the columns.

(request-list-item probably needs to be refactored to use the list from constants.js rather than its own list, but that's another bug)
Attachment #8863330 - Flags: review?(ntim.bugs)
Attached image ordering issue.png
Comment on attachment 8863330 [details]
Bug 1356867 - Add scheme column to netmonitor.

https://reviewboard.mozilla.org/r/135112/#review138042

> The order in this file needs to match the one in request-list-item, otherwise the headers will appear in different order than the columns.
> 
> (request-list-item probably needs to be refactored to use the list from constants.js rather than its own list, but that's another bug)

Ah, I didn't spot the problem in my original screenshot as well :( Thanks it's fixed now.

Should I open a ticket for the request-list-item.js & constants.js header list rerefactoring or is there one already?
Attached image scheme_fixed.png
Comment on attachment 8863330 [details]
Bug 1356867 - Add scheme column to netmonitor.

https://reviewboard.mozilla.org/r/135112/#review138048

Great, thanks!

::: devtools/client/netmonitor/test/head.js:432
(Diff revision 2)
> +  is(target.querySelector(".requests-list-scheme").textContent,
> +    scheme, "The displayed remote IP is correct.");
> +
> +  is(target.querySelector(".requests-list-scheme").getAttribute("title"),
> +    scheme, "The tooltip remote IP is correct.");

nit: change "remote IP" to "scheme"
Attachment #8863330 - Flags: review?(ntim.bugs) → review+
(In reply to Vangelis Katsikaros from comment #8)
> Should I open a ticket for the request-list-item.js & constants.js header
> list rerefactoring or is there one already?

Just filed one: bug 1361034
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/65b4e02611f0
Add scheme column to netmonitor. r=ntim
https://hg.mozilla.org/mozilla-central/rev/65b4e02611f0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Blocks: 1361426
Keywords: dev-doc-needed
I have reproduced this bug with Nightly 55.0a1 (2017-04-04) on Ubuntu 16.04, 64 bit!

The fix is now verified on Latest Nightly.

Build ID 	20170524100215
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170524]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.