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

Mon Sep 14 10:15:44 PDT 2009

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

--- Comment #24 from Timothy Hatcher <timothy at hatcher.name>  2009-09-14 10:15:42 PDT ---
(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.

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