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

[bugzilla-daemon at webkit.org]

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





--- Comment #29 from Timothy Hatcher <timothy at hatcher.name>  2009-09-16 09:25:35 PDT ---
(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.

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