1354508 - Add filter option for network requests checking for a specific response header

Status: VERIFIED FIXED

(DevTools :: Netmonitor, enhancement)

Description

[Sebastian Zartner [:sebo]]

The filter field within the Network Monitor should support a filter option that checks the requests for a specific HTTP header in the response.

The Chrome DevTools provide a 'has-response-header' filter for this purpose.

Sebastian

Comment 1

[Vangelis Katsikaros]

I think I know what to tweak for this bug. Could I have this assigned?

Should I use the same filter name, ie 'has-response-header'?

Comment 2

[Tim Nguyen :ntim]

(In reply to Vangelis Katsikaros from comment #1)
> I think I know what to tweak for this bug. Could I have this assigned?
I've given you permission to assign yourself to bugs, so you can do it on any bug next time :)

> Should I use the same filter name, ie 'has-response-header'?

Yes, has-response-header.

Comment 3

[Vangelis Katsikaros]

Thanks Tim!

Comment 4

[Vangelis Katsikaros]

Attachment: has-response-header.png

Comment 5

[Vangelis Katsikaros]

Attachment: Bug 1354508 - Add filter option for network requests checking for a specific response header.

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

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8858114 [details]
Bug 1354508 - Add filter option for network requests checking for a specific response header.

https://reviewboard.mozilla.org/r/130084/#review132708

Looks good generally, thanks for working on this!

I have a few questions before granting a ship-it.

::: devtools/client/netmonitor/src/utils/filter-text-utils.js:133
(Diff revision 1)
>        break;
>      case "remote-ip":
>        match = `${item.remoteAddress}:${item.remotePort}`.toLowerCase().includes(value);
>        break;
> +    case "has-response-header":
> +      if (typeof item.responseHeaders === "object") {

Does item.hasOwnProperty("responseHeaders") work instead ? or does that omit some cases ?

::: devtools/client/netmonitor/src/utils/filter-text-utils.js:134
(Diff revision 1)
> +        let responseHeaders = item.responseHeaders.headers;
> +        let r = responseHeaders.findIndex((e) => {
> +          return e.name.toLowerCase() === value;
> +        });

You can simplify this a bit:

let { headers } = item.responseHeaders;
match = headers.findIndex(h => h.name.toLowerCase() === value) > -1;

::: devtools/client/netmonitor/src/utils/filter-text-utils.js:134
(Diff revision 1)
> +        let responseHeaders = item.responseHeaders.headers;
> +        let r = responseHeaders.findIndex((e) => {
> +          return e.name.toLowerCase() === value;
> +        });

You can simplify this a bit:

let { headers } = item.responseHeaders;
match = headers.findIndex(h => h.name.toLowerCase() === value) > 0;

::: devtools/client/netmonitor/src/utils/filter-text-utils.js:138
(Diff revision 1)
> +      if (typeof item.responseHeaders === "object") {
> +        let responseHeaders = item.responseHeaders.headers;
> +        let r = responseHeaders.findIndex((e) => {
> +          return e.name.toLowerCase() === value;
> +        });
> +        match = r > 0;

Shouldn't that be `> -1` instead of `> 0`? Otherwise, if the first index is matched, it will return false.

Or am I missing something here (eg. the first item has a special meaning?) ?

Comment 7

[Vangelis Katsikaros]

Comment on attachment 8858114 [details]
Bug 1354508 - Add filter option for network requests checking for a specific response header.

https://reviewboard.mozilla.org/r/130084/#review132708

> Does item.hasOwnProperty("responseHeaders") work instead ? or does that omit some cases ?

This twisted my brain a bit, and I apologize for missing something obvious.

With:

```
@@ -128,6 +129,19 @@ function isFlagFilterMatch(item, { type,
     case "remote-ip":
       match = `${item.remoteAddress}:${item.remotePort}`.toLowerCase().includes(value);
       break;
+    case "has-response-header":
+console.log(         typeof item);
+console.log(         typeof item.responseHeaders);
+console.log(         typeof item.responseHeaders.headers);
+console.log(         typeof item.responseHeaders === "object");
+console.log(item.hasOwnProperty("responseHeaders"));
+      if (item.hasOwnProperty("responseHeaders")) {
+        let { headers } = item.responseHeaders;
+        match = headers.findIndex(h => h.name.toLowerCase() === value) > -1;
+      } else {
+        match = false;
+      }
+      break;
```
I get in the browser console (if I filter the network panel) the output:
```
object  filter-text-utils.js:133:1
object  filter-text-utils.js:134:1
object  filter-text-utils.js:135:1
true  filter-text-utils.js:136:1
false  filter-text-utils.js:137:1
```
with `item.hasOwnProperty("responseHeaders")` giving `false` whereas the key exists in the object.

I initially decided to workaround this, but it seems there is a lesson to be learned here, so I was wondering if I could get some feedback.

> You can simplify this a bit:
> 
> let { headers } = item.responseHeaders;
> match = headers.findIndex(h => h.name.toLowerCase() === value) > -1;

Neater

> Shouldn't that be `> -1` instead of `> 0`? Otherwise, if the first index is matched, it will return false.
> 
> Or am I missing something here (eg. the first item has a special meaning?) ?

Ah! I meant to type ">= 0" (but didn't) :/ However, your suggestion hints more cleanly to the return value of findIndex

Comment 8

[Tim Nguyen :ntim]

Comment on attachment 8858114 [details]
Bug 1354508 - Add filter option for network requests checking for a specific response header.

https://reviewboard.mozilla.org/r/130084/#review132916

r+ with my 2 comments addressed.

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8858114 [details]
Bug 1354508 - Add filter option for network requests checking for a specific response header.

https://reviewboard.mozilla.org/r/130084/#review132708

> This twisted my brain a bit, and I apologize for missing something obvious.
> 
> With:
> 
> ```
> @@ -128,6 +129,19 @@ function isFlagFilterMatch(item, { type,
>      case "remote-ip":
>        match = `${item.remoteAddress}:${item.remotePort}`.toLowerCase().includes(value);
>        break;
> +    case "has-response-header":
> +console.log(         typeof item);
> +console.log(         typeof item.responseHeaders);
> +console.log(         typeof item.responseHeaders.headers);
> +console.log(         typeof item.responseHeaders === "object");
> +console.log(item.hasOwnProperty("responseHeaders"));
> +      if (item.hasOwnProperty("responseHeaders")) {
> +        let { headers } = item.responseHeaders;
> +        match = headers.findIndex(h => h.name.toLowerCase() === value) > -1;
> +      } else {
> +        match = false;
> +      }
> +      break;
> ```
> I get in the browser console (if I filter the network panel) the output:
> ```
> object  filter-text-utils.js:133:1
> object  filter-text-utils.js:134:1
> object  filter-text-utils.js:135:1
> true  filter-text-utils.js:136:1
> false  filter-text-utils.js:137:1
> ```
> with `item.hasOwnProperty("responseHeaders")` giving `false` whereas the key exists in the object.
> 
> I initially decided to workaround this, but it seems there is a lesson to be learned here, so I was wondering if I could get some feedback.

Sounds fine then, dropping this issue.

Comment 10

[Vangelis Katsikaros]

Comment on attachment 8858114 [details]
Bug 1354508 - Add filter option for network requests checking for a specific response header.

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

Comment 11

[Vangelis Katsikaros]

Tim, as far as the process is concerned, do I also need to trigger a try, on top of the push to mozreview?

Also, I would be curious to find an explanation for the hasOwnProperty behavior in https://bugzilla.mozilla.org/show_bug.cgi?id=1354508#c7 , I really can't figure it out :/

Comment 12

[Tim Nguyen :ntim]

(In reply to Vangelis Katsikaros from comment #11)
> Tim, as far as the process is concerned, do I also need to trigger a try, on
> top of the push to mozreview?
This patch should be safe enough for tests, so no need for a try push.

> Also, I would be curious to find an explanation for the hasOwnProperty
> behavior in https://bugzilla.mozilla.org/show_bug.cgi?id=1354508#c7 , I
> really can't figure it out :/

Not sure, probably because responseHeaders is not defined as a property but as a getter or something else.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a9ce1980dcc
Add filter option for network requests checking for a specific response header. r=ntim

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6a9ce1980dcc

Comment 15

[Md. Majedul islam]

I have reproduced this bug with Nightly 55.0a1 (2017-04-07) on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Nightly !

Build ID 	20170427030231
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170426]

Comment 16

[Sebastian Zartner [:sebo]]

Added description of the filter to https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Filtering_by_properties and a developer release note to https://developer.mozilla.org/en-US/Firefox/Releases/55#Developer_Tools.

Vangelis, can you please have a look at the documentation and let me know whether it's ok?

Sebastian

Comment 17

[Vangelis Katsikaros]

Thanks Sebastian, it looks fine.

Comment 18

[Sebastian Zartner [:sebo]]

Great! Thanks for the fast response!

Sebastian