[webkit-reviews] review denied: [Bug 27514] add support for watched expressions : [Attachment 39458] proposed patch - 2009/09/11

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied Patrick Mueller
<pmuellr at yahoo.com>'s request for review:
Bug 27514: add support for watched expressions
https://bugs.webkit.org/show_bug.cgi?id=27514

Attachment 39458: proposed patch - 2009/09/11
https://bugs.webkit.org/attachment.cgi?id=39458&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
> +    _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!