B.J. Herbison Reporter
	Description • 13 years ago

Allow an easy way to view the DOM for a node with a DOM button. The new sidebar would display the same information currently shown by: <highlight><control-shift-K>inspect($0)

	Paul Rouget [:paul]
	Comment 1 • 13 years ago

You mean a JavaScript object view? Doesn't have to be a only-DOM view.
And how is that related to bug 63830?

	B.J. Herbison Reporter
	Comment 2 • 13 years ago

Yes, I think it would be useful to show any properties of the object the highlighter points to.

(Sorry about the block -- select error trying to copy the bug number of the Highlighter meta bug.)

	Paul Rouget [:paul]
	Comment 3 • 13 years ago

So the plan is to move the JS Object inspector from its own window into the Web Console (bug 704180).

	Rob Campbell [:rc] (:robcee)
	Comment 4 • 13 years ago

(In reply to Paul Rouget [:paul] from comment #3)
> So the plan is to move the JS Object inspector from its own window into the
> Web Console (bug 704180).

for web console inspected objects. I don't think that applies to this bug.

Maybe this should be next to the HTML view?

	Paul Rouget [:paul]
	Comment 5 • 13 years ago

(In reply to Rob Campbell [:rc] (robcee) from comment #4)
> (In reply to Paul Rouget [:paul] from comment #3)
> > So the plan is to move the JS Object inspector from its own window into the
> > Web Console (bug 704180).
> 
> for web console inspected objects. I don't think that applies to this bug.
> 
> Maybe this should be next to the HTML view?

So a button next to "style", something like "JS Object", that would open the Web Console and show the javascript-obj-view? Or you think we should open it in the sidebar (which would be weird, because the same tool could be display in the sidebar and in the webconsole)?

	Paul Rouget [:paul]
	Comment 6 • 13 years ago

Well, actually, opening the js-obj-view in the Console if opened from the Console, and in the sidebar if opened from the highlighter makes sense.

	Paul Rouget [:paul]
	Comment 7 • 13 years ago

(In reply to Rob Campbell [:rc] (robcee) from comment #4)
> Maybe this should be next to the HTML view?

You mean splitting the HTML view in two?
I think it would be better to have it in the sidebar.

	Rob Campbell [:rc] (:robcee)
	Comment 8 • 13 years ago

except the sidebar is labeled "Style" currently. I also think, since we have a grouping of style tools in the sidebar, it might make more sense to put a "DOM Object" viewer into the same panel as the HTML tree. Or we could pop it up separately as we do now as long as we don't get into more xul panel bugs.

One of the reasons we didn't include an Object button in the Highlighter was because I was selling "Annotations" as a possible way to get information about the object into the Highlighter view. Some people deemed the Object button as "too fringe", though I kind of disagreed at the time.

There is a workaround available if you open the Web Console and type $0 or inspect($0) to open the property viewer directly.

	Paul Rouget [:paul]
	Comment 9 • 13 years ago

(In reply to Rob Campbell [:rc] (robcee) from comment #8)
> except the sidebar is labeled "Style" currently. I also think, since we have
> a grouping of style tools in the sidebar, it might make more sense to put a
> "DOM Object" viewer into the same panel as the HTML tree. Or we could pop it
> up separately as we do now as long as we don't get into more xul panel bugs.

This tool doesn't have a name yet, but I don't think it's a DOM-only tool. It's a bit more general (we can inspect the whole JS structure, not just the DOM methods/properties). I would call it JS Object View.

This JS Object View can be open in the Web Console (`inspect(window)`). This will happen in bug 704180.

Sounds like we want to have a button that will inspect the node too (this bug).

We can simply open this tool in the Web Console since we will get inline-inspect working. But:
a) from the inspector, the user probably doesn't want to have the whole Web Console;
b) this view would fit better in a vertical view.

So we need to find a better slot for it.

If I'm not mistaken... if we would have had the modular sidebar (from where we could dock/undock tools, and maybe stack tools, with an accordion), we would just add it there. But we don't have this modular sidebar. For now, we have a style-specific sidebar.

We could move it into the HTML Panel. But:
a) It's a horizontal view;
b) We will have to move it the sidebar once it will get the modular sidebar.

I see 3 options:
a) We open the Web Console when the user click on the button. It's horizontal, but at least, we won't have to "kill" the feature when we will be able to host the JS Object view into the future modular sidebar;
b) We hide the style sidebar, and create a JS sidebar;
c) We start working on the modular sidebar now.

	Paul Rouget [:paul]
	Comment 11 • 13 years ago

triage, filter on centaur

	Paul Rouget [:paul]
	Comment 12 • 13 years ago

We want to move the buttons inside the sidebar. See bug 717929.

	Paul Rouget [:paul]
	Comment 13 • 13 years ago

(P3, as discussed with Kevin)

	Paul Rouget [:paul]
	Comment 14 • 12 years ago

The Toolbox + Sidebar.jsm makes things much easier.

Bug triage, filter on PINKISBEAUTIFUL

	Paul Rouget [:paul]
	Comment 15 • 12 years ago

Sorry sorry, wrong bug!

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 20 • 11 years ago

Probably the most irritating thing for most Firebug users is that we have no DOM tab. In Firebug I can easily inspect an object and click on the DOM tab (In fact, I can use the inspector whilst the DOM tab is open, which makes it very easy to select nodes.

Firebug also allows me to right-click an HTML node  and "Inspect in DOM panel." In our native tools I have to open a scratchpad or the web console, use a selector to get a node and then click Scratchpad's "Inspect" button. This is hardly discoverable nor as simple to use as Firebug's DOM panel.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 21 • 11 years ago

The VariablesView is a pretty self-contained widget now, so using it inside the inspector shouldn't be too hard.

	Patrick Brosset <:pbro>
	Comment 22 • 11 years ago

I will try to take a look at this one. It would indeed be a pretty good addition to our tools.
I've looked at the code a bit and the only unclear part that I'll look into now is how to get an objectActor grip for a dom node in the inspector.
Other than that, it should be pretty straight forward with the VariablesView.

The other thing is we have 4 tabs in the sidepanel already, this would be a 5th! We might need some re-thinking here.
@darrin: thoughts on this?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 23 • 11 years ago

The obvious thing would be to show the DOM panel where the Fonts tab is and to have an ellipse button to show less useful tools (e.g. Fonts). We need something like that anyway as the side panel pushes tabs off to the right when it is too narrow.

Darrin may have something better though.

	Patrick Brosset <:pbro>
	Comment 24 • 11 years ago

When it comes to actually retrieving properties from the currently selected node, I'm wondering what's the best course of action.

On one side we have the DOM view that will live in the inspector, where we have a reference to the currently selected NodeFront.
And on the other side, we have the VariableView widget that works well with ObjectActor's grips.

We could:

(a) implement a new WalkerActor method that, based on the passed node, would send back the grip. However, this requires to have a ThreadActor (see http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#2832) which the Walker does not have.
(b) alternatively, the new WalkerActor method could simply send a new response packet containing the list of key/value pairs for all of the node's properties. This way, we wouldn't use the VariableView's ability to work with grips at all.
(c) or find a way to call the webconsole's server which does the job already (i.e. when you click on the output link for a DOM node).

I'm leaning towards (a) but haven't yet been able to figure out how to make it happen.

	Patrick Brosset <:pbro>
	Comment 25 • 11 years ago

(c) isn't very practical as it requires a "evaluateJS" packet type which takes a string input whereas we have a DOM node front here.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 26 • 11 years ago

Mihai can elaborate more on this, but you don't need much machinery to use the debugger APIs with object actors. Unfortunately this is not well documented, but see how WebConsoleActor creates ObjectActors internally (pretending to be a ThreadActor by mocking _gripDepth and createValueGrip mostly). If your actor implements the right methods, the code that will give you the grip should roughly be:

let grip = actor.createValueGrip(actor.makeDebuggeeValue(domObject));

	Patrick Brosset <:pbro>
	Comment 27 • 11 years ago

Thanks Panos for the help.
I've managed to implement the premise of solution (a). I now have problems related to how the VariablesView widget fetches properties, but this seems related to the actorpool which I need to maintain on my side.

No need to needinfo Mihai anymore for now. I'll probably come back with more questions later though :)

	Mihai Sucan [:msucan]
	Comment 28 • 11 years ago

Patrick, I believe solution (a) is the one you should continue work on, as you are already doing.

As Panos pointed out, you only need _gripDepth and createValueGrip(). For createValueGrip() I maintain my own actor pool, because the lifetime of object actors is different in the webconsole, when compared to debugging. During debugging you have thread-lifetime and pause-lifetime actors.

It would be nice if you could reuse the console actor, to avoid code duplication.

	Patrick Brosset <:pbro>
	Comment 29 • 11 years ago

Dave, do you have suggestions as to how I could reuse the WebConsoleActor's ability to retrieve object actor grips from the inspector actor?

It seems to me that the inspector, being a protocol.js type actor, does not have access to the globally loaded actors.

	Patrick Brosset <:pbro>
	Comment 30 • 11 years ago

Status update: I've managed to get the DOM view sidebar working but it's a bit of a hack right now.

It works by acting as a ThreadActor, just like the WebConsoleActor does, and in doing so, can easily create ObjectActorGrips that have the thread lifetime instead.

The reason it's hacky right now is that from the WalkerActor I need access to 2 classes defined in script.js: ThreadActor and ObjectActor. Although the WebConsoleActor has access to these classes from the global scope, the WalkerActor does not.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 31 • 11 years ago

I think you are right, protocol.js-based actors are loaded in separate globals, so perhaps it's time we split the common bits of script.js into separate objects that can be used by both old-style actors (script.js, webconsole.js) and modern ones, like inspector.js. Moving the ObjectActor to an object.js module sounds straightforward and a good place to start.

	Patrick Brosset <:pbro>
	Comment 32 • 11 years ago

Darrin, here's a work in progress to add an ellipsis to the sidebar tabbox in case there's not enough room: http://www.youtube.com/watch?v=K2EHMZK9ybU
As you suggested, it would be nice to have an icon for this.

	Patrick Brosset <:pbro>
	Comment 36 • 11 years ago

Attaching my current patches for this bug.
They're not ready for review just yet, but I'm getting there.

Here's a status of the progress:

part 1:

- New "DOM" sidebar panel in the inspector.
- Reacts as the other panels do: refresh on new selection, empties if no node selected.
- Also reloads on node mutation.
- Uses telemetry.
- Is localized.
- Uses the VariablesView widget to display the properties.
- Goes to the server to translate the NodeFront into an ObjectActorGrip for the VariablesView to work (it does this similarly to how the WebConsole does it: that is by playing the role of the ThreadActor, by overriding some of its methods and managing its own actor pool).
This part isn't very very nice, but it turns out protocol.js actors aren't loaded in the server's global scope, so they don't have access to all actors by default. So what I did was split script.js in several files as I could. For instance, ObjectActor is now in a separate module that can be required by other actors. But I wasn't able to split the ThreadActor in a separate module as it requires simply too many dependencies.

part 2:

- Adds a new option to the ToolSidebar widget that shows an allTabs menu to the right when there's not enough space to let all tabs be visible. This menu contains a drop-down of all tabs.
- Makes the netmonitor also use ToolSidebar so it has the same feature. For this, I had to improve again ToolSidebar in a way that it now also works with existing <tab> elements.

part 3:

- Adds a simple test. I need to keep working on it and add new tests for the inspector and netmonitor sidebars.

	Patrick Brosset <:pbro>
	Comment 37 • 11 years ago

Comment on attachment 8363914 [details] [diff] [review]
bug704094-inspector-domview-part1-Adds-the-sidebar.patch

Mihai, getting a review on the part that returns actor grips would be awesome since you did this for the webconsole.

	Mihai Sucan [:msucan]
	Comment 38 • 11 years ago

Comment on attachment 8363914 [details] [diff] [review]
bug704094-inspector-domview-part1-Adds-the-sidebar.patch

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

Patch looks good and I'm really glad to see we will have a DOM panel in the inspector - finally. ;)

Do you plan to add editing of properties in this patch? If not, please file a follow-up bug - we should allow editing of properties, like the debugger and console do.

::: browser/devtools/dominspector/dominspector-panel.xul
@@ +9,5 @@
> +<?xml-stylesheet href="chrome://browser/skin/devtools/common.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://browser/skin/devtools/widgets.css" type="text/css"?>
> +<?xul-overlay href="chrome://global/content/editMenuOverlay.xul"?>
> +
> +<xul:window xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"

Why not make XUL the default namespace?

@@ +18,5 @@
> +  <script type="application/javascript;version=1.8"
> +          src="chrome://browser/content/devtools/theme-switching.js"/>
> +  <script type="application/javascript;version=1.8"
> +          src="chrome://browser/content/devtools/dominspector.js"/>
> +  <xul:script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>

Why do you mix <script> tags with and without the xul prefix?

@@ +24,5 @@
> +  <xul:box id="variable-view"
> +       class="devtools-responsive-container theme-body"
> +       flex="1"/>
> +
> +  <script type="application/javascript;version=1.8">

We have bug 960728 about removing all of the inlined scripts and styles. Please avoid adding this inlined script here.

::: browser/devtools/dominspector/dominspector.js
@@ +11,5 @@
> +Cu.import("resource:///modules/devtools/VariablesViewController.jsm");
> +Cu.import("resource://gre/modules/devtools/dbg-client.jsm");
> +Cu.import("resource://gre/modules/devtools/Loader.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +let promise = devtools.require("sdk/core/promise");

nit: you might want to lazy import/require modules you dont need when someone does require("dominspector").

Also, you might want to use Promise.jsm - the promise implementation that doesn't swallow exceptions.

@@ +13,5 @@
> +Cu.import("resource://gre/modules/devtools/Loader.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +let promise = devtools.require("sdk/core/promise");
> +
> +function DomInspector(inspector, rootEl) {

Please add jsdoc comments to functions/public methods.

::: toolkit/components/telemetry/Histograms.json
@@ +5071,5 @@
>    },
> +  "DEVTOOLS_DOMINSPECTOR_OPENED_BOOLEAN": {
> +    "expires_in_version": "never",
> +    "kind": "boolean",
> +    "description": "How many times has the devtool's DOM Inspector been opened?"

This might be confusing. Which DOM inspector? The markup view is a type of a DOM inspector.

I would suggest: How many times has the DOM panel been opened in the Inspector devtool?

... better suggestions are welcome.

I see below you use this in more places. Why not call it the DOM panel? Even in the devtools folder "inspector" versus "dominspector" could be confusing. You could put the DOM panel in devtools/inspector/ - a dom-panel.js and .xul.

I dont know if we have any "policy", but I was under the impression that the top level devtools/ folder should be used for tools.

(these concerns do not block the review, just mentioning my thoughts - no strong opinions on this. If you feel inclined to do something, maybe you should ping paul/joe to ask.)

::: toolkit/devtools/server/actors/inspector.js
@@ +866,5 @@
>     */
>    initialize: function(conn, tabActor, options) {
>      protocol.Actor.prototype.initialize.call(this, conn);
> +
> +    this.conn.addActorPool(this);

Why do you do this here? All protocol.js Actors do this, if I correctly understand protocol.js.

@@ +926,5 @@
> +
> +    this.dbg.enabled = false;
> +    this.dbg = null;
> +
> +    this.conn.removeActorPool(this);

Same question as above.

@@ +1946,5 @@
> +   * widget
> +   */
> +  getActorGripForNode: method(function(node) {
> +    let dbgGlobal = this.dbg.makeGlobalObjectReference(this.rootWin);
> +    let value = dbgGlobal.makeDebuggeeValue(node.rawNode);

What happens if rawNode comes from an iframe? For the webconsole we needed to get the global from the rawNode, not assume rootWin. See makeDebuggeeValue() in the console actor.

@@ +1960,5 @@
> +
> +  // Making the walker act as a ThreadActor so it can create node value grips.
> +  // This is somewhat similar to what the webconsole actor does, except that
> +  // the ThreadActor class isn't globally available here.
> +  // FIXME: the ThreadActor should be moved to its own separate module so it

Is this something you want to do in this bug or do you want to file a follow-up? If the latter, please add the bug number in the comment.

::: toolkit/devtools/server/actors/object-stringifier.js
@@ +16,5 @@
> + *        The object to stringify.
> + * @return String
> + *         The stringification for the object.
> + */
> +exports.stringify = function(obj) {

Did you make any changes in the code you moved from script.js into this file? If yes, do you have a diff?

::: toolkit/devtools/server/actors/object.js
@@ +8,5 @@
> +
> +const {Cu} = require("chrome");
> +const {DebuggerServer} = Cu.import("resource://gre/modules/devtools/dbg-server.jsm", {});
> +const {DevToolsUtils} = Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm", {});
> +const {stringify} = require("devtools/server/actors/object-stringifier");

Why do you move out the stringifiers from script.js into their own file? They seem to be only used by the ObjectActor (object.js). Also, they are deprecated until bug 952197 is fixed - we now have the object previewers (which you can find in script.js).

@@ +18,5 @@
> + *        The debuggee object.
> + * @param aThreadActor ThreadActor
> + *        The parent thread actor for this object.
> + */
> +function ObjectActor(aObj, aThreadActor) {

Did you make any changes in the code you moved from script.js into this file? If yes, do you have a diff?

::: toolkit/devtools/server/actors/webconsole.js
@@ -260,5 @@
>  
>    _createValueGrip: ThreadActor.prototype.createValueGrip,
>    _stringIsLong: ThreadActor.prototype._stringIsLong,
> -  _findProtoChain: ThreadActor.prototype._findProtoChain,
> -  _removeFromProtoChain: ThreadActor.prototype._removeFromProtoChain,

Please also remove the references to _protoChains.

	Mihai Sucan [:msucan]
	Comment 39 • 11 years ago

(In reply to Patrick Brosset [:pbrosset] from comment #37)
> Comment on attachment 8363914 [details] [diff] [review]
> bug704094-inspector-domview-part1-Adds-the-sidebar.patch
> 
> Mihai, getting a review on the part that returns actor grips would be
> awesome since you did this for the webconsole.

That part seems fine. Is the threadActor always available on the tabActor? I understand these are wip patches, so dont worry about all the comments, I noted them so we dont forget things with the next update.

	Patrick Brosset <:pbro>
	Comment 41 • 10 years ago

NIing Mike on this old bug which I never finished ...
Getting this landed would be great! For info, I also started bug 1101569 to make the sidebar easier to use when there are many tabs. So attachment 8363915 [details] [diff] [review] should be ignored here.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 42 • 9 years ago

I don't have time to work on this at the moment. I would prefer a sidebar than our current context menu implementation because I think it would be more discoverable and useable.

	Gabriel Luong [:gl] (ΦωΦ)
	Comment 45 • 8 years ago

Resetting assignee due to lack of activity.

	Dan
	Comment 47 • 6 years ago

I have a colleague that still uses an ancient build of Firefox so they can us Firebug and the dom properties panel.  In Fx, currently, the right-click, "show dom properties" gets kind of close. In Firebug, that panel was on the right in a tabbed interface, and in Fx, it's in a panel below (using the console), but that's a minor issue.  The real issue is that I cannot seem to edit the dom properties in the "inspect($0)" anymore.  I think I used to be able to in Fx, but I know I was able to in Firebug DOM Properties.

Fx dev tools is better than Firebug in most ways, now, but this seems a major oversight to me.  It was one of the best features of Firebug.