[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