Closed
Bug 1150715
Opened 9 years ago
Closed 9 years ago
Implement "Copy Request/Response Headers" context menu items
Categories
(DevTools :: Netmonitor, defect, P1)
Tracking
(firefox40 fixed)
RESOLVED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: canuckistani, Assigned: janx)
References
(Blocks 1 open bug)
Details
(Whiteboard: [polish-backlog][difficulty=easy])
Attachments
(1 file, 4 obsolete files)
11.97 KB,
patch
|
janx
:
review+
|
Details | Diff | Splinter Review |
Feature from Firebug's net panel. Sample result: Request: HTTP/1.1 200 OK Server: cloudflare-nginx Date: Thu, 02 Apr 2015 22:01:49 GMT Content-Type: text/css Last-Modified: Mon, 26 Jan 2015 04:44:37 GMT Vary: Accept-Encoding Etag: W/"54c5c635-b3a" Expires: Thu, 23 Jan 2025 07:37:31 GMT Cache-Control: public, max-age=309605742 cf-cache-status: HIT cf-ray: 1d0fd083999a1bd9-SEA Content-Encoding: gzip X-Firefox-Spdy: 3.1 Response HTTP/1.1 200 OK Server: cloudflare-nginx Date: Thu, 02 Apr 2015 22:01:49 GMT Content-Type: text/css Last-Modified: Mon, 26 Jan 2015 04:44:37 GMT Vary: Accept-Encoding Etag: W/"54c5c635-b3a" Expires: Thu, 23 Jan 2025 07:37:31 GMT Cache-Control: public, max-age=309605742 cf-cache-status: HIT cf-ray: 1d0fd083999a1bd9-SEA Content-Encoding: gzip X-Firefox-Spdy: 3.1
Reporter | ||
Comment 1•9 years ago
|
||
Assigning P1 priority for some devedition-40 bugs. Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
Assignee | ||
Comment 2•9 years ago
|
||
Jeff, what's the use case for this? Request headers can already be copied with the `curl` command, and I'm wondering what the uses are for copying request/response headers as plain text. Is it for describing network-related bugs? Or spoofing requests?
Flags: needinfo?(jgriffiths)
Reporter | ||
Comment 3•9 years ago
|
||
(In reply to Jan Keromnes [:janx] from comment #2) > Jeff, what's the use case for this? Request headers can already be copied > with the `curl` command, and I'm wondering what the uses are for copying > request/response headers as plain text. Is it for describing network-related > bugs? Or spoofing requests? Copy as curl creates a giant string with no line-breaks, causing the user to have to parse the string in order to tease out the headers just to look at them. This is a little different - I think the use case is, as a developer I sometimes need to copy out headers and use them in scripts / tools aside from curl. For some context, this is a feature from Firebug that Chrome also implements. When faced with the prospect of no longer being able to use Firebug, users from that community complain that utilities like this one are missing from our devtools but are available in Firefbug and chrome.
Blocks: firebug-gaps
Flags: needinfo?(jgriffiths)
Assignee | ||
Comment 4•9 years ago
|
||
Ok, makes sense. I'll work on this next.
Assignee: nobody → janx
Status: NEW → ASSIGNED
Assignee | ||
Updated•9 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 5•9 years ago
|
||
I took a quick shot at this, Victor please have a look. Example Request Headers: GET /en-US/docs/Web/CSS/transform HTTP/1.1 Host: developer.mozilla.org User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0 Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8 Accept-Language: en-US,en;q=0.5 Accept-Encoding: gzip, deflate Cookie: optimizelySegments=%7B%22245875585%22%3A%22direct%22%2C%22245617832%22%3A%22none%22%2C%22246048108%22%3A%22false%22%2C%22245677587%22%3A%22ff%22%2C%22869421433%22%3A%22true%22%2C%22237061344%22%3A%22none%22%2C%22237335298%22%3A%22search%22%2C%22743670347%22%3A%22true%22%2C%22237485170%22%3A%22false%22%2C%22237321400%22%3A%22ff%22%2C%22704844240%22%3A%22true%22%2C%221867940538%22%3A%22true%22%2C%22697533825%22%3A%22true%22%7D; optimizelyEndUserId=oeu1407766779101r0.14400902268863291; optimizelyBuckets=%7B%222706700712%22%3A%222706010241%22%2C%222702600192%22%3A%222709140171%22%7D; __utma=150903082.369776409.1407766779.1426850783.1428944244.6; __utmz=150903082.1426850783.5.2.utmcsr=google|utmccn=(organic)|utmcmd=organic|utmctr=(not%20provided); csrftoken=eSjVCIblTWcrI9Hnchol7Ci3AH7qQmKY; _ga=GA1.2.369776409.1407766779; _gat=1; optimizelyPendingLogEvents=%5B%5D Connection: keep-alive If-None-Match: "3ad6b821d7708577f72d59262d04618e08851636" Cache-Control: max-age=0 Example Response Headers: HTTP/1.1 200 OK Server: Apache Vary: Cookie, Accept-Encoding X-Backend-Server: developer1.webapp.scl3.mozilla.com Content-Type: text/html; charset=utf-8 Access-Control-Allow-Credentials: false Content-Encoding: gzip Date: Fri, 17 Apr 2015 12:59:57 GMT Keep-Alive: timeout=5, max=977 X-kuma-revision: 787162 Transfer-Encoding: chunked Access-Control-Allow-Origin: * Etag: "3ad6b821d7708577f72d59262d04618e08851636" Connection: Keep-Alive X-Frame-Options: DENY Last-Modified: Fri, 17 Apr 2015 01:14:48 GMT Access-Control-Allow-Methods: GET X-Cache-Info: not cacheable; response had too large vary data Before going forward: - I'm unsure about stripping the "\r" from the rawHeaders (Windows probably needs them), but gNetwork.getString() didn't seem to do anything useful. - I could sort the headers (by regenerating raw headers from the headers array, mapping it to LocaleCompare and formatting as plain text). Would that be better? - I'll add a test.
Assignee | ||
Updated•9 years ago
|
Attachment #8593945 -
Flags: review?(vporof)
Assignee | ||
Comment 6•9 years ago
|
||
Jeff, is there value in sorting the raw headers alphabetically?
Flags: needinfo?(jgriffiths)
Reporter | ||
Comment 7•9 years ago
|
||
(In reply to Jan Keromnes [:janx] from comment #6) > Jeff, is there value in sorting the raw headers alphabetically? No, I think from the user's perspective it is important that they headers are copied exactly as they are emitted by the server. Also - Victor is on PTO this and next week, you could see about review from someone else.
Flags: needinfo?(jgriffiths)
Comment 8•9 years ago
|
||
I can review the front end stuff having read and refactored every line if you want
Comment 9•9 years ago
|
||
Comment on attachment 8593945 [details] [diff] [review] Implement 'copy request/response headers' context menu items. Review of attachment 8593945 [details] [diff] [review]: ----------------------------------------------------------------- Stealing this review. Looks good, some comments below which I think will introduce race conditions in some places, especially in large file downloads, or stubbing tests. Should add a test for copyRequestHeaders/copyResponseHeaders functions, as I suspect removing \r will only work on windows. The other context menu tests are pretty similar so should be pretty straight forward ::: browser/devtools/netmonitor/netmonitor-view.js @@ +596,5 @@ > + * Copy the raw request headers from the currently selected item. > + */ > + copyRequestHeaders: function() { > + let selected = this.selectedItem.attachment; > + let rawHeaders = selected.requestHeaders.rawHeaders.replace(/\r/g, "").trim(); Should probably replace \n too for this and the copyResponseHeaders @@ +1776,5 @@ > + let copyRequestHeadersElement = $("#request-menu-context-copy-request-headers"); > + copyRequestHeadersElement.hidden = !selectedItem || !selectedItem.attachment.responseContent; > + > + let copyResponseHeadersElement = $("#response-menu-context-copy-response-headers"); > + copyResponseHeadersElement.hidden = !selectedItem || !selectedItem.attachment.responseContent; Shouldn't these menu conditionals be based on `selectedItem.attachment.requestHeaders` and `selectedItem.attachment.responseHeaders`? If the response is a giant file, for example, I believe responseContent won't exist until everything is streamed in and the request is finalized, but we should be able to copy request headers almost immediately, and response headers after the first few bytes come back in the response.
Attachment #8593945 -
Flags: review?(vporof)
Assignee | ||
Comment 10•9 years ago
|
||
Thanks Jordan! > Should add a test for copyRequestHeaders/copyResponseHeaders functions Yes, I'm planning to add tests for these, as mentioned in comment 5. > ::: browser/devtools/netmonitor/netmonitor-view.js > @@ +596,5 @@ > > + * Copy the raw request headers from the currently selected item. > > + */ > > + copyRequestHeaders: function() { > > + let selected = this.selectedItem.attachment; > > + let rawHeaders = selected.requestHeaders.rawHeaders.replace(/\r/g, "").trim(); > > Should probably replace \n too for this and the copyResponseHeaders Perhaps it's not obvious what I'm trying to do here. On linux systems, when you copy rawHeaders as is, you get something like: > HTTP/1.1 200 OK > > Server: Apache > > Vary: Cookie, Accept-Encoding > because the lines are separated by \r\n, producing a double line break. However, I guess I should probably leave the \r on Windows (that's what I wanted to confirm in this review). On the other hand, replacing both \r and \n would produce an output like: > HTTP/1.1 200 OKServer: ApacheVary: Cookie, Accept-Encoding which is not useful. > @@ +1776,5 @@ > > + let copyRequestHeadersElement = $("#request-menu-context-copy-request-headers"); > > + copyRequestHeadersElement.hidden = !selectedItem || !selectedItem.attachment.responseContent; > > + > > + let copyResponseHeadersElement = $("#response-menu-context-copy-response-headers"); > > + copyResponseHeadersElement.hidden = !selectedItem || !selectedItem.attachment.responseContent; > > Shouldn't these menu conditionals be based on > `selectedItem.attachment.requestHeaders` and > `selectedItem.attachment.responseHeaders`? If the response is a giant file, > for example, I believe responseContent won't exist until everything is > streamed in and the request is finalized, but we should be able to copy > request headers almost immediately, and response headers after the first few > bytes come back in the response. Good point! I wanted to wait for everything to complete before allowing to copy headers, but there is actually no point in waiting, so I'll use your suggestion.
Assignee | ||
Comment 11•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=47d4bdf19180
Attachment #8593945 -
Attachment is obsolete: true
Reporter | ||
Comment 12•9 years ago
|
||
(In reply to Jan Keromnes [:janx] from comment #11) > Created attachment 8594838 [details] [diff] [review] > Implement "Copy Request/Response Headers" context menu items. > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=47d4bdf19180 Looks great!
Assignee | ||
Comment 13•9 years ago
|
||
Thanks Jeff! However the try run doesn't look so great, I'll investigate.
Assignee | ||
Comment 14•9 years ago
|
||
Looks like some responses show a slightly different "Last-Modified" time, and some requests also have an extra "Cookie: bob=true; tom=cool" header.
Assignee | ||
Comment 15•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51c38afd0d9f
Attachment #8594838 -
Attachment is obsolete: true
Assignee | ||
Comment 16•9 years ago
|
||
Got "Last-Modified" this time, but adding "run-sequentially" to browser.ini didn't help with the left-over "Cookie" header. I guess I'll have to remove it in the test.
Assignee | ||
Comment 17•9 years ago
|
||
Should be green now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7dc06ad0642 Jordan, please have a look.
Attachment #8595411 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8595443 -
Flags: review?(jsantell)
Comment 18•9 years ago
|
||
Comment on attachment 8595443 [details] [diff] [review] Implement "Copy Request/Response Headers" context menu items. Review of attachment 8595443 [details] [diff] [review]: ----------------------------------------------------------------- Some notes, but looks good! ::: browser/locales/en-US/chrome/browser/devtools/netmonitor.dtd @@ +295,5 @@ > +<!ENTITY netmonitorUI.context.copyResponseHeaders.accesskey "H"> > + > +<!-- LOCALIZATION NOTE (netmonitorUI.context.copyRequestHeaders): This is the label displayed > + - on the context menu that copies the selected item's request headers --> > +<!ENTITY netmonitorUI.context.copyRequestHeaders "Copy Request Headers"> Does this need an accessKey as well? Not sure what that really does.
Attachment #8595443 -
Flags: review?(jsantell) → review+
Assignee | ||
Comment 19•9 years ago
|
||
Thanks Jordan! (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #18) > > +<!ENTITY netmonitorUI.context.copyRequestHeaders "Copy Request Headers"> > > Does this need an accessKey as well? Not sure what that really does. Probably not, I've assigned an accessKey (H) only to "Copy Response Headers", arguably the most useful set of headers. Please let me know if you strongly feel that "Copy Request Headers" also needs an accessKey.
Keywords: checkin-needed
Reporter | ||
Comment 20•9 years ago
|
||
(In reply to Jan Keromnes [:janx] from comment #19) > Thanks Jordan! > > (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #18) > > > +<!ENTITY netmonitorUI.context.copyRequestHeaders "Copy Request Headers"> > > > > Does this need an accessKey as well? Not sure what that really does. > > Probably not, I've assigned an accessKey (H) only to "Copy Response > Headers", arguably the most useful set of headers. Please let me know if you > strongly feel that "Copy Request Headers" also needs an accessKey. Response headers are not necessarily the most useful headers - request headers contain important bits like user agent, cache-control, pragma, accept*. The use case for copying request headers is to copy them into a script, make modifications to debug server headers, output & cache configuration. I think there's an overlap here with copy as curl, but the thing I like about using copied request headers like this is you could just keep them as-is in a text file where they're easily editable and have a script that uses them. I had someone demo for me this exact script, and they claimed ( I think correctly ) that if you need to repeatedly hit a url and vary the headers this is much nicer than using the curl command line we generate. What do you think of a modifier key + H for one?
Assignee | ||
Comment 21•9 years ago
|
||
Thanks for your input Jeff, the use case you mention does make a lot of sense. I'll try to add accessKeys for all the context menu items in a follow-up.
> What do you think of a modifier key + H for one?
I don't think that's possible for context menu access keys. Maybe I'm wrong, but there are also plenty of other letters we could use.
Assignee | ||
Updated•9 years ago
|
Summary: Implement 'copy request/response headers' context menu items → Implement "Copy Request/Response Headers" context menu items
Assignee | ||
Updated•9 years ago
|
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
Comment 22•9 years ago
|
||
(In reply to Jan Keromnes [:janx] from comment #17) > Created attachment 8595443 [details] [diff] [review] > Implement "Copy Request/Response Headers" context menu items. > > Should be green now: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7dc06ad0642 > > Jordan, please have a look. https://treeherder.mozilla.org/logviewer.html#?job_id=6812956&repo=try 5040 INFO TEST-UNEXPECTED-FAIL | browser/devtools/netmonitor/test/browser_net_copy_headers.js | Clipboard contains the currently selected item's request headers. - Got GET http://example.com/browser/browser/devtools/netmonitor/test/html_simple-test-page.html HTTP/1.1 5041 INFO TEST-UNEXPECTED-FAIL | browser/devtools/netmonitor/test/browser_net_copy_headers.js | Clipboard contains the currently selected item's response headers. - Got HTTP/1.1 200 OK
Keywords: checkin-needed
Assignee | ||
Comment 23•9 years ago
|
||
Windows-only breakage on try: My code puts a string containing "\r\n" into the clipboard, but when it comes back in tests all the "\r" are gone. I'll have a look and see if just "\n" line endings are ok on Windows.
Assignee | ||
Comment 24•9 years ago
|
||
The Windows problem only affected the tests. I fixed the test and I'll keep Jordan's r+. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e654a4eb397e Details: On Windows, copying to clipboard requires line endings to be "\r\n", or else pasting into Notepad will input a single, unbroken line. However, in tests, getting the clipboard content seems to replace "\r\n" endings with "\n". I simply changed the test to also expect "\n" instead of "\r\n" on Windows.
Attachment #8595443 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8598681 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 25•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/def73891e5bf
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
Comment 26•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/def73891e5bf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=easy][fixed-in-fx-team] → [devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 40
Comment 27•9 years ago
|
||
Any reason for "Copy Request Headers" having an accesskey, while "Copy Response Headers" doesn't have one?
Reporter | ||
Comment 28•9 years ago
|
||
(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #27) > Any reason for "Copy Request Headers" having an accesskey, while "Copy > Response Headers" doesn't have one? There is a follow-up bug to address this bug 1158046
Reporter | ||
Updated•8 years ago
|
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•