1150715 - Implement "Copy Request/Response Headers" context menu items

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

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

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'

Comment 2

[Jan Keromnes [:janx]]

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?

Comment 3

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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.

Comment 4

[Jan Keromnes [:janx]]

Ok, makes sense. I'll work on this next.

Comment 5

[Jan Keromnes [:janx]]

Attachment: Implement 'copy request/response headers' context menu items.

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.

Comment 6

[Jan Keromnes [:janx]]

Jeff, is there value in sorting the raw headers alphabetically?

Comment 7

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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.

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

I can review the front end stuff having read and refactored every line if you want

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 10

[Jan Keromnes [:janx]]

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.

Comment 11

[Jan Keromnes [:janx]]

Attachment: Implement "Copy Request/Response Headers" context menu items.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47d4bdf19180

Comment 12

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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!

Comment 13

[Jan Keromnes [:janx]]

Thanks Jeff! However the try run doesn't look so great, I'll investigate.

Comment 14

[Jan Keromnes [:janx]]

Looks like some responses show a slightly different "Last-Modified" time, and some requests also have an extra "Cookie: bob=true; tom=cool" header.

Comment 15

[Jan Keromnes [:janx]]

Attachment: Implement "Copy Request/Response Headers" context menu items.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51c38afd0d9f

Comment 16

[Jan Keromnes [:janx]]

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.

Comment 17

[Jan Keromnes [:janx]]

Attachment: 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.

Comment 18

[Jordan Santell [:jsantell] [@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.

Comment 19

[Jan Keromnes [:janx]]

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.

Comment 20

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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?

Comment 21

[Jan Keromnes [:janx]]

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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

(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

Comment 23

[Jan Keromnes [:janx]]

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.

Comment 24

[Jan Keromnes [:janx]]

Attachment: Implement "Copy Request/Response Headers" context menu items.

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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/def73891e5bf

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/def73891e5bf

Comment 27

[Francesco Lodolo [:flod]]

Any reason for "Copy Request Headers" having an accesskey, while "Copy Response Headers" doesn't have one?

Comment 28

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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