1025778 - Save value as global variable in console

Status: VERIFIED FIXED

(DevTools :: Object Inspector, defect)

Description

[Patrick Brosset <:pbro>]

ChromeDevTools have this: https://plus.google.com/+UmarHansa/posts/1fNMTusN1Y6
and it seems to be pretty useful when you want to access the value of a property after having resumed execution.

Comment 1

[Cork]

For reference as an alternative solution. Firebug has "Use in Command Line" and then you can access it from $p.

Comment 2

[Patrick Brosset <:pbro>]

Hey Panos, would you be able to provide a rough description of the steps needed here? Or forward the NI? to someone else.
Someone has expressed interest in this bug on IRC today.

Comment 3

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

Note that we have bug 1154363 for Firebug's approach. In both cases we need a context menu item, but it's not clear to me whether we want to install this to one or all of the following places: variables view item, console output result, markup view node.

Patrick you would know best about the markup view, but for the other two I can point to ConsoleContextMenu in webconsole.js that handles the context menu items for the web console. We would probably have to set some additional metadata on the expression evaluation result (if we don't do it already) to indicate that it's a node that should display the context menu item and filter accordingly in ConsoleContextMenu.shouldHideMenuItem and ConsoleContextMenu.getSelectionMetadata. The actual menu entries are defined in webconsole.xul.

VariablesView.xul needs to get a similar context menu declaration as webconsole.xul and a handler similar to ConsoleContextMenu in VariablesView.jsm. Victor may have some thoughts about this as well.

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.

Comment 4

[Patrick Brosset <:pbro>]

For the markup-view:

We already have a menu item today named "Show DOM Properties" which basically opens the split-console and send the `inspect($0)` command.
$0 always points to the currently selected node in the inspector, and the `inspect` command opens the variables-view in the console to list the properties of whatever is passed in to it.

The goal here is to assign an other global variable ($whatever) to point to any DOM node, without it having to be selected in the inspector.
And indeed, as Panos said, we need a contextual menu item for this.

The menu that appears when right-clicking nodes in the inspector is defined in inspector.xul (inspectorPopupSet):
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector.xul#33

When the right-click occurs, the method _setupNodeMenu in inspector-panel.js is called to enable/disable items if needed, depending on the type of node:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#642

Comment 5

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

(In reply to Panos Astithas [:past] from comment #3)
> Note that we have bug 1154363 for Firebug's approach. In both cases we need
> a context menu item, but it's not clear to me whether we want to install
> this to one or all of the following places: variables view item, console
> output result, markup view node.

Just to be clear - does this bug cover the console item context menu or the variables viewer?

Comment 6

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

Not sure what Patrick had in mind, but I guess it depends on how far whoever picks this wants to take it? Any suggestions on which would be more valuable to implement first?

Comment 7

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

I now see this bug and bug 1154363 as being distinctly different:

* here we're integrating the debugger's variable view with the console
* in bug 1154363 the aim is to implement the firebug feature where dom nodes can be referred to in the console

I think both are useful and the key thing is to make sure we use similar terminology for the context menus when we implement each.

I'd like to get this bug fixed soon. James - can you estimate difficulty for this bug and add the [difficulty=?] whiteboard? More to the point - is this a good first/next bug?

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Jeff Griffiths (:canuckistani) from comment #7)
> I now see this bug and bug 1154363 as being distinctly different:
> 
> * here we're integrating the debugger's variable view with the console
> * in bug 1154363 the aim is to implement the firebug feature where dom nodes
> can be referred to in the console
> 
> I think both are useful and the key thing is to make sure we use similar
> terminology for the context menus when we implement each.

Jeff, do we want to reuse the "$p" name for the feature when selecting an object in the console?  That's how Firebug works (it exposes $p as a global variable when you use the feature from either the inspector or the console).  The benefit to using the chrome-style "tempN" is that you can keep more than one reference.

I ask because it will affect the implementation in Bug 1154363 if we decide to share $p for both cases.  Further, if we do go with the "tempN" thing from the console, any reason why to not use it for the inspector as well as opposed to sticking with $p there?

Comment 9

[Simon Lindholm]

> * in bug 1154363 the aim is to implement the firebug feature where dom nodes can be referred to in the console
FTR, the Firebug feature works for all objects (everywhere in the UI), not just DOM nodes. The main use I've found from it is to refer to console.log'd objects, but I can imagine objects/functions gotten from a debugger or closure view being useful as well.

> (it exposes $p as a global variable when you use the feature from either the inspector or the console)
It's actually local to the command line. Firebug's auto-completion works for command line getters.

(I think I personally like the "tempN" names better, because command line history that stops working can be quite annoying. The choice of $p in Firebug was mainly because it was the simplest thing to implement, plus consistency with $0.)

Comment 10

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

(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to Jeff Griffiths (:canuckistani) from comment #7)
> > I now see this bug and bug 1154363 as being distinctly different:
> > 
> > * here we're integrating the debugger's variable view with the console
> > * in bug 1154363 the aim is to implement the firebug feature where dom nodes
> > can be referred to in the console
> > 
> > I think both are useful and the key thing is to make sure we use similar
> > terminology for the context menus when we implement each.
> 
> Jeff, do we want to reuse the "$p" name for the feature when selecting an
> object in the console?  That's how Firebug works (it exposes $p as a global
> variable when you use the feature from either the inspector or the console).
> The benefit to using the chrome-style "tempN" is that you can keep more than
> one reference.
> 
> I ask because it will affect the implementation in Bug 1154363 if we decide
> to share $p for both cases.  Further, if we do go with the "tempN" thing
> from the console, any reason why to not use it for the inspector as well as
> opposed to sticking with $p there?

I think I agree: something like "tempN" is more easily understandable ( at least for English speakers that are programmers ). If we use a convention we should use it across the tools, however I think it might be worth using slightly / memorably different conventions for different things might be worth considering. This bug deals with any variable of any type visible from the debugger, whereas bug 1154363 deals exclusively with dom nodes.

I've got a couple of other use cases as possible followups:

* as I add global references to various things, it could be handy to have a list of them kept around that I could then iterate over. I think this is a pretty advanced micro-feature, but I could see it's usefulness.
* we should expose a command-line helper for this, eg for a scenario where I'm broken inside a function scope and am already using the split console to evaluate code. I could then just run a command eg `globalizer($varName)` - this could return the new variable's name as a string?

( note: `globalizer` is meant to be a suggestion SO TERRIBLE you'll be motivated to think of something better )

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Jeff Griffiths (:canuckistani) from comment #10)
> I've got a couple of other use cases as possible followups:
> 
> * as I add global references to various things, it could be handy to have a
> list of them kept around that I could then iterate over. I think this is a
> pretty advanced micro-feature, but I could see it's usefulness.

Take a look at the notes on this patch on bug 1154363: https://bugzilla.mozilla.org/show_bug.cgi?id=1154363#c7.  Her version uses a 'temp' variable for all the globals that's an array so it could be iterated.  I suggested maybe switching that over to the tempN convention so that it's easier to check and make sure that the variable name doesn't exist in content before reusing it, but I'm sure we could come up with a way to handle that.

Comment 12

[James Long (:jlongster)]

(In reply to Jeff Griffiths (:canuckistani) from comment #7)
> I now see this bug and bug 1154363 as being distinctly different:
> 
> * here we're integrating the debugger's variable view with the console
> * in bug 1154363 the aim is to implement the firebug feature where dom nodes
> can be referred to in the console
> 
> I think both are useful and the key thing is to make sure we use similar
> terminology for the context menus when we implement each.
> 
> I'd like to get this bug fixed soon. James - can you estimate difficulty for
> this bug and add the [difficulty=?] whiteboard? More to the point - is this
> a good first/next bug?

Been in-and-out on PTO lately. So the bug here specifically is just for the variables view at least? That wouldn't cover right-clicking on an object in the console.

I don't think any of this is really hard, but it might be something that touches several parts of our tools. It would be a "good second bug" in my opinion: shouldn't be too hard if you've at least touched our tools before. I'll mark it easy because it should be relatively easy if you know you're way around the tools already.

I like tempN as well.

Comment 13

[Brian Grinstead [:bgrins]]

Also want to make sure we don't duplicate work being done in Bug 1154363, and make sure they follow the same naming convention for variables - marking as dependent since that one already has a patch attached, but either could really be done first.

Comment 15

[Brian Grinstead [:bgrins]]

I think we can use bindObjectActor for this and access the object as _self, although we will need an extra option to tell that not to use the object's own global as the dbgWindow for evaluation (to prevent tempN being set on a different global in the case of iframes).  Or add a new option like bindObjectActorWithoutGlobal that does this.

Comment 16

[Brian Grinstead [:bgrins]]

Attachment: console-var-WIP.patch

Panos, I'm looking for feedback on this approach, specifically with how I'm adding a selectedObjectActor option that behaves similarly to bindObjectActor.  I needed to have a separate option for it, since we want to evaluate the frame with _self as a binding to the object, but still want `this` to refer to the root global (so that the evaluation assigns the temp variables into the right window and not an iframe's window).

I also considered piggybacking on the selectedNodeActor functionality somehow and assigning the object to $0, but went with this approach since the assignment (as of now) isn't intended to be used directly from jsterm, and this way we could share some code with bindObjectActor  in the webconsole actor.

Comment 17

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

Comment on attachment 8672105 [details] [diff] [review]
console-var-WIP.patch

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

I'm fine with this approach.

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1025778 - Save value as global variable in console;r=jlongster

Bug 1025778 - Save value as global variable in console;r=jlongster

Comment 19

[Brian Grinstead [:bgrins]]

Note to self: need to handle backwards compat to hide the context menu item when a server doesn't support the selectedNodeActor functionality.

Comment 20

[James Long (:jlongster)]

Comment on attachment 8673911 [details]
MozReview Request: Bug 1025778 - Save value as global variable in console;r=jlongster

https://reviewboard.mozilla.org/r/22145/#review19913

::: devtools/client/webconsole/console-output.js:2545
(Diff revision 1)
> +    }`;

So when the user does this, they access it by typing `temp0`, or whichever `temp#`? Also, when this reaches 1000 it will always be overwriting the 1000 element and not keeping the right "history" but I guess that's ok. Not sure if users would ever hit the 1000 limit.

How do we tell the user which `temp#` it saved as?

::: devtools/server/actors/webconsole.js:1159
(Diff revision 1)
> +                                       aOptions.selectedObjectActor);

I don't actually see where `selectedObjectActor` makes a difference. The code here does `||` with `bindObjectActor` and it, and doesn't ever seem to branch specifically for `selectedObjectActor`. Where is it doing different stuff?

Comment 21

[James Long (:jlongster)]

I'll r+ when I understand the patch a little better, not too familiar with the console so asking some questions first.

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to James Long (:jlongster) from comment #20)
> Comment on attachment 8673911 [details]
> MozReview Request: Bug 1025778 - Save value as global variable in
> console;r=jlongster
> 
> https://reviewboard.mozilla.org/r/22145/#review19913
> 
> ::: devtools/client/webconsole/console-output.js:2545
> (Diff revision 1)
> > +    }`;
> 
> So when the user does this, they access it by typing `temp0`, or whichever
> `temp#`? Also, when this reaches 1000 it will always be overwriting the 1000
> element and not keeping the right "history" but I guess that's ok. Not sure
> if users would ever hit the 1000 limit.

Yes, it will overwrite the 1000 element.  This is the same behavior as bug 1154363 - I think we could have an improvement here where we reuse a current temp# object if it's an exact match for the object we are trying to set.  So if you do this multiple times on the same object you'll get temp0 each time.  Although I'd prefer to do that in a follow up and hit both features at the same time.

> 
> How do we tell the user which `temp#` it saved as?
> 

It auto fills the console input with the string 'temp#' once the request finishes due to the call to `this.output.owner.jsterm.setInputValue(res.result)`.

> ::: devtools/server/actors/webconsole.js:1159
> (Diff revision 1)
> > +                                       aOptions.selectedObjectActor);
> 
> I don't actually see where `selectedObjectActor` makes a difference. The
> code here does `||` with `bindObjectActor` and it, and doesn't ever seem to
> branch specifically for `selectedObjectActor`. Where is it doing different
> stuff?

It's instead branching on aOptions.bindObjectActor, which is meant to be mutually exclusive to selectedObjectActor (could probably use a comment to that effect).  Since they pretty much do the same thing - add a binding of _self to the object - only with bindObjectActor it also sets the global for the execution to match the global for the object.

The reason for the difference is that if we just used bindObjectActor and you ran this on an object coming from a different iframe, then temp0 would end up getting defined in the iframe's window.

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 8673911 [details]
MozReview Request: Bug 1025778 - Save value as global variable in console;r=jlongster

Bug 1025778 - Save value as global variable in console;r=jlongster

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #23)
> Comment on attachment 8673911 [details]
> MozReview Request: Bug 1025778 - Save value as global variable in
> console;r=jlongster
> 
> Bug 1025778 - Save value as global variable in console;r=jlongster

This update adds a trait on the server so the frontend can hide the menu item when debugging an older server

Comment 25

[James Long (:jlongster)]

Comment on attachment 8673911 [details]
MozReview Request: Bug 1025778 - Save value as global variable in console;r=jlongster

https://reviewboard.mozilla.org/r/22145/#review20647

Sorry this took so long. I just read all the code and I understand how it works. Nothing jumped out at me, looks fine!

Comment 26

[Brian Grinstead [:bgrins]]

Thanks for the review - try push looks generally good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c43c4c2a86d

Comment 27

[Pulsebot]

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

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/400e0827e26c

Comment 29

[Brian Grinstead [:bgrins]]

Attachment: console-storeasglobal.gif

Comment 30

[Brian Grinstead [:bgrins]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Able to get a reference to logged variables to use in the console.  New feature, requested in https://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/9648300--store-as-global-variable-option-for-js-console
[Suggested wording]: Right click on a logged object in the console to store it as a global variable on the page
[Links (documentation, blog post, etc)]: Here's an animated gif of the feature: https://bug1025778.bmoattachments.org/attachment.cgi?id=8680676.  No documentation yet but planning on it

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Aurora44 release notes.

Comment 32

[Khalid Syfullah Zaman [:khalid32]]

I have reproduced this bug on Nightly 33.0a1 (2014-06-16) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Aurora 44.0a2!

Build ID: 20151110004043
User Agent: Mozilla/5.0 (X11; Linux i686; rv:44.0) Gecko/20100101 Firefox/44.0

[bugday-20151111]

Comment 33

[Saheda Reza [:Antora]]

It reproduced on  nightly 33.0a1 according to (2014-06-16)

This feature is available now on Latest Developer Edition ...It's fixed .
Latest Developer Edition
Build ID 	20151114004217
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:44.0) Gecko/20100101 Firefox/44.0

Tested OS---32bit Windows 8.1

Comment 34

[Mohammad Maruf Islam]

As this bug is verified on both Linux(Comment 32) and Windows (Comment 33),I am marking this as verified.