add support for watched expressions

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Patrick Mueller]

Firefox has a concept of "watch expressions", which are expressions the use can type into the same area as the currently visible objects (during a debug pause).  Note these are expressions, and not simple variable names.  The expressions are evaluated each time the debugger pauses.  The expressions are rendered in the UI exactly the same as variables, only instead of the variable/property name, the expression is listed.

Developers I work with that use FireBug have indicated that this support is quite valuable to them.

We should add this to Web Inspector.

[Patrick Mueller]

I've started working on this.

[Patrick Mueller]

Created attachment 33275 [details]
mock up of watched expressions UI

Mock up of the UI.  The Watched Expressions is a new sidebar pane as a peer of Call Stack, Scope Variables, etc.  

Somehow we need to have a way to add a new expression.  Something like a button where the italic "Add watched expression" label is in the image.  

Looks like the ObjectPropertiesSection class can be largely reused, with a bit of refactoring.  The "property names" in this case, like "19+23" are the expressions, the values are the results of evaluating the expression.  Objects with properties should be drill-down-able, like window.HTMLElement is in the example, which ObjectPropertiesSection handles already.  

Evaluations which cause exceptions need to be reported, as they are in the bare "x" example (ReferenceError), though this needs to be prettied up with an icon of some kind, and probably a different color.

There will need to be affordances/gestures to delete an expression, and to edit the expression.  May want to add an accelerator to watch the expression indicated by text selected in the source view.

The expressions should be sorted by time added.  Perhaps they should be positionable via drag-drop, eventually.

[Patrick Mueller]

Some notes from IRC yesterday:

[1:18p] pmuellr: BTW, I'm in need of a gesture to add new watched expressions;
hacked something you can see here ("Add watched expressions" is clickable,
adds a new empty expression): https://bugs.webkit.org/attachment.cgi?id=33275

[1:30p] xenon: pmuellr: i would sugesst making "add watched expression"
be at the bottom and be a rounded bubble button

[1:32p] pmuellr: xenon: makes sense; is there an example of rounded bubble
button style I can borrow? not many "button" in the UI right now

[1:33p] xenon: pmuellr: yes, the "enable" views in the inspector have them

[1:33p] JoePeck: pmuellr: on the splash screen to enable / disable Scripts
Panel or something [1:34p] JoePeck: pmuellr: in the css ".panel-enabler-view
button" [1:34p] JoePeck: and the related styles

[1:35p] pmuellr: I'll need to get the button smaller :-) will play with, thx

[1:36p] JoePeck: pmuellr: I threw it in a pane, this is what it looked like:
http://tr.im/uCN1?grabup maybe it is too big

[1:28p] xenon: pmuellr: i guess every time it is paused/stepped you eval the
expressions?

[1:30p] pmuellr: xenon: in FireBug, the expressions also show when not paused
- ie, when you're "running". Also, evaluated in the current frame. Again, all
seems doable right now.

[1:37p] pmuellr: I also want a "hover over expression enables 'delete me'
button", on the right, like style's checkboxes appear when hovered.
Suggestions? mini-trashcan?

[1:44p] xenon: pmuellr: apple usually uses the "no smoking" don't symbol
[1:45p] xenon: pmuellr: or a X

[Patrick Mueller]

Also happened to think: per Bug 27502 , we might want to add a hot key to allow the selected text in a source view to be added to the watched expressions list.

[Patrick Mueller]

Created attachment 34170 [details]
patch containing work so far

a mostly working version of the watch expressions based off of revision 46507 (slightly old).

Not ready for review, but thought I'd get the work up here, in a state where folks can play with it.

Couple of things to add:

- refresh list when debugger pauses, when the call stack frame changes
- provide a better gesture for delete; currently you edit the expression, delete all the text.  Some kind of hover enabled button, like CSS properties checkboxes?
- save / reload watched expressions between debugger invocations

Not sure what to do with the "Add" button; kinda sticks out.  Maybe a slightly smaller font?
Not sure if we want to keep the "Refresh" button, but I think so; there is no way I can think of that we could keep the list updated while an application is running (not paused), without just refreshing it every so often.  Which doesn't make any sense.  But someone may want to refresh the values without pausing the application.

[Patrick Mueller]

cc'ing some folks; should be posting a proposed patch today

[Patrick Mueller]

Created attachment 39057 [details]
proposed patch - 2009/09/04

final for now, definitely working version

Couple things to clean up, which I'd like to do later, after getting the meat of the code in:

- I suspect folks won't like the red delete icon that shows up in the right column; I don't, but couldn't find anything better.  Suggestions?  I was thinking maybe taking the existing icon I'm using, but convert it to grayscale.

- not really happy with the Add and Refresh buttons.  Too big.  I tried a smaller font at one point, one pt smaller, but that seemed too small.  Was wondering about making these link-like instead of button-like.

- there are some changes to existing code elsewhere that should be made; pfeldman suggested some proposed changes to make the Object.describe() vs proxy.description stuff simpler, making the console.eval code "public" (maybe we should move it somewhere else since it's not console specific any more), and property sorting should move to utilities.  I'd like to do these later.

[Patrick Mueller]

Created attachment 39058 [details]
current watch expressions pane

Current version of the Watch Expressions pane.  You can see the HUUUUGE Add/Refresh buttons, the red delete buttons on the right, the general layout of the expressions and their values, and the color coding of eval errors.

[Pavel Feldman]

> +
> +        // WebInspector.ConsoleView._evalInInspectedWindow should be made 'public'
> +        WebInspector.console._evalInInspectedWindow("(" + this.expression + ")", callback);

_evalInInspectedWindow is used in console where we don't know whether we are on breakpoint or not. If watch expressions are only applicable to the paused state, you should use ScriptsPanel.prototype.evaluateInSelectedCallFrame or doEvalInCallFrame (your watch expressions are not supposed to mutate the app state).

> +
> +        // Note, should provide a better way to get the "description";
> +        // would prefer to just use Object.describe(), but returns
> +        // "Object" for new Date() compared to the proxy which returns
> +        // the date string
> +        
> +        // if a proxy object, get description from there
> +        if (value && value.objectId && value.description)
> +            this.valueElement.textContent = value.description;
> +            
> +        // if not a proxy object, use Object.describe()
> +        else
> +            this.valueElement.textContent = Object.describe(value, true);
> +

Object.describe is not available (will soon be not available) in the frontend. It is declared in the InjectedScript and should only be applied to the real objects (not proxies) in the InjectedScript.js. Look at how values
are formatted in ConsoleView's _format method for reverence on how to render evaluation results instead.

> +    _refreshExpressionPropertiesCallback: function(properties)
> +    {
> +        this.removeChildren();
> +
> +        if (!properties) {
> +            this.hasChildren = false;
> +            return;
> +        }
> +
> +        // WebInspector.ObjectPropertiesSection.prototype._displaySort should be
> +        // moved to something like utilities.js
> +        properties.sort(WebInspector.ObjectPropertiesSection.prototype._displaySort);
> +
> +        for (var i = 0; i < properties.length; ++i) {
> +            this.appendChild(new this.treeOutline.section.treeElementConstructor(properties[i]));
> +        }
> +        this.hasChildren = properties.length > 0;
> +    },
> +

This snippet as well as the InspectorController.getProperties(value, true, callback) above seems to be a large functionality dupe of what happens in ObjectPropertiesSection. Why not to use ObjectPropertiesSection explicitly?
It would handle all the bits of rendering and expansion. Doesn't it work for you?

I now recall that ObjectPropertiesSection can have problems rendering primitive values that are being returned from the evaluate without being wrapped with ObjectProxies. But that should be fairly easy to fix in the InjectedScript.evaluateOn body. It would actually simplify ConsoleView's formatting as well.
If that is the case, I can fix that in the evaluate code and you would be able to nuke most of the duplicated code.

[Patrick Mueller]

(In reply to comment #9)

> _evalInInspectedWindow is used in console where we don't know whether we are on
> breakpoint or not. If watch expressions are only applicable to the paused
> state, you should use ScriptsPanel.prototype.evaluateInSelectedCallFrame or
> doEvalInCallFrame (your watch expressions are not supposed to mutate the app
> state).

As currently implemented, watch expressions are applicable outside of paused states.  For instance, you can bring up the Web Inspector, go to the Scripts panel, add watch expressions, and use the Refresh key to update without having to set a break point.  Whether or not this is useful, don't know.  I can imagine it could be.  I'm not sure if FB does this or not.

I understand the thought that watches shouldn't mutate app state, but of course it's impossible to prevent that.  I'm not sure it's such a bad thing.  Some power user might be able to do something useful with it.

So, for the time being, I do still need something like _evalInInspectedWindow()

> Object.describe is not available (will soon be not available) in the frontend.
> It is declared in the InjectedScript and should only be applied to the real
> objects (not proxies) in the InjectedScript.js. Look at how values
> are formatted in ConsoleView's _format method for reverence on how to render
> evaluation results instead.

I haven't looked at those, but I will.  It would be nice to be consistent in terms of what we're displaying for a particular object, in the various places it can be displayed.
 
> This snippet as well as the InspectorController.getProperties(value, true,
> callback) above seems to be a large functionality dupe of what happens in
> ObjectPropertiesSection. Why not to use ObjectPropertiesSection explicitly?
> It would handle all the bits of rendering and expansion. Doesn't it work for
> you?

I needed more behaviour than the plain old ObjectPropertiesSection had.  I did try that first though; the watch expressions were keys in an object whose values were the eval()'d version of the keys.  For various reasons it turned out to be too much work to make it all work in the existing framework instead of writing the relatively small amount of code that I did.

I do reuse a lot of the behaviour out of the ObjectProperties code; and especially all the stuff underneath the roots of the trees - the children of the expressions themselves are ObjectPropertiesElements (or whatever).
 
> I now recall that ObjectPropertiesSection can have problems rendering primitive
> values that are being returned from the evaluate without being wrapped with
> ObjectProxies. But that should be fairly easy to fix in the
> InjectedScript.evaluateOn body. It would actually simplify ConsoleView's
> formatting as well.
> If that is the case, I can fix that in the evaluate code and you would be able
> to nuke most of the duplicated code.

[Timothy Hatcher]

(In reply to comment #10)
> (In reply to comment #9)
> 
> > _evalInInspectedWindow is used in console where we don't know whether we are on
> > breakpoint or not. If watch expressions are only applicable to the paused
> > state, you should use ScriptsPanel.prototype.evaluateInSelectedCallFrame or
> > doEvalInCallFrame (your watch expressions are not supposed to mutate the app
> > state).
> 
> As currently implemented, watch expressions are applicable outside of paused
> states.  For instance, you can bring up the Web Inspector, go to the Scripts
> panel, add watch expressions, and use the Refresh key to update without having
> to set a break point.  Whether or not this is useful, don't know.  I can
> imagine it could be.  I'm not sure if FB does this or not.

I do think that will be useful.

I will look at the patch in detail later, likely after the holidy. Can you attach a screenshot?

[Patrick Mueller]

attachment 39058 [details] from comment 8 shows a flat list of expressions.  In that shot, you can see the disclosure triangle for "this", which is a DOMWindow, which when expanded is the same thing you'd see in the locals pane while paused, for instance.

The expressions are the purple bits to the left, and are editable by double clicking anywhere on the line.

Reading the manual test case in the patch might explain a little better how it works.

Maybe doing a movie would be better.  if I only had Snow Leopard installed!!  Actually I think I have some other screen capture software.

[Pavel Feldman]

>> I needed more behaviour than the plain old ObjectPropertiesSection had.
>> I did try that first though; the watch expressions were keys in an 
>> object whose values were the eval()'d version of the keys.  
>> For various reasons it turned out to be too much work to make it all
>> work in the existing framework instead
>> of writing the relatively small amount of code that I did.
>> I do reuse a lot of the behaviour out of the ObjectProperties code;
>> and especially all the stuff underneath the roots of the trees - 
>> the children of the expressions themselves are 
>> ObjectPropertiesElements (or whatever).

So each watched expression is an ObjectPropertiesSection with the watched expression as atitle and ObjectProxy value. Why doesn't it work? Try opening console and typing "dir(document.documentElement)". You'll get ObjectPropertySection formatted for the result. I'd like that code only to deal with rendering objects that came from the inspected page. InspectorController.getProperties should not be called anywhere else. Frontend is only dealing with visual representation of stuff, it should only call it in order to render the result and the renderer is already there.

[Pavel Feldman]

I looked at the code and I think I know what you should try. Inherit WebInspector.WatchExpressionsSection from WebInspector.ObjectPropertiesSection. In the inherited class, override update() method so that instead of the original code:

var self = this;
var callback = function(properties) {
    if (!properties)
        return;
    self._update(properties);
};
InjectedScriptAccess.getProperties(this.object, this.ignoreHasOwnProperty, callback);

you would do something like:

var properties = [];
for (all watch expressions) {
    properties.push(new WebInspector.ObjectPropertyProxy(watchExpression /* name */, evalResultObjectProxy /* value */);
}
this.update(properties);

This way your section will get populated with exactly what you want. Instead of property names you will have your watch expressions. They will be infinitely expandable (allowing browsing their substructures). There are couple of things to do before it works nice:

1. It will not work for primitive values until InspectorController::wrapObject is changed to wrap them. I could not do it within 2 minutes since primitive unconditional wrapping spoils autocomplete code. I'll see if I can complete it before my flight today.

2. You should render evaluation exceptions manually.

[Pavel Feldman]

Created attachment 39114 [details]
WebInspector.ObjectPropertiesSection - based rendering

Here is how I would approach the rendering. It is just few lines of code and they don't touch internals / query for properties via injected script, etc. I removed all the editing-related functionality and error-handling for the sake of the snippet simplicity - everything can be added on top of this. Note that it also gives one ability to edit values in place (such as in object properties section). I will attach a screenshot with how it looks shortly.

[Pavel Feldman]

Created attachment 39115 [details]
WebInspector.ObjectPropertiesSection - based screenshot

[Timothy Hatcher]

Pavel's approch makes sense to me. That is how I would approch it.

[Patrick Mueller]

Yup.  Looks good!

[Patrick Mueller]

(In reply to comment #18)
> Yup.  Looks good!

Looked good for a minute anyway.  :-)

I'm finding I'm having to greatly complicate the code, including some light (but safe!) refactoring of the ObjectProperties* classes, in order to make this work.  For instance, I still need the top level entries in the section to be a new class of TreeElement, but want to use ObjectPropertyTreeElement for everything below, and this will require making the treeElement constructor parameterized, instead of using the one the section says to use.

I suspect, the result, assuming I can get it working, will be a little less clear as to what's going on here than the original version I wrote.

Everyone ok with that?  At this point, it seems like I'm purely trying to avoid using InjectedScriptAccess.getProperties(), which seems kind of silly for the effort.

[Timothy Hatcher]

Refactoring the ObjectProperties classes to be more flexable is the correct approch. When we get around to improving those in the future (we have some planned improvments) I don't want to do the changes twice. Refactoring and sharing now means the watchpoints will get any benefits in the future. It isn't about avoiding InjectedScriptAccess.getProperties().

[Patrick Mueller]

Created attachment 39458 [details]
proposed patch - 2009/09/11

Refactored version of previous patch, to subclass ObjectPropertiesSection and friends rather than use it compositionally.

The code is smaller than before, but less clear, as it relies on intimate knowledge superclass/framework behavior (my opinion).

Two changes were required in ObjectPropertiesSection.js:

- needed to allow additional customization of the tree element constructor - in this case, I needed a special class for the root level entries in the tree (the expressions), which have different behaviour than properties.  This could be shaped differently somehow, haven't thought about the best way to do it - perhaps something like a Strategy object.  The change I've made should make it easier for people who need a unique tree element constructor in the roots rather than below the roots.

- second change was to make nameElement an "instance variable" instead of a local, as I need access to it later.  As a local, the reference to it was lost after it was created.  Note that StylesSidebarPane.js  also appears to creates an instance of nameElement in the same vein, but that class doesn't use ObjectPropertiesSection, so there's no conflict.  I saw no other potential conflcts for exposing this property.

There are some comments in WatchExpressionSidebarPane.js for things that didn't feel quite right, or were working around some existing bugs.

[Timothy Hatcher]

Comment on attachment 39458 [details]
proposed patch - 2009/09/11

> +    _update: function(properties, treeElementConstructor)

This optional argument should be called rootTreeElementConstructor to better match what you use it for.


>      this.sidebarPanes.callstack = new WebInspector.CallStackSidebarPane();
> +    this.sidebarPanes.watchExpressions = new WebInspector.WatchExpressionsSidebarPane();
>      this.sidebarPanes.scopechain = new WebInspector.ScopeChainSidebarPane();

Watch Expressions should go after this.sidebarPanes.scopechain, since callstack is closly related to scopechain they should be next to each other visually.


> +    // ideally we'd intelligently expand if we have any watch expressions,
> +    // but there's a timing error of some kind with the InjectedScriptAccess,
> +    // so forcing expanded = false, and refresh upon expansion
> +    this.expanded = false;
> +    this.onexpand = this.refreshExpressions.bind(this);

I can see how this would be too early to expand, but I think you might be able to expand in ScriptsPanel.reset, which should be called at a good time.


> +//------------------------------------------------------------------------------

No need for the comment divider, but I guess it dosen't hurt.


> +    this.treeElementConstructor = WebInspector.ObjectPropertyTreeElement;

The default is already WebInspector.ObjectPropertyTreeElement. You shouldn't need to se this.


> +WebInspector.WatchExpressionsSection.prototype = {
> +
> +    update: function()

Remove the extra line before update.


> +            // the null check catches some other cases, like null itself,
> +            // and NaN

Just make this one comment on one line. I also find comments with a starting capital letter easier to read.


> +            properties.push(property);
> +            if (properties.length == propertyCount)
> +                this._update(properties, WebInspector.WatchExpressionTreeElement);

And extra line before the if would help readability and separate things. A comment before the if would also help, it took me a bit to undertstand, I had to find the other comment later when you are setting propertyCount.


> +        for (var i=0; i<this.watchExpressions.length; ++i) {

Need spaces around the "=" and "<".


> +        for (var i=0; i<this.watchExpressions.length; ++i) {

Ditto.


> +            WebInspector.console._evalInInspectedWindow(expression, appendResult.bind(this, expression, i));

You should make _evalInInspectedWindow public. I would recomend moving it to inspector.js can calling it WebInspector.evalInInspectedWindow. That is where it should have been all along.


> +        this.expanded = (propertyCount != 0);

I don't think you should do this for every update. I think expanding once when there are expressions to show is fine, but I don't want this pane to auto expand all the time when stepping through JS. Wack-a-Mole is not fun when you can't win!


> +    _displaySort: function(propertyA, propertyB)
> +    {
> +        if (propertyA.watchIndex == propertyB.watchIndex)
> +            return 0;
> +        else if (propertyA.watchIndex < propertyB.watchIndex)
> +            return -1;
> +        else
> +            return 1;
> +    },

This function looks unused. Remove.


> +        // unpretty code here; nowhere else do we drill down
> +        // into tree elements, but we need to do it here as 
> +        // the newly added element needs to be added to the 
> +        // tree BEFORE we start editing it.

This comment isn't helpful. The code is fine.


> +        for (var i=0; i<children.length; ++i)

Need spaces around "=" and "<".


> +        if (!window.JSON)
> +            return [];

When will this fail? Can be removed.


> +        }
> +        catch(e) {

This should be "} catch (e) {".


> +        if (!window.JSON)
> +            return;

Ditto. Can be removed.


> +        for (var i=0; i<this.watchExpressions.length; i++)

Need spaces around the "=" and "<".


> +    }
> +
> +}

Remove the empty line.


> +        var deleteButton;
> +        deleteButton = document.createElement("input");

Can be combined onto one line. Also "button" elements are better to use.


> +        if (this.property.name === "")
> +            this.nameElement.textContent = "\xA0\xA0\xA0\xA0\xA0\xA0\xA0\xA0\xA0";

Why do you do this? If my guess is correct, I think a single (normal) space will be fine.


> +        WebInspector.startEditing(
> +            this.nameElement,
> +            this.editingCommitted.bind(this),
> +            this.editingCancelled.bind(this),
> +            context);

Just put this on one line. Wrapped function calls are annoying to read.


> +        if (expression === "")
> +            expression = undefined;

It is better to use null than undefined. But why do you even need this? Testing !expression is true for "" as well as undefined and null.


> +    },
> +
> +}

Remove the empty line.


> +    background-image: url(Images/errorIcon.png);

I don't think errorIcon is the best thing to use. You should add a FIXME comment and file a bug requesting new art and assign it to me.


This patch is much nicer than the earlier version! Viva less code!

[Patrick Mueller]

(In reply to comment #22)

> Watch Expressions should go after this.sidebarPanes.scopechain, since callstack
> is closly related to scopechain they should be next to each other visually.

Actually, I'm wondering if it should go BEFORE scopechain (callstack).  Rationale would be that while Call Stack and Scope Variables grow and shrink, sometimes wildly as you traverse around your code (step/go), Watch Expressions grows/shrinks at a user-controlled pace - and probably not at all in most cases.  This would provide some stability to the location of the pane, especially if it's something you're watching closely.

In terms of the space it eats, my intention was to have the Watch Expressions pane only be expanded upon opening of the inspector if there are actually Watch Expressions in the list.  If there are none, it should open in expanded=false mode.  So if you're not using Watch Expressions, they only eat the section header in terms of vertical space.

> > +//------------------------------------------------------------------------------
> 
> No need for the comment divider, but I guess it dosen't hurt.

Woops.  This is a personal coding style of mine - I put these comment dividers in front of every function to help break the code visually into distinct chunks.  Will remove, unless we're going for a new coding style :-)

> > +            WebInspector.console._evalInInspectedWindow(expression, appendResult.bind(this, expression, i));
> 
> You should make _evalInInspectedWindow public. I would recomend moving it to
> inspector.js can calling it WebInspector.evalInInspectedWindow. That is where
> it should have been all along.

I agree this API needs to be made public and moved somewhere else.  But I'd like to do it in a separate patch:

- I would need to patch ConsoleView.js which otherwise has nothing to do with this patch

- I'd like to actually step back and see if we can find a good home for all these target-introspective APIs.  Putting it just on WebInspector itself probably doesn't make too much sense - perhaps a new class called TargetMirror or something? 

> 
> 
> > +        this.expanded = (propertyCount != 0);
> 
> I don't think you should do this for every update. I think expanding once when
> there are expressions to show is fine, but I don't want this pane to auto
> expand all the time when stepping through JS. Wack-a-Mole is not fun when you
> can't win!

This isn't the pane expanding, this is the treeoutline inside the pane expanding.  The problem was that if the list of Watch Expressions is empty, but the Watch Expressions pane is expanded, you get a free 25px or so empty vertical space.  This hack defeats that.  But it was a bit of a hack, from memory.  I believe the existing panes (Call Stack et al) don't run into this problem, as they all have a "No [whatevers]" tree element they add when their list is empty.  

Now that we have the ability to add "menu buttons" to the pane headers (like the Styles header), perhaps the best thing to do is to move the buttons in Watch Expressions to new buttons in the header, and then provide a "No Watch Expressions" label like the other panes.  If so, I'd like to do this in a separate patch, as it's not immediately clear to me if we would want two header buttons, or one 'tool' button with two drop downs, or something else.

> > +    _displaySort: function(propertyA, propertyB)
> > +    {
> > +        if (propertyA.watchIndex == propertyB.watchIndex)
> > +            return 0;
> > +        else if (propertyA.watchIndex < propertyB.watchIndex)
> > +            return -1;
> > +        else
> > +            return 1;
> > +    },
> 
> This function looks unused. Remove.

The hidden pain points of dealing with subclassing.  This method is used by a superclass.  If I deleted it, the Watch Expressions would be sorted alphabetically (or something - it would assume the expressions are property names).  I'll make this method "public" in the superclass, rename it to something better - comparePropertyNames?, and add a comment in the Watch Expressions code indicating that this is used by the superclass.

> > +        // unpretty code here; nowhere else do we drill down
> > +        // into tree elements, but we need to do it here as 
> > +        // the newly added element needs to be added to the 
> > +        // tree BEFORE we start editing it.
> 
> This comment isn't helpful. The code is fine.

I happen to disagree, I find the code rather icky.  But it was a bit of a passive/aggressive comment, so will delete :-)

> > +        if (!window.JSON)
> > +            return [];
> 
> When will this fail? Can be removed.

The original code was written when window.JSON wasn't always available!  Will remove the checks.
 
> > +        if (this.property.name === "")
> > +            this.nameElement.textContent = "\xA0\xA0\xA0\xA0\xA0\xA0\xA0\xA0\xA0";
> 
> Why do you do this? If my guess is correct, I think a single (normal) space
> will be fine.

I'm curious what your guess is.  Using these non-breaking spaces allows for an obvious visual selection box when a new Watch Expression is added.  A single space yields a text box that's so small you wouldn't even know you can edit text in there.  If there was a way to css this issue away, that'd be great.  Something in the WebInspector.element editing protocol that would set an initial interesting horizontal width when the initial element contents are empty.  Beyond my css-foo though.

The way it works now seems pretty pleasing to me.  You start with an "interesting width" - maybe 5-7 char wide text box, selected, which makes it pretty obvious where to start typing your expression.  As you start typing, the initially selected nbsp's are replaced with the user-entered contents.  It might be a little clearer to make the entire horizontal line selected during the entire edit, but I don't have a clue how we'd do that with the current element editing framework.

[Timothy Hatcher]

(In reply to comment #23)
> (In reply to comment #22)
> 
> > Watch Expressions should go after this.sidebarPanes.scopechain, since callstack
> > is closly related to scopechain they should be next to each other visually.
> 
> Actually, I'm wondering if it should go BEFORE scopechain (callstack). 
> Rationale would be that while Call Stack and Scope Variables grow and shrink,
> sometimes wildly as you traverse around your code (step/go), Watch Expressions
> grows/shrinks at a user-controlled pace - and probably not at all in most
> cases.  This would provide some stability to the location of the pane,
> especially if it's something you're watching closely.
> 
> In terms of the space it eats, my intention was to have the Watch Expressions
> pane only be expanded upon opening of the inspector if there are actually Watch
> Expressions in the list.  If there are none, it should open in expanded=false
> mode.  So if you're not using Watch Expressions, they only eat the section
> header in terms of vertical space.

Before seems fine too, but the main part about the Scripts panel is the debugger and those two panes. But not shifting too much is always good.


> > > +            WebInspector.console._evalInInspectedWindow(expression, appendResult.bind(this, expression, i));
> > 
> > You should make _evalInInspectedWindow public. I would recomend moving it to
> > inspector.js can calling it WebInspector.evalInInspectedWindow. That is where
> > it should have been all along.
> 
> I agree this API needs to be made public and moved somewhere else.  But I'd
> like to do it in a separate patch:
> 
> - I would need to patch ConsoleView.js which otherwise has nothing to do with
> this patch
> 
> - I'd like to actually step back and see if we can find a good home for all
> these target-introspective APIs.  Putting it just on WebInspector itself
> probably doesn't make too much sense - perhaps a new class called TargetMirror
> or something? 

Either make it public and leave it on ConsoleView in this patch or move it to the WebInspector object. I don't want this to land with a private function being used. Feel free to file follow up bugs to give it a better home, but this can't land until the underscrore is dropped. Don't feel bad about touching ConsoleView.js.


> > > +        this.expanded = (propertyCount != 0);
> > 
> > I don't think you should do this for every update. I think expanding once when
> > there are expressions to show is fine, but I don't want this pane to auto
> > expand all the time when stepping through JS. Wack-a-Mole is not fun when you
> > can't win!
> 
> This isn't the pane expanding, this is the treeoutline inside the pane
> expanding.  The problem was that if the list of Watch Expressions is empty, but
> the Watch Expressions pane is expanded, you get a free 25px or so empty
> vertical space.  This hack defeats that.  But it was a bit of a hack, from
> memory.  I believe the existing panes (Call Stack et al) don't run into this
> problem, as they all have a "No [whatevers]" tree element they add when their
> list is empty.  
> 
> Now that we have the ability to add "menu buttons" to the pane headers (like
> the Styles header), perhaps the best thing to do is to move the buttons in
> Watch Expressions to new buttons in the header, and then provide a "No Watch
> Expressions" label like the other panes.  If so, I'd like to do this in a
> separate patch, as it's not immediately clear to me if we would want two header
> buttons, or one 'tool' button with two drop downs, or something else.

I see. Yes, this would be a better UI. Fine to fix in a follow up patch.


> > > +    _displaySort: function(propertyA, propertyB)
> > > +    {
> > > +        if (propertyA.watchIndex == propertyB.watchIndex)
> > > +            return 0;
> > > +        else if (propertyA.watchIndex < propertyB.watchIndex)
> > > +            return -1;
> > > +        else
> > > +            return 1;
> > > +    },
> > 
> > This function looks unused. Remove.
> 
> The hidden pain points of dealing with subclassing.  This method is used by a
> superclass.  If I deleted it, the Watch Expressions would be sorted
> alphabetically (or something - it would assume the expressions are property
> names).  I'll make this method "public" in the superclass, rename it to
> something better - comparePropertyNames?, and add a comment in the Watch
> Expressions code indicating that this is used by the superclass.

I see. Yes, make it public. The "comparePropertyNames" name is good.


> > > +        if (this.property.name === "")
> > > +            this.nameElement.textContent = "\xA0\xA0\xA0\xA0\xA0\xA0\xA0\xA0\xA0";
> > 
> > Why do you do this? If my guess is correct, I think a single (normal) space
> > will be fine.
> 
> I'm curious what your guess is.  Using these non-breaking spaces allows for an
> obvious visual selection box when a new Watch Expression is added.  A single
> space yields a text box that's so small you wouldn't even know you can edit
> text in there.  If there was a way to css this issue away, that'd be great. 
> Something in the WebInspector.element editing protocol that would set an
> initial interesting horizontal width when the initial element contents are
> empty.  Beyond my css-foo though.
> 
> The way it works now seems pretty pleasing to me.  You start with an
> "interesting width" - maybe 5-7 char wide text box, selected, which makes it
> pretty obvious where to start typing your expression.  As you start typing, the
> initially selected nbsp's are replaced with the user-entered contents.  It
> might be a little clearer to make the entire horizontal line selected during
> the entire edit, but I don't have a clue how we'd do that with the current
> element editing framework.

My guess was what you described. It would be better to do this with CSS. Settign the width of the element to be 100% should work. Maybe you can make new (empty) watchpoints have a special style class that you can use to control the width.

[Patrick Mueller]

Created attachment 39610 [details]
proposed patch - 2009/09/15

All the suggested changes were made.

Additional things:

- I had to move the displaySort business around - turns out I wasn't sorting the properties inside the expressions themselves in the WatchExpressions correctly.  To get this to work right for all the ObjectProperty* bits, the functionality was moved to the tree element prototype.  The sort logic itself was unchanged.

- the callback in ObjectPropertyTreeElement.onpopulate() wasn't working correctly, but I couldn't figure out why; changed from using the "self = this" type of closure reference to bind(this) on the callback, which worked fine.

- had to make the ": " element in the property items a span itself, so I could hide it during edit

- changed the edit code to use css to turn the name span into a block element with width:100%.  Looks better.  No nbsp hacks!

[Timothy Hatcher]

Comment on attachment 39610 [details]
proposed patch - 2009/09/15

> +        properties.sort(rootTreeElementConstructor.prototype.comparePropertyNames);

This should just be rootTreeElementConstructor.comparePropertyNames, to allow instances to override the prototype.

> +            if (property.name === " ") 

Maybe everywhere you check for " " you should just trim whitspace (String.trimWhitespace in utilities.js) and look for "" or !property.name?

I would r+ this, but I would like to see the prototype thing fixed, even though it isn't an issue today.

[Patrick Mueller]

(In reply to comment #26)
> (From update of attachment 39610 [details])
> > +        properties.sort(rootTreeElementConstructor.prototype.comparePropertyNames);
> 
> This should just be rootTreeElementConstructor.comparePropertyNames, to allow
> instances to override the prototype.

Whats uber-confusing about this, is that the sorting takes place before passing the names to the class which is going to handle them.  And it needs to be inheritable, so that ScopeVariableTreeElement picks up the default from ObjectProperties, but allowing the root collection of WatchExpressions to do something different.  Took me a while to even get it to work correctly in all cases with this scheme.

Let me noodle on this a bit more, clearly the code is more complicated than it should be.  It might well be that making it instance-based customizable is actually the simplest thing to do.

> > +            if (property.name === " ") 
> 
> Maybe everywhere you check for " " you should just trim whitspace
> (String.trimWhitespace in utilities.js) and look for "" or !property.name?
> 
> I would r+ this, but I would like to see the prototype thing fixed, even though
> it isn't an issue today.

I'm down to using a single " " character as the signal for "this is a newly added element which needs to be edited".  Empty space, or actually, as you suggest testing for (!whatever) is used everywhere else to signal "this item needs to be deleted".  A little confusing.  Since I don't really use the " " character productively (or don't need to), I'm thinking maybe I should making this some kind of non-string constant that I check for, just to make this more clear.

[Patrick Mueller]

Created attachment 39648 [details]
proposed patch - 2009/09/16

Since the previous patch:

- element added when user selects "Add" now uses a 'constant' for the name, WebInspector.WatchExpressionsSection.NEW_WATCH_EXPRESSION, rather than just " " as a special case, to make things a little clearer.  The value has also been changed to "\xA0", which works as well as space where it needs to (evaluates without issues, is stripped with trimWhitespace(), etc), and is less likely in negatively interact with functioning in the future if for some reason someone wants to use " " for a valid or special purpose in the expressions.

- WatchExpressionsSection.update() was previously calling ObjectPropertiesSection._update().  I renamed the _update() method to updateProperties() in all callers and the single implementation, to make it public, and make the name a little more obvious.

- property compare has been jiggled around.  It's now available as a 'class property' - WebInspector.ObjectPropertiesSection.PropertyComparer for ObjectProperties* stuff, and WebInspector.WatchExpressionsSection.PropertyComparer for Watch Expressions.  The reason it can't easily be just a regular instance method, is for two reasons:

   - it's not an instance method, it's a function - it doesn't get called with a this.  It would be confusing to have it as an instance method (function on a prototype), since there is no reason to ever call it that way.
   
   - it's used in two places in ObjectProperties*; once in ObjectPropertiesSection to sort the top level list of properties, and then in ObjectPropertyTreeElement to sort all the further nested properties.  That second one needs to reference the comparer function somehow.
   
It's very confusing to sort out how the second reference should be obtained, programmatically, as the hierarchies get tangly, and most confusingly, the properties are sorted before being handed off to the element tree constructors (which is one place you might think to hang this function).  
   
To make things a little more straight-forward, what I've done is added the comparator functions to the relevent section contructors as properties (not on the prototype, just properties off the constructor), as mentioned above.  I've then added a further customization parameter in the top-level element building function in ObjectPropertiesSection, now called updateProperties (see above), to take an optional comparator.  This will default to use the version in ObjectPropertiesSection.  WatchExpressions will pass in an override.  In the second use of the comparator, in the ObjectPropertyTreeElement, I've hardcoded the reference to the ObjectPropertiesSection, since today that's all anyone uses.
   
I think the result is fairly easy to understand.  It doesn't neccessarily feel like the best shape, but I think that's largely because the ObjectProperties* class hierarchies are getting a bit convoluted in general.  It was the most natural thing I came up with.  If we need to extend them further, it might be time for some more serious refactoring.

[Timothy Hatcher]

(In reply to comment #27)
> (In reply to comment #26)
> > (From update of attachment 39610 [details] [details])
> > > +        properties.sort(rootTreeElementConstructor.prototype.comparePropertyNames);
> > 
> > This should just be rootTreeElementConstructor.comparePropertyNames, to allow
> > instances to override the prototype.
> 
> Whats uber-confusing about this, is that the sorting takes place before passing
> the names to the class which is going to handle them.  And it needs to be
> inheritable, so that ScopeVariableTreeElement picks up the default from
> ObjectProperties, but allowing the root collection of WatchExpressions to do
> something different.  Took me a while to even get it to work correctly in all
> cases with this scheme.
> 
> Let me noodle on this a bit more, clearly the code is more complicated than it
> should be.  It might well be that making it instance-based customizable is
> actually the simplest thing to do.

All I was saying is that you should be able to just change it to rootTreeElementConstructor.comparePropertyNames and it will just work. I don't see the problem where the instance hasn't been told about the properties yet. This function is designed to not need internal data. Just like _displaySort was. All you needed to do was rename it.

> > > +            if (property.name === " ") 
> > 
> > Maybe everywhere you check for " " you should just trim whitspace
> > (String.trimWhitespace in utilities.js) and look for "" or !property.name?
> > 
> > I would r+ this, but I would like to see the prototype thing fixed, even though
> > it isn't an issue today.
> 
> I'm down to using a single " " character as the signal for "this is a newly
> added element which needs to be edited".  Empty space, or actually, as you
> suggest testing for (!whatever) is used everywhere else to signal "this item
> needs to be deleted".  A little confusing.  Since I don't really use the " "
> character productively (or don't need to), I'm thinking maybe I should making
> this some kind of non-string constant that I check for, just to make this more
> clear.

Yes, using a named constant is good.

[Timothy Hatcher]

Comment on attachment 39648 [details]
proposed patch - 2009/09/16

> +WebInspector.ObjectPropertiesSection.PropertyComparer = function(propertyA, propertyB) 

WebInspector.ObjectPropertiesSection.CompareProperties would be a better name. PropertyComparer sounds more like a class name.


> +};

No need for the semicolon.


> +    
>  WebInspector.ObjectPropertiesSection.prototype = {

The compare function you added should come after the prototype, so the prototype and the constructor are next to each other.


> +    updateProperties: function(properties, rootTreeElementConstructor, propertyComparer)

I think propertyComparer should be rootPropertyComparer since it is only used for root items.


> +WebInspector.WatchExpressionsSection.NEW_WATCH_EXPRESSION = "\xA0";

This should not be all caps, it should be camelcase.


> +WebInspector.WatchExpressionsSection.PropertyComparer = function(propertyA, propertyB) 

Ditto (re: the PropertyComparer name)


> +};

No need for the semicolon.


You also need to add WatchExpressionsSidebarPane.js to the inspector/front-end/WebKit.qrc, WebCore.gypi and WebCore.vcproj/WebCore.vcproj for the other platforms.

[Timothy Hatcher]

(In reply to comment #28)
> - element added when user selects "Add" now uses a 'constant' for the name,
> WebInspector.WatchExpressionsSection.NEW_WATCH_EXPRESSION, rather than just " "
> as a special case, to make things a little clearer.  The value has also been
> changed to "\xA0", which works as well as space where it needs to (evaluates
> without issues, is stripped with trimWhitespace(), etc), and is less likely in
> negatively interact with functioning in the future if for some reason someone
> wants to use " " for a valid or special purpose in the expressions.

This is fine, but we don't use all caps. Should be CamelCase.


> - property compare has been jiggled around.  It's now available as a 'class
> property' - WebInspector.ObjectPropertiesSection.PropertyComparer for
> ObjectProperties* stuff, and
> WebInspector.WatchExpressionsSection.PropertyComparer for Watch Expressions. 

This is a good solution.


> It's very confusing to sort out how the second reference should be obtained,
> programmatically, as the hierarchies get tangly, and most confusingly, the
> properties are sorted before being handed off to the element tree constructors
> (which is one place you might think to hang this function).  

It could be passed to the ObjectPropertiesSection constructor to store and hold. There is a argument on the constructor already for the tree element constructor.

> I think the result is fairly easy to understand.  It doesn't neccessarily feel
> like the best shape, but I think that's largely because the ObjectProperties*
> class hierarchies are getting a bit convoluted in general.  It was the most
> natural thing I came up with.  If we need to extend them further, it might be
> time for some more serious refactoring.

The solution is good. Better than it was before. But I don't see what is wrong with the class hierarchies.

[Patrick Mueller]

Created attachment 39657 [details]
proposed patch - 2009/09/16 - 2

all issues fixed.

I didn't actually test the gypi, vcproj or qrc file - I'm getting set up now to build on Windows (dusting off a dusty ole box).

[WebKit Commit Bot]

Comment on attachment 39657 [details]
proposed patch - 2009/09/16 - 2

Clearing flags on attachment: 39657

Committed r48431: <http://trac.webkit.org/changeset/48431>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.