Comment on attachment 406164 [details] [diff] [review] patch >+ content/inspector/viewers/accessibleProps/accessiblePropViewerMrg.js (resources/content/viewers/accessibleProps/accessiblePropViewrMgr.js) This line looks very wrong...

(In reply to comment #1) > (From update of attachment 406164 [details] [diff] [review]) > >+ content/inspector/viewers/accessibleProps/accessiblePropViewerMrg.js (resources/content/viewers/accessibleProps/accessiblePropViewrMgr.js) > This line looks very wrong... Yes, I found it already and fixed locally. Sorry I didn't say about this.

Comment on attachment 406164 [details] [diff] [review] patch >+ if (this.viewers[id].update(aAccessible)) >+ tab.removeAttribute("hidden"); >+ else >+ tab.setAttribute("hidden", "true"); Nit: just set tab.hidden directly. >+ this.tabboxElm.selectedIndex = 0; This is annoying, since when I want to look at a number of table cells I keep having to switch the tab back... obviously if the selected tab is hidden then you need to reset it, but it would be nice if you didn't do it every time. >+ var idx = this.tabboxElm.selectedIndex; >+ for (var id in this.viewers) { Instead of this loop, you should compute the property name from the selected tab's id (or make them the same) and look up the viewer directly. >+ this.clear = function tableCellViewer_clear() >+ { >+ this.mTreeBox.view = null; Clear the column/row labels too? >+function TableCellTreeView(aTableCell) >+{ >+ this.tableCell = aTableCell; >+} >+ >+TableCellTreeView.prototype = new inBaseTreeView(); >+ >+TableCellTreeView.prototype.__defineGetter__( >+ "rowCount", >+ function rowCount() >+ { >+ this.columnHeaderCells = this.tableCell.columnHeaderCells; >+ this.columnHeaderCellsLen = (this.columnHeaderCells ? >+ this.columnHeaderCells.length : 0); >+ >+ this.rowHeaderCells = this.tableCell.rowHeaderCells; >+ this.rowHeaderCellsLen = (this.rowHeaderCells ? >+ this.rowHeaderCells.length : 0); >+ >+ return 1 + this.columnHeaderCellsLen + this.rowHeaderCellsLen; >+ } Can the row count change? If it can, and you don't call rowCountChanged, then the tree can get confused. If it can't, then you don't need a function, you can just compute it ahead of time in the constructor. >+ DOMNode[" accessible "] = accessNode; What does this achieve? >+ <tabpanels flex="1"> >+ <tabpanel flex="1"> >+ <tree flex="1"> Nit: don't need "tabpanel", any element will do. Also, don't need flex on the tabpanels children. So, you can simplify this to <tabpanels flex="1"> <tree> etc. >+ <tabpanel orient="vertical"> No flex :-) But just use a <vbox>.

(In reply to comment #3) > >+ this.tabboxElm.selectedIndex = 0; > This is annoying, since when I want to look at a number of table cells I keep > having to switch the tab back... obviously if the selected tab is hidden then > you need to reset it, but it would be nice if you didn't do it every time. Yeah, this is great feature and I realized it's pretty annoying when there is no it. But I was lazy to add it at that point. I reformed :) > Can the row count change? If it can, and you don't call rowCountChanged, then > the tree can get confused. If it can't, then you don't need a function, you can > just compute it ahead of time in the constructor. Yes if DOM is changed, but it's hard to track. So I changed to precomputed value. > >+ DOMNode[" accessible "] = accessNode; > What does this achieve? Some accessible views use this accessible object instead of DOM node.

Neil's comments are addressed.

Comment on attachment 406397 [details] [diff] [review] patch2 >+ this.tabboxElm.selectedIndex = this.getCurrentViewerIdx(); I think you overengineered this. Something like this would do: if (this.tabboxElm.selectedTab.hidden) this.tabboxElm.selectedIndex = 0; >+ var viwerid = tab.getAttribute("id").replace("tab_", ""); Nit: viewerid typo Nit: tab.id

(In reply to comment #6) > (From update of attachment 406397 [details] [diff] [review]) > >+ this.tabboxElm.selectedIndex = this.getCurrentViewerIdx(); > I think you overengineered this. Something like this would do: > if (this.tabboxElm.selectedTab.hidden) > this.tabboxElm.selectedIndex = 0; I guess this won't work, if user changes view and then get back to this one again. I think this should nice feature.

Comment on attachment 406397 [details] [diff] [review] patch2 general nit: missing javadoc comments on most of these methods. Please add them. >+++ b/resources/content/viewers/accessibleProps/accessiblePropViewerMgr.js >+ * The Initial Developer of the Original Code is >+ * Mozilla Corporation. Mozilla Foundation. >+//////////////////////////////////////////////////////////////////////////////// >+// Global >+//////////////////////////////////////////////////////////////////////////////// nit: I'd like the standardize on this format please: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManagerUI.js#48 >+/** >+ * Used to show additional properties of the accessible in tabbox. >+ */ nit: no @param aPaneElm for documentation (please start description on new line like this http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#60) >+ //////////////////////// >+ // private nit: extend all the way, like other comments r=sdwilsh with nits fixed.

landed on http://hg.mozilla.org/dom-inspector/rev/b05c6d3b5b68 with Shawn's and Neil's comments addressed.