1356867 - Add scheme column to netmonitor

Status: VERIFIED FIXED

(DevTools :: Netmonitor, enhancement)

Description

[Tim Nguyen :ntim]

Scheme: https/http/ftp/...

Comment 1

[Vangelis Katsikaros]

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?

Comment 2

[Tim Nguyen :ntim]

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

Comment 3

[Ricky Chien [:rickychien] (inactive)]

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

Comment 4

[Vangelis Katsikaros]

Attachment: scheme-column-n-filter.png

Comment 5

[Vangelis Katsikaros]

Attachment: Bug 1356867 - Add scheme column to netmonitor.

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

Comment 6

[Tim Nguyen :ntim]

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)

Comment 7

[Tim Nguyen :ntim]

Attachment: ordering issue.png

Comment 8

[Vangelis Katsikaros]

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?

Comment 9

[Vangelis Katsikaros]

Comment on attachment 8863330 [details]
Bug 1356867 - Add scheme column to netmonitor.

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

Comment 10

[Vangelis Katsikaros]

Attachment: scheme_fixed.png

Comment 11

[Tim Nguyen :ntim]

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"

Comment 12

[Tim Nguyen :ntim]

(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

Comment 13

[Vangelis Katsikaros]

Comment on attachment 8863330 [details]
Bug 1356867 - Add scheme column to netmonitor.

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65b4e02611f0
Add scheme column to netmonitor. r=ntim

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/65b4e02611f0

Comment 16

[Md. Majedul islam]

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]