1005870 - Add `copy` command to console

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Brian Grinstead [:bgrins]]

A handy feature in Chrome is the ability to run a command like copy(1+1) and have "2" on your clipboard afterwords (https://developers.google.com/chrome-developer-tools/docs/commandline-api#copyobject).  This is especially useful if there is a long string that is a pain to go back and highlight in the console to copy and paste.

It seems to either copy element.outerHTML if it is a DOM node or call JSON.stringify on the result and copy that to the clipboard.  For instance:

1) copy("hi") copies the string "hi"
2) copy({foo: "bar"}) copies the string:
{
  foo: "bar"
}
3) copy(new Date()) copies the string:
"2014-05-05T11:54:04.077Z"
4) copy($0) copies the outerHTML of the currently selected node

Comment 1

[David Teller [:Yoric] - still alive but not very active]

I would be interested in having this feature, so if I have a little time, I might wish to work on it. Any suggestion how I could get started?

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #1)
> I would be interested in having this feature, so if I have a little time, I
> might wish to work on it. Any suggestion how I could get started?

Great! I believe you will need to set a `copy` function on aOwner.sandbox in JSTermHelpers it will be added to the web console.  For instance, `$$` is a shorthand for querySelectorAll [1].  Then add a new test in toolkit/webconsole/test kind of like test_jsterm_cd_iframe.html.

As for getting the actual string to copy, I guess you could just try/catch JSON.stringify on the argument, but there is probably a better way.  One wrinkle in the plan is that we need to check for a DOM node and get the outer HTML from it, which is implemented on the Walker Actor (and a similar usage can be seen in the inspector panel [2]).  Forwarding question to Rob.

[1]: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/utils.js#1533
[2]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#787

Comment 3

[Brian Grinstead [:bgrins]]

Clearing needinfo as this was discussed further on irc

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Adding a copy() command to devtools

Here's a first attempt.
If I understand correctly, robcee is the best reviewer.

Comment 5

[Rob Campbell [:rc] (:robcee)]

marking as assigned before taking a look at the patch.

Comment 6

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8448760 [details] [diff] [review]
Adding a copy() command to devtools

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

Overall looks good to me. The blurb about executing content code has me a little scared. Would like some more detail there.

A comment about the test included below.

ship it!

::: browser/devtools/webconsole/test/browser_console_copy_command.js
@@ +15,5 @@
> +    "fugiat nulla pariatur. Excepteur sint occaecat cupidatat non " +
> +    "proident, sunt in culpa qui officia deserunt mollit anim id est laborum." +
> +    new Date();
> +
> +//let ID = Date.now();

commented line left in intentionally?

@@ +76,5 @@
> +      () => {
> +        let command = "copy(" + source + ")";
> +        info("Attempting to copy: " + source);
> +        info("Executing command: " + command);
> +        gJSTerm.execute(command, msg => {

this isn't going to produce a test failure if the command doesn't work. Just an info. You should should probably capture msg and replace the info() messages with is(msg, undefined, "Command success: " + command); or similar.

@@ +89,5 @@
> +      deferredResult.reject);
> +
> +    yield deferredResult.promise;
> +  }
> +});

nice use of Tasks for this test file.

::: toolkit/devtools/webconsole/utils.js
@@ +1779,5 @@
> +        payload = aValue.outerHTML;
> +      } else if (typeof aValue == "string") {
> +        payload = aValue;
> +      } else {
> +        // Note that this may execute arbitrary content code.

this is a little scary. In which conditions is this going to execute content code?

Comment 7

[David Teller [:Yoric] - still alive but not very active]

(In reply to Rob Campbell [:rc] (:robcee) from comment #6)
> ::: toolkit/devtools/webconsole/utils.js
> @@ +1779,5 @@
> > +        payload = aValue.outerHTML;
> > +      } else if (typeof aValue == "string") {
> > +        payload = aValue;
> > +      } else {
> > +        // Note that this may execute arbitrary content code.
> 
> this is a little scary. In which conditions is this going to execute content
> code?

Now, I may be wrong, but if `aValue` is a proxy or has getters, they will be executed, won't they?

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8448760 [details] [diff] [review]
Adding a copy() command to devtools

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

::: toolkit/devtools/webconsole/utils.js
@@ +1783,5 @@
> +        // Note that this may execute arbitrary content code.
> +        payload = JSON.stringify(aValue, null, "  ");
> +      }
> +    } catch (ex) {
> +      payload = "/* " + ex  + " */";

Any thoughts about doing aValue.toString() as a fallback instead of putting the exception on the clipboard?

Then the result of running:

var o1 = {};
var o2 = {a:o1};
o1['b'] = o2;
copy(o1);

Will be "[object Object]" instead of "TypeError: cyclic object value"

Comment 9

[Brian Grinstead [:bgrins]]

> Now, I may be wrong, but if `aValue` is a proxy or has getters, they will be
> executed, won't they?

Yes, and I think that's OK.  This command will only be run when the user requests it from the console, and doing a stringify gives the most useful and expected results.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> ::: toolkit/devtools/webconsole/utils.js
> @@ +1783,5 @@
> > +        // Note that this may execute arbitrary content code.
> > +        payload = JSON.stringify(aValue, null, "  ");
> > +      }
> > +    } catch (ex) {
> > +      payload = "/* " + ex  + " */";
> 
> Any thoughts about doing aValue.toString() as a fallback instead of putting
> the exception on the clipboard?

I think that the error is a bit more useful.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: Adding a copy() command to devtools, v2

Try: https://tbpl.mozilla.org/?tree=Try&rev=18ab3153b990

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for leaks in the new test.
https://hg.mozilla.org/integration/fx-team/rev/549927402a52

https://tbpl.mozilla.org/php/getParsedLog.php?id=43049067&tree=Fx-Team

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: Adding a copy() command to devtools, v3

https://tbpl.mozilla.org/?tree=Try&rev=11c0102f7617

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Carsten Book [:Tomcat]]

sorry had to back this out again for memory leaks like https://tbpl.mozilla.org/php/getParsedLog.php?id=43247207&tree=Fx-Team

Comment 17

[Mihai Sucan [:msucan]]

Comment on attachment 8451007 [details] [diff] [review]
Adding a copy() command to devtools, v3

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

Thanks for your patch - this is looking good. Quick drive-by comments below.

::: browser/devtools/webconsole/test/browser_console_copy_command.js
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Tests that the $0 console helper works as intended.

Comment needs to be updated.

@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Tests that the $0 console helper works as intended.
> +
> +let gWebConsole, gJSTerm;

These vars are not cleared when the test ends. Might cause the memleaks.

@@ +28,5 @@
> +  }, true);
> +
> +  content.location = "data:text/html;charset=utf-8,test for copy helper in web console";
> +
> +  yield deferredFocus.promise;

Instead of using this code to open a tab you could simply do yield loadTab("data:...").

@@ +50,5 @@
> +  doc.body.appendChild(div);
> +  doc.body.appendChild(div2);
> +
> +  let deferredConsole = Promise.defer();
> +  openConsole(gBrowser.selectedTab, deferredConsole.resolve);

openConsole() returns a promise.

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: console-copy.patch

Rebased and updated test based on Comment 17 to be e10s compatible and (hopefully) not leak.  Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de8d46ef74d6

Comment 19

[Brian Grinstead [:bgrins]]

Try push looks good, pushing to fx-team:

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

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Brian Grinstead [:bgrins]]

Will, there is a new jsterm helper inside of the webconsole for the `copy` command, that will copy information onto your clipboard.  If it's a DOM node it will copy the outer HTML, if it's a string it will copy directly, and for any other kind of object it will try to JSON.stringify it.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

added to Aurora 38 notes for Developers as:

`copy` command added to console

Comment 23

[Will Bamberg [:wbamberg]]

Brian, I've updated https://developer.mozilla.org/en-US/docs/Tools/Web_Console#Helper_commands. OK?
(the Console page needs to be updated and split into sub-pages, I think).

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Will Bamberg [:wbamberg] from comment #23)
> Brian, I've updated
> https://developer.mozilla.org/en-US/docs/Tools/Web_Console#Helper_commands.
> OK?
> (the Console page needs to be updated and split into sub-pages, I think).

Looks good, thanks.  Yes the page is getting very large, but looking through it I'm not sure what would be good candidate for being split out into a sub page.