1154363 - Use $variable from inspector in console

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

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

Attachment: firebug-use-cli.gif

Firebug allows you to use various objects in the console as a global with a temp name - see attached gif.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

I've put some thoughts about how to implement this in bug 1025778 comment 3.

Comment 2

[Florent Fayolle]

Cc'ing Simon Lindholm who implemented the feature in Firebug.

Florent

Comment 3

[Florent Fayolle]

Also see documentation from Firebug: https://getfirebug.com/wiki/index.php/$p

PS: if anyone could just make this bug depend on bug 1164157, that would be cool (I don't have the rights for doing so).

Florent

Comment 4

[Lin Clark [:linclark]]

Attachment: Bug1154363.patch

This patch adds "Use in Command Line" to the contextual menu. When clicked, it outputs $0 to jsterm.

It doesn't currently have the autocompletion that was in Firebug. I asked about this in #devtools and it sounded like it isn't a requirement at the moment.

Any and all feedback welcome :)

Comment 5

[Lin Clark [:linclark]]

Attachment: use-in-command-line.gif

Here's a gif that shows what the patch in comment 4 does.

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8639482 [details] [diff] [review]
Bug1154363.patch

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

This is the right idea as far as exposing this to the inspector via the context menu, but I think we will want to store a new reference of the current value of $0 to keep a reference of the node that was selected, since the $0 reference will change out from under you as you change the currently selected node in the inspector.

This gets a little complicated, but see Panos' comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1025778#c3.  Specifically there are two different potential implementations depending on how we want to store the variable:

"For storing the value as a global, we would need to use the Debugger API method evalInGlobal, probably just by using JSTerm.requestEvaluation from the frontend."

"If we want to store it to Firebug's $p, we probably shouldn't make $p visible to the page, but implement it as a member of WebConsoleCommands in toolkit/devtools/webconsole/utils.js. In that case triggering the context menu item would store a reference to the value in an internal map that would be used to retrieve the values of $p in subsequent evaluations."

I'm not actually sure how we want to proceed between those two options, but I'm leaning towards the first (storing a global variable on the page with a reference to the DOM node).  As a bonus, this would get autocomplete to work.  Asked for more info in Bug 1025778 Comment 8.  Once we decide that I can go into more detail on the implementation.

In the meantime, can you please start on a basic test for this feature?  You could add a case to this test that basically selects a node and then clicks the context menu item: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/test/browser_inspector_menu-04-other.js.  Obviously we will have to make a decision on how to implement it before we get the test to actually do anything useful though

Comment 7

[Lin Clark [:linclark]]

Attachment: Bug1154363.patch

Thanks for the pointers.

I gave it a shot with JSTerm.requestEvaluation. I created a temp array which holds all of the nodes that have been output to the command line. I liked this because it makes it possible to output all of your temp variables at once, but could change that if we want to mimic the Chrome devtools naming (e.g. temp1, temp2) from the gif in Bug 1025778 Comment 1.

I ran into problems when trying to test it. 

  1. There were interactions with testShowDOMProperties() when I had it in browser_inspector_menu-04-other.js. I split it out into it's own file, though it would probably be possible to leave it in the same file and work around the interactions between the two tests.

  2. The test needs to wait until the call to jsterm.requestEvaluation and jsterm.setInputValue have run. I couldn't figure out which event I could have the yeild listen to.

  3. Once I do get the yeild in place, I'm not sure what to test for.

I think the functionality is going in the right direction, but please let me know if it's not. I'm happy to try it a different way.

Comment 8

[Lin Clark [:linclark]]

Attachment: use-in-command-line.gif

.gif shows how patch in Comment 7 is working

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8641838 [details] [diff] [review]
Bug1154363.patch

Marking myself so I don't lose track of it

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8641838 [details] [diff] [review]
Bug1154363.patch

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

This is great!

Can you do an `hg mv` for the 'other' test so the blame history is preserved?  Here's what you can do to fix that up:

hg qref -X browser/devtools/inspector/test/browser_inspector_menu-04-other.js
hg revert browser/devtools/inspector/test/browser_inspector_menu-04-other.js
rm browser/devtools/inspector/test/browser_inspector_menu-05-other.js
hg mv browser/devtools/inspector/test/browser_inspector_menu-04-other.js browser/devtools/inspector/test/browser_inspector_menu-05-other.js

::: browser/devtools/inspector/inspector-panel.js
@@ +983,5 @@
> +    this._toolbox.openSplitConsole().then(() => {
> +      let panel = this._toolbox.getPanel("webconsole");
> +      let jsterm = panel.hud.jsterm;
> +
> +      let evalString = "if (temp === undefined) var temp = []; temp.push($0)"

Since temp could be a semi-commonly used by a page, we should probably either pick a different name or handle that somehow by coming up with a new name.  This might be why the tempN thing is a little easier, since we could iterate until we find an available name

let i = 0;
while (("temp" + i) in window && i < 1000) {
  i++;
}
window["temp" + i] = $0;
"temp" + i;

This would avoid overwriting existing tempN names in most cases.  Note we could do something similar if we went with a temp array to come up with an unused name

@@ +989,5 @@
> +        selectedNodeActor: this.selection.nodeFront.actorID,
> +      };
> +      jsterm.requestEvaluation(evalString, options).then((res) => {
> +        var i = res.result - 1;
> +        jsterm.setInputValue(`temp[${i}]`);

You could emit an event on the inspector object here when it's all done:

this.emit("commandline-var-ready") or something like that, then you could use that event in the test with `yield inspector.once("commandline-var-ready");`

::: browser/devtools/inspector/test/browser_inspector_menu-04-use-in-command-line.js
@@ +23,5 @@
> +    yield consoleOpened;
> +
> +    ok(toolbox.splitConsole, "Split console is shown.");
> +
> +    todo(false, "Verify that temp variable has been added and $0 pushed in.");

Once it's yielded, maybe the easiest thing to assert would be "let result = yield jsterm.execute()" which will return the result and you could make sure it's the right DOM node.  Here's an example that does something similar: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/test/browser_webconsole_bug_653531_highlighter_console_helper.js#77

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> hg qref -X browser/devtools/inspector/test/browser_inspector_menu-04-other.js
> hg revert browser/devtools/inspector/test/browser_inspector_menu-04-other.js
> rm browser/devtools/inspector/test/browser_inspector_menu-05-other.js
> hg mv browser/devtools/inspector/test/browser_inspector_menu-04-other.js
> browser/devtools/inspector/test/browser_inspector_menu-05-other.js

If you aren't using hg / mq then I'm sure there is a different way to do this, if you ask in irc then someone will probably know the process

Comment 12

[Lin Clark [:linclark]]

Attachment: Bug1154363.patch

Thanks!

Here's a patch that incorporates the feedback.

1. hg mv - I needed to use the --find-renames flag with the git command that outputs the patch
2. I switched to tempN-style variables which just store each node separately.
3. I added the event emitter
4. I added a working test

We may need to discuss the variable naming more given the comments in Bug 1025778.

Comment 13

[Lin Clark [:linclark]]

Attachment: use-in-command-line.gif

Here's a gif for the patch in Comment 12

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8644463 [details] [diff] [review]
Bug1154363.patch

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

::: browser/devtools/inspector/inspector-panel.js
@@ +978,5 @@
>  
>    /**
> +   * Use in Command Line
> +   */
> +  useInCommandLine: function() {

Panos, can you give feedback on the requestEvaluation call here?  In particular, can you think of a way that a page could use this to execute script with jsterm privileges?  I don't think so since we are using `"foo" in window` and not `window["foo"] == undefined`, where the latter could set up a getter on foo and execute whatever script we are calling.  I'm not positive it would even be an issue if so, but just want another set of eyes on it.

Comment 15

[Simon Lindholm]

(Third party opinion: there should be no real difference between `"foo" in window` and `window["foo"] == undefined` from a security perspective, because of proxies:

> window.__proto__ = new Proxy(window.__proto__, {
>   has: function(r, p) {
>     if (p.startsWith("temp")) console.log("has? " + p);
>     return Reflect.has(r, p);
>   }
> });

If the jsterm script is run with chrome privileges, and I don't think it is, a tiny info leak might be possible after bug 589199 lands by declaring "let window = crossOriginWindow". Otherwise this should be safe - the scope chain with which jsterm commands are executed never leaks.)

Question, though:

> +      let evalString = `let i = 0;

Does "i" get leaked to global scope when this runs?

And I suppose it might be nice to use "this" instead of "window", in case someone shadows the latter, or if support for this feature eventually gets extended to workers.

Comment 16

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

(In reply to Lin Clark from comment #13)
> Created attachment 8644464 [details]
> use-in-command-line.gif
> 
> Here's a gif for the patch in Comment 12

Awesome!

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #14)
> Comment on attachment 8644463 [details] [diff] [review]
> Bug1154363.patch
> 
> Review of attachment 8644463 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/inspector/inspector-panel.js
> @@ +978,5 @@
> >  
> >    /**
> > +   * Use in Command Line
> > +   */
> > +  useInCommandLine: function() {
> 
> Panos, can you give feedback on the requestEvaluation call here?  In
> particular, can you think of a way that a page could use this to execute
> script with jsterm privileges?  I don't think so since we are using `"foo"
> in window` and not `window["foo"] == undefined`, where the latter could set
> up a getter on foo and execute whatever script we are calling.  I'm not
> positive it would even be an issue if so, but just want another set of eyes
> on it.

And to be clear Panos, I was thinking specifically of issues like reported in Bug 1176653.  But after further investigation, these seem like strictly different issues.  Just want to have a second set of eyes on it.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8644463 [details] [diff] [review]
Bug1154363.patch

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

::: browser/devtools/inspector/inspector-panel.js
@@ +984,5 @@
> +      let panel = this._toolbox.getPanel("webconsole");
> +      let jsterm = panel.hud.jsterm;
> +
> +      let evalString = `let i = 0;
> +        while (("temp" + i) in window && i < 1000) {

I can't think of anything wrong with this approach, but I would prefer using getOwnPropertyNames as suggested by bholley.

@@ +987,5 @@
> +      let evalString = `let i = 0;
> +        while (("temp" + i) in window && i < 1000) {
> +          i++;
> +        }
> +        window["temp" + i] = $0;

A safer approach would not store the temporary variables in the page, but have them only as console helpers, but as I understand it we decided to go with the other approach so this should be fine.

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to Simon Lindholm from comment #15)
> (Third party opinion: there should be no real difference between `"foo" in
> window` and `window["foo"] == undefined` from a security perspective,
> because of proxies:
> 
> > window.__proto__ = new Proxy(window.__proto__, {
> >   has: function(r, p) {
> >     if (p.startsWith("temp")) console.log("has? " + p);
> >     return Reflect.has(r, p);
> >   }
> > });

Thanks for the info, that's a good point. In our case I think checking 'in' (or hasOwnProperty) is still better so that we don't accidentally trigger any getters.

> If the jsterm script is run with chrome privileges, and I don't think it is,
> a tiny info leak might be possible after bug 589199 lands by declaring "let
> window = crossOriginWindow". Otherwise this should be safe - the scope chain
> with which jsterm commands are executed never leaks.)

The commands do not run with chrome privileges

> Question, though:
> 
> > +      let evalString = `let i = 0;
> 
> Does "i" get leaked to global scope when this runs?

No it doesn't

> And I suppose it might be nice to use "this" instead of "window", in case
> someone shadows the latter, or if support for this feature eventually gets
> extended to workers.

This particular temp assignment won't get ported to workers since it relies on DOM nodes existing.  And I don't think you can overwrite the 'window' object, at least when I test `window = {}; window === this; // true`

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8644463 [details] [diff] [review]
Bug1154363.patch

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

Lin, this is great!  Really looking forward to shipping this, and sorry for the delay.  Just a couple of small notes, and some questions for others before we can get this landed.  In the meantime, here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05e17e61b04c

::: browser/devtools/inspector/inspector-panel.js
@@ +984,5 @@
> +      let panel = this._toolbox.getPanel("webconsole");
> +      let jsterm = panel.hud.jsterm;
> +
> +      let evalString = `let i = 0;
> +        while (("temp" + i) in window && i < 1000) {

I think this is a different situation - in that case we were iterating over keys - something like `for (let foo in window) { ... }`, while here we are using 'in' to check for existence of a property.  Maybe we could use hasOwnProperty instead of 'in' though?  Not sure if there is an advantage of one over the other for this case.

::: browser/devtools/inspector/test/browser_inspector_menu-04-use-in-command-line.js
@@ +22,5 @@
> +
> +    let hud = toolbox.getPanel("webconsole").hud;
> +    let jsterm = hud.jsterm;
> +
> +    let jstermInput = jsterm.hud.document.getElementById("jsterm-input");

I think it's fine to just querySelector(".jsterm-input-node") and not add the ID to the webconsole.xul, since it's just for the test

::: browser/locales/en-US/chrome/browser/devtools/inspector.dtd
@@ +108,5 @@
> +<!-- LOCALIZATION NOTE (inspectorUseInCommandLine.label): This is the label
> +     shown in the inspector contextual-menu for the item that outputs a
> +     variable for the current node to the console. When triggered,
> +     this item opens the split Console. -->
> +<!ENTITY inspectorUseInCommandLine.label       "Use in Command Line">

Jeff, any opinion about how we present this feature in the context menu?  "Use in Command Line" / "Use in Console" / "Store as global" / "Save as global variable", etc.

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> ::: browser/devtools/inspector/inspector-panel.js
> @@ +984,5 @@
> > +      let panel = this._toolbox.getPanel("webconsole");
> > +      let jsterm = panel.hud.jsterm;
> > +
> > +      let evalString = `let i = 0;
> > +        while (("temp" + i) in window && i < 1000) {
> 
> I think this is a different situation - in that case we were iterating over
> keys - something like `for (let foo in window) { ... }`, while here we are
> using 'in' to check for existence of a property.  Maybe we could use
> hasOwnProperty instead of 'in' though?  Not sure if there is an advantage of
> one over the other for this case.

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> ::: browser/locales/en-US/chrome/browser/devtools/inspector.dtd
> @@ +108,5 @@
> > +<!-- LOCALIZATION NOTE (inspectorUseInCommandLine.label): This is the label
> > +     shown in the inspector contextual-menu for the item that outputs a
> > +     variable for the current node to the console. When triggered,
> > +     this item opens the split Console. -->
> > +<!ENTITY inspectorUseInCommandLine.label       "Use in Command Line">
> 
> Jeff, any opinion about how we present this feature in the context menu? 
> "Use in Command Line" / "Use in Console" / "Store as global" / "Save as
> global variable", etc.

Comment 23

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

(In reply to Brian Grinstead [:bgrins] from comment #22)
..
> > Jeff, any opinion about how we present this feature in the context menu? 
> > "Use in Command Line" / "Use in Console" / "Store as global" / "Save as
> > global variable", etc.

I think I prefer "Use in Console" - what the user experiences for this ( and related features like bug 1025778 ) is that the value becomes available with a specific name in the console, this is where the utility is in this feature. Sure, we're also adding a global onto window but I think this more direct, practical language is better. It tells the user what to expect - that a new variable will be created for use in the console, and that they will get a visual cue of what happened.

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Jeff Griffiths (:canuckistani) from comment #23)
> (In reply to Brian Grinstead [:bgrins] from comment #22)
> ..
> > > Jeff, any opinion about how we present this feature in the context menu? 
> > > "Use in Command Line" / "Use in Console" / "Store as global" / "Save as
> > > global variable", etc.
> 
> I think I prefer "Use in Console" - what the user experiences for this ( and
> related features like bug 1025778 ) is that the value becomes available with
> a specific name in the console, this is where the utility is in this
> feature. Sure, we're also adding a global onto window but I think this more
> direct, practical language is better. It tells the user what to expect -
> that a new variable will be created for use in the console, and that they
> will get a visual cue of what happened.

"Use in Console" works for me.  Lin, can you please update the text to match?

Comment 25

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> ::: browser/devtools/inspector/inspector-panel.js
> @@ +984,5 @@
> > +      let panel = this._toolbox.getPanel("webconsole");
> > +      let jsterm = panel.hud.jsterm;
> > +
> > +      let evalString = `let i = 0;
> > +        while (("temp" + i) in window && i < 1000) {
> 
> I think this is a different situation - in that case we were iterating over
> keys - something like `for (let foo in window) { ... }`, while here we are
> using 'in' to check for existence of a property.  Maybe we could use
> hasOwnProperty instead of 'in' though?  Not sure if there is an advantage of
> one over the other for this case.

Right, hasOwnProperty would do for me. I mainly wanted to avoid the prototype chain lookups. Probably even:

Object.prototype.hasOwnProperty.call(window, "document")

Comment 26

[Lin Clark [:linclark]]

Attachment: Bug1154363.patch

Thanks for all of the review and feedback.

I'm pretty sure that this patch addresses all of the issues. The patch:

1. uses querySelector(".jsterm-input-node") and removes the id I had added
2. changes the name of the feature from "Use in Command Line" to "Use in Console"
3. uses hasOwnProperty() instead of in

That was what I culled from the comments. Let me know if I missed anything.

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 8655069 [details] [diff] [review]
Bug1154363.patch

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

Great!  r=me with a couple of string updates.  Can you update the patch commit message to read something like 'Assign the selected node from the inspector to a temp variable for use in the console'.  You can re-upload that patch with an r+

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84bd722b529e

::: browser/devtools/inspector/inspector-panel.js
@@ +976,5 @@
>      });
>    },
>  
>    /**
> +   * Use in Console

Nit: can you expand this comment a bit?  Maybe something like:

Takes the currently selected node in the inspector and assigns it to
a temp variable on the content window.  Also opens the split console
and autofills it with the temp variable.

Comment 28

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #27)
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=84bd722b529e

Ah, there are some issues in the test where the split console is being re-opened on the next test run.  We need to add a cleanup function to any test that uses this command and clear the split console pref:

  registerCleanupFunction(() => {
    Services.prefs.clearUserPref("devtools.toolbox.splitconsoleEnabled");
  });

Comment 29

[Lin Clark [:linclark]]

Attachment: Bug1154363.patch

Comment 30

[Lin Clark [:linclark]]

Sorry, accidentally hit enter early on that last comment.
----

Ah, I should have run the full inspector suite before posting. Thanks for the suggestion, it worked.

It revealed another issue, though. The other test checks that "inspect($0)" is the first entry in the history. That assertion fails because the history from the console vars test persists between the tests. 

In this patch, I run jsterm.clearHistory() at the end of testUseInConsole(). It might be preferable to make that an explicit part of the cleanup. To do that, I think I'd have to declare jsterm at the top of the file and then depend on it being defined in the test (because it isn't available until the console is opened). That seems like it could be brittle.

Should I keep the clearHistory() call where it is in this patch? Or is there a better approach?

Comment 31

[Brian Grinstead [:bgrins]]

(In reply to Lin Clark from comment #30)
> Sorry, accidentally hit enter early on that last comment.
> ----
> 
> Ah, I should have run the full inspector suite before posting. Thanks for
> the suggestion, it worked.
> 
> It revealed another issue, though. The other test checks that "inspect($0)"
> is the first entry in the history. That assertion fails because the history
> from the console vars test persists between the tests. 
> 
> In this patch, I run jsterm.clearHistory() at the end of testUseInConsole().
> It might be preferable to make that an explicit part of the cleanup. To do
> that, I think I'd have to declare jsterm at the top of the file and then
> depend on it being defined in the test (because it isn't available until the
> console is opened). That seems like it could be brittle.
> 
> Should I keep the clearHistory() call where it is in this patch? Or is there
> a better approach?

I think it's fine to keep clearHistory() where it is.  Here's a try push with the patch applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=78b077057713

Comment 32

[Pulsebot]

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

Comment 33

[Simon Lindholm]

(In reply to Brian Grinstead [:bgrins] from comment #19)
> Thanks for the info, that's a good point. In our case I think checking 'in'
> (or hasOwnProperty) is still better so that we don't accidentally trigger
> any getters.

Certainly. (I don't see the advantage of hasOwnProperty over in, since |window.hasOwnProperty| itself does a prototype-traversing [[Get]], but ah well.)

> And I don't think you can overwrite the 'window'
> object, at least when I test `window = {}; window === this; // true`

Right, "window" is a non-configurable non-writable property of the global object. However, after bug 589199 lands, |let window = {}| will declare a binding on a scope right before the global object that shadows that property. I.e., |let a = 1;| will make |a === 1|, but |window.a === undefined|. There's some discussion about changing this going on on the es-discuss list: https://esdiscuss.org/topic/global-lexical-tier

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0cfdf0771f2a

Comment 35

[Liz Henry (:lizzard) (relman/hg->git project)]

Release Note Request (optional, but appreciated)
[Why is this notable]: Nice feature for dev edition notes, suggested by DT team
[Suggested wording]: Use $variable from developer tools inspector in console
[Links (documentation, blog post, etc)]:

If you have improvements for the release note wording, please comment and needinfo me. Thanks!

Comment 36

[Brian Grinstead [:bgrins]]

Suggested wording - something like this: "New 'Use in Console' context menu item in Inspector that stores the selected element in a temporary variable."

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks, that's much more clear.  I fiddled it slightly to be "New 'Use in Console' context menu item in Inspector to store selected element in a temporary variable" (just to make it a bit shorter)

Comment 38

[Will Bamberg [:wbamberg]]

I've added this: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML#Element_popup_menu.

OK?

Comment 39

[Sebastian Zartner [:sebo]]

Absolutely. Thanks Will!

Sebastian

Comment 40

[Sebastian Zartner [:sebo]]

I also added it to https://developer.mozilla.org/en-US/Firefox/Releases/43.

Sebastian

Comment 41

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

whoops