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

VERIFIED FIXED in Firefox 55

Status

enhancement
VERIFIED FIXED
2 years ago
11 months ago

People

(Reporter: sebo, Assigned: vkatsikaros)

Tracking

(Blocks 1 bug, {dev-doc-complete, good-first-bug})

55 Branch
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

Attachments

(2 attachments)

Reporter

Description

2 years ago
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
Reporter

Updated

2 years ago
See Also: → 1354509
Reporter

Updated

2 years ago
Blocks: netmonitor-filtering
No longer blocks: 1041895

Updated

2 years ago
Keywords: good-first-bug
Assignee

Comment 1

2 years ago
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'?
Flags: needinfo?(ntim.bugs)

Updated

2 years ago
Assignee: nobody → vkatsikaros
Flags: needinfo?(ntim.bugs)

Comment 2

2 years ago
(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.
Assignee

Comment 3

2 years ago
Thanks Tim!
Assignee

Comment 4

2 years ago

Comment 6

2 years ago
mozreview-review
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?) ?
Assignee

Comment 7

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review
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.
Attachment #8858114 - Flags: review?(ntim.bugs) → review+

Comment 9

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Assignee

Comment 11

2 years ago
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 :/
Flags: needinfo?(ntim.bugs)

Comment 12

2 years ago
(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.
Flags: needinfo?(ntim.bugs)

Comment 13

2 years ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6a9ce1980dcc
Add filter option for network requests checking for a specific response header. r=ntim
https://hg.mozilla.org/mozilla-central/rev/6a9ce1980dcc
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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]

Updated

2 years ago
Status: RESOLVED → VERIFIED
Keywords: dev-doc-needed
Reporter

Comment 16

2 years ago
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
Flags: needinfo?(vkatsikaros)
Assignee

Comment 17

2 years ago
Thanks Sebastian, it looks fine.
Flags: needinfo?(vkatsikaros)
Reporter

Comment 18

2 years ago
Great! Thanks for the fast response!

Sebastian

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.