[Webkit-unassigned] [Bug 27514] add support for watched expressions

Mon Sep 14 06:41:39 PDT 2009

https://bugs.webkit.org/show_bug.cgi?id=27514

--- Comment #23 from Patrick Mueller <pmuellr at yahoo.com>  2009-09-14 06:41:38 PDT ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.