Closed Bug 1150715 Opened 9 years ago Closed 9 years ago

Implement "Copy Request/Response Headers" context menu items

Categories

(DevTools :: Netmonitor, defect, P1)

36 Branch
defect

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)

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
See Also: → 955933
See Also: → 1150717
Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
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)
(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)
Ok, makes sense. I'll work on this next.
Assignee: nobody → janx
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
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.
Attachment #8593945 - Flags: review?(vporof)
Jeff, is there value in sorting the raw headers alphabetically?
Flags: needinfo?(jgriffiths)
(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)
I can review the front end stuff having read and refactored every line if you want
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)
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.
(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!
Thanks Jeff! However the try run doesn't look so great, I'll investigate.
Looks like some responses show a slightly different "Last-Modified" time, and some requests also have an extra "Cookie: bob=true; tom=cool" header.
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.
Should be green now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7dc06ad0642

Jordan, please have a look.
Attachment #8595411 - Attachment is obsolete: true
Attachment #8595443 - Flags: review?(jsantell)
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+
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
(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?
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.
Blocks: 1158046
Summary: Implement 'copy request/response headers' context menu items → Implement "Copy Request/Response Headers" context menu items
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
(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
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.
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
Attachment #8598681 - Flags: review+
Keywords: checkin-needed
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]
https://hg.mozilla.org/mozilla-central/rev/def73891e5bf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=easy][fixed-in-fx-team] → [devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 40
Any reason for "Copy Request Headers" having an accesskey, while "Copy Response Headers" doesn't have one?
(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
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.