Closed
Bug 1354508
Opened 6 years ago
Closed 6 years ago
Add filter option for network requests checking for a specific response header
Categories
(DevTools :: Netmonitor, enhancement)
Tracking
(firefox55 verified)
VERIFIED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | verified |
People
(Reporter: sebo, Assigned: vkatsikaros)
References
Details
(Keywords: dev-doc-complete, good-first-bug)
Attachments
(2 files)
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•6 years ago
|
Updated•6 years ago
|
Keywords: good-first-bug
Assignee | ||
Comment 1•6 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•6 years ago
|
Assignee: nobody → vkatsikaros
Flags: needinfo?(ntim.bugs)
Comment 2•6 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•6 years ago
|
||
Thanks Tim!
Assignee | ||
Comment 4•6 years ago
|
||
Comment hidden (mozreview-request) |
Comment 6•6 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•6 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•6 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•6 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•6 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•6 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•6 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
![]() |
||
Comment 14•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6a9ce1980dcc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment 15•6 years ago
|
||
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•6 years ago
|
Reporter | ||
Comment 16•6 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)
Reporter | ||
Comment 18•6 years ago
|
||
Great! Thanks for the fast response! Sebastian
Keywords: dev-doc-needed → dev-doc-complete
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•