Add `copy` command to console

RESOLVED FIXED in Firefox 38

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: bgrins, Assigned: Yoric)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
Firefox 38
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed, relnote-firefox 38+)

Details

(Whiteboard: [console-papercuts])

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

5 years ago
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
Reporter

Updated

5 years ago
Whiteboard: [console][console-api]
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?
Flags: needinfo?(bgrinstead)
Reporter

Comment 2

5 years ago
(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
Flags: needinfo?(bgrinstead) → needinfo?(rcampbell)
Reporter

Comment 3

5 years ago
Clearing needinfo as this was discussed further on irc
Flags: needinfo?(rcampbell)
Here's a first attempt.
If I understand correctly, robcee is the best reviewer.
Assignee: nobody → dteller
Attachment #8448760 - Flags: review?(rcampbell)
marking as assigned before taking a look at the patch.
Status: NEW → ASSIGNED
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?
Attachment #8448760 - Flags: review?(rcampbell) → review+
(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?
Reporter

Comment 8

5 years ago
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"
Reporter

Comment 9

5 years ago
> 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.
(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.
https://hg.mozilla.org/integration/fx-team/rev/f007a1c34042
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [console][console-api] → [console][console-api][fixed-in-fx-team]
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
Whiteboard: [console][console-api][fixed-in-fx-team] → [console][console-api]
https://hg.mozilla.org/integration/fx-team/rev/161b47c4a8ad
Keywords: checkin-needed
Whiteboard: [console][console-api] → [console][console-api][fixed-in-fx-team]
sorry had to back this out again for memory leaks like https://tbpl.mozilla.org/php/getParsedLog.php?id=43247207&tree=Fx-Team
Whiteboard: [console][console-api][fixed-in-fx-team] → [console][console-api]
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.
Whiteboard: [console][console-api] → [console-papercuts]
Reporter

Comment 18

4 years ago
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
Attachment #8451007 - Attachment is obsolete: true
Attachment #8559500 - Flags: review+
Reporter

Comment 19

4 years ago
Try push looks good, pushing to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/a18bcb21d735
Whiteboard: [console-papercuts] → [fixed-in-fx-team][console-papercuts]
https://hg.mozilla.org/mozilla-central/rev/a18bcb21d735
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][console-papercuts] → [console-papercuts]
Target Milestone: --- → Firefox 38
Reporter

Comment 21

4 years ago
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.
Flags: needinfo?(wbamberg)
Keywords: dev-doc-needed
added to Aurora 38 notes for Developers as:

`copy` command added to console
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).
Flags: needinfo?(bgrinstead)
Reporter

Comment 24

4 years ago
(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.
Flags: needinfo?(wbamberg)
Flags: needinfo?(bgrinstead)

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.