Last Comment Bug 337674 - Make it possible to also inspect accessibles (nsIAccessible) objects
: Make it possible to also inspect accessibles (nsIAccessible) objects
Status: RESOLVED FIXED
: access
Product: Other Applications
Classification: Client Software
Component: DOM Inspector (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: ---
Assigned To: alexander :surkov
:
:
Mentors:
Depends on: 343997 394253
Blocks: a11yDOMi
  Show dependency treegraph
 
Reported: 2006-05-12 01:07 PDT by Håkan Waara
Modified: 2007-08-29 19:42 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (36.12 KB, patch)
2006-05-14 17:55 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
screenshot (89.81 KB, image/jpeg)
2006-05-15 02:31 PDT, alexander :surkov
no flags Details
patch2 (36.23 KB, patch)
2006-05-15 18:01 PDT, alexander :surkov
timeless: review-
Details | Diff | Splinter Review
sceenshot2 (54.76 KB, image/jpeg)
2006-05-18 00:12 PDT, alexander :surkov
no flags Details
patch3 (116.20 KB, patch)
2006-05-18 04:13 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch4 (117.31 KB, patch)
2006-05-18 04:31 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
movepatch1 (73.55 KB, patch)
2006-05-19 04:00 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
newpatch (43.87 KB, patch)
2006-05-19 04:03 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
movepatch2 (73.55 KB, patch)
2006-05-23 00:09 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
newpatch2 (43.55 KB, patch)
2006-05-23 00:18 PDT, alexander :surkov
hwaara: review-
Details | Diff | Splinter Review
movepatch3 (92.88 KB, patch)
2006-05-26 22:39 PDT, alexander :surkov
timeless: review-
Details | Diff | Splinter Review
patch5 (120.41 KB, patch)
2006-05-29 22:37 PDT, alexander :surkov
ajvincent: review-
Details | Diff | Splinter Review
patch6 (125.34 KB, patch)
2006-05-30 05:35 PDT, alexander :surkov
ajvincent: review-
Details | Diff | Splinter Review
patch7 (89.00 KB, patch)
2006-06-06 08:34 PDT, alexander :surkov
ajvincent: review-
Details | Diff | Splinter Review
patch8 (84.40 KB, patch)
2006-06-07 09:53 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch9 (83.76 KB, patch)
2006-06-10 10:57 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
screenshot from Windows (with disable-accessibility) (11.64 KB, image/png)
2006-06-10 16:30 PDT, Alex Vincent [:WeirdAl]
no flags Details
patch10 (86.16 KB, patch)
2006-06-14 02:23 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch11 (113.17 KB, patch)
2006-06-14 20:59 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch12 (71.58 KB, patch)
2006-07-08 20:36 PDT, alexander :surkov
timeless: review+
neil: superreview-
Details | Diff | Splinter Review
patch13 (74.56 KB, patch)
2006-08-01 21:02 PDT, alexander :surkov
neil: superreview+
Details | Diff | Splinter Review
patch for checkin (63.61 KB, patch)
2006-08-04 10:56 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
latest patch (74.57 KB, patch)
2006-08-07 11:06 PDT, alexander :surkov
no flags Details | Diff | Splinter Review

Description Håkan Waara 2006-05-12 01:07:32 PDT
Most XUL objects have an associated nsIAccessible (see the XBL), and in fact there's a whole tree of those.  

It would be neat if the DOM inspector could alsp see them, and they'd be inspectable.
Comment 1 alexander :surkov 2006-05-12 02:19:07 PDT
(In reply to comment #0)
> Most XUL objects have an associated nsIAccessible (see the XBL), and in fact
> there's a whole tree of those.  
> 
> It would be neat if the DOM inspector could alsp see them, and they'd be
> inspectable.
> 

Not only I guess, HMTL too :)
Comment 2 timeless 2006-05-12 02:46:39 PDT
we're accepting patches (i'll probably have to try to review on a sunday)
Comment 3 alexander :surkov 2006-05-13 00:29:45 PDT
I'll try to implement that.

I guess view list of left panel should be extended by 'accessible tree' view. I think 'accessible tree' view should expand 'DOM Nodes' view, it should mark accessible nodes by some way (f.x. making bold). When 'accessible tree' view is choosed then view list of right panel has additional item 'accessible object' (for sure it should be visible only for accessible nodes). I guess 'accessible object' should expand 'jsObject' view and should show nsIAccessible object. Probably we need a one more view - shortcut of main properties of nsIAccessible object that will include 'state'/'role'/'value' and so on.
Comment 4 alexander :surkov 2006-05-14 17:55:33 PDT
Created attachment 221996 [details] [diff] [review]
patch

I went ahead by simplest way. It allows me to not do a lot changes in DOM Inspector code. The patch adds preference to show accessible nodes in 'dom' view, all accessible nodes have additional view ('accessibleObject').

I can see two problem for the patch:
1) popupOverlay.xul doesn't use locales
2) 'modern' theme isn't supported
3) accessibleObject.xul is just a copy of jsObject.xul

In general I prefer to have the next behaviour:
1) We should have additional view 'accessibleTree' for left panel, the view should be based on 'dom' view.
2) If 'accessibleTree' view is choosed then accessible items should have 'accessibleObject' view for right panel.

Thease items assume more changes in DOMInspector, therefore I leaved out them.

At last this feature is cool thing and usefull :)
Comment 5 Håkan Waara 2006-05-15 02:11:21 PDT
Comment on attachment 221996 [details] [diff] [review]
patch

First of all, nice work Alex!

A few things:

1) You'll need to #ifdef a lot of stuff, in order to not make a dependency on accessibility.

Keep in mind that lots of people have that off, but still want dom inspector to work.

2) You sure we want a11y rows to be bold?

I think a screenshot showing this feature would be cool, for ui review.  :)
Comment 6 alexander :surkov 2006-05-15 02:31:48 PDT
Created attachment 222019 [details]
screenshot

(In reply to comment #5)
> 2) You sure we want a11y rows to be bold?
> 
> I think a screenshot showing this feature would be cool, for ui review.  :)
> 

I don't think that bold is the best. But what is better?
Comment 7 Håkan Waara 2006-05-15 02:47:31 PDT
(In reply to comment #6)
> > 2) You sure we want a11y rows to be bold?
> > 
> > I think a screenshot showing this feature would be cool, for ui review.  :)
> > 
> 
> I don't think that bold is the best. But what is better?
> 

OK, now I understand what's bold.  That looks good to me.  If there are people that consider themselves dom inspector people, they should probably also review this. :-)
Comment 8 alexander :surkov 2006-05-15 18:01:51 PDT
Created attachment 222129 [details] [diff] [review]
patch2

> A few things:
> 
> 1) You'll need to #ifdef a lot of stuff, in order to not make a dependency on
> accessibility.

Agree. I added some #ifdef/#endif preprocessor inctructions. I didn't change GetShowAccessibleNodes/SetShowAccessibleNodes though, I think it would be more appropriate if thease methods will generate exception in mozilla build that doesn't have accessibility support. Right?

And there is a question do xul/js files have mechanism like #ifdef/#endif?
Comment 9 timeless 2006-05-15 22:40:34 PDT
Comment on attachment 222129 [details] [diff] [review]
patch2

please use cvs diff so that i know what version you're munging and can use my scripts to manage your patches.

>@@ -370,6 +392,21 @@ inDOMView::GetCellProperties(PRInt32 row
>       properties->AppendElement(kNotationNodeAtom);
>       break;
>   }
>+
>+#ifdef ACCESSIBILITY
>+  if (mShowAccessibleNodes) {
>+    nsCOMPtr<nsIAccessibilityService> accService =
>+      do_GetService("@mozilla.org/accessibilityService;1");
>+    NS_ENSURE_TRUE(accService, NS_ERROR_FAILURE);

this should *not* be fatal. if accessibility service isn't available, domi should still work <period>.

mozilla/extensions/inspector/resources/content/viewers/accessibleObject/accessibleObject.js	

>\ No newline at end of file

please fix this (applies to each file where diff complains about it)

>@@ -23,6 +23,9 @@
>+    <command id="cmd:toggleAccessibleNodes" viewer="dom"
>+             oncommand="inspector.getViewer('dom').toggleAccessibleNodes(true)"/>

Please include a way to disable the command if the accessible stuff is not supported or not available.

>         accesskey="&cmdToggleAnonContent.accesskey;"
>         observes="cmd:toggleAnon"/>
> 
>+      <menuitem id="item:toggleAccessibleNodes"
>+        class="menuitem-iconic"
>+        type="checkbox"
>+        label="Show Accessible Nodes"

this isn't localization friendly, please follow pattern.

this applies to all strings inline in xul files (and really user facing strings in js/xbl files, i'm not going to leave similar bits or items in this comment).

>+++ mozilla/extensions/inspector/resources/content/viewers/jsObject/jsObjectViewer.js	2006-05-16 09:53:41.000000000 +0900

this feels like you copied an entire portion of a file to a new name. I wonder if this should be a cvs copy followed by a diff to remove the other bits you weren't moving.

thanks for working on this.
Comment 10 alexander :surkov 2006-05-15 23:34:53 PDT
Thank for review. I have a few questions.

> please use cvs diff so that i know what version you're munging and can use my
> scripts to manage your patches.

Currently I use GNU diff util for patch making. Can you give me more info how can I use cvs diff, what additional software should I install. Please say me where can I find your scripts and how can I use them?
 
> >@@ -23,6 +23,9 @@
> >+    <command id="cmd:toggleAccessibleNodes" viewer="dom"
> >+             oncommand="inspector.getViewer('dom').toggleAccessibleNodes(true)"/>
> 
> Please include a way to disable the command if the accessible stuff is not
> supported or not available.
 
I don't have any idea how can I do it. Probably do you have a hint for me? What do 'disable command' means?
 
> >+      <menuitem id="item:toggleAccessibleNodes"
> >+        class="menuitem-iconic"
> >+        type="checkbox"
> >+        label="Show Accessible Nodes"
> 
> this isn't localization friendly, please follow pattern.
 
Right. But I can add the only english dtd entity for every locale. Is it proper for you?
 
> this applies to all strings inline in xul files (and really user facing strings
> in js/xbl files, i'm not going to leave similar bits or items in this comment).
 
Sorry. I don't know english a well, can you please reformulate it? Thank you.
 
> >+++ mozilla/extensions/inspector/resources/content/viewers/jsObject/jsObjectViewer.js 2006-05-16 09:53:41.000000000 +0900
> 
> this feels like you copied an entire portion of a file to a new name. I wonder
> if this should be a cvs copy followed by a diff to remove the other bits you
> weren't moving.
 
Yes, I moved 'jsObjectViewer' object code into file jsObjectViewer.js. Do you think how to save cvs history of moved code? I don't know a much cvs but if there is a way to not lose history then that way should be choosen.

Can I see you in irc chat?
Comment 11 Håkan Waara 2006-05-16 00:43:53 PDT
I'll see if I can answer your questions.

> Thank for review. I have a few questions.
> 
> > please use cvs diff so that i know what version you're munging and can use my
> > scripts to manage your patches.
> 
> Currently I use GNU diff util for patch making. Can you give me more info how
> can I use cvs diff, what additional software should I install. Please say me
> where can I find your scripts and how can I use them?

You should use the cvs diff -u8p command. Please see http://developer.mozilla.org/en/docs/Creating_a_patch

> 
> > >@@ -23,6 +23,9 @@
> > >+    <command id="cmd:toggleAccessibleNodes" viewer="dom"
> > >+             oncommand="inspector.getViewer('dom').toggleAccessibleNodes(true)"/>
> > 
> > Please include a way to disable the command if the accessible stuff is not
> > supported or not available.
> 
> I don't have any idea how can I do it. Probably do you have a hint for me? What
> do 'disable command' means?

Probably you want this to be hidden på default, and then explicitly make it visible if the #ifdef accessibility code is run.

> 
> > >+      <menuitem id="item:toggleAccessibleNodes"
> > >+        class="menuitem-iconic"
> > >+        type="checkbox"
> > >+        label="Show Accessible Nodes"
> > 
> > this isn't localization friendly, please follow pattern.
> 
> Right. But I can add the only english dtd entity for every locale. Is it proper
> for you?

Yes, we keep the english one in the tree, and it will be localized before shipping.

For a XUL tutorial, please see http://developer.mozilla.org/en/docs/XUL_Tutorial

> 
> > this applies to all strings inline in xul files (and really user facing strings
> > in js/xbl files, i'm not going to leave similar bits or items in this comment).
> 
> Sorry. I don't know english a well, can you please reformulate it? Thank you.
> 

In your patch, if CVS complains about "Missing newline at the end of the file", you should add a newline at that file. Some systems want it that way; it's an old UNIX-thing I believe...
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-05-16 04:54:59 PDT
(In reply to comment #11)
> Yes, we keep the english one in the tree, and it will be localized before
> shipping.

Actually, DOMi is a little different... since the localization is in-tree, all localizations need to be added before the patch lands, otherwise the build will break. See bug 109481, for example. This makes it difficult to coordinate large patch landings since it requires effort to have localizers do the work before the patch lands.
Comment 13 alexander :surkov 2006-05-16 20:50:10 PDT
The approach that I went by isn't not simplest as I think before (see comment #4). The causes are localization issue (I should add dtd entity for every locale!!!) and command disabling issue (I don't see a nice way how to support it). I belive I should go by the second approach that I described in comment #3. Please let me get back to you with the new patch in few days.
Comment 14 Håkan Waara 2006-05-17 01:44:38 PDT
The easiest way to make this nicely even without a11y, should be to separate as much code as possible out (like you did with accessibleObject.js), and make to sure load that inside an #ifdef.

One way to separate also the XUL is to use an overlay.

Also, to make stuff disabled/hidden by default, you can add hidden="true" or whatever to the element, and then toggle it via the DOM.
Comment 15 alexander :surkov 2006-05-18 00:12:04 PDT
Created attachment 222463 [details]
sceenshot2

Here is screenshot showing how accessibility support will look for DOMInspector.
Comment 16 alexander :surkov 2006-05-18 02:06:16 PDT
(In reply to comment #11)
> I'll see if I can answer your questions.
> 
> > Thank for review. I have a few questions.
> > 
> > > please use cvs diff so that i know what version you're munging and can use my
> > > scripts to manage your patches.
> > 
> > Currently I use GNU diff util for patch making. Can you give me more info how
> > can I use cvs diff, what additional software should I install. Please say me
> > where can I find your scripts and how can I use them?
> 
> You should use the cvs diff -u8p command. Please see
> http://developer.mozilla.org/en/docs/Creating_a_patch

How can I inlcude new fiels/directories into patch by using cvs diff? (cvs diff -u8p skips new files.)
Comment 17 Christian :Biesinger (don't email me, ping me on IRC) 2006-05-18 02:28:55 PDT
the mentioned developer.mozilla.org article explains that.
Comment 18 alexander :surkov 2006-05-18 02:33:34 PDT
(In reply to comment #17)
> the mentioned developer.mozilla.org article explains that.
> 

Yes, and I should have rights to write mozilla repository :).
Comment 19 alexander :surkov 2006-05-18 02:35:45 PDT
Beaufour gave me own workaround script for patch making, I'll try to use it.
Comment 20 Christian :Biesinger (don't email me, ping me on IRC) 2006-05-18 02:56:18 PDT
it also says "The solution is to use cvsdo utility [1], which edits CVS/Entries to make cvs think the file is added to repository"
Comment 21 alexander :surkov 2006-05-18 04:13:32 PDT
Created attachment 222485 [details] [diff] [review]
patch3

The patch adds accessibility support by the way that was described in comment #3.

I moved many code into new files, therefore code history will be losed. How can I do fix it?
Comment 22 alexander :surkov 2006-05-18 04:31:12 PDT
Created attachment 222486 [details] [diff] [review]
patch4

Sorry the previous patch had some errors.
Comment 23 Håkan Waara 2006-05-18 04:41:01 PDT
Alexander, wanna post a patch with only the real changes included? It's hard to review when all moved files are diffed as "new".
Comment 24 alexander :surkov 2006-05-18 17:58:28 PDT
(In reply to comment #23)
> Alexander, wanna post a patch with only the real changes included? It's hard to
> review when all moved files are diffed as "new".
> 

It's fine for me but how? Probably should I file new bug for rewrites 'jsObject'/'dom' views? But in that case moved code into new files will be marked as new. Any idea?
Comment 25 alexander :surkov 2006-05-19 04:00:23 PDT
Created attachment 222610 [details] [diff] [review]
movepatch1

It's patch for files with moved code.
Comment 26 alexander :surkov 2006-05-19 04:03:11 PDT
Created attachment 222612 [details] [diff] [review]
newpatch

It's patch for modyfied code. Though dom.xul/domViewer.xul and jsObject.xul/jsObjectViewer.xul are moved code too mainly with a little changes: root node of domViewer.xul/jsObjectViewer.xul is overlay now.
Comment 27 alexander :surkov 2006-05-23 00:09:33 PDT
Created attachment 223000 [details] [diff] [review]
movepatch2

updated to trunk
Comment 28 alexander :surkov 2006-05-23 00:18:13 PDT
Created attachment 223001 [details] [diff] [review]
newpatch2

updated to trunk
Comment 29 alexander :surkov 2006-05-24 02:57:42 PDT
Comment on attachment 223001 [details] [diff] [review]
newpatch2

I set hwaara on initial review, as before I'd like to get one more review from timeless. I removed request from him to prevent review dublications.
Comment 30 Håkan Waara 2006-05-24 05:47:52 PDT
Comment on attachment 223001 [details] [diff] [review]
newpatch2

Ok, I found some things to fix, but for the chrome parts, I think maybe we should find someone else to do a good review unless timeless want to be the only reviewer. I emailed Alex Vincent (WeirdAl) to see if he can help us out.

>Index: extensions/inspector/jar.mn
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/inspector/jar.mn,v
>retrieving revision 1.20
>diff -u -p -U8 -r1.20 jar.mn
>--- extensions/inspector/jar.mn	13 May 2006 14:17:22 -0000	1.20
>+++ extensions/inspector/jar.mn	23 May 2006 07:15:27 -0000
>@@ -80,37 +80,45 @@ inspector.jar:
>   content/inspector/viewers/computedStyle/computedStyle.xul           (resources/content/viewers/computedStyle/computedStyle.xul)
>   content/inspector/viewers/dom/FindDialog.js                         (resources/content/viewers/dom/FindDialog.js)
>   content/inspector/viewers/dom/columnsDialog.js                      (resources/content/viewers/dom/columnsDialog.js)
>   content/inspector/viewers/dom/columnsDialog.xul                     (resources/content/viewers/dom/columnsDialog.xul)
>   content/inspector/viewers/dom/commandOverlay.xul                    (resources/content/viewers/dom/commandOverlay.xul)
>   content/inspector/viewers/dom/findDialog.xul                        (resources/content/viewers/dom/findDialog.xul)
>   content/inspector/viewers/dom/keysetOverlay.xul                     (resources/content/viewers/dom/keysetOverlay.xul)
>   content/inspector/viewers/dom/popupOverlay.xul                      (resources/content/viewers/dom/popupOverlay.xul)
>+  content/inspector/viewers/dom/domViewer.xul                         (resources/content/viewers/dom/domViewer.xul)
>+  content/inspector/viewers/dom/domViewer.js                          (resources/content/viewers/dom/domViewer.js)
>   content/inspector/viewers/dom/dom.xul                               (resources/content/viewers/dom/dom.xul)
>   content/inspector/viewers/dom/dom.js                                (resources/content/viewers/dom/dom.js)
>   content/inspector/viewers/dom/pseudoClassDialog.js                  (resources/content/viewers/dom/pseudoClassDialog.js)
>   content/inspector/viewers/dom/pseudoClassDialog.xul                 (resources/content/viewers/dom/pseudoClassDialog.xul)
>   content/inspector/viewers/boxModel/boxModel.js                      (resources/content/viewers/boxModel/boxModel.js)
>   content/inspector/viewers/boxModel/boxModel.xul                     (resources/content/viewers/boxModel/boxModel.xul)
>+  content/inspector/viewers/jsObject/jsObjectViewer.js                (resources/content/viewers/jsObject/jsObjectViewer.js)
>+  content/inspector/viewers/jsObject/jsObjectViewer.xul               (resources/content/viewers/jsObject/jsObjectViewer.xul)
>   content/inspector/viewers/jsObject/jsObject.js                      (resources/content/viewers/jsObject/jsObject.js)
>   content/inspector/viewers/jsObject/jsObject.xul                     (resources/content/viewers/jsObject/jsObject.xul)
>   content/inspector/viewers/jsObject/evalExprDialog.js                (resources/content/viewers/jsObject/evalExprDialog.js)
>   content/inspector/viewers/jsObject/evalExprDialog.xul               (resources/content/viewers/jsObject/evalExprDialog.xul)
>   content/inspector/viewers/domNode/domNode.js                        (resources/content/viewers/domNode/domNode.js)
>   content/inspector/viewers/domNode/domNode.xul                       (resources/content/viewers/domNode/domNode.xul)
>   content/inspector/viewers/styleRules/commandOverlay.xul             (resources/content/viewers/styleRules/commandOverlay.xul)
>   content/inspector/viewers/styleRules/keysetOverlay.xul              (resources/content/viewers/styleRules/keysetOverlay.xul)
>   content/inspector/viewers/styleRules/popupOverlay.xul               (resources/content/viewers/styleRules/popupOverlay.xul)
>   content/inspector/viewers/styleRules/styleRules.js                  (resources/content/viewers/styleRules/styleRules.js)
>   content/inspector/viewers/styleRules/styleRules.xul                 (resources/content/viewers/styleRules/styleRules.xul)
>   content/inspector/viewers/stylesheets/stylesheets.js                (resources/content/viewers/stylesheets/stylesheets.js)
>   content/inspector/viewers/stylesheets/stylesheets.xul               (resources/content/viewers/stylesheets/stylesheets.xul)
>   content/inspector/viewers/xblBindings/xblBindings.js                (resources/content/viewers/xblBindings/xblBindings.js)
>   content/inspector/viewers/xblBindings/xblBindings.xul               (resources/content/viewers/xblBindings/xblBindings.xul)
>+  content/inspector/viewers/accessibleObject/accessibleObject.js      (resources/content/viewers/accessibleObject/accessibleObject.js)
>+  content/inspector/viewers/accessibleObject/accessibleObject.xul     (resources/content/viewers/accessibleObject/accessibleObject.xul)
>+  content/inspector/viewers/accessibleTree/accessibleTree.js          (resources/content/viewers/accessibleTree/accessibleTree.js)
>+  content/inspector/viewers/accessibleTree/accessibleTree.xul         (resources/content/viewers/accessibleTree/accessibleTree.xul)
> % skin inspector classic/1.0 %skin/classic/inspector/

Maybe this should be ifdeffed?

> * skin/classic/inspector/contents.rdf                                 (resources/skin/classic/contents.rdf)
>   skin/classic/inspector/btnFind.gif                                  (resources/skin/classic/btnFind.gif)
>   skin/classic/inspector/btnFind-dis.gif                              (resources/skin/classic/btnFind-dis.gif)
>   skin/classic/inspector/btnSelecting.gif                             (resources/skin/classic/btnSelecting.gif)
>   skin/classic/inspector/btnSelecting-act.gif                         (resources/skin/classic/btnSelecting-act.gif)
>   skin/classic/inspector/btnSelecting-dis.gif                         (resources/skin/classic/btnSelecting-dis.gif)
>   skin/classic/inspector/ImageSearchItem.gif                          (resources/skin/classic/ImageSearchItem.gif)
>Index: layout/inspector/src/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/layout/inspector/src/Makefile.in,v
>retrieving revision 1.29
>diff -u -p -U8 -r1.29 Makefile.in
>--- layout/inspector/src/Makefile.in	19 May 2006 18:59:39 -0000	1.29
>+++ layout/inspector/src/Makefile.in	23 May 2006 07:15:28 -0000
>@@ -56,16 +56,20 @@ REQUIRES	= xpcom \
> 		  widget \
> 		  locale \
> 		  necko \
> 		  docshell \
> 		  view \
> 		  webshell \

Add the ifdef here instead. Otherwise you're adding the a11y module _after_ the $(NULL) item, which doesn't feel really good.

> 		  $(NULL)
> 
>+ifdef ACCESSIBILITY
>+REQUIRES        += accessibility
>+endif
>+

> CPPSRCS= \
>   inDOMView.cpp \
>   inDeepTreeWalker.cpp \
>   inFlasher.cpp \
>   inSearchLoop.cpp \
>   inCSSValueSearch.cpp \
>   inFileSearch.cpp \
>   inDOMUtils.cpp \


The other things look OK to me. But a real chrome guy should look at it really.
Comment 31 Håkan Waara 2006-05-24 05:49:24 PDT
Comment on attachment 223000 [details] [diff] [review]
movepatch2

Unsetting review flag for now on the move patch, since it just moves code. I'll defer to WeirdAl/Timeless on it.
Comment 32 Alex Vincent [:WeirdAl] 2006-05-24 08:21:00 PDT
Comment on attachment 223001 [details] [diff] [review]
newpatch2

Just for convenience's sake, I'd like to see a manual diff between the original files and the new files.  Use -w if you need to.

>Index: extensions/inspector/resources/content/viewers/dom/domViewer.xul
>+<overlay orient="vertical"
>+      xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

Let's try to line up the attributes please.

>+  <commandset id="cmdsDOMViewer">
>+    <command id="cmdInspectInNewWindow" 
>+      oncommand="viewer.cmdInspectInNewWindow()"/>

Semi-colon after the function call.  Same problem repeated a few times.

>+    <treecols>
>+      <!-- These labels don't need to be localized since they are defined by DOM APIs -->
>+      <treecol id="colNodeName" label="nodeName" primary="true" persist="width,hidden,ordinal" flex="3"/>

This line's a little long; I'd prefer we broke it up into several lines and keep the length under 80 characters per line.  Same problem repeated a few times.

>Index: extensions/inspector/resources/content/viewers/jsObject/jsObjectViewer.xul
>+<?xml-stylesheet href="chrome://inspector/skin/"?>

Technically, it may not be required to explicitly state inspector.css, but I'd still like to see it there.

>+<overlay
>+      xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

Please fix indentation.

>+  <popupset id="psPopups">
>+    <popup id="popupContext">
>+      <menuitem label="&inspectNewWindow.label;" observes="cmdInspectInNewView"/>
>+      <menuseparator/>
>+      <menuitem label="&jsCopyValue.label;" observes="cmdCopyValue"/>
>+      <menuitem label="&jsEval.label;" observes="cmdEvalExpr"/>
>+    </popup>
>+  </popupset>

Remove the popupset tags. (See bug 121774, a bug I never really fixed.)

>+ * The Original Code is mozilla.org code.

I'd prefer something a little more specific.


>+  viewer.uid getter = function()
>+  {
>+    return "accessibleTree";
>+  }

Two things.  One, I think this syntax is wrong (unless JS1.7 has changed this).  You might do better passing in an argument to the constructor for DOMViewer.

>+  viewer.initialize = function(aPane)
>+  {
>+    this.mDOMView.showAccessibleNodes = true;
>+    this.__proto__.initialize.call(this, aPane);
>+  }

Two, please name your anonymous functions:
viewer.initialize = function initialize(aPane)

>+    try { 
>+      accObject = accService.getAccessibleFor(aObject);
>+    } catch(e) {}

Don't leave a catch block empty.  Do something with the exception, even if it's only dump(e + "\n").  Ideally, figure out the conditions under which an exception is normally thrown, handle those (as in "catch (e if foo)"), and any unexpected exceptions should propagate.

>? extensions/inspector/resources/content/viewers/accessibleObject
>? extensions/inspector/resources/content/viewers/accessibleTree
>? extensions/inspector/resources/content/viewers/dom/domViewer.xul
>? extensions/inspector/resources/content/viewers/jsObject/jsObjectViewer.xul

cvsdo is your friend.  :)

>Index: extensions/inspector/resources/content/inspector.xml
>+          var uid = this.mLinkedViewer ? this.mLinkedViewer.uid : "";

Where is mLinkedViewer set?  If it's not set anywhere, at the very least this will cause a strict warning.

>Index: extensions/inspector/resources/content/res/viewer-registry.rdf
>+  ins:linkedViewers
>+                The view will be available only in conjunction with viewers that

The final word ('that') isn't necessary here.

>+    <rdf:li><rdf:Description
>+      ins:uid="accessibleTree"
>+      ins:panels="bxDocPanel"
>+      ins:description="Accessible Tree"
>+      ins:filter="try { Components.classes['@mozilla.org/accessibilityService;1'].getService(Components.interfaces.nsIAccessibilityService); return object instanceof Components.interfaces.nsIDOMDocument;; } catch(e) { return false; }"/>
>+    </rdf:li>

<rdf:Description (attrs)>
  <ins:filter>
    try {
      Components.classes['@mozilla.org/accessibilityService;1']
                .getService(Components.interfaces.nsIAccessibilityService);
      return (object instanceof Components.interfaces.nsIDOMDocument);
    }
    catch (e)
    {
      return false;
    }
  </ins:filter>
</rdf:Description>

One of the nice things about RDF is that properties can be attributes or child elements.  Try this approach; shorter lines.

I don't understand why you have the getService() call, when you don't assign it to anything.

>Index: layout/inspector/public/inIDOMView.idl
>+  attribute boolean showAccessibleNodes;

Doesn't this require a UUID update?

Overall, I want to apply one patch locally (sorry, hwaara) and run some tests on it first to see if I understand what this does to DOM Inspector.  So my review comments are minor and not too detailed until I actually see for myself what this does.
Comment 33 alexander :surkov 2006-05-26 22:39:17 PDT
Created attachment 223529 [details] [diff] [review]
movepatch3

Sorry for delay. Alex, your comments assume changes in DOMView/JSObjectView objects. Therefore I prefer to review and check in the movepatch before the fixing the rest of your notes. This patch are move code (partially DOMView and JSObjectView objects) from dom.js/jsobject.js files to domView.js/jsObjectView.js files. Also the patch move the code from dom.xul/jsObject.xul to domView.xul/jsObjectView.xul files. I fixed your comments about code style issues.
Comment 34 Alex Vincent [:WeirdAl] 2006-05-26 23:55:32 PDT
Comment on attachment 223529 [details] [diff] [review]
movepatch3

I guess I just became an Inspector reviewer.  :)  (I'm not officially named as a DOM Inspector peer, so I'll transfer the review request over to timeless after I'm done.  That said, I'm happy to help, and anyone who asks me for review should expect a response within 48 hours.)

Please also reference http://beaufour.dk/jst-review/?patch=223529 - in particular, observe the F, L and W warnings.  (This reviewer simulacrum was written for C++ based patches, so the rest you can probably ignore.)  Note L warnings don't apply to jar.mn files.

I'm somewhat concerned about maintaining the cvs revision history from old files to new; I'm not sure how to handle that in this case.  That said, the below comments for CVS moved files I will not hold you responsible for in this bug.  I'll leave that call up to timeless.

>Index: extensions/inspector/resources/content/viewers/dom/domViewer.js
>+ * Contributor(s):
>+ *   Joe Hewitt <hewitt@netscape.com> (original author)
>+ *   Jason Barnabe <jason_barnabe@fastmail.fm>

If you want to add yourself, please go ahead.

>+  toXML: function(aNode)

Since you're copying the whole file, go ahead and give all these anonymous functions names.

>+  {
>+    // we'll use XML serializer, if available
>+    if (typeof XMLSerializer != "undefined")

Not quite good enough.  I'd prefer we explicitly asked if XMLSerializer was a function.  Better yet, let's explicitly try to create a XMLSerializer via XPCOM.  Reference

http://lxr.mozilla.org/seamonkey/source/mail/extensions/newsblog/content/feed-parser.js#41

and also the use of XPCU in this file.

If you do this, wrap it in a try...catch block, and have the failure case call on _toXML.  I realize this isn't your fault, but I wasn't paying close enough attention.

>+    if (aNode.nodeType == Node.ELEMENT_NODE) {

Let's fix this classic botch.  We should never refer to the Node constructor.  This happens frequently in the file.

Components.interfaces.nsIDOMNode.ELEMENT_NODE

Feel free to set a constant such as nsIDOMNode, or reuse one provided elsewhere.

>+  ///////////////////////////////////////////////////////////////////////////
>+  // Takes an element from the document being inspected, finds the treeitem
>+  // which represents it in the DOM tree and selects that treeitem.
>+  //
>+  // @param aEl - element from the document being inspected
>+  ///////////////////////////////////////////////////////////////////////////
>+
>+  selectElementInTree: function(aEl, aExpand, aQuickie)

I don't suppose I can reasonably ask you to JavaDoc all the functions in this file.  That's a lot.  Still, where
there is JavaDoc-style commenting, it should be updated and done in the proper style:

/**
 * (description)
 *
 * @param aEl      element from the document being inspected
 * @param aExpand  ?
 * @param aQuickie ?
 *
 * @return ?
 */

>+  // XX re-implement custom columns code some-day  

Let's give that a proper 3rd X, so we pay attention to it :)

>+      try {
>+        flasher.element = aElement;
>+        flasher.start();
>+      } catch (ex) {
>+      }

This is one of my personal pet peeves:  an empty catch block.  Dump the exception or include a relevant comment, please.

>+  onPrefChanged: function(aName)
>+  {
>+    if (aName == "inspector.dom.showAnon")
>+      this.setFlashSelected(PrefUtils.getPref("inspector.blink.on"));
>+
>+    if (aName == "inspector.blink.on")
>+      this.setFlashSelected(PrefUtils.getPref("inspector.blink.on"));

Why split the condition like this?

>+    if (this.mFlasher) {
>+      if (aName == "inspector.blink.border-color") {
>+        this.mFlasher.color = PrefUtils.getPref("inspector.blink.border-color");
>+      } else if (aName == "inspector.blink.border-width") {
>+        this.mFlasher.thickness = PrefUtils.getPref("inspector.blink.border-width");
>+      } else if (aName == "inspector.blink.duration") {
>+        this.mFlasher.duration = PrefUtils.getPref("inspector.blink.duration");
>+      } else if (aName == "inspector.blink.speed") {
>+        this.mFlasher.speed = PrefUtils.getPref("inspector.blink.speed");
>+      } else if (aName == "inspector.blink.invert") {
>+        this.mFlasher.invert = PrefUtils.getPref("inspector.blink.invert");
>+      }

This code's a little ugly, but I can live with it.  Suggest (not require) a switch-case block on aName.

>+  findDocuments: function(aDoc, aArray)
>+  {
>+    this.addKidsToArray(aDoc.getElementsByTagName("frame"), aArray);
>+    this.addKidsToArray(aDoc.getElementsByTagName("iframe"), aArray);

My instinct says to explicitly name the namespaces we care about:  null (for HTML documents), XHTML, and XUL.  Again, not required.

>Index: extensions/inspector/resources/content/viewers/dom/domViewer.xul

Where's the license boilerplate for this file?  I just looked at the CVS log for dom.xul, and there's no indication it was ever removed.

>+    <command id="cmdInspectInNewWindow" 
>+             oncommand="viewer.cmdInspectInNewWindow()"/>

Semi-colons after JS statements, please.

>+  <!--============================= POPUPS ============================== -->

Since we only have one popup, this line can go away.

>Index: extensions/inspector/resources/content/viewers/jsObject/jsObjectViewer.js
>+  doEvalExpr: function(aExpr, aItem, aNewView)
>+  {
>+    // TODO: I should really write some C++ code to execute the 
>+    // js code in the js context of the inspected window

I want a bug filed for this.  If there's already a bug, cite it here please.  Let's also make this a XXX comment.  (I wonder who would be good to cc on this... bz?)

>Index: extensions/inspector/resources/content/viewers/jsObject/jsObjectViewer.xul

Missing boilerplate again.

I just hit a stack overflow crash on trunk testing this patch, but I've verified on trunk it isn't your crash.
Comment 35 timeless 2006-05-27 17:26:47 PDT
Comment on attachment 223529 [details] [diff] [review]
movepatch3

i'd rather wait to see the next round of the patch, i don't disagree w/ weirdal's comments.
Comment 36 alexander :surkov 2006-05-28 19:37:49 PDT
Alex, thank you for very detailed review. Really I did't await to get the such sort of reivew :) because patch contains moved code. I'm not code author and I don't know it pretty well therefore I can get some difficulties to fix issues you pointed me. And I'm afraid to wake up sleeping dog. For sure I'm agree with you comments and if you and timeless want for thease issues fixing then I'll do it.
Comment 37 alexander :surkov 2006-05-28 19:45:16 PDT
(In reply to comment #35)
> (From update of attachment 223529 [details] [diff] [review] [edit])
> i'd rather wait to see the next round of the patch, i don't disagree w/
> weirdal's comments.
> 

Please say me how the patch should look. I can do the patch all in one. You can apply it and see what it do, but it will be not easy to review. I can't do just next round of the patch because it assumes changes in moved code per previous Alex review (comment#32). Please say me what do you expect?
Comment 38 alexander :surkov 2006-05-29 22:37:43 PDT
Created attachment 223741 [details] [diff] [review]
patch5

(In reply to comment #32)

> Let's try to line up the attributes please.

> Semi-colon after the function call.  Same problem repeated a few times.

> This line's a little long; I'd prefer we broke it up into several lines and
> keep the length under 80 characters per line.  Same problem repeated a few
> times.

I guess it's fixed.

> Remove the popupset tags. (See bug 121774, a bug I never really fixed.)

Fixed.
> >+  viewer.uid getter = function()
> >+  {
> >+    return "accessibleTree";
> >+  }
> 
> I think this syntax is wrong (unless JS1.7 has changed this).
>  You might do better passing in an argument to the constructor for DOMViewer.

Fixed, I use __defineGetter__/__defineSetter__

> 
> Two, please name your anonymous functions:
> viewer.initialize = function initialize(aPane)

Fixed.

> >+    try { 
> >+      accObject = accService.getAccessibleFor(aObject);
> >+    } catch(e) {}
> 
> Don't leave a catch block empty.  Do something with the exception, even if it's
> only dump(e + "\n").  Ideally, figure out the conditions under which an
> exception is normally thrown, handle those (as in "catch (e if foo)"), and any
> unexpected exceptions should propagate.

I added dump() function.

> >? extensions/inspector/resources/content/viewers/accessibleObject
> >? extensions/inspector/resources/content/viewers/accessibleTree
> >? extensions/inspector/resources/content/viewers/dom/domViewer.xul
> >? extensions/inspector/resources/content/viewers/jsObject/jsObjectViewer.xul
> 
> cvsdo is your friend.  :)

Already yes :)

> 
> >Index: extensions/inspector/resources/content/inspector.xml
> >+          var uid = this.mLinkedViewer ? this.mLinkedViewer.uid : "";
> 
> Where is mLinkedViewer set?  If it's not set anywhere, at the very least this
> will cause a strict warning.

mLinkedViewer is setted in observerLinkedViewer() method when selection is changed in linked panel. I didn't get why it will cause a warning?

> <rdf:Description (attrs)>
>   <ins:filter>
>     try {
>       Components.classes['@mozilla.org/accessibilityService;1']
>                 .getService(Components.interfaces.nsIAccessibilityService);
>       return (object instanceof Components.interfaces.nsIDOMDocument);
>     }
>     catch (e)
>     {
>       return false;
>     }
>   </ins:filter>
> </rdf:Description>
> 
> One of the nice things about RDF is that properties can be attributes or child
> elements.  Try this approach; shorter lines.

Really, yes. I didn't know about it. It's cool thing.

> I don't understand why you have the getService() call, when you don't assign it
> to anything.

I'm just trying to check wheter accessibility service is available. If I can't get the service then I shouldn't show a panel.


> >Index: layout/inspector/public/inIDOMView.idl
> >+  attribute boolean showAccessibleNodes;
> 
> Doesn't this require a UUID update?

I guess, yes. Fixed.

(In reply to comment #34)
> (From update of attachment 223529 [details] [diff] [review] [edit])

> >Index: extensions/inspector/resources/content/viewers/dom/domViewer.js
> >+ * Contributor(s):
> >+ *   Joe Hewitt <hewitt@netscape.com> (original author)
> >+ *   Jason Barnabe <jason_barnabe@fastmail.fm>
> 
> If you want to add yourself, please go ahead.

I like to see my name at every mozilla source file but I afraid that file will be parsed too long if anyone who do some changes will add oneself to contributers. More I guess contributers is responsibility and I can't allow myself to be responsible for code I don't know good. :)

> >+  toXML: function(aNode)
> 
> Since you're copying the whole file, go ahead and give all these anonymous
> functions names.

Fixed.

> >+  {
> >+    // we'll use XML serializer, if available
> >+    if (typeof XMLSerializer != "undefined")
> 
> Not quite good enough.  I'd prefer we explicitly asked if XMLSerializer was a
> function.  Better yet, let's explicitly try to create a XMLSerializer via
> XPCOM.
> 
> If you do this, wrap it in a try...catch block, and have the failure case call
> on _toXML.  I realize this isn't your fault, but I wasn't paying close enough
> attention.
> 

I puted XMLSerializer call into try/catch block.

> >+    if (aNode.nodeType == Node.ELEMENT_NODE) {
> 
> Let's fix this classic botch.  We should never refer to the Node constructor. 
> This happens frequently in the file.

I guess it's fixed.

> I don't suppose I can reasonably ask you to JavaDoc all the functions in this
> file.  That's a lot.  Still, where
> there is JavaDoc-style commenting, it should be updated and done in the proper
> style:

Fixed.
> >+  // XX re-implement custom columns code some-day  
> Let's give that a proper 3rd X, so we pay attention to it :)
Fixed.

> >+  onPrefChanged: function(aName)
> >+  {
> >+    if (aName == "inspector.dom.showAnon")
> >+      this.setFlashSelected(PrefUtils.getPref("inspector.blink.on"));
> >+
> >+    if (aName == "inspector.blink.on")
> >+      this.setFlashSelected(PrefUtils.getPref("inspector.blink.on"));
> 
> Why split the condition like this?

I'm not sure but I think the spiling is unneeded. Fixed.

> 
> >+    if (this.mFlasher) {
> >+      if (aName == "inspector.blink.border-color") {
> >+        this.mFlasher.color = PrefUtils.getPref("inspector.blink.border-color");
> >+      } else if (aName == "inspector.blink.border-width") {
> >+        this.mFlasher.thickness = PrefUtils.getPref("inspector.blink.border-width");
> >+      } else if (aName == "inspector.blink.duration") {
> >+        this.mFlasher.duration = PrefUtils.getPref("inspector.blink.duration");
> >+      } else if (aName == "inspector.blink.speed") {
> >+        this.mFlasher.speed = PrefUtils.getPref("inspector.blink.speed");
> >+      } else if (aName == "inspector.blink.invert") {
> >+        this.mFlasher.invert = PrefUtils.getPref("inspector.blink.invert");
> >+      }
> 
> This code's a little ugly, but I can live with it.  Suggest (not require) a
> switch-case block on aName.

I used switch/case block now.

> >+  findDocuments: function(aDoc, aArray)
> >+  {
> >+    this.addKidsToArray(aDoc.getElementsByTagName("frame"), aArray);
> >+    this.addKidsToArray(aDoc.getElementsByTagName("iframe"), aArray);
> 
> My instinct says to explicitly name the namespaces we care about:  null (for
> HTML documents), XHTML, and XUL.  Again, not required.

Fixed.

> >Index: extensions/inspector/resources/content/viewers/dom/domViewer.xul
> 
> Where's the license boilerplate for this file?  I just looked at the CVS log
> for dom.xul, and there's no indication it was ever removed.

I added license boilerplate, I reference on Joe Hewitt.
> 
> >Index: extensions/inspector/resources/content/viewers/jsObject/jsObjectViewer.js
> >+  doEvalExpr: function(aExpr, aItem, aNewView)
> >+  {
> >+    // TODO: I should really write some C++ code to execute the 
> >+    // js code in the js context of the inspected window
> 
> I want a bug filed for this.  If there's already a bug, cite it here please. 
> Let's also make this a XXX comment.  (I wonder who would be good to cc on
> this... bz?)

Not fixed yet.
Comment 39 Alex Vincent [:WeirdAl] 2006-05-30 01:22:44 PDT
Comment on attachment 223741 [details] [diff] [review]
patch5

Please use the -p option in generating cvs diff's - function names help give context to diffs.  Specifically, "cvs diff -pu8N extensions/inspector".

>Index: extensions/inspector/resources/content/ViewerRegistry.js
>+  findViewersForObject: function(aObject, aPanelId, aLinkedViewerUID)

Anonymous function.  Bad karma for reviewers.  :)

>Index: extensions/inspector/resources/content/res/viewer-registry.rdf
>+            Components.classes['@mozilla.org/accessibilityService;1'].
>+              getService(Components.interfaces.nsIAccessibilityService);

Generally speaking, the second period (for getService) should be on the second line, and lined up with the first dot:

Components.classes[...]
          .getService(...);

>Index: extensions/inspector/resources/content/viewers/accessibleObject/accessibleObject.js
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Foundation.

surkov says he has a consulting agreement with Mozilla Foundation (not Corporation, I asked), so this is technically correct.

>+      var ti = this.addTreeItem(this.mTreeKids,
>+                                bundle.getString("root.title"), accObject,
>+                                accObject);

It looks like each argument here should be on its own line - so please move accObject down a line.  There are other cases of this problem, which I won't require a fix for, but I do recommend it.

>Index: extensions/inspector/resources/content/viewers/accessibleObject/accessibleObject.xul
>+  <script type="application/x-javascript"

Nit:  We now support application/javascript, so the x-factor is no longer necessary.  (Don't go out of your way to fix this, though - application/x-javascript is also valid.)

>Index: extensions/inspector/resources/content/viewers/dom/domViewer.js
>+  isCommandEnabled: function isCommandEnabled(aCommand)
>+          canPaste = node ? node.nodeType != Node.ATTRIBUTE_NODE : false;

Missed this one.

>+  onContextCreate: function onContextCreate(aPP)
>+  {
>+    var mi, cmd;
>+    for (var i = 0; i < aPP.childNodes.length; ++i) {
>+      mi = aPP.childNodes[i];
>+      if (mi.hasAttribute("observes")) {
>+        cmd = document.getElementById(mi.getAttribute("observes"));
>+        if (cmd && cmd.hasAttribute("isvalid")) {
>+          try {
>+            var isValid = new Function(cmd.getAttribute("isvalid"));
>+          } catch (ex) { /* die quietly on syntax error in handler */ }

My concern here is that if you have an exception, isValid() isn't overwritten, so you could have a false positive.  In the catch, you should probably say:

isValid = function return_false() { return false; }

If you do this, move the declaration for isValid to the top of the function.

>+  cmdInspectBrowserIsValid: function cmdInspectBrowserIsValid()
>+    return n == "tabbrowser" || n == "browser" || n == "iframe" ||
>+      n == "frame" || n == "editor";

switch (n) {
  case "tabbrowser":
  case "browser":
  case "frame":
  case "iframe":
  case "editor":
    return true;
}

return false;

>+  cmdInspectBrowser: function cmdInspectBrowser()
>+  {
>+    var node = this.selectedNode;
>+    var n = node.localName.toLowerCase();
>+    if (node && n == "browser" && node.namespaceURI == kXULNSURI) {
>+      // xul browser
>+      this.subject = node.contentDocument;
>+    } else if (node && n == "tabbrowser" && node.namespaceURI == kXULNSURI) {
>+      // xul tabbrowser
>+      this.subject = node.contentDocument;
>+    } else if (n == "iframe" && node.namespaceURI == kXULNSURI) {
>+      // xul iframe
>+      this.subject = node.contentDocument;
>+    } else if (n == "iframe" || n == "frame") {
>+      // html iframe or frame
>+      this.subject = node.contentDocument;
>+    } else if (n == "editor") {
>+      // editor
>+      this.subject = node.contentDocument;
>+    }
>+  },

<foo:editor/>.  I think this function can be rewritten a bit:

if (node.namespaceURI = kXULNSURI)
{
  switch (n)
  {
    case "browser":
    case "tabbrowser":
    case "editor":
    case "iframe":
      this.subject = node.contentDocument;
  }
}

if ((node.ownerDocument instanceof Components.interfaces.nsIDOMHTMLDocument) &&
    (n == "iframe" || n == "frame")) {
  this.subject = node.contentDocument;
}

etc. etc.

>+    try {
>+      // we'll use XML serializer, if available
>+      xmltext = (new XMLSerializer()).serializeToString(aNode);

Didn't I say something about this in my last review?

>+          s += line + (i < aNode.attributes.length-1 ? "\n"+attrIndent : "");

Spacing on either side of the second + character.

>+    } else if (aNode.nodeType == Node.TEXT_NODE) {
>+      s += InsUtil.unicodeToEntity(aNode.data);
>+    } else if (aNode.nodeType == Node.COMMENT_NODE) {
>+      s += line + "<!--" + InsUtil.unicodeToEntity(aNode.data) + "-->\n";
>+    } else if (aNode.nodeType == Node.DOCUMENT_NODE) {

nsIDOMNode.

>+  startFind: function startFind(aType, aDir)
>+      this.mFindParams[i-2] = arguments[i];

[i - 2]

>+  /**
>+   * Takes an element from the document being inspected, finds the treeitem
>+   * which represents it in the DOM tree and selects that treeitem.
>+   *
>+   * @param aEl - element from the document being inspected
>+   * @param aExpand - XXX: add comment for the argument
>+   * @param aQuickie - XXX: add comment for the argument
>+   */
I don't want to see these XXX comments in another patch.  Explain to me what these are.  (I can probably figure it out, but your average developer doesn't want to figure it out.)

>+  canPaste: function canPaste(aFlavour)
>+  {
>+    return aFlavour == "Inspector-DOM-Node";
>+  }

I have a hunch this is wrong.  I can't put my finger on it, but LXR only shows "Inspector-DOM-Node" once in the mozilla.org codebase - and this patch only replaces that line.

>Index: extensions/inspector/resources/content/viewers/dom/domViewer.xul
>+   - The Initial Developer of the Original Code is
>+   - Netscape Communications Corporation.
>+   - Portions created by the Initial Developer are Copyright (C) 2006

The copyright year doesn't jibe well with the initial developer.  Netscape doesn't exist as a mozilla.org contributor anymore.  Same for jsObjectViewer.xul.

Now, for test results.  This is what I'm going to r- the patch for.

(1) I don't see much of a clear difference between DOM Nodes and Accessible Tree.  One thing I do see is that I can't turn on/off whitespace text node showing or anonymous node showing when I'm in Accessible Tree mode.  The dropdown for that is disabled in the Accessible Tree viewer.

(2) Could you try putting the UI for turning on/off accessibility options in that dropdown?  It would make more sense there, and might not require code changes as drastic.  (You could still generate the nsIAccessible stuff, and just not show it, maybe.)  In addition, this might mean we don't have to redraw the tree every time.

(3) Right-hand panel, Accessible Object and JavaScript Object again appear remarkably similar.  Again, the UI for turning on/off accessibility options can be placed in a dropdown to the right of the viewer selector.

(4) You apparently haven't picked up a recent change to JSObject viewer (right hand panel) where the JSObject viewer automatically expands out the root item in the tree.  Yours doesn't do that.

(5) I'm getting an assertion in running this code.

###!!! ASSERTION: element not in the document: 'doc', file /home/ajvincent/mozbuilds/mozilla/layout/base/nsChildIterator.cpp, line 62

I hit this switching from JavaScript Object to Accessible Object in the right-hand panel.

(6) I'm getting another assertion in using the dropdown while in DOM Nodes viewer (left-hand panel) and changing the "Show whitespace nodes" option.

###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!

This is pretty much always bad. It usually means that native code is
making a callback to an interface implemented in JavaScript, but the
document where the JS object was created has already been cleared and the
global properties of that document's window are *gone*. Generally this
indicates a problem that should be addressed in the design and use of the
callback code.
: 'Error', file /home/ajvincent/mozbuilds/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 587
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "viewer is not defined" {file: "chrome://inspector/content/viewers/dom/domViewer.js" line: 1032}]' when calling method: [nsIObserver::observe]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://inspector/content/jsutil/system/PrefUtils.js :: anonymous :: line 94"  data: yes]
************************************************************

Neither of these assertions are violated in Inspector without your patch applied.  Assertion failures are basically crashes, so I can't recommend r+ on this without having those assertions fixed or at least bugs filed for them... and you'd have to work pretty hard to convince me to let it go with bugs filed.
Comment 40 alexander :surkov 2006-05-30 05:35:07 PDT
Created attachment 223767 [details] [diff] [review]
patch6

(In reply to comment #39)

I guess nits and code styling issues should be fixed. Though I'm afraid I missed something.

> 
> >+    try {
> >+      // we'll use XML serializer, if available
> >+      xmltext = (new XMLSerializer()).serializeToString(aNode);
> 
> Didn't I say something about this in my last review?

Yes, you said something about it.

> >+  {
> >+    // we'll use XML serializer, if available
> >+    if (typeof XMLSerializer != "undefined")

> Not quite good enough.  I'd prefer we explicitly asked if XMLSerializer was a
> function.  Better yet, let's explicitly try to create a XMLSerializer via
> XPCOM.  Reference

> http://lxr.mozilla.org/seamonkey/source/mail/extensions/newsblog/content/feed-parser.js#41

> and also the use of XPCU in this file.

> If you do this, wrap it in a try...catch block, and have the failure case call
> on _toXML.  I realize this isn't your fault, but I wasn't paying close enough
> attention.

I guess I did it under your words. I can't see a benefits in XPCU using.

> Now, for test results.  This is what I'm going to r- the patch for.
> 
> (1) I don't see much of a clear difference between DOM Nodes and Accessible
> Tree.  One thing I do see is that I can't turn on/off whitespace text node
> showing or anonymous node showing when I'm in Accessible Tree mode.  The
> dropdown for that is disabled in the Accessible Tree viewer.
> 
> (2) Could you try putting the UI for turning on/off accessibility options in
> that dropdown?  It would make more sense there, and might not require code
> changes as drastic.  (You could still generate the nsIAccessible stuff, and
> just not show it, maybe.)  In addition, this might mean we don't have to redraw
> the tree every time.
> 

> (3) Right-hand panel, Accessible Object and JavaScript Object again appear
> remarkably similar.  Again, the UI for turning on/off accessibility options can
> be placed in a dropdown to the right of the viewer selector.
> 
> (4) You apparently haven't picked up a recent change to JSObject viewer (right
> hand panel) where the JSObject viewer automatically expands out the root item
> in the tree.  Yours doesn't do that.

I combined 'dom' view and 'accessibleTree' view. Probably you'right that way is better. Please look at current approach.

> (5) I'm getting an assertion in running this code.

I failed to get assertions after code rewrite.
Comment 41 Alex Vincent [:WeirdAl] 2006-05-30 10:22:57 PDT
Comment on attachment 223767 [details] [diff] [review]
patch6

And the patches keep getting bigger.  :)  I'm kidding about that, of course, but I am not kidding when I remind you about observing the F, L and W warnings from http://beaufour.dk/jst-review/ (except for jar.mn).

>Index: extensions/inspector/resources/content/ViewerRegistry.js
>+  /**
>+   * Returns the absolute url where the xul file for a viewer can be found.
>+   *
>+   * @param long aIndex - the index of the entry representing the viewer
>+   * @return wstring - the fully cannonized url
>+   */
>+  getEntryURL: function getEntryURL(aIndex)

Add a line with asterisk before the @return, please.  My apologies for not being clear on that earlier.

>Index: extensions/inspector/resources/content/res/viewer-registry.rdf
>+  ins:filter    This is JavaScript function that serves to enable/hide the view
>+                if it doesn't meet with conditions imposed for passed objects as
>+                function arguments. The first argument is nsIDOMNode object (or
>+                in the case of JavaScript Object viewer, JavaScript object),
>+                the argument name is "object". The second argument is linked
>+                viewer object (it is not null for right panel's views), the
>+                argument name is "linkedPanel".
>+                For example, ins:filter=
>+                "return object instanceof Components.interfaces.nsIDOMDocument;"
>+                is a JavaScript fragment ViewerRegistry.js uses to define a
>+                filter function if the panel's subject node is an instance of
>+                nsIDOMDocument.

The English could use a little bit of cleaning up (this is me-as-former-journalist speaking, not me-as-reviewer).  I'm not 100% certain what I want to change here.

>+            var accService =
>+              Components.classes['@mozilla.org/accessibilityService;1'].
>+                         getService(Components.interfaces.nsIAccessibilityService);

Second period is still on the wrong line.

>Index: extensions/inspector/resources/content/viewers/accessibleObject/accessibleObject.js
>+  accService = Components.classes['@mozilla.org/accessibilityService;1'].
>+               getService(Components.interfaces.nsIAccessibilityService);

Ditto.

>Index: extensions/inspector/resources/content/viewers/dom/commandOverlay.xul
>+    <command id="cmd:toggleAccessibleNodes" viewer="dom"
>+             oncommand="inspector.getViewer('dom').toggleAccessibleNodes(true)"/>

Again with the semicolon at the end of the oncommand attribute.  I notice the rest of the file doesn't do that, but I want to see it at least for new code.

>Index: extensions/inspector/resources/content/viewers/dom/dom.js
>+  cmdInspectBrowserIsValid: function cmdInspectBrowserIsValid()
>   {
>     var node = viewer.selectedNode;
>-    if (!node || node.nodeType != Node.ELEMENT_NODE) return false;
>-    
>-    var n = node.localName.toLowerCase();
>-    return n == "tabbrowser" || n == "browser" || n == "iframe" || n == "frame" || n == "editor";
>+    if (!node || node.nodeType != nsIDOMNode.ELEMENT_NODE) return false;
>+
>+    var nodeName = node.localName.toLowerCase();
>+    switch (nodeName) {
>+      case "tabbrowser":
>+      case "browser":
>+      case "iframe":
>+      case "frame":
>+      case "editor":
>+        return true;
>+    }
>+    return false;
>   },

I goofed on reviewing this before.  Sorry!  Comparing cmdInspectBrowserIsValid with cmdInspectBrowser, it's pretty obvious they should be similar; perhaps the latter should call the former?

>+    if (node.ownerDocument instanceof Components.interfaces.nsIDOMHTMLDocument) {
>+      switch (nodeName) {
>+        case "iframe":
>+        case "frame":
>+          this.subject = node.contentDocument;
>+          break;
>+      }

I goofed again here.  We're still not checking the namespace of the node.  There are two conditions we care about:

(1) The ownerDocument is a HTMLDocument, not a XMLDocument and node has one of these tag names (this covers HTML).  Reference http://weblogs.mozillazine.org/weirdal/archives/005746.html for some old-ish code I wrote a while ago which may be useful if we still don't QI to XMLDocument for XHTML docs.

(2) The ownerDocument is a HTMLDocument, and node has the XHTML namespace (this covers XHTML).

I know this is getting convoluted, and I do apologize for that.  Maybe one of the DOM folks knows a shortcut or two that I'm missing.

>-////////////////////////////////////////////////////////////////////////////
>-//// Listener Objects
>+/**
>+ * Listener Objects
>+ */

Again, this comment change isn't correct (this shouldn't be a JavaDoc-style comment).

>Index: extensions/inspector/resources/content/viewers/dom/popupOverlay.xul
>   <popupset id="ppsViewerPopupset">
>     <menupopup id="ppViewerContext-dom">
>       <menuitem id="item:find"
>-        label="&cmdShowFindDialog.label;"
>-        accesskey="&cmdShowFindDialog.accesskey;"
>-        key="key:find"
>-        observes="cmd:find"/>
>+                label="&cmdShowFindDialog.label;"
>+                accesskey="&cmdShowFindDialog.accesskey;"
>+                key="key:find"
>+                observes="cmd:find"/>

You're going to hate me for saying this, but it must be said now.  (I only said it here because the popupset element wasn't removed this time.)

A lot of the changes you've made are fixing up whitespace, naming anonymous functions, etc. - all good stuff per my first review... if they were in new files.  Considering the original files are being modified, these changes mess up CVS blame without really adding new value.  (I got chewed out on this sort of thing myself about two months ago.) This sort of cleanup work should be done incrementally as we work on patches for the old code, not wholesale as part of a bigger bug fix.  I have to r- this patch for that.

That said, I still want the Node -> nsIDOMNode changes.  That's a potential hole, and more serious than the cvs blame issue.  (Check with timeless first before filing a new patch, though - he may feel it's not worth it, and I don't want you to work harder than you have to to get r+/sr+.)

Let me be absolutely clear:  it's all right and a Good Idea to make these sort of retroactive changes on new and/or newly-copied files for the CVS repo.  It's not all right to do these changes on currently-existing files.

>Index: extensions/inspector/resources/locale/ca/viewers/dom.dtd
>+<!ENTITY cmdToggleAccessibleNodes.label "Show Accessible Nodes">
>+<!ENTITY cmdToggleAccessibleNodes.accesskey "c">

Heh.  I realize you don't have the facilities to do the translations all by yourself.  I don't know how Inspector localization requests are handled, but now's a good time for both of us to figure that out.

Test results:
(1) I'm still seeing test result 3 from comment 39.
(2) I'm still seeing test result 5 (the first assertion) from comment 39.  Steps to reproduce:
* Load http://www.mozilla.org/projects/seamonkey/start/ into browser window.
* Ctrl+Shift+I to launch Inspector from browser.
* Find Label element (#document > HTML > BODY > #container > #header > #search > DIV > LABEL)
* Right-hand panel, switch to Accessible Object viewer.
* Right-hand panel, switch to JavaScript Object viewer - assertion fires.

Stack trace at http://mozilla.pastebin.com/747139 .
Comment 42 alexander :surkov 2006-05-31 10:31:40 PDT
Me (In reply to comment #39)

> 
> (1) I don't see much of a clear difference between DOM Nodes and Accessible
> Tree.  One thing I do see is that I can't turn on/off whitespace text node
> showing or anonymous node showing when I'm in Accessible Tree mode.  The
> dropdown for that is disabled in the Accessible Tree viewer.
> 
> (2) Could you try putting the UI for turning on/off accessibility options in
> that dropdown?  It would make more sense there, and might not require code
> changes as drastic.  (You could still generate the nsIAccessible stuff, and
> just not show it, maybe.)  In addition, this might mean we don't have to redraw
> the tree every time.
> 
> (3) Right-hand panel, Accessible Object and JavaScript Object again appear
> remarkably similar.  Again, the UI for turning on/off accessibility options can
> be placed in a dropdown to the right of the viewer selector.
> 
> (4) You apparently haven't picked up a recent change to JSObject viewer (right
> hand panel) where the JSObject viewer automatically expands out the root item
> in the tree.  Yours doesn't do that.

Me and Alex have some different points of view about how accessible features should be realized for DOMInspector. You can see Alex's thoughts above. My thoughts are below. I think accessible tree/accessible object should have separate view that conradicts by some way with above Alex's comments.

It seems to me Alex thinks (sorry for my assumptions) view is way to display some sort of data, I think view is data. Therefore I guess f.x. Alex proposes to show accessible object in 'jsObject' view since accessible object and dom node object are javascript objects. I think dom node object and accessible objects are two different properties of dom node and therefore they should have separate views. In the same manner I think about accessible tree since accessible tree is not dom tree.

It's interesting to me to know opinions of people around accessibility (f.x. Hwaara) and DOMInspector people (f.x. timeless) and for sure not categorized in that manner people opinions are wellcome too :).
Comment 43 alexander :surkov 2006-06-05 00:50:49 PDT
Alex, I talked with timeless on irc. He is with you about left panel and he is with me about right panel. In general it's suits for me. How about you?
Comment 44 Alex Vincent [:WeirdAl] 2006-06-05 00:52:29 PDT
Except for maybe caillon weighing in, he's the boss.
Comment 45 alexander :surkov 2006-06-06 08:34:03 PDT
Created attachment 224563 [details] [diff] [review]
patch7

> >Index: extensions/inspector/resources/content/res/viewer-registry.rdf
> >+  ins:filter    This is JavaScript function that serves to enable/hide the view
> >+                if it doesn't meet with conditions imposed for passed objects as
> >+                function arguments. The first argument is nsIDOMNode object (or
> >+                in the case of JavaScript Object viewer, JavaScript object),
> >+                the argument name is "object". The second argument is linked
> >+                viewer object (it is not null for right panel's views), the
> >+                argument name is "linkedPanel".
> >+                For example, ins:filter=
> >+                "return object instanceof Components.interfaces.nsIDOMDocument;"
> >+                is a JavaScript fragment ViewerRegistry.js uses to define a
> >+                filter function if the panel's subject node is an instance of
> >+                nsIDOMDocument.
> 
> The English could use a little bit of cleaning up (this is
> me-as-former-journalist speaking, not me-as-reviewer).  I'm not 100% certain
> what I want to change here.

I reformulated this, but if comment is wrong from point of view of former-journalist (or at least man knowing english) then please help me to change it.

> Let me be absolutely clear:  it's all right and a Good Idea to make these sort
> of retroactive changes on new and/or newly-copied files for the CVS repo.  It's
> not all right to do these changes on currently-existing files.

Ok, I removed lot of such changes from my patch.

> You're going to hate me for saying this, but it must be said now.  (I only said
> it here because the popupset element wasn't removed this time.)

Corresponding to previous note let me file the bug for it.

> Heh.  I realize you don't have the facilities to do the translations all by
> yourself.  I don't know how Inspector localization requests are handled, but
> now's a good time for both of us to figure that out.

Right, I'm afraid to translate russian locale even since I don't know how some terms sounds right. Probably I should file separate bug for it.

> (2) I'm still seeing test result 5 (the first assertion) from comment 39. 

Let me file new bug for this :)
Comment 46 Alex Vincent [:WeirdAl] 2006-06-06 11:43:44 PDT
Comment on attachment 224563 [details] [diff] [review]
patch7

>> Let me be absolutely clear:  it's all right and a Good Idea to make these sort
>> of retroactive changes on new and/or newly-copied files for the CVS repo.  It's
>> not all right to do these changes on currently-existing files.

>Ok, I removed lot of such changes from my patch.

Not enough of them - see below.  For the most part, though, I like this patch a lot.

Kill the Windows line-endings.

>Index: mozilla/extensions/inspector/resources/content/ViewerRegistry.js
>+  /**
>+   * Determines if an object is eligible to be viewed by a particular viewer.
>+   *
>+   * @param Object aObject - the object being checked for eligibility
>+   * @param long aIndex - the index of the entry
>+   *
>+   * @return boolean - true if object can be viewed
>+   */
>+  objectMatchesEntry: function objectMatchesEntry(aObject, aLinkedViewer, aIndex)

You missed a @param for aLinkedViewer.

>Index: mozilla/extensions/inspector/resources/content/res/viewer-registry.rdf
>-    <rdf:li><rdf:Description 
>+    <rdf:li><rdf:Description

I don't think we want to fix the spacing on these.  Just let these go.

>Index: mozilla/extensions/inspector/resources/content/viewers/dom/commandOverlay.xul
>-             oncommand="inspector.getViewer('dom').selectByClick()"/>
>+             oncommand="inspector.getViewer('dom').selectByClick();"/>

The semicolon change here isn't worth it.  Just let these go as well.  :)

>Index: mozilla/extensions/inspector/resources/content/viewers/dom/popupOverlay.xul
>       <menuitem id="item:find"
>-        label="&cmdShowFindDialog.label;"
>-        accesskey="&cmdShowFindDialog.accesskey;"
>-        key="key:find"
>-        observes="cmd:find"/>
>+                label="&cmdShowFindDialog.label;"
>+                accesskey="&cmdShowFindDialog.accesskey;"
>+                key="key:find"
>+                observes="cmd:find"/>

These whitespace changes aren't needed either.

I won't be able to test this until tonight; if you want to submit a new patch before then, that's fine.
Comment 47 Alex Vincent [:WeirdAl] 2006-06-06 12:14:02 PDT
Comment on attachment 224563 [details] [diff] [review]
patch7

In a build with accessibility disabled, we're broken:

Warning ``reference to undefined property this.mLinkedViewer'' [-s] in file ``chrome://inspector/content/inspector.xml'', line 665, character 0.

Error ``aLinkedViewer is not defined'' [x-] in file ``chrome://inspector/content/ViewerRegistry.js'', line 155, character 0.

Sorry, buddy.  I guess I need to test in builds with accessibility disabled, too.
Comment 48 alexander :surkov 2006-06-07 09:53:08 PDT
Created attachment 224714 [details] [diff] [review]
patch8
Comment 49 Alex Vincent [:WeirdAl] 2006-06-07 21:35:09 PDT
(Stripping trailing CRs from patch.)

Look up dos2unix.  :)

I looked at the patch earlier this afternoon, and I didn't spot anything to critique.  That said, in a build with --disable-accessibility (typical for MSVC8 free edition), the "Show Accessible Nodes" menu item in the DOM Nodes panel dropdown menu should be either disabled or not included at all.  I smell a need for XUL preprocessing.  :)

I won't r- the patch for that.  Still to come, the litmu... err, Linux test. :)
Comment 50 alexander :surkov 2006-06-10 10:57:08 PDT
Created attachment 225146 [details] [diff] [review]
patch9

(In reply to comment #49)
> (Stripping trailing CRs from patch.)
> 
> Look up dos2unix.  :)
> 

Patch has unix line separating.

> I looked at the patch earlier this afternoon, and I didn't spot anything to
> critique.  That said, in a build with --disable-accessibility (typical for
> MSVC8 free edition), the "Show Accessible Nodes" menu item in the DOM Nodes
> panel dropdown menu should be either disabled or not included at all.  I smell
> a need for XUL preprocessing.  :)
> 
> I won't r- the patch for that.  Still to come, the litmu... err, Linux test. :)
> 

If accessibility is not accessible :) then releated menuitem is disabeld.
Comment 51 Alex Vincent [:WeirdAl] 2006-06-10 16:30:31 PDT
Created attachment 225167 [details]
screenshot from Windows (with disable-accessibility)

This is what I was referring to - the menuitem isn't disabled (or gone), but --disable-accessibility was part of the build config.
Comment 52 alexander :surkov 2006-06-10 19:28:07 PDT
(In reply to comment #51)
> Created an attachment (id=225167) [edit]
> screenshot from Windows (with disable-accessibility)
> 
> This is what I was referring to - the menuitem isn't disabled (or gone), but
> --disable-accessibility was part of the build config.
> 

It looks strange. I builded mozilla with "ac_add_options --disable-accessibility" option on Winodows and I menuitem "Show Accessible Nodes" is disabled. Do you build clean mozilla copy (not on top of previous mozilla build)?
Comment 53 Alex Vincent [:WeirdAl] 2006-06-11 20:44:21 PDT
Is your patch updated to latest trunk?  I'm seeing conflicts.
Comment 54 alexander :surkov 2006-06-14 02:23:04 PDT
Created attachment 225555 [details] [diff] [review]
patch10

(In reply to comment #53)
> Is your patch updated to latest trunk?  I'm seeing conflicts.
> 

Fixed.
Comment 55 Alex Vincent [:WeirdAl] 2006-06-14 11:04:45 PDT
Comment on attachment 225555 [details] [diff] [review]
patch10

(Stripping trailing CRs from patch.)

*tsk, tsk*

>Index: mozilla/extensions/inspector/resources/content/viewers/dom/dom.js
>+  toggleAccessibleNodes: function toggleAccessibleNodes(aRebuild)

>+  setAccessibleNodes: function setAccessibleNodes(aValue, aRebuild)

Nit: Could you javadoc these new functions?  (Not a hard requirement.)

>Index: mozilla/extensions/inspector/resources/content/viewers/jsObject/jsObjectViewer.js

Same nit in this file.

>+    // XXX: TODO: I should really write some C++ code to execute the
>+    // js code in the js context of the inspected window

Please, file the bug for this now and cite the bug number here.

>Index: mozilla/layout/inspector/src/inDOMView.cpp
>+inDOMView::SetShowAccessibleNodes(PRBool aShowAccessibleNodes)
>+{
>+#ifdef ACCESSIBILITY
>+  mShowAccessibleNodes = aShowAccessibleNodes;
>+#else
>+  mShowAccessibleNodes = PR_FALSE;
>+#endif
>+  return NS_OK;
>+}

I'm thinking a little about this, and wondering why we arbitrarily set it to false and return NS_OK at the same time ifndef ACCESSIBILITY.  With --disable-accessibility, the user shouldn't be able to call on this function, so I think this should return a XPCOM error code, at least if the user's passing in true, and maybe all the time.

Of course, if accessibility is enabled, then NS_OK is the right return code.

I'm undecided about this, so if you don't feel it's necessary, don't worry about it.  I raise the point for future reviews, though.

>@@ -365,16 +391,32 @@ inDOMView::GetCellProperties(PRInt32 row
>+#ifdef ACCESSIBILITY
>+  if (mShowAccessibleNodes) {
>+    nsCOMPtr<nsIAccessibilityService> accService =
>+      do_GetService("@mozilla.org/accessibilityService;1");

Is this service always built with a11y enabled?  If so, let's NS_ENSURE_TRUE on this, or modify the code to assign a rv which we NS_ENSURE_SUCCESS on.

The patch looks very clean to me, and indeed, testing with --disable-accessibility now disables the "Show Accessible Nodes" option.  Very nice.  Continue to eyeball http://beaufour.dk/jst-review/?patch=225555&file= for other nits.

I'll finish my review tonight, when I get home to test this with accessibility enabled.
Comment 56 alexander :surkov 2006-06-14 20:59:26 PDT
Created attachment 225668 [details] [diff] [review]
patch11

(In reply to comment #55)
> Nit: Could you javadoc these new functions?  (Not a hard requirement.)

Fixed.

> >+    // XXX: TODO: I should really write some C++ code to execute the
> >+    // js code in the js context of the inspected window
> 
> Please, file the bug for this now and cite the bug number here.

Fixed, bug number is 341598.
> I'm thinking a little about this, and wondering why we arbitrarily set it to
> false and return NS_OK at the same time ifndef ACCESSIBILITY.  With
> --disable-accessibility, the user shouldn't be able to call on this function,
> so I think this should return a XPCOM error code, at least if the user's
> passing in true, and maybe all the time.

I guess you're right. That's way is fine for me.

> Is this service always built with a11y enabled?  If so, let's NS_ENSURE_TRUE on
> this, or modify the code to assign a rv which we NS_ENSURE_SUCCESS on.

Yes, this service are always built with ally enabled afaik.

> The patch looks very clean to me, and indeed, testing with
> --disable-accessibility now disables the "Show Accessible Nodes" option.  Very
> nice.  Continue to eyeball http://beaufour.dk/jst-review/?patch=225555&file=
> for other nits.

I fixed some nits.
Comment 57 Alex Vincent [:WeirdAl] 2006-06-14 23:35:52 PDT
Comment on attachment 225668 [details] [diff] [review]
patch11

>+  setAccessibleNodes: function setAccessibleNodes(aValue, aRebuild)
>+  {
>+    try {
>+      this.mDOMView.showAccessibleNodes = aValue;
>+    } catch (e) {
>+      // inDOMView::showAccessibleNodes can fail when mozilla build you're
>+      // using is without accessbility support. It means 'Show Accessible
>+      // Nodes' option is not accessible.
>+      aValue = false;
>+    }

Personal opinion: I actually don't want this exception caught and silenced.  Because the menuitem that would enable this is disabled, the exception that comes from showAccessibleNodes should propagate.

Don't submit a new patch for this, though.  It could be this is correct by review standards.
Comment 58 Alex Vincent [:WeirdAl] 2006-06-15 00:42:46 PDT
Comment on attachment 225668 [details] [diff] [review]
patch11

Test results:

(1) Opening DOM-I on http://www.mozilla.org/projects/seamonkey/start/, I see the #document node is bolded, but when I go to the right-hand panel to select the Accessible Object panel, I don't see that option.  It becomes available after I select another node, and go back to #document.  Please look into this.

(2) I'm still seeing test result 5 (the first assertion) from comment 39.

I'm really concerned about that; I'm consistently reproducing it, and you're not seeing it.

Overall, I'm pleased enough to give this patch an unofficial r+.  Please ask timeless for reviews for the next few days if you submit new patches, as I will be out of town and offline.

timeless: there are two or three nits I found (including that blasted assertion) which surkov hasn't answered yet; I'll leave it up to you to decide what should be done.
Comment 59 alexander :surkov 2006-06-15 22:01:13 PDT
(In reply to comment #58)
> (From update of attachment 225668 [details] [diff] [review] [edit])
> Test results:
> 
> (1) Opening DOM-I on http://www.mozilla.org/projects/seamonkey/start/, I see
> the #document node is bolded, but when I go to the right-hand panel to select
> the Accessible Object panel, I don't see that option.  It becomes available
> after I select another node, and go back to #document.  Please look into this.

Right, I see it. I'll fix it.

> (2) I'm still seeing test result 5 (the first assertion) from comment 39.
> 
> I'm really concerned about that; I'm consistently reproducing it, and you're
> not seeing it.

As before I can't see that assertion.
Comment 60 alexander :surkov 2006-07-08 20:36:29 PDT
Created attachment 228569 [details] [diff] [review]
patch12

> (1) Opening DOM-I on http://www.mozilla.org/projects/seamonkey/start/, I see
> the #document node is bolded, but when I go to the right-hand panel to select
> the Accessible Object panel, I don't see that option.  It becomes available
> after I select another node, and go back to #document.  Please look into this.

Fixed.
Comment 61 timeless 2006-07-08 22:40:05 PDT
Comment on attachment 228569 [details] [diff] [review]
patch12

i'd rather not have the ifdefs in c++, they should be rewritable in some js code (possibly requiring an extra *generic* domi interface to allow the c++ to ask js for help)

i'd rather the copied source involve a cvs copy (this statement is module approval for the cvs copy) followed by the patch removing what it doesn't need from the two files (original, copy).

in a later bug, we should change this:
window.addEventListener("load", AccessibleObjectViewer_initialize, false);
function AccessibleObjectViewer_initialize()

so that it uses standard interCaps since it's not a class.

  emptyTree: function emptyTree(aTreeKids)
    while (aTreeKids.hasChildNodes()) {
      aTreeKids.removeChild(aTreeKids.lastChild);
i think neil said removeChild(....firstChild) was cheaper

mozilla/extensions/inspector/resources/content/viewers/jsObject/jsObjectViewer.js
        value = value.replace(/\n|\r|\t|\v/g, " ");

i wonder if that regexp can be /\s/g or /\s+/g (to be dealt with in a later bug).

  addTreeItem: function addTreeItem(aTreeChildren, aName, aValue, aObject)
    // listen for changes to open attribute
    this.mTreeKids.addEventListener("DOMAttrModified", onTreeItemAttrModified,
                                    false);
// XXX why do we do this each time? :(

all of these things can be addressed in later bugs
Comment 62 alexander :surkov 2006-07-08 22:48:33 PDT
waiting for when bug 343997 will be fixed (coping jsObject.js file to jsObjectViewer.js file)
Comment 63 neil@parkwaycc.co.uk 2006-07-09 13:08:56 PDT
(In reply to comment #61)
>   emptyTree: function emptyTree(aTreeKids)
>     while (aTreeKids.hasChildNodes()) {
>       aTreeKids.removeChild(aTreeKids.lastChild);
> i think neil said removeChild(....firstChild) was cheaper
No, I said I think lastChild was cheaper.
Comment 64 neil@parkwaycc.co.uk 2006-07-21 07:25:34 PDT
Comment on attachment 228569 [details] [diff] [review]
patch12

>   skin/classic/inspector/viewers/xblBindings/xblBindings.css          (resources/skin/classic/viewers/xblBindings/xblBindings.css)
>+  content/inspector/viewers/accessibleObject/accessibleObject.js      (resources/content/viewers/accessibleObject/accessibleObject.js)
>+  content/inspector/viewers/accessibleObject/accessibleObject.xul     (resources/content/viewers/accessibleObject/accessibleObject.xul)
These belong in alphabetical order, not in some random location.

>Index: extensions/inspector/resources/content/ViewerRegistry.js
I don't think these changes make sense.

>+    // check if accessibility service is accessible
Sorry, I just thought that was so humorous :-)

>+      try {
>+        var accService =
>+              Components.classes['@mozilla.org/accessibilityService;1']
>+                        .getService(Components.interfaces.nsIAccessibilityService);
>+      } catch (e) {
>+        cmd.setAttribute("disabled", "true");
>+      }
Rather than using try/catch I think you should find that
if (!('@mozilla.org/accessibilityService;1' in Components.classes))
should work.

>+          if (!linkedViewer || linkedViewer.uid != "dom" ||
>+              !linkedViewer.getAccessibleNodes())
>+            return false;
I don't think that the panel should depend on the loaded viewer.

>+            return accService.getAccessibleFor(object);
You didn't actually verify that the selection is a node.

>+  viewer.__defineGetter__(
>+    "uid",
>+    function uidGetter()
getUid?

>+  viewer.__defineSetter__(
>+    "subject",
>+    function subjectSetter(aObject)
setSubject?

>Index: extensions/inspector/resources/content/viewers/dom/commandOverlay.xul
Not sure why, but my inspector menus didn't work after I applied the patch.

As for the #ifdefs, there are already plenty of them in layout.
Comment 65 alexander :surkov 2006-07-24 01:54:29 PDT
(In reply to comment #64)

> >Index: extensions/inspector/resources/content/ViewerRegistry.js
> I don't think these changes make sense.

I didn't get why. Can you explain?

> 
> >+    // check if accessibility service is accessible
> Sorry, I just thought that was so humorous :-)

Yes, it sounds so (I knew it), but my English didn't allow me to figure out how it should be :). Can you advise me?

> 
> >+          if (!linkedViewer || linkedViewer.uid != "dom" ||
> >+              !linkedViewer.getAccessibleNodes())
> >+            return false;
> I don't think that the panel should depend on the loaded viewer.

Actually, panel hasn't dependency on loaded viewer, but viewer has that dependency on other viewer. Viewer of left panel defines viewers set that can be loaded into right panel.

I agree with rest of comments and I'll fix them in following up patch.
Comment 66 alexander :surkov 2006-08-01 21:02:55 PDT
Created attachment 231711 [details] [diff] [review]
patch13
Comment 67 neil@parkwaycc.co.uk 2006-08-02 08:11:08 PDT
(In reply to comment #65)
>(In reply to comment #64)
>>>Index: extensions/inspector/resources/content/ViewerRegistry.js
>>I don't think these changes make sense.
>I didn't get why. Can you explain?
This actually relates to the viewer dependency issue below.

>>>+    // check if accessibility service is accessible
>>Sorry, I just thought that was so humorous :-)
>Yes, it sounds so (I knew it), but my English didn't allow me to figure out how
>it should be :). Can you advise me?
I like "available".

>>>+          if (!linkedViewer || linkedViewer.uid != "dom" ||
>>>+              !linkedViewer.getAccessibleNodes())
>>>+            return false;
>>I don't think that the panel should depend on the loaded viewer.
>Actually, panel hasn't dependency on loaded viewer, but viewer has that
>dependency on other viewer. Viewer of left panel defines viewers set that can
>be loaded into right panel.
Actually I did mean that left viewer... I just don't see the point of going to all the trouble to introduce the dependency of that panel on the dom viewer.
Comment 68 neil@parkwaycc.co.uk 2006-08-02 09:41:08 PDT
Comment on attachment 231711 [details] [diff] [review]
patch13

I really don't like the link between the display of the accessible nodes in the left pane and the selection of the accessible object in the right pane. But I guess you can have sr=me if you fix these last two issues here:

>-  {"NOTATION_NODE", &inDOMView::kNotationNodeAtom}
>+  {"NOTATION_NODE", &inDOMView::kNotationNodeAtom},
>+  {"ACCESSIBLE_NODE", &inDOMView::kAccessibleNodeAtom}
This change is incorrect. This is a table of DOM node types, not DOMI atoms.

>+    nsCOMPtr<nsIAccessible> accessible;
>+    nsresult rv = accService->GetAccessibleFor(node->node,
>+                                               getter_AddRefs(accessible));
>+    if (NS_SUCCEEDED(rv))
>+      properties->AppendElement(kAccessibleNodeAtom);
You should null-check accessible instead, in case GetAccessibleFor is improved to simply return null for inaccessible nodes.
Comment 69 alexander :surkov 2006-08-02 10:24:42 PDT
(In reply to comment #68)
> (From update of attachment 231711 [details] [diff] [review] [edit])
> I really don't like the link between the display of the accessible nodes in the
> left pane and the selection of the accessible object in the right pane.

In any way I don't see any ugliness in that because left and right views are related with each other. I just maked the relation explictly.

> But I
> guess you can have sr=me if you fix these last two issues here:
> 
> >-  {"NOTATION_NODE", &inDOMView::kNotationNodeAtom}
> >+  {"NOTATION_NODE", &inDOMView::kNotationNodeAtom},
> >+  {"ACCESSIBLE_NODE", &inDOMView::kAccessibleNodeAtom}
> This change is incorrect. This is a table of DOM node types, not DOMI atoms.

I agree these atoms are DOM node types currently but they are used to style dom nodes in tree only I guess. From that point of view it seems to me changes are looked reasonable. For sure I'm not know excelent DOMI and if you insist then I'll fix it. Do you prefer to have separate structure for kAccessibleNodeAtom?
Comment 70 alexander :surkov 2006-08-04 10:56:43 PDT
Created attachment 232155 [details] [diff] [review]
patch for checkin

(In reply to comment #68)
> (From update of attachment 231711 [details] [diff] [review] [edit])
> I really don't like the link between the display of the accessible nodes in the
> left pane and the selection of the accessible object in the right pane. But I
> guess you can have sr=me if you fix these last two issues here:
> 
> >-  {"NOTATION_NODE", &inDOMView::kNotationNodeAtom}
> >+  {"NOTATION_NODE", &inDOMView::kNotationNodeAtom},
> >+  {"ACCESSIBLE_NODE", &inDOMView::kAccessibleNodeAtom}
> This change is incorrect. This is a table of DOM node types, not DOMI atoms.

After talk with Neil on irc we decide to leave it as it is.

> >+    nsCOMPtr<nsIAccessible> accessible;
> >+    nsresult rv = accService->GetAccessibleFor(node->node,
> >+                                               getter_AddRefs(accessible));
> >+    if (NS_SUCCEEDED(rv))
> >+      properties->AppendElement(kAccessibleNodeAtom);
> You should null-check accessible instead, in case GetAccessibleFor is improved
> to simply return null for inaccessible nodes.
> 

Fixed.
Comment 71 alexander :surkov 2006-08-07 11:06:29 PDT
Created attachment 232564 [details] [diff] [review]
latest patch

forgot to include new files in previous patch
Comment 72 Aaron Leventhal 2006-08-07 12:54:39 PDT
Checked in.
Comment 73 David Baron :dbaron: ⌚️UTC-8 2007-02-27 18:36:06 PST
It seems somewhat disturbing to see ACCESSIBLE_NODE added to a list that otherwise comes from http://www.w3.org/TR/REC-DOM-Level-1/level-one-core.html#ID-1950641247 , without any distinction.  It's not a node, so why should it look like everything in the list of node types?  It seems like it should be called "HAS_ACCESSIBLE" and kHasAccessibleAtom.

Likewise the showAccessibleNodes pref really shouldn't be so similar to the showWhitespaceNodes pref -- the current name makes it seem like turning the pref off will cause things not to be shown.  Shouldn't the pref really be called something like "highlightNodesWithAccessibleObjects"?  (And it seems like the highlighting could be independent of whether there's a panel for the accessibility info -- why wouldn't one want that panel to be present, even when the node isn't highlighted?  It just makes the feature less discoverable.)
Comment 74 alexander :surkov 2007-02-28 07:54:00 PST
(In reply to comment #73)
> It seems somewhat disturbing to see ACCESSIBLE_NODE added to a list that
> otherwise comes from
> http://www.w3.org/TR/REC-DOM-Level-1/level-one-core.html#ID-1950641247 ,
> without any distinction.  It's not a node, so why should it look like
> everything in the list of node types?  It seems like it should be called
> "HAS_ACCESSIBLE" and kHasAccessibleAtom.

Accessible node is DOM node that is accessible for AT i.e. it has related accessible object.

Note You need to log in before you can comment on or make changes to this bug.