It's begun to bug me enough now. STR: 1. Open about:blank 2. Open the JavaScript Object viewer in the right pane on the BODY node 3. Set the property "fum" on the BODY node (using Evaluate JavaScript… from the context menu, or some other out-of-band means) 4. Close the root node "(Subject)" 5. Open the root node Expected results: "fum" is now locatable underneath the root node. Actual results: "fum" is nowhere to be found. Because of the tree view implementation (which is a content tree view, by the way...), the row data gets cached the first time a row group is expanded. While there are valid reasons why rows don't automatically appear as objects' members are dynamically added/removed/updated, I'd expect that closing the parent row and reopening would regenerate row data based on object keys. (Aside: The fact that it's using a content tree at all is pretty gross to me as well. I'd prefer that in the course of this, that gets ripped out and replaced with a JS implementation.)

A note: Recall that getSelectedRowObjects is a utility method that comes along with inBaseTreeView. It expects the view to implement getRowObjectFromIndex. That's what the getSelectedRowObjects call in JSOVr_GetSelectedObject is about. Another bonus of the custom view: If an object with no properties suddenly gains some or all of the properties on an object are deleted, the twisties now appear or disappear as appropriate whenever the corresponding row is invalidated. And another: No more DOM mutation listeners.

Comment on attachment 535850 [details] [diff] [review] Use JS tree view >- this.mSubject = accObject; >- this.emptyTree(this.mTreeKids); >- var ti = this.addTreeItem(this.mTreeKids, >- bundle.getString("root.title"), >- accObject, >- accObject); >- ti.setAttribute("open", "true"); >- >- this.mObsMan.dispatchEvent("subjectChange", { subject: accObject }); >+ superSubjectSetter.call(this, accObject); I don't think this change belongs in this patch. But I'll comment on it anyway: I think it would be more readable if the superclass object had a "protected" setSubject() helper method for the subclass to call. >+JSObjectView.prototype.jsValueToString = function JSOV_JSValueToString(aVal) > { >+ if (aVal === null) { >+ return "null"; > } >- return null; >+ if (aVal === undefined) { >+ return "undefined"; By comparison, the old code wrapped these in ()s. Any reason for the change? I notice the old code also replaced newlines with spaces. >+JSObjectView.makeKeySortComparator = >+ function JSOV_MakeKeySortComparator(aObject) >+{ >+ return function JSOV_KeySortComparator(a, b) I don't see how it helps to have this function local to makeKeySortComparator when it can be local to its caller. >+ // Splice in the new keys. >+ var after = aIndex + 1; >+ Array.prototype.splice.apply(this.mKeys, ([after, 0]).concat(insertedKeys)); [You can also write this as this.mKeys = this.mKeys.splice(0, after).concat(insertedKeys).concat(this.mKeys); ] >+ // Shift down the data. >+ var rowsInserted = insertedKeys.length; [You can also write this as this.mValues = this.mValues.splice(0, after).concat(new Array(rowsInserted)).concat(this.mValues); etc.] >+ this.mOpen[i] = false; [Technically the array contains non-true values already...] >+ this.mLevels[i] = level; [Shame JS has no array fill method.] >+ bo.beginUpdateBatch(); Not worth a batch for this trivial change. >+JSObjectView.prototype.hasNextSibling = [This could almost belong on inBaseView but I guess it would have to call getLevel all the time.]

(In reply to comment #2) > Comment on attachment 535850 [details] [diff] [review] [review] > Use JS tree view > > >- this.mSubject = accObject; > >- this.emptyTree(this.mTreeKids); > >- var ti = this.addTreeItem(this.mTreeKids, > >- bundle.getString("root.title"), > >- accObject, > >- accObject); > >- ti.setAttribute("open", "true"); > >- > >- this.mObsMan.dispatchEvent("subjectChange", { subject: accObject }); > >+ superSubjectSetter.call(this, accObject); > I don't think this change belongs in this patch. It does. The accessibleObject viewer pulls in jsObjectViewer.xul and will break otherwise. > >+JSObjectView.prototype.jsValueToString = function JSOV_JSValueToString(aVal) > > { > >+ if (aVal === null) { > >+ return "null"; > > } > >- return null; > >+ if (aVal === undefined) { > >+ return "undefined"; > By comparison, the old code wrapped these in ()s. Any reason for the change? Only because I couldn't figure out why they were there to begin with. > I notice the old code also replaced newlines with spaces. The original had the effect of making it largely useless, IMO. Trees already flatten the text to one line in the cells. The difference is that before, the tooltip showed the flattened text, making it difficult to read. Now for functions, for example, the toString pretty-printing stays intact. > > >+JSObjectView.makeKeySortComparator = > >+ function JSOV_MakeKeySortComparator(aObject) > >+{ > >+ return function JSOV_KeySortComparator(a, b) > I don't see how it helps to have this function local to > makeKeySortComparator when it can be local to its caller. Well yeah, but then, e.g., collapseRow and expandRow could be inlined also (and I wrote it that way at first). expandRow (the caller) seemed hairy enough; it's a screenful. Want me to change it? > >+ bo.beginUpdateBatch(); > Not worth a batch for this trivial change. The IDL says "Notify the tree that the view is about to perform a batch update, that is, add, remove or invalidate several rows at once." Is it not possible for a paint to occur in the middle of these operations? <http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#2823> I don't know what the deal is on concurrency here.

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 535850 [details] [diff] [review]) > > >+ superSubjectSetter.call(this, accObject); > > I don't think this change belongs in this patch. > It does. The accessibleObject viewer pulls in jsObjectViewer.xul and will > break otherwise. Ah, I see now. I still think some sort of init method would work better though. > > >+ if (aVal === undefined) { > > >+ return "undefined"; > > By comparison, the old code wrapped these in ()s. Any reason for the change? > Only because I couldn't figure out why they were there to begin with. Fair enough. > > I notice the old code also replaced newlines with spaces. > The original had the effect of making it largely useless, IMO. Trees > already flatten the text to one line in the cells. The difference is that > before, the tooltip showed the flattened text, making it difficult to read. > Now for functions, for example, the toString pretty-printing stays intact. Ooh, shiny! > > >+JSObjectView.makeKeySortComparator = > > >+ function JSOV_MakeKeySortComparator(aObject) > > >+{ > > >+ return function JSOV_KeySortComparator(a, b) > > I don't see how it helps to have this function local to > > makeKeySortComparator when it can be local to its caller. > Well yeah, but then, e.g., collapseRow and expandRow could be inlined also > (and I wrote it that way at first). expandRow (the caller) seemed hairy > enough; it's a screenful. I said I wanted the local function to be in the same method as its caller. It occurs to me that, rather than moving the function, moving the call to sort to be in the same method that contains the local function is a perfectly satisfactory alternative, i.e. something like this.sortObjectKeys(obj, insertedKeys); > > >+ bo.beginUpdateBatch(); > > Not worth a batch for this trivial change. > The IDL says "Notify the tree that the view is about to perform a batch > update, that is, add, remove or invalidate several rows at once." Is it not > possible for a paint to occur in the middle of these operations? Hmm, the IDL really needs to say several calls to add, remove or invalidate rows. And you are doing two calls, but I don't think that's enough to be several, since a batch update always invalidates the whole tree anyway. (The one edge case is removing and reinserting a row.)

Comment on attachment 536201 [details] [diff] [review] JS tree view v2 (In reply to comment #2) > >+ // Shift down the data. > >+ var rowsInserted = insertedKeys.length; > [You can also write this as this.mValues = this.mValues.splice(0, > after).concat(new Array(rowsInserted)).concat(this.mValues); etc.] I spent a stupid amount of time looking into the complexity and performance of three approaches. The trouble with the one above, on paper, is that the number of data movements involved is .splice(0, after) |xs| + |zs| + .concat(…) |ys| + .concat(this.mValues) |zs| where xs is the list of elements that are before the insertion point in the original array, ys is the list of inserted elements, and zs is the list of elements that are after the insertion point in the original array, with some assumptions that the operations backed by native code are performed in a straightforward way, i.e., without taking any shortcuts that I'm not accounting for. That's |zs| data movements more than the approach with the data-shifting loop. So I ran some tests, and the one above, the Array.prototype.splice.apply(destination, ([0, after]).concat(source)) approach, and the loop seemed to be about the same. Then I realized that since I didn't do a reduced testcase focusing only on data movements, it's possible/likely that other junk was dominating the time measurements. But I don't care anymore. I ultimately went with the Array.prototype.splice.apply approach over the splice, concat, concat approach for two stylistic reasons: 1. I don't like how the side effects of splice change the way the second concat would seem to work at first glance, and 2. It doesn't modify the original array to hold the final result, which means adding a return value and assigning to the original array. (In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 535850 [details] [diff] [review] [review]) > > > >+ superSubjectSetter.call(this, accObject); > > > I don't think this change belongs in this patch. > > It does. The accessibleObject viewer pulls in jsObjectViewer.xul and will > > break otherwise. > Ah, I see now. I still think some sort of init method would work better > though. Oh, yeah, agreed. > I said I wanted the local function to be in the same method as its caller. > It occurs to me that, rather than moving the function, moving the call to > sort to be in the same method that contains the local function is a > perfectly satisfactory alternative, i.e. something like > > this.sortObjectKeys(obj, insertedKeys); Okay. > Hmm, the IDL really needs to say several calls to add, remove or invalidate > rows. And you are doing two calls Which calls are the two we're talking about here?

(In reply to comment #6) > (In reply to comment #2) > > >+ // Shift down the data. > > >+ var rowsInserted = insertedKeys.length; > > [You can also write this as this.mValues = this.mValues.splice(0, > > after).concat(new Array(rowsInserted)).concat(this.mValues); etc.] > I spent a stupid amount of time looking into the complexity and performance > of three approaches. More time than you would have saved by picking one approach and sticking to it ;-) > 1. I don't like how the side effects of splice change the way the second > concat would seem to work at first glance, and Fair enough. > 2. It doesn't modify the original array to hold the final result, which > means adding a return value and assigning to the original array. [Note that the previous version of the code modified the member directly.] > > Hmm, the IDL really needs to say several calls to add, remove or invalidate > > rows. And you are doing two calls > Which calls are the two we're talking about here? rowCountChanged and invalidateRow.

(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #2) > > > >+ // Shift down the data. > > > >+ var rowsInserted = insertedKeys.length; > > > [You can also write this as this.mValues = this.mValues.splice(0, > > > after).concat(new Array(rowsInserted)).concat(this.mValues); etc.] > > I spent a stupid amount of time looking into the complexity and performance > > of three approaches. > More time than you would have saved by picking one approach and sticking to > it ;-) Right. That's what I meant to say. Not to make a complaint about undue hardship. > > 2. It doesn't modify the original array to hold the final result, which > > means adding a return value and assigning to the original array. > [Note that the previous version of the code modified the member directly.] Attachment 536201 [details] [diff] does, too. The splice, concat, concat approach doesn't (requiring this.mKeys = …, etc.), while the approach I chose does.

(In reply to comment #8) > (In reply to comment #7) > > [Note that the previous version of the code modified the member directly.] > Attachment 536201 [details] [diff] does, too. No, it passes a reference to the array to a helper function, which therefore doesn't know which member you're modifying, although this isn't a problem.

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > [Note that the previous version of the code modified the member directly.] > > Attachment 536201 [details] [diff] does, too. > No, it passes a reference to the array to a helper function, which therefore > doesn't know which member you're modifying, although this isn't a problem. All right. Wrong interpretation of "directly" on my part.

Comment on attachment 536201 [details] [diff] [review] JS tree view v2 I thought I had another comment, but I can't remember what it was now. >+ str = Object.toString(aVal); Object.toString() doesn't work for me. (Well, it always returns the string of Object, which isn't what you want.) >+ var level = this.mLevels[aIndex]; >+ var count = 0; >+ for (let i = aIndex + 1, n = this.mRowCount; i < n; ++i) { >+ if (this.mLevels[i] <= level) { >+ break; >+ } >+ ++count; >+ } >+ >+ return count; It seems to me that i == aIndex + count + 1 at all times. So you could avoid a variable. var i = aIndex + 1; while (i <= n && this.mLevels[i] > level) ++i; return i - aIndex - 1; >+ Array.prototype.splice.apply(aDestination, ([aIndex, 0]).concat(aSource)); Nit: don't need ()s around []s >+ Components.utils.reportError("Unrooted rows present"); [Not sure whether it's legal to request the parent of a leftmost row.]

(In reply to comment #11) > >+ str = Object.toString(aVal); > Object.toString() doesn't work for me. (Well, it always returns the string > of Object, which isn't what you want.) Hm, yeah. I thought toString was a "generic native" <http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#522", but it looks like the bulk of String and Array methods are the only such methods. > >+ Array.prototype.splice.apply(aDestination, ([aIndex, 0]).concat(aSource)); > Nit: don't need ()s around []s [Feels weird without 'em, though.] > >+ Components.utils.reportError("Unrooted rows present"); > [Not sure whether it's legal to request the parent of a leftmost row.] It doesn't look like it, but I don't know what you're getting at. Remove the reportError call? The return in the tail, too? Besides a 0-level row (at index > 0), this path would also be reached if there were some non-0-level rows before the first occurrence of a 0-level row. Granted, that's not going to happen, but if we remove that line and the return, it would make sense to remove the index checking at the start, too. The last two lines of that function are only there as a safeguard and to avoid function-might-not-always-return paranoia.

(In reply to comment #12) > Besides a 0-level row (at index > 0), this path would also be reached if > there were some non-0-level rows before the first occurrence of a 0-level > row. Granted, that's not going to happen, but if we remove that line and > the return, it would make sense to remove the index checking at the start, > too. Ah, I forgot, you only have one 0-level row, and it's at index 0, and I doubt anyone's interested in its parent index. Makes sense.

Comment on attachment 539019 [details] [diff] [review] JS tree view v3, address comment 11 >+JSObjectView.prototype.jsValueToString = function JSOV_JSValueToString(aVal) > { >+ if (aVal === null) { >+ return "null"; > } >+ if (aVal === undefined) { >+ return "undefined"; >+ } >+ >+ var str; >+ try { >+ str = aVal.toString(); I've been poking around and it seems to me that you can replace this with str = String(aVal) as that works with null and undefined too.

Pushed with that change: http://hg.mozilla.org/dom-inspector/rev/978c037fbb7e