Closed Bug 1052856 Opened 6 years ago Closed 6 years ago

"Copy as curl" should use --compressed instead of -H "accept-encoding gzip..."

Categories

(DevTools :: Netmonitor, defect)

31 Branch
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: bagder, Assigned: retornam, Mentored)

References

(Depends on 1 open bug)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 2 obsolete files)

When using "Copy as curl" on a web page that was sent with compressed content-encoding (like a page here from bugzilla), you'll get -H 'Accept-Encoding: gzip, deflate' included among the options.

That header will lead to curl asking for a compressed response but without the option that asks it to automatically decompress that data. This leads to the user getting "rubbish" (gzipped data) instead of the expected plain html.

curl's --compressed option includes the correct request header _and_ makes it automatically decompress downloaded compressed content.
Mentor: jryans
Whiteboard: [good first bug][lang=js]
hi! I'M new here , Please assign this bug to me!
Hi Abhirav!  For new contributors, we don't assign the bug until a first patch appears, to keep it available for others until then.

Please take a look at our guide[1] to getting started with the code base.

For this specific bug, I imagine we'd need to modify this block[2] to special case the use of compression headers as Daniel mentions in comment 0.

Also, we should update the cURL tests[3] to cover this change.

If you have any questions, you can check "need info" below and fill in my email, or come to the #devtools IRC room.

[1]: https://wiki.mozilla.org/DevTools/Hacking
[2]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/Curl.jsm#116
[3]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/test/browser_net_copy_as_curl.js
Attached patch bug-1052856.patch (obsolete) — Splinter Review
Attachment #8479406 - Flags: feedback?(jryans)
Assignee: nobody → mozbugs.retornam
Comment on attachment 8479406 [details] [diff] [review]
bug-1052856.patch

Review of attachment 8479406 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall!

You'll also need to update the test[1] to cover this change.

[1]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/netmonitor/test/browser_net_copy_as_curl.js

::: browser/devtools/shared/Curl.jsm
@@ +119,5 @@
>        headers = headers.concat(multipartHeaders);
>      }
>      for (let i = 0; i < headers.length; i++) {
>        let header = headers[i];
> +      if (header.name === "Accept-Encoding" || header.name === "Content-Encoding"){

Content-Encoding is a response header, so only Accept-Encoding is needed here.
Attachment #8479406 - Flags: feedback?(jryans) → feedback+
Attached file bug-1052856.patch (obsolete) —
Attachment #8479406 - Attachment is obsolete: true
Attachment #8480066 - Flags: review?(jryans)
Comment on attachment 8480066 [details]
bug-1052856.patch

Review of attachment 8480066 [details]:
-----------------------------------------------------------------

Seems good to me!  Do you have try access to run tests, or should I push it to try for you?
Attachment #8480066 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] from comment #6)
> Comment on attachment 8480066 [details]
> bug-1052856.patch
> 
> Review of attachment 8480066 [details]:
> -----------------------------------------------------------------
> 
> Seems good to me!  Do you have try access to run tests, or should I push it
> to try for you?

I dont have access to try. Please push for me thanks
It looks like the tests are failing on try.  Please take a look at the failures.  Also, make sure the tests are passing on your local machine.
Attachment #8480925 - Flags: review?(jryans)
I fixed the issue.
Attachment #8480066 - Attachment is obsolete: true
Attachment #8480066 - Attachment is patch: false
Comment on attachment 8480925 [details] [diff] [review]
bug-1052856.patch

Review of attachment 8480925 [details] [diff] [review]:
-----------------------------------------------------------------

Hopefully it works this time!

Try: https://tbpl.mozilla.org/?tree=Try&rev=0be411351126
Attachment #8480925 - Flags: review?(jryans) → review+
Try looks good, thanks for working on this!  See the wiki[1] for more DevTools bugs you may be interested in.

[1]: https://wiki.mozilla.org/DevTools/GetInvolved#Mentored_and_Good_First_Bugs
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/8d6dc19c9f94
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8d6dc19c9f94
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 34
Product: Firefox → DevTools
Depends on: 1605540
You need to log in before you can comment on or make changes to this bug.