User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.11) Gecko/20050728 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.11) Gecko/20050728 In the DOM Inspector, you can select multiple nodes (ctrl+click) in the tree. However, when you try to delete (del key or right click: delete), only the most recently selected node is deleted. Afterewards, the others remain highlighted but are not active (pressing del does nothing for the still remaining highlighted nodes in the tree). Reproducible: Always Steps to Reproduce: 1. Open DOM Inspector and load a page. 2. Expand the tree atleast one level. 3. Hold ctrl and click multiple nodes, each will be highlighted for your selection. 4. Press the del key. Actual Results: Only the most recently selected node in the multiple selection gets deleted. Expected Results: Each selected node should have been deleted in a single delete command.

(In reply to comment #0) > Afterewards, the others remain highlighted > but are not active (pressing del does nothing for the still remaining > highlighted nodes in the tree). Fixed by bug 297660?

Yes, that part was fixed by bug 297660. Next time, please only use the most recent trunk build (Mozilla1.7 is well over a year old) to test if the bug still exists. http://ftp.scarlet.be/pub/mozilla.org/firefox/nightly/latest-trunk/ The multiple selection not deleting the whole multiple selection is a valid bug, though.

I can confirm this.

Not only can I confirm, but I can submit a patch :) Someone care to assign this to me please?

Sure, nice job, Shawn!

are any of these fixes going to make their way to FF2? ~B

Comment on attachment 225933 [details] [diff] [review] Patch to fix > cmdEditDelete.prototype = > { > node: null, > nextSibling: null, > parentNode: null, >+ > Fix that. >+ for (var i = 0; i < this.node.length; ++i) { >+ if (this.nextSibling[i]) >+ this.parentNode[i].insertBefore(this.node[i], this.nextSibling[i]); >+ else >+ this.parentNode[i].appendChild(this.node[i]); >+ } insertBefore acts like appendChild if the second argument is null, so do insertBefore unconditionally.

Comment on attachment 225933 [details] [diff] [review] Patch to fix seems fine w/ me (address np's comment), thanks for contributing

Comment on attachment 225933 [details] [diff] [review] Patch to fix >+ > Oops ;-) > doTransaction: function() > { >+ var node = this.node ? this.node : viewer.selectedNodes; > if (node) { > this.node = node; >+ this.nextSibling = []; >+ this.parentNode = []; >+ for (var i = 0; i < node.length; ++i) { >+ this.nextSibling.push(node[i].nextSibling); >+ this.parentNode.push(node[i].parentNode); >+ node[i].parentNode.removeChild(node[i]); >+ } >+ var ind = node.length - 1; >+ var selectNode = this.nextSibling[ind]; >+ if (!selectNode) selectNode = node[ind].previousSibling; >+ if (!selectNode) selectNode = this.parentNode[ind]; > viewer.selectElementInTree(selectNode); > } > }, Ok, so there are several problems here: 1) you should rename all occurences of node to nodes since it's an array 2) you save all the nodes and their next siblings, but you might end up deleting the next siblings, which means you need to undo in reverse order 3) when determining the node to select you get its previous sibling although the node has already been deleted by then Note: if you're careful, you might find it easier to delete in reverse order >+ viewer.selectElementInTree(this.node[0]); I guess there's no easy way to select all the undeleted nodes?

r=timeless >address np's comment check - see remarks below >Oops ;-) Indeed >1) you should rename all occurences of node to nodes since it's an array check - you didn't have me do that on Bug 341256, and it hasn't committed, so if you want me to fix that there, let me know. >2) you save all the nodes and their next siblings, but you might end up >deleting the next siblings, which means you need to undo in reverse order fixed by keeping track of previous siblings >3) when determining the node to select you get its previous sibling although >the node has already been deleted by then take next now (at least I think I addressed this properly) >I guess there's no easy way to select all the undeleted nodes? not without rewriting parts of selectElementInTree, but I haven't been able to get my head around that code yet, so I'll go with a no comment for now. After having some issues, I finally got this working. I could not delete backwards due to some length complications that occurred with undo. So, delete goes up the tree, and undo goes down. Basically, if you deleted a child node of another node you were deleting, problems occurred on undo. Since I'm keeping track of previousSiblings now instead of nextSiblings, I have to have the conditioning in there to make sure the previousSibling exists for appendChild/insertBefore.

Comment on attachment 226070 [details] [diff] [review] Revised patch >+ var ind = nodes.length - 1; >+ var selectNode = this.previousSiblings[ind]; >+ if (!selectNode) selectNode = nodes[ind].nextSibling; >+ if (!selectNode) selectNode = this.parentNodes[ind]; Ah, but you're still referring to the last deleted node's next sibling after you've actually deleted. You're now also preferring the previous sibling. >+ if (this.previousSiblings[i]) >+ this.parentNodes[i].insertBefore(this.nodes[i], >+ this.previousSiblings[i].nextSibling); >+ else >+ this.parentNodes[i].appendChild(this.nodes[i]); Except if the node had no previous sibling this means it was the first node...

(In reply to comment #10) >1) you should rename all occurences of node to nodes since it's an array >check - you didn't have me do that on Bug 341256, and it hasn't committed, so >if you want me to fix that there, let me know. I admit, I overlooked it, so that would be good. By the way, I wouldn't bother with your comment about editing multiline attributes, if someone really wants to do that they can set their newlines preference to allow newlines in textboxes (which is the Linux default).

Comment on attachment 226070 [details] [diff] [review] Revised patch > viewer.selectElementInTree(selectNode); >- node.parentNode.removeChild(node); It turns out that this is a crucial point in the old code; you must select the new node before removing the old node, otherwise the right hand panel flips into JS Object view. And I think the previousSibling stuff is wrong anyway.

(In reply to comment #13) > (From update of attachment 226070 [details] [diff] [review] [edit]) > > viewer.selectElementInTree(selectNode); > >- node.parentNode.removeChild(node); > It turns out that this is a crucial point in the old code; you must select the > new node before removing the old node, otherwise the right hand panel flips > into JS Object view. And I think the previousSibling stuff is wrong anyway. Yeah, I've noticed that but haven't had much time to actually work on this. This bug has been a lot more difficult than I had originally anticipated. Hope to have something by the end of next week.

So, I have put a lot of thought into how the best way to accomplish this might be. As of now, there are a few issues: 1) Currently discussed algorithms neglect the case that a node is selected that is the parent to another node that is also selected. Ideally, we should check for this and just delete the parent since the child nodes won't matter anymore. This has caused much grief for me. 2) Going up or down the tree for deleting has serious complications for undo. Storing either the previous sibling or the next sibiling isn't reliable as you could possible delete one or both leaving you with no reference node for reinsertion. 3) There is no easy way to select the previously selected nodes upon undo when there are with the current code. It will work fine with only one node selected. These problems are not impossible, but it isn't easy to address all of them. To deal with issue #2, we could possible store the parent node if more than one node is deleted. For undo, we simply replae the new parent node with the old one and we'll be ok. This would be difficult to implement, but I think it is possible. As for #3, well the code to select a node seemed really complex to begin with, so I'm not exactly eager to tackle that. I'm open to suggestions.

So, there's no good way to determine a common parent node between an arbitrary number of nodes. Would it be a bad idea to store the whole document for undo? In that case, I'd have to figure out how to select all the nodes again upon undo, but I think that's possible (should be, just a matter of figuring it out).

(In reply to comment #16) > So, there's no good way to determine a common parent node between an arbitrary > number of nodes. Do you mean ancestor? If so, I can think of an algorithm, but no, I can't think of any existing magic function that would do it. >Would it be a bad idea to store the whole document for undo? So you'd make a copy of the document? Sounds expensive. Also, is it even possible to swap out one document for another?

(In reply to comment #17) > (In reply to comment #16) > > So, there's no good way to determine a common parent node between an arbitrary > > number of nodes. > > Do you mean ancestor? If so, I can think of an algorithm, but no, I can't think > of any existing magic function that would do it. Yes, I meant ancestor, and I haven't had much coming up with an algorithm. I'll implement it if you want to toss out some pointers. > >Would it be a bad idea to store the whole document for undo? > > So you'd make a copy of the document? Sounds expensive. Also, is it even > possible to swap out one document for another? I haven't tested it, but I was afraid it was too expensive. Ideally, finding the common ancestor is probably our best bet.

while (node1) { while (node2) { if (node1 == node2) { return node1; } node2 = node2.parentNode; } node1 = node1.parentNode; } //if you get here, they're somehow not in the same document For a third node, just use the result of that function as the first argument, and the third node as the second argument.

This is much nicer. Not sure if it's complete yet, but it's certainly a step in the right direction.

So there is at least one issue with this. When you select more than one node, when you undo, the ancestor is not expanded (we don't know how far we'd have to expand and what not). I don't know of an easy way around this. New patch to address timeless's irc review momentairly.

Addresses comments on irc. Some test cases (all using the minefield start page) 1) Expand the div with id "container" 2) Select a node and delete it 3) Undo it Expected results: node is reslected and reinserted (same behavior before this patch) 1) Expand the div with id "container" 2) Select any two nodes in it and delete them Expected results: Nodes go away, div#container is selected 3) Undo it Expected results: Nodes come back, div#container is not expanded 1) Expand the div with id "container" 2) Select one or more nodes in it 3) Select it 4) Delete Expected results: Next sibling of div#container is selected, div#container is deleted 5) Undo Expected results: div#container reappears, and is selected (not expanded).

It should be noted that that patch doesn't quite work because I forgot a closing }. I fixed it locally, but won't submit a new patch.

Comment on attachment 270844 [details] [diff] [review] v2.1 This doesn't work with redo.

I'm starting to think that this bug is impossible to do with the undo/redo stack...

I'm done with this for awhile (per comment #25). If someone feels ambitious, please take a look.

(In reply to comment #2) > Yes, that part was fixed by bug 297660. > Next time, please only use the most recent trunk build (Mozilla1.7 is well over > a year old) to test if the bug still exists. > http://ftp.scarlet.be/pub/mozilla.org/firefox/nightly/latest-trunk/ > > The multiple selection not deleting the whole multiple selection is a valid bug, > though. Guys, please, take a look at this bug. It has been hanging here for a while and is still reproducible in Firefox 3.6.3 for windows XP.

Of course it's reproducible. It's written to work that way.

So I have this working, mostly, but bz says (via the newsgroups) that it's not possible to delete an anonymous element that's a child of the bound element. I guess the thing to do here, as with attempted deletion of a document node, is to delete what's possible, and tell the user about whatever we couldn't delete. We can get around notifying the user, though, in cases where the bound element is going to be removed as well, since it will be effectively the same as if we had removed the anon first.

(In reply to http://groups.google.com/group/mozilla.dev.tech.xbl/browse_frm/thread/c1ec57e66448b02f) > I'd start off by not allowing deletion of anonymous content for now, > probably.... There's really just no way to do it, and often enough it's not > even clear what it would mean to do it. > > -Boris I can delete anonymous content with the DOM Inspector today, so long as its parent is another anon, and not the bound element itself (maybe you were referring to the children of bound elements I originally asked about, though). It seems to me to be technically the same as if the binding were removing nodes itself. It might be silly to do so, but just as silly as deleting arbitrary nodes on a Web page that might have script relying on its existence. Node deletion is already kind of a user-beware situation.

> so long as its parent is another anon Or rather so long as it's a DOM child of its parent (so not anonymous wrt its parent). > It seems to me to be technically the same as if the binding were removing nodes > itself. With your "so long" caveat, yes. A binding can't remove root anonymous content nodes, though.

Might as well get this out there.

Oh yeah, there are some other changes here besides delete: *XMLSerializer has been around since at least Mozilla 1.2, I think, so I dropped _toXML *Non-Delete commands' criteria in isCommandEnabled were changed as well *If no command is enabled in a nested menupopup (e.g., Paste), the menupopup is disabled altogether.

Comment on attachment 443302 [details] [diff] [review] delete for multiple select with contiguous and non-contiguous regions (From update of attachment 443302 [details] [diff] [review]) >+ function DVr_TransactionExecuting(aData, aMethod, aBegin) Ho ho! Turns out we have nsITransactionListener for this.

So here's the deal, there's nothing wrong with this patch (well, as far as unreviewed patches go). It works. Everyone could have multi-node deletion tomorrow if the review and superreview came fast enough, and there were no nits about the patch, and I haven't overlooked anything. But my solution was to create a transactionExecuting method on the viewer that is called once before the transaction method goes through, and once after. Fun plan, but this is exactly what nsITransactionListener is for, and so it should be used instead. The problem is that to use transaction listeners, we have to alter the inspector binding to get them registered. This is all right, but we've got a fine mess of how commands are executed due to bug 179621 being partially fixed. Getting the binding to register listeners before fixing bug 179621 would create two headaches: 1. Figuring out what needs to be hacked around in the inspector binding, and 2. Probably making Phase 3 of bug 179621 more complicated. Instead of doing this, bug 179621 needs to be resolved first, then we can proceed with multi-node deletion goodness. (And beyond that, the rest of the DOM Nodes viewer commands, bug 546170, and commands from any other viewers that do silly stuff like misuse currentIndex—bug 5333127—or assume that the originating viewer is still loaded when it's not—no bug filed for this, but I've mentioned it in bug 546170.)

Other stuff that I didn't mention in the last patch: make the viewer currentIndex-clean, especially in isCommandEnabled use isCommandEnabled for viewer commands like cmdBlink and cmdInspectInNewWindow change it so that the aforementioned commands are all handled by the same function, instead of different ones called by separate onpopupshowing attributes No r?/sr? for now, because this relies on the patches for bug 179621, and I'm waiting on sr+ from Neil on attachment 445635 [details] [diff] [review].

This is pretty much the same as attachment 447398 [details] [diff] [review], but I got rid of the pending listener stuff in the inspector binding by moving the transaction manager instantiation code out of the initialize method and into the constructor, which is the way I should have done it for bug 179621. I also clarified some comments and made it so that the paste commands besides paste before and after operate on the selection instead of currentIndex. This differs from the last patch, because the only paste command that I had operating on the selection was paste replace. There are two small changes from the last patch regarding showNodeInTree. Some notes about the overall changes: This is much larger than Shawn's patch; it increases overall file size by like 40%. I think the biggest causes are: 1. My selection algorithm behaves differently. 2. Shawn was finding a common ancestor and storing snapshot of the tree. And if it makes it any better, about a third of that growth is from comments. There's room for optimization in this, too. For example, we can reduce the number of DOM operations in the event that a deleted node is the ancestor of another deleted node by just deleting that node. There are a lot of things like that, but I'd rather get this big chunk checked in, eventually fix the other commands to work with multiple selection, and then make those kinds of incremental improvements, including recovery from external manipulation. (E.g., you insert/delete/paste some nodes, then are moving through the undo/redo and surprise!, some other script has been messing with the DOM and the sibling/parent lists we created for that operation are not up to date.)

You might want to have Neil review this. It's going to be a few weeks before I can still (sorry!).

Comment on attachment 461734 [details] [diff] [review] Circumvent bitrot from 578775 landing; remove some cruft I included by accident in last patch, and tweak some comments >- >- var tmClassID = "@mozilla.org/transactionmanager;1"; >- var tmIface = "nsITransactionManager"; >- this.mTransactionManager = XPCU.createInstance(tmClassID, tmIface); ... >+ >+ this.mTransactionManager = >+ XPCU.createInstance("@mozilla.org/transactionmanager;1", >+ "nsITransactionManager"); [Why this change?] >+ mDisableSelectionChangeCount: 0, [Might be better named mSelectionBatchNestCount] >- selectedNode = new XPCNativeWrapper(viewer.selectedNode, "nodeType", >+ currentNode = new XPCNativeWrapper(viewer.currentNode, "nodeType", > "parentNode", "childNodes"); >- if (selectedNode.parentNode) { >- parentNode = new XPCNativeWrapper(selectedNode.parentNode, >+ if (currentNode.parentNode) { >+ parentNode = new XPCNativeWrapper(currentNode.parentNode, > "nodeType"); As I recall, the versions of Gecko that we currently support all provide the 1-arg XPCNativeWrapper version that explicitly wraps its derived properties, so we can fix this to currentNode = new XPCNativeWrapper(viewer.currentNode); parentNode = currentNode.parentNode; >+ case "cmdInspectBrowser": >+ var selectedNode = this.selectedNode; >+ if (!(selectedNode instanceof nsIDOMElement)) { >+ return false; >+ } >+ >+ var n = selectedNode.localName.toLowerCase(); >+ return n == "tabbrowser" || n == "browser" || n == "iframe" || >+ n == "frame" || n == "editor"; [This is hardly an ideal test... it's not easy to think of a good one though] >+ var command = unwrapJSObject(aTransaction); This is flawed. aTransaction isn't a content dom node, which is what unwrapJSObject was designed for. Just use .wrappedJSObject directly. >+ // The approach is broken down as follows: >+ // 1. Locating a nearby node that's visible in the tree and won't >+ // disappear. Choose this as our guess. >+ // 2. Verifying that the guess isn't a descendant of a node that will >+ // deleted. The problem with this is that if it turns out you're deleting the parent of your guess, you will keep guessing all the children until you run out. The way around this is to start with the current node, and look at its ancestors, and see whether any of them are in the deletable nodes list. Choose the highest deletable ancestor (if any) as your initial guess. Then (unless your guess itself is deletable) you will know that you will never guess a node with a deletable ancestor. In pseudocode: var guess = this.currentNode; for (var temp = guess; temp != root; temp = temp.parentInView) if (temp in deleteableNodes) guess = temp; >+ this.getRowIndexFromNode(guess) >= 0) { guess is a sibling of a visible node. So it will always be visible. >+ getNearestSelectedIndex: function DVr_GetNearestSelectedIndex(aRowIndex) >+ { >+ var guessIndex, guessDistance = Number.POSITIVE_INFINITY; >+ var indexes = this.getSelectedIndexes(); >+ for (let i = 0, n = indexes.length; i < n; ++i) { >+ var index = indexes[i]; >+ var distance = Math.abs(aRowIndex - index); >+ if (distance < guessDistance) { >+ guessDistance = distance; >+ guessIndex = index; >+ } >+ } >+ return guessDistance == Number.POSITIVE_INFINITY ? -1 : guessIndex; You can do much better than this because getSelectedIndexes always returns indexes in ascending order. There are four easy cases: 1) no selected nodes 2) one selected node 3) row on or before first selected 4) row on or after last selected. Otherwise do a (binary) search, and when you are reduced to two rows (one before, one after) return the nearest. >+ var subject = cmd || el; >+ if (isEnabled) { >+ hasEnabled = true; >+ if (subject) { el always exists so subject does too. >+ if (checkValid) { >+ el.removeAttribute("hidden"); >+ } [Could write this as if (checkValid) el.hidden = !isEnabled; ] >+ this.constructor = cmdEditDelete; [Does setting this on the prototype not work?] >+cmdEditDelete.sortComparator = function Delete_SortComparator(a, b) [See below] >+ if (!aAncestor || aAncestor == rootNode) { >+ return 1; >+ } >+ return -1; What about cousin nodes? >+cmdEditDelete.isDeletable = function Delete_IsDeletable(aNode, aFailure) >+cmdEditDelete.NODE_NULL = 1; >+cmdEditDelete.NO_PARENT = 2; >+cmdEditDelete.NOT_EXPLICIT_CHILD = 3; Why are these set on the function and not on the prototype? >+ // We can't use XPCU, since globals won't be in our scope when this viewer >+ // is destroyed. So we have this instance method to log to the console, and >+ // set up the console service on the first call. Why is this running after the viewer is destroyed? >+ // Can't use global class/interface constants, either. >+ var iface = Components.interfaces.nsIConsoleService; >+ var classID = "@mozilla.org/consoleservice;1"; >+ this.mConsoleService = Components.classes[classID].getService(iface); Can't use Components either, so there. >+ this.logString(aMessage); Infinite loop in the failure case.

(In reply to comment #40) > Comment on attachment 461734 [details] [diff] [review] > Circumvent bitrot from 578775 landing; remove some cruft I included by accident > in last patch, and tweak some comments > > >- > >- var tmClassID = "@mozilla.org/transactionmanager;1"; > >- var tmIface = "nsITransactionManager"; > >- this.mTransactionManager = XPCU.createInstance(tmClassID, tmIface); > ... > >+ > >+ this.mTransactionManager = > >+ XPCU.createInstance("@mozilla.org/transactionmanager;1", > >+ "nsITransactionManager"); > [Why this change?] It was supposed to prevent a race condition that it seems will never occur. > >+ var command = unwrapJSObject(aTransaction); > This is flawed. aTransaction isn't a content dom node, which is what > unwrapJSObject was designed for. Just use .wrappedJSObject directly. Dang, I thought XPCNativeWrapper.unwrap was supposed to solve all of the world's problems. > >+ // The approach is broken down as follows: > >+ // 1. Locating a nearby node that's visible in the tree and won't > >+ // disappear. Choose this as our guess. > >+ // 2. Verifying that the guess isn't a descendant of a node that will > >+ // deleted. > The problem with this is that if it turns out you're deleting the parent of > your guess, you will keep guessing all the children until you run out. > > The way around this is... Ah! When I considered this, I mistook it for just trading a breadth-oriented approach for a depth-oriented one with respective disadvantages. > >+ this.getRowIndexFromNode(guess) >= 0) { > guess is a sibling of a visible node. So it will always be visible. What makes you so sure? The same path is taken for redo, as well as do. When you redo, it's possible that you've since collapsed an ancestor row. You're not guaranteed that any of the nodes have corresponding rows visible in the tree, since cmdEditDelete by design operates only on nodes, and not rows of a tree view. > >+cmdEditDelete.sortComparator = function Delete_SortComparator(a, b) > [See below] > > >+ if (!aAncestor || aAncestor == rootNode) { > >+ return 1; > >+ } > >+ return -1; > What about cousin nodes? Not important. The clash we're trying to avoid by using the special ordering doesn't exist for cousins. (Since we use the next sibling as an insertion reference, the clash we're talking about here would be to try to do nodeParent.insertBefore(undeletedNode, insertionRef) to restore undeletedNode before we've made sure insertionRef has been restored, in the event that it was also deleted.) Suppose we have the following tree: A B C D E F G I think what you're pointing out is that the sort comparator can result in this being ordered as B,C,F,A,D,E,G or F,B,C,A,D,E,G. The good news is we don't care; our deletion/undeletion algorithm should handle both. > >+cmdEditDelete.isDeletable = function Delete_IsDeletable(aNode, aFailure) > >+cmdEditDelete.NODE_NULL = 1; > >+cmdEditDelete.NO_PARENT = 2; > >+cmdEditDelete.NOT_EXPLICIT_CHILD = 3; > Why are these set on the function and not on the prototype? They don't have much to do with the instances. The constants are there for reasons similar to why Number.MAX_VALUE isn't Number.prototype.MAX_VALUE in JS. I'd argue that the same is enough of a reason to justify isDeletable being there as well, from a high-level perspective. And now that I think about it, the viewer does use isDeletable as a "static" method where no instance exists. See isCommandEnabled for cmdEditDelete. > >+ // We can't use XPCU, since globals won't be in our scope when this viewer > >+ // is destroyed. So we have this instance method to log to the console, and > >+ // set up the console service on the first call. > Why is this running after the viewer is destroyed? Recall that viewer panes are just browsers loading individual viewer XUL (which load viewer-specific JS executing in the scope of that viewer). When a command is invoked, the viewer gives its parent panelset that command, which is an nsITransaction. The transaction manager handling all of this is internal to the panelset, and so stays relevant for the duration of the inspector window's life. Even after you've switched from, e.g., the DOM Node viewer to the Style Rules viewer, the transaction manager is still holding refs in its stack to any commands the user did while the DOM Node viewer was open. Assuming in the transaction logic that |viewer| is always going to be there is a problem I've mentioned a couple of times, probably most visibly in bug 546170. --- You know, I'd written the above, then tested the code in question, and I'm getting errors when I try to undo after switching to another viewer. (Although I could have sworn I had this working at some point...). TypeError: (void 0) is not a constructor It's complaining about this line: this.mParents[idx].insertBefore(this.nodes[idx], this.mSiblings[idx]); Specifically, trying to access insertBefore (or anything else, really) on this.mParents[idx] gives the error. I would *think* this would be fine, even after original viewer has unloaded, because mParents (and mSiblings, etc.) are just holding refs to nodes in a document that still exists (the inspected document; i.e., not the viewer that was unloaded). But I guess not. This seems like a "big picture" problem wrt to the way the command objects are handed about and stick around even after the viewer is destroyed. (Some DOM candy-coating that knows about these as elements/their protos are getting clobbered or something when we unload? I dunno.) Any hints on tweaks to accomplish the desired effect? I *really* think we should be able to undo/redo without regard to whether we may or may not actually have some particular view of the data. Diddling the DOM via JS objects that were put in place before unloading works fine after the viewer is destroyed for the CSS Rules viewer, FWIW. > > >+ this.logString(aMessage); > Infinite loop in the failure case. Huh? Where?

(In reply to comment #41) > (In reply to comment #40) > > This is flawed. aTransaction isn't a content dom node, which is what > > unwrapJSObject was designed for. Just use .wrappedJSObject directly. > Dang, I thought XPCNativeWrapper.unwrap was supposed to solve all of the > world's problems. It only unwraps XPCNativeWrappers, not XPCWrappedNatives. Easy mistake ;-) > > >+ this.getRowIndexFromNode(guess) >= 0) { > > guess is a sibling of a visible node. So it will always be visible. > What makes you so sure? The same path is taken for redo, as well as do. Thanks for pointing that out. > I think what you're pointing out is that the sort comparator can result in this > being ordered as B,C,F,A,D,E,G or F,B,C,A,D,E,G. The good news is we don't > care; our deletion/undeletion algorithm should handle both. Thanks for explaining. > > >+cmdEditDelete.isDeletable = function Delete_IsDeletable(aNode, aFailure) > > >+cmdEditDelete.NODE_NULL = 1; > > >+cmdEditDelete.NO_PARENT = 2; > > >+cmdEditDelete.NOT_EXPLICIT_CHILD = 3; > > Why are these set on the function and not on the prototype? > They don't have much to do with the instances. The constants are there for > reasons similar to why Number.MAX_VALUE isn't Number.prototype.MAX_VALUE in JS. I guess I'm too used to xpconnect exposing interface enums on the instances. > This seems like a "big picture" problem wrt to the way the command objects are > handed about and stick around even after the viewer is destroyed. (Some DOM > candy-coating that knows about these as elements/their protos are getting > clobbered or something when we unload? I dunno.) Any hints on tweaks to > accomplish the desired effect? The only way I know to avoid this is not to use objects created in a window scope. Instead you need one of a) C++ objects b) JS components c) JS modules. > Diddling the DOM via JS objects that were put in place before unloading works > fine after the viewer is destroyed for the CSS Rules viewer, FWIW. You might still get assertions in a debug build though. > > >+ this.logString(aMessage); > > Infinite loop in the failure case. > Huh? Where? If the getService fails, then you just keep trying over and over again.

Comment on attachment 461734 [details] [diff] [review] I want to split off some of this before having another go. Bug 589299 - Make the DOM Nodes viewer currentIndex-clean. Bug 589300 - Consolidate onCommandPopupShowing and onContextCreate in DOM Nodes viewer (In reply to comment #42) > > This seems like a "big picture" problem wrt to the way the command objects are > > handed about and stick around even after the viewer is destroyed. (Some DOM > > candy-coating that knows about these as elements/their protos are getting > > clobbered or something when we unload? I dunno.) Any hints on tweaks to > > accomplish the desired effect? > The only way I know to avoid this is not to use objects created in a window > scope. Instead you need one of a) C++ objects b) JS components c) JS modules. Then I'll probably just change the inspector—viewer contract to allow viewers to ask the inspector to load their (non-transient) transaction definitions in the inspector scope, or maybe in less ephemeral sandboxes. Although I'll probably just disregard the fact that trying to undo won't work if you've unloaded the transaction's originating viewer. All of that's already broken right now anyway.

I'll say it again: anonymous content ruins everything. Here's the comment describing a problem I identified sometime between that last comment I left in August and in the last few days when I started looking at getting this fixed again: // XXX If we delete a child of an anonymous node and that anonymous node's // binding parent, the nodes array and the mParents array (and potentially // the mSiblings array) will continue to needlessly reference nodes from // that anonymous subtree; even if the binding parent ever gets restored // via undo, a new anonymous content tree will be created for it. So we // should drop our references to the nodes in the old anonymous content // tree. // We can even go further than that. The saved child-of-anonymous node // isn't going to get reinserted anywhere useful, so why reinsert it? // What should we do in that case? Sync the attributes from the old anon // to the corresponding node in the new content tree, then update our ref // to that one? I've been considering how to approach this off and on for the last day or so. In the last few minutes, I think I've convinced myself that the following wouldn't be so awful: Check to see if any of the deleted nodes' binding parents are also going to be deleted. If so, just ignore any selected anons from that binding's tree wrt to the stuff we keep track of to make undo work, and prompt with, "Some of the selected nodes are part of an anonymous subtree that will be destroyed. Deleting those anonymous nodes cannot be undone. Would you like to continue?".

(In reply to comment #44) > Check to see if any of the deleted nodes' binding parents are also going to be > deleted. If so, just ignore any selected anons from that binding's tree wrt to > the stuff we keep track of to make undo work, and prompt with, "Some of the > selected nodes are part of an anonymous subtree that will be destroyed. > Deleting those anonymous nodes cannot be undone. Would you like to continue?". I wouldn't bother with the prompt. We already have cases in which undo fails ;-)

(In reply to comment #45) > I wouldn't bother with the prompt. Aw, I already have it coded up, though. > We already have cases in which undo fails Mm, yeah, but to me that's not good and something to be avoided. In other cases, when we fail, we fail hard or bizarrely. Here, prompt aside, there'd be proactive checks to detect imminent failure and deal with it appropriately/at least eliminate (hopefully all of) the worst side effects.

I was originally leaning the other way and thinking of warning when anonymous content is enabled and any of the nodes that you're deleting has a binding, but I guess that would get too annoying. (In reply to comment #46) > (In reply to comment #45) > > I wouldn't bother with the prompt. > Aw, I already have it coded up, though. Don't worry, it wasn't a strong opinion.

(In reply to comment #42) > (In reply to comment #41) > > (In reply to comment #40) > > > >+ this.logString(aMessage); > > > Infinite loop in the failure case. > > Huh? Where? > If the getService fails, then you just keep trying over and over again. Huh? If getService fails -> this.mConsoleService = null -> |"mConsoleService" in this| is true on the recursive call and this.mConsoleService == false in the next conditional -> dump(aMessage)

Comment on attachment 534183 [details] [diff] [review] Another go, now that other parts have been split off and taken care of >+ var withoutFilter = function DVr_GetNextInNextBestChain_IsIgnorable(aNode) >+ { >+ return viewer.isIgnorableNode(aNode); >+ }; >+ var withFilter = >+ function DVr_GetNextInNextBestChain_FilteredIsIgnorable(aNode) >+ { >+ return viewer.isIgnorableNode(aNode) || aFilterFn(aNode); >+ }; Why write var <name> = function <name>...; instead of just function <name>...? >+ // Flag this transaction as no good and that it should be discarded. >+ this.canceled = true; Throw an exception perhaps? >+ if ("mConsoleService" in this) { OK, so I must have misread this the previous time. Possibly if you'd written the condition the other way around it would have been more obvious: if (!(("mConsoleService" in this)) { try ... catch ... } if (this.mConsoleService) ... else ...

(In reply to comment #50) > Comment on attachment 534183 [details] [diff] [review] [review] > Another go, now that other parts have been split off and taken care of > > >+ var withoutFilter = function DVr_GetNextInNextBestChain_IsIgnorable(aNode) > >+ { > >+ return viewer.isIgnorableNode(aNode); > >+ }; > >+ var withFilter = > >+ function DVr_GetNextInNextBestChain_FilteredIsIgnorable(aNode) > >+ { > >+ return viewer.isIgnorableNode(aNode) || aFilterFn(aNode); > >+ }; > Why write var <name> = function <name>...; instead of just function > <name>...? So we can get the pseudo-namespacing with the function name without having to type out that name below. > >+ // Flag this transaction as no good and that it should be discarded. > >+ this.canceled = true; > Throw an exception perhaps? > > >+ if ("mConsoleService" in this) { > OK, so I must have misread this the previous time. Possibly if you'd written > the condition the other way around it would have been more obvious: > if (!(("mConsoleService" in this)) { > try ... > catch ... > } > if (this.mConsoleService) > ... > else > ... Do you want me to do that?

I also found two errors regarding cmdEditDelete. In the constructor, the condition for prompting used the return value from Array.prototype.indexOf instead of checking that it's greater than or equal to 0. And in cmdEditDelete.sortComparator, I used a bitwise & instead of logical &&.

Comment on attachment 535947 [details] [diff] [review] address comment 50 and fix two other errors >+ throw new Error("User canceled transaction"); [I think I've seen you do this somewhere else and I either forgot the question or the answer, so I'll ask again: any particular advantage in throwing an Error?] >+ this.logString(aMessage); >+ } >+ else { >+ // This is not the first call. [Not quite what I suggested, which removed the recursive call and the else clause. If you don't like that then go back to the previous version.]

(In reply to comment #53) > Comment on attachment 535947 [details] [diff] [review] [review] > address comment 50 and fix two other errors > > >+ throw new Error("User canceled transaction"); > [I think I've seen you do this somewhere else and I either forgot the > question or the answer, so I'll ask again: any particular advantage in > throwing an Error?] : (comment #50) > >+ // Flag this transaction as no good and that it should be discarded. > >+ this.canceled = true; > Throw an exception perhaps? :) (Unless your question is "why are you throwing a generic Error [instead of something more specific]?".) > >+ this.logString(aMessage); > >+ } > >+ else { > >+ // This is not the first call. > [Not quite what I suggested, which removed the recursive call and the else > clause. If you don't like that then go back to the previous version.] Oops. I wasn't paying attention.

(In reply to comment #54) > (Unless your question is "why are you throwing a generic Error [instead > of something more specific]?".) Actually I meant [instead of a string].

So throw a string?

(In reply to comment #56) > So throw a string? Well, I had this vague memory of someone giving a reason to throw an Error rather than a string, and I was rather hoping it was you ;-)

File name, line number, and stack trace, maybe?

(In reply to comment #58) > File name, line number, and stack trace, maybe? Ah yes, that was probably it. (Although seeing as you always catch it...)

Pushed: http://hg.mozilla.org/dom-inspector/rev/800b7694f8ed