Explanations at the code. I'm sorry if there are any language error. I am not an English speaker, but if any is found, I will fix it. Thank you for reviewing it.

There were some tabs instead of spaces.

Comment on attachment 631565 [details] [diff] [review] removing tabs from patch You're duplicating code, which is wrong. Instead, the getCellValue and setCellValue methods of mWatchView already do the right thing. All you need is the right row index and column.

Could you take a look into this new patch, Neil? Thank you. The function that is called when a key is pressed has been changed to onKeyPressed to make it easier any future related modification. A problem I found when testing the patch was when only one child event is unwatched and then SPACE key is pressed, it becomes watched -as expected- but the parent doesn't change to watched. At the setCellValue() function, it seems that the only case it should be changed is when parent is checked and child is unchecked. Is this a bug?

Comment on attachment 631768 [details] [diff] [review] patch 2 There is a line which is longer that the 80 characters limit.

Line length in attachment 631768 [details] [diff] [review] corrected and removed unnecessary invalidate() call.

Comment on attachment 631771 [details] [diff] [review] patch 3 >+ onKeyPressed: function onKeyPressed(anEvent) Nobody else calls it anEvent. Please rename it to aEvent. (Same below.) >+ this.toggleEventWatching = function watchview_toggleEventWatching(anEvent) [My preference would have been for toggleEventWatched] >+ var cols = this.mTree.columns; >+ var colWatch = cols.getColumnFor(document.getElementById("welIsWatched")); Actually it's a lot easier than that, just use var colWatched = this.mTree.columns.welIsWatched; >+ var cws = (!this.getCellValue(idx, colWatch)).toString(); >+ >+ // setCellValue() needs the new value to be passed as a string. >+ this.setCellValue(idx, colWatch, cws); I wish there was a better way to write this... Perhaps renaming "cws" to "newValue" would make the code more readable. Also move the .toString() to the setCellValue line like this: var newValue = !this.getCellValue(idx, colWatched); // setCellValue() needs the new value to be passed as a string. this.setCellValue(idx, colWatched, newValue.toString()); > onselect="viewer.onWatchViewItemSelected();" >+ onkeypress="viewer.onKeyPressed(event);" Maybe "onWatchViewKeyPressed" would be better? Asking surkov for feedback on my variable renaming suggestions.

I realized that the event object is in fact not needed in the toggleEventWatched() function, so it is now removed. Waiting for Alexander feedback.

Comment on attachment 631771 [details] [diff] [review] patch 3 Review of attachment 631771 [details] [diff] [review]: ----------------------------------------------------------------- > Asking surkov for feedback on my variable renaming suggestions. I agree on each suggestion >>+ onKeyPressed: function onKeyPressed(anEvent) > Nobody else calls it anEvent. Please rename it to aEvent. (Same below.) right, because 'a' is not an indefinite article but a first letter of 'argument' I believe

Comment on attachment 631771 [details] [diff] [review] patch 3 OK, r=me with my comment #7 changes addressed. Thanks!

I think this is the way to attach a patch when it has been reviewed ok except it needed some changes.

I addressed the problems in comment 7. Furthermore, I removed the unneeded call to invalidate(). This last change wasn't in the reviewed patch 3, if it is ok, could you check-in patch 4, please Neil? Thank you.

(In reply to Javi Rueda from comment #12) > I addressed the problems in comment 7. Furthermore, I removed the unneeded > call to invalidate(). This last change wasn't in the reviewed patch 3 Actually patch 3 had already removed the call, so that's OK anyway.

Pushed dom-inspector changeset 2d993155b488.