1150717 - Implement "Copy URL Parameters" context menu item

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

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

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

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]]

Attachment: Implement "Copy URL Parameters" context menu item. r=

WIP

Comment 3

[Jan Keromnes [:janx]]

Attachment: Implement "Copy URL Parameters" context menu item.

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

Comment 4

[Jan Keromnes [:janx]]

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.

Comment 5

[Jan Keromnes [:janx]]

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 6

[Brian Grinstead [:bgrins]]

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.

Comment 7

[Jan Keromnes [:janx]]

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

Comment 8

[Jan Keromnes [:janx]]

Attachment: Test request with no params in the Network Monitor.

Comment 9

[Jan Keromnes [:janx]]

Attachment: Implement "Copy URL Parameters" context menu item.

Comment 10

[Jan Keromnes [:janx]]

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

Comment 11

[Brian Grinstead [:bgrins]]

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

Comment 12

[Brian Grinstead [:bgrins]]

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

Comment 13

[Brian Grinstead [:bgrins]]

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

Comment 14

[Jan Keromnes [:janx]]

Thanks Brian!

Try (same link as before) https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d31ba611c33

Comment 15

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

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.

Comment 16

[Jan Keromnes [:janx]]

Attachment: Implement "Copy URL Parameters" context menu item.

Now splitting query string parameters onto new lines, to keep Firebug's behavior.

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

Comment 17

[Jan Keromnes [:janx]]

Comment on attachment 8602535 [details] [diff] [review]
Implement "Copy URL Parameters" context menu item.

(keeping Brian's r+)

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/47643a12ed87
https://hg.mozilla.org/integration/fx-team/rev/ecf299ff9d4b

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 20

[Jan Keromnes [:janx]]

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

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/47643a12ed87
https://hg.mozilla.org/mozilla-central/rev/ecf299ff9d4b

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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