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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Sep 12 17:50:08 PDT 2009


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


Timothy Hatcher <timothy at hatcher.name> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #39458|review?                     |review-
               Flag|                            |




--- Comment #22 from Timothy Hatcher <timothy at hatcher.name>  2009-09-12 17:50:07 PDT ---
(From update of attachment 39458)
> +    _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!

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



More information about the webkit-unassigned mailing list