Closed Bug 1150717 Opened 5 years ago Closed 5 years ago

Implement "Copy URL Parameters" context menu item

Categories

(DevTools :: Netmonitor, defect, P1)

36 Branch
x86
macOS
defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: canuckistani, Assigned: janx)

References

Details

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

Attachments

(2 files, 3 obsolete files)

A gap from Firebug.

Sample result: 

URL: 
http://fonts.googleapis.com/css?family=Merriweather:900,900italic,300,300italic

Copied text: 
family=Merriweather:900,900italic,300,300italic
Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
Assignee: nobody → janx
Status: NEW → ASSIGNED
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
Brian, please have a look. This feature is available in the context menu of a Network Monitor item, but only if there actually is a query string in the request URL.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2fb30d245dd
Attachment #8596650 - Attachment is obsolete: true
Comment on attachment 8597246 [details] [diff] [review]
Implement "Copy URL Parameters" context menu item.

Notes:
- I use "URL Parameters" and "Query String" interchangeably (the word "Parameters" comes from Firebug).
- Maybe we'll also want to add a "Copy POST Parameters" feature (form data), that's also a Firebug gap.
- The try run contains a dependency patch from bug 1150715, which is still in flight.
Attachment #8597246 - Flags: review?(bgrinstead)
Summary: Implement 'copy url parameters' context menu item → Implement "Copy URL Parameters" context menu item
Depends on: 1150715
Note: The problem with bug 1150715 is Windows line endings. I'll rebase this patch eventually, but it's not affected so it won't really change; leaving the review flag.
Comment on attachment 8597246 [details] [diff] [review]
Implement "Copy URL Parameters" context menu item.

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

Looks good, please rebase the patch and add the extra test cases to make sure the menu item is in sync with the selected request

::: browser/devtools/netmonitor/test/browser_net_copy_params.js
@@ +14,5 @@
> +
> +    RequestsMenu.lazyUpdate = false;
> +
> +    Task.spawn(function () {
> +      yield waitForNetworkEvents(aMonitor, 0, 6);

This should also test a URLs in which nothing can be copied, making sure the menu item is hidden.  And also make sure the menu item is visible when it should be.
Attachment #8597246 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #6)
> Looks good, please rebase the patch and add the extra test cases to make
> sure the menu item is in sync with the selected request

Thanks! I'll rebase and improve the test.

> > +    Task.spawn(function () {
> > +      yield waitForNetworkEvents(aMonitor, 0, 6);
> 
> This should also test a URLs in which nothing can be copied, making sure the
> menu item is hidden.  And also make sure the menu item is visible when it
> should be.

Ah, good point, I'll look into this as well.
Attachment #8597246 - Attachment is obsolete: true
Attachment #8602106 - Flags: review?(bgrinstead)
Attachment #8602107 - Flags: review?(bgrinstead)
Rebased and added no-params test case to both "Copy URL Parameters", and "Params panel" test in a separate patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d31ba611c33
Blocks: 1158046
(In reply to Jan Keromnes [:janx] from comment #10)
>  aadded no-params test case to both "Copy URL Parameters", and
> "Params panel" test in a separate patch.

Does the test case in Attachment 8602106 [details] [diff] cover the "Copy URL Parameters" case?  It looks like it's just covering params panel.
Flags: needinfo?(janx)
(In reply to Brian Grinstead [:bgrins] from comment #11)
> (In reply to Jan Keromnes [:janx] from comment #10)
> >  aadded no-params test case to both "Copy URL Parameters", and
> > "Params panel" test in a separate patch.
> 
> Does the test case in Attachment 8602106 [details] [diff] cover the "Copy
> URL Parameters" case?  It looks like it's just covering params panel.

Never mind, I see the case is in the other patch - I just had trouble parsing that sentence.
Flags: needinfo?(janx)
Attachment #8602107 - Flags: review?(bgrinstead) → review+
Comment on attachment 8602106 [details] [diff] [review]
Test request with no params in the Network Monitor.

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

This patch needs to land first or the two patches should be folded together, because browser_net_copy_params.js from the other patch relies on the changes html_params-test-page.html
Attachment #8602106 - Flags: review?(bgrinstead) → review+
Thanks Brian!

Try (same link as before) https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d31ba611c33
Keywords: checkin-needed
Took a look at the try build, looks great.

We have a slight difference with Firebug, that the example I provided for you did not account for. Sorry!

Sample URL: 

https://secure.gravatar.com/avatar/f133142b5dac380727077d43731802d0?d=mm&size=64

On Firebug:

d=mm
size=64

In Firefox with this patch applied: 

d=mm&size=64

I'm needinfo'ing Honza: can we get an opinion / some context as to why Firebug splits the params out? I don't think we need it frankly. I think you should ship this as-is.
Flags: needinfo?(odvarko)
Now splitting query string parameters onto new lines, to keep Firebug's behavior.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb12661a94f7
Attachment #8602107 - Attachment is obsolete: true
Comment on attachment 8602535 [details] [diff] [review]
Implement "Copy URL Parameters" context menu item.

(keeping Brian's r+)
Attachment #8602535 - Flags: review+
(In reply to Jeff Griffiths (:canuckistani) from comment #15)
> I'm needinfo'ing Honza: can we get an opinion / some context as to why
> Firebug splits the params out? I don't think we need it frankly.
It's just that it improves readability when every param
is on its own line.

Honza
Flags: needinfo?(odvarko)
I think I've seen options like "Copy URL" and "Copy URL with Parameters" in similar tools, which could also be useful.

Also I think Pulsebot forgot [fixed-in-fx-team].
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
(In reply to Jan Keromnes [:janx] from comment #20)
> I think I've seen options like "Copy URL" and "Copy URL with Parameters" in
> similar tools, which could also be useful.
Yep. Firebug has "Copy Location" (i.e. copy URL) that copies the entire URL including parameters. Of course params wouldn't be parsed out in this case.

Honza
https://hg.mozilla.org/mozilla-central/rev/47643a12ed87
https://hg.mozilla.org/mozilla-central/rev/ecf299ff9d4b
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
Jeff, I just noticed your message on IRCC:

> [18:24] canuckistani [#devtools] Honza: wrt copying url parameters - I guess the use case I had in
> mind was that I'd want to copy the parameters of a request from say a Live site and then use them 
> against a local server, so I wouldn't want them split up like that
> [18:24] canuckistani [#devtools] Honza: but I suspected that somewhere in Firebug's history there
> were reasons for doing it the way it is

Yes, I am also seeing two main use cases:

1. Copy URL params for inspection
Params are parsed out of the URL and nicely separated by new lines. This allows the user to paste them into any editor or directly into a document. It's nicely readable and formatted for humans.

2. Copy Location (aka Copy URL) for further processing
The entire URL include params (that are kept in the original form) and copied. This allows the user to pass it further in to another tool (cut the URL from the params is simple no special action is necessary, never requested anyway). This output targets further processing by machines.

3. Copy as cURL
This is special version of #2, requested by users.

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