Closed Bug 955933 Opened 6 years ago Closed 5 years ago

Allow copying the network response string

Categories

(DevTools :: Netmonitor, defect, P1)

x86
macOS
defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: mossop, Assigned: gl)

References

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(1 file, 4 obsolete files)

After a JSON request I can see the JSON data in the right panel of the network monitor. Clicking a value opens a text box with the value selected. Right clicking shows a menu with the usual edit commands. Clicking copy doesn't add the value to the clipboard though.
This patch adds a "Copy Response as String" item to the network request context menu.
The item is hidden if the response is not ready yet or doesn't have a text representation.
Comment on attachment 8443939 [details] [diff] [review]
Allow copying the network response string.

Hello Victor, can you please have a look at this? Thanks!
Attachment #8443939 - Flags: review?(vporof)
Comment on attachment 8443939 [details] [diff] [review]
Allow copying the network response string.

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

Looks good! Please add a test as well. They are in devtools/netmonitor/test, and browser_net_copy_url.js looks like a good one to clone.
Attachment #8443939 - Flags: review?(vporof) → feedback+
Duplicate of this bug: 1048705
Attached patch 955933.patch (obsolete) — Splinter Review
Rebased and added a test case from the original patch, which implements a context menu item for copying the response.

The original problem described by mossop in comment 1 seems to be working for me since I can click copy the value from the context menu of a value's textbox in the json inspector in the network response.
Attachment #8443939 - Attachment is obsolete: true
Attachment #8584200 - Flags: review?(vporof)
Attachment #8584200 - Flags: review?(vporof) → review+
Assignee: nobody → gabriel.luong
Summary: Can't copy values from the JSON inspector → Allow copying the network response string
Attached patch 955933.patch (obsolete) — Splinter Review
Rebased browser.ini 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa5c129d9682
Attachment #8584200 - Attachment is obsolete: true
Attachment #8586439 - Flags: review+
Keywords: checkin-needed
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2505119&repo=fx-team
Flags: needinfo?(gabriel.luong)
Whiteboard: [fixed-in-fx-team]
Why are you using an external accesskey (G), when the label is "Copy Response"?
(In reply to Francesco Lodolo [:flod] (UTC+1) from comment #9)
> Why are you using an external accesskey (G), when the label is "Copy
> Response"?

I mainly kept it from the original patch. What would you advise?
Flags: needinfo?(gabriel.luong)
It's an accesskey, you should use a character available in the string unless all of them are already taken.
Blocks: 1132119
Whiteboard: [devedition-40]
Attached patch 955933.patch (obsolete) — Splinter Review
Couldn't reproduce the error locally. I changed the access key for copyResponse ("R") and an existing accesskey for EditAndResend ("E", previously "R").
Attachment #8586439 - Attachment is obsolete: true
(In reply to Gabriel Luong (:gl) from comment #13)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b323060e45f

Thanks - works really well!
Comment on attachment 8588947 [details] [diff] [review]
955933.patch

@vporof: Not quite sure if this sure needs another review, but only thing that changed was the access key. I couldn't reproduce the error seen when the patch was landed in fx-team and try build is green again. Would you advise trying to land the patch again?

@flod: is there any localization issue if an accesskey was simply swapped out for another?
Flags: needinfo?(francesco.lodolo)
Attachment #8588947 - Flags: review?(vporof)
(In reply to Gabriel Luong (:gl) from comment #15)
> @flod: is there any localization issue if an accesskey was simply swapped
> out for another?

No, you're free to swap and reorganize accesskeys as you need, each locale should pick their own accesskeys and check for conflicts.

The only border case is when you move a string into a completely different context (e.g. you move a preference from one panel to another), and that might create unexpected conflicts, but it's not what happened here.
Flags: needinfo?(francesco.lodolo)
Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
Attachment #8588947 - Flags: review?(vporof) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/f6f3ab17d638
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [devedition-40] → [devedition-40][fixed-in-fx-team]
Backed out for browser_net_copy_as_curl.js failures in e10s mode.
https://hg.mozilla.org/integration/fx-team/rev/a811b79720ac

4675 INFO TEST-UNEXPECTED-FAIL | browser/devtools/netmonitor/test/browser_net_copy_as_curl.js | Timed out while polling clipboard for pasted data. -
Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:wait:910
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForClipboard/wait/<:927
null:null:0
4676 INFO TEST-UNEXPECTED-FAIL | browser/devtools/netmonitor/test/browser_net_copy_as_curl.js | Creating a cURL command for the currently selected item was unsuccessful. -
Stack trace:
chrome://mochitests/content/browser/browser/devtools/netmonitor/test/browser_net_copy_as_curl.js:onFailure:61
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:wait:912
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForClipboard/wait/<:927
null:null:0
Whiteboard: [devedition-40][fixed-in-fx-team] → [devedition-40]
Attached patch 955933.patchSplinter Review
It appears that I added the new test between the curl-test and its skip-if declaration and thus enabled the already failing curl test for e10s. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b683bf273760
Attachment #8588947 - Attachment is obsolete: true
Attachment #8593178 - Flags: review+
Keywords: checkin-needed
Status: NEW → ASSIGNED
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/033d7036b755
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=easy][fixed-in-fx-team] → [devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 40
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
This is somewhat arbitrary place from where to copy the response, I was looking for it on the Response tab. I found it in the context menu of the _Request_ line only after reading this bug :)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.