Last Comment Bug 534527 - Accessibility needs new DOM API
: Accessibility needs new DOM API
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: treea11y 530081 544498
  Show dependency treegraph
 
Reported: 2009-12-13 10:22 PST by David Bolter [:davidb]
Modified: 2010-07-29 07:29 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+


Attachments
patch (26.06 KB, patch)
2010-01-28 09:48 PST, alexander :surkov
bugs: review+
Details | Diff | Review
patch2 (26.51 KB, patch)
2010-02-01 05:22 PST, alexander :surkov
no flags Details | Diff | Review
patch3 (28.67 KB, patch)
2010-02-01 20:39 PST, alexander :surkov
no flags Details | Diff | Review
patch4 (28.08 KB, patch)
2010-02-04 09:42 PST, alexander :surkov
no flags Details | Diff | Review
patch5 (28.10 KB, patch)
2010-02-05 12:08 PST, alexander :surkov
no flags Details | Diff | Review
patch6 (28.18 KB, patch)
2010-02-09 09:17 PST, alexander :surkov
no flags Details | Diff | Review
patch7 (29.07 KB, patch)
2010-02-10 01:20 PST, alexander :surkov
bugs: review+
roc: superreview+
Details | Diff | Review
patch8 (32.50 KB, patch)
2010-02-11 08:54 PST, alexander :surkov
no flags Details | Diff | Review
bz patch (21.48 KB, patch)
2010-02-12 10:42 PST, alexander :surkov
no flags Details | Diff | Review
bz patch2 (21.79 KB, patch)
2010-02-12 11:07 PST, alexander :surkov
no flags Details | Diff | Review
bz patch3 (21.78 KB, patch)
2010-02-15 02:16 PST, alexander :surkov
bugs: review+
bzbarsky: superreview+
Details | Diff | Review

Description David Bolter [:davidb] 2009-12-13 10:22:28 PST
To gather information about content our gecko accessibility module walks DOM nodes as well as frames. We walk frames because there is sometimes anonymous frame content that we can't get through DOM API.

Walking frames is brittle because it is possible our accessibility code can indirectly cause frames to be deallocated. Using nsWeakFrames is a bit of a band-aid as frames really shouldn't be expected to hang around, and we will just fail to walk into the content we need sometimes -- and layout should be free to deallocated frames whenever necessary.

Let's add API so that we can walk into all the content we need. The ability to get a frame from nsIContent might be a good start -- let's discuss.

Note we should keep in mind frame content generated from :before and :after rules; for additional reading: nsAccessibleTreeWalker.cpp
Comment 1 alexander :surkov 2009-12-13 12:09:23 PST
You might want to keep in mind list bullet frame as well.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-12-13 16:00:47 PST
list bullets, really? That is a problem because list bullets don't have associated content ndoes we can return.
Comment 3 alexander :surkov 2009-12-13 17:31:14 PST
(In reply to comment #2)
> list bullets, really? That is a problem because list bullets don't have
> associated content ndoes we can return.

Right. That's probably the thing we should care separately like we do now. But any way it would be nice it it will be available from the new API.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-12-13 17:33:15 PST
I suppose we might be able to create an anonymous content node representing a list bullet, just for this API.
Comment 5 alexander :surkov 2009-12-13 17:37:37 PST
(In reply to comment #4)
> I suppose we might be able to create an anonymous content node representing a
> list bullet, just for this API.

that's great
Comment 6 David Bolter [:davidb] 2009-12-13 17:47:30 PST
Do we think this API will be used outside accessibility?
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-12-13 18:00:28 PST
I don't know. Probably.
Comment 8 alexander :surkov 2010-01-26 09:29:44 PST
Ok, I think we want interface like

interface nsThoughoutTreeWalker : public nsIContent
{
  nsINode* getChildAt(nsINode* aParent, PRUint32 aIndex);
  nsINode* getParent(nsINode* aParent); 
};

this should cover cases:
1) walking into XBL (work with result children set)
2) walking into native anonymous content
3) getting list bullet content (it should be created)
4) getting generated content (:after and :before), if I get right it has a content
5) walking through documents (the iframe child should be nsIDocument node).

We don't want to get XUL tree cells since they may have neither content or frame.

David, how does it sound?
Comment 9 alexander :surkov 2010-01-26 09:30:53 PST
(In reply to comment #8)
> Ok, I think we want interface like
> 
> interface nsThoughoutTreeWalker : public nsIContent

sorry, public nsISupports.
Comment 10 David Bolter [:davidb] 2010-01-26 11:28:25 PST
(In reply to comment #8)
> David, how does it sound?

Yep I think this roughly fits what we discussed today on IRC. One tree to walk would simplify our code a lot, not sure what the implementation is going to look like in layout.
Comment 11 alexander :surkov 2010-01-26 12:02:22 PST
(In reply to comment #8)
> Ok, I think we want interface like
> 
> interface nsThoughoutTreeWalker : public nsIContent
> {
>   nsINode* getChildAt(nsINode* aParent, PRUint32 aIndex);

Technically we don't give direct access to i-th child, we would be happy with

nsINode* getNextChild(nsINode* aParent, nsINode* aChild);

>   nsINode* getParent(nsINode* aParent); 

I think we could live without this method since
1) we need to create accessible from top to bottom
2) nsINode::GetParent() returns the real parent

However I think it would be nice to have it since it could return host element of nsIDocument (HTML:iframe case).
Comment 12 alexander :surkov 2010-01-26 12:09:21 PST
(In reply to comment #8)

> this should cover cases:
> 2) walking into native anonymous content

if I read the code right then the problem of this the anonymous content is known when nsCSSFrameConstructor constructs the frame and nodes for anonymous content. Anonymous content is bind to the tree but it doesn't alter the parent bindings children. Therefore it's not accessible from nsINode traversal methods. After this point the anonymous content can be obtained from frame tree.
Comment 13 alexander :surkov 2010-01-26 12:29:10 PST
The same is applicable to generated content which is created in nsCSSFrameConstructor::ProcessChildren together with anonymous content. However I'm not sure ProcessChildren works for all possible cases.

It seems like we don't have direct access to implicit content (generated content, native anon content and XBL anon content). Perhaps it makes sense to cache it when it's constructed in nsCSSFrameConstructor. What's the best way?
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-26 12:31:23 PST
I don't think this is the right interface. It could get very confusing when the tree changes between calls to getChildAt or getNextChild. Things could also get confusing when you call getParent.

Instead I think you should have a method that gets all the current children of a node into an array.
Comment 15 alexander :surkov 2010-01-26 12:38:54 PST
(In reply to comment #14)

> Instead I think you should have a method that gets all the current children of
> a node into an array.

This interface is similar to nsIContent, what's happen if children are changed when you traverse through them by getChildAt()?

I'm thinking about this notation since it should be quicker than having an array. Actually we don't need to keep this interface instance for a long time (it should be always local), we need to traverse through children and create accessible objects.
Comment 16 David Bolter [:davidb] 2010-01-26 13:10:06 PST
(In reply to comment #14)
> I don't think this is the right interface. It could get very confusing when the
> tree changes between calls to getChildAt or getNextChild.

Yes.

> Things could also get
> confusing when you call getParent.

Yes. Note things get confusing in /accessible today ;)

> Instead I think you should have a method that gets all the current children of
> a node into an array.

Just to be clear, where would the method live? What children would go in the array?
Comment 17 alexander :surkov 2010-01-26 13:14:05 PST
(In reply to comment #15)
> (In reply to comment #14)
> 
> > Instead I think you should have a method that gets all the current children of
> > a node into an array.
> 
> I'm thinking about this notation since it should be quicker than having an
> array. 

I mean we are happy with the array if it won't created every time when we request it. If implementation will be close to nsBindingManager::GetAnonymousContent then it's great.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-26 13:24:02 PST
(In reply to comment #15)
> > Instead I think you should have a method that gets all the current children of
> > a node into an array.
> 
> This interface is similar to nsIContent, what's happen if children are changed
> when you traverse through them by getChildAt()?

Something bad. But layout/content code can avoid flushing between calls to getChildAt. Here, you need an interface that can cope with flushing between calls.

I would just have something like

class nsIFullTreeWalker : public nsISupports
{
  virtual void GetChildren(nsINode* aNode, nsCOMArray<nsIContent>* aChildren) = 0;
  virtual already_AddRefed<nsINode> GetParent(nsINode* aParent) = 0;
};

In theory, you could get into loops if flushes keep occurring ... e.g., nodes might constantly move around in the tree. In practice, I don't think you need to worry about that. At worst it's a hang, not a security problem.
Comment 19 alexander :surkov 2010-01-26 13:33:55 PST
(In reply to comment #18)
> (In reply to comment #15)
> > > Instead I think you should have a method that gets all the current children of
> > > a node into an array.
> > 
> > This interface is similar to nsIContent, what's happen if children are changed
> > when you traverse through them by getChildAt()?
> 
> Something bad. But layout/content code can avoid flushing between calls to
> getChildAt. Here, you need an interface that can cope with flushing between
> calls.

In general we shouldn't trigger layout flushing when we create accessible tree because if flushing occurs then we won't create correct accessible tree which is bad.
Comment 20 alexander :surkov 2010-01-26 13:42:06 PST
(In reply to comment #18)

>   virtual void GetChildren(nsINode* aNode, nsCOMArray<nsIContent>* aChildren) =
> 0;

Any way GetChildren approach would fit great for XBL bindigns since nsBingingManager allows to get cached children array (if we don't take into account the binding can have generated content).

How can GetChildren be implemented to return native anon and generated content? Should the array be calculated every time? Can it be obtained based on the frame tree entirely? Or should it get nsIContent children or XBL binding children arrays and morph them by generated or native anon content insertion?
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-26 14:17:51 PST
(In reply to comment #19)
> In general we shouldn't trigger layout flushing when we create accessible tree
> because if flushing occurs then we won't create correct accessible tree which
> is bad.

Yes, but the problem is that you do anyway.

(In reply to comment #20)
> How can GetChildren be implemented to return native anon and generated content?
> Should the array be calculated every time? Can it be obtained based on the
> frame tree entirely?

You'll have to get them from the frame tree. We'll need to extend nsIFrame with a method to get the native anonymous content for the frame, and implement that method for each frame that has native anonymous content.

Then you can implement GetChildren by gathering the XBL child list (including the real children), the native anonymous content, and the generated content.
Comment 22 alexander :surkov 2010-01-27 06:36:46 PST
(In reply to comment #21)

> You'll have to get them from the frame tree. We'll need to extend nsIFrame with
> a method to get the native anonymous content for the frame, and implement that
> method for each frame that has native anonymous content.

So the frame should keep an array of native anonymous content, right?

> 
> Then you can implement GetChildren by gathering the XBL child list (including
> the real children), the native anonymous content, and the generated content.

How can I find the generated content? Should I traverse all frame children, get their content and check its node names (nsGkAtoms::mozgeneratedcontentbefore or nsGkAtoms::mozgeneratedcontentafter)?
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-27 12:19:41 PST
(In reply to comment #22)
> (In reply to comment #21)
> > You'll have to get them from the frame tree. We'll need to extend nsIFrame
> > with a method to get the native anonymous content for the frame, and
> > implement that method for each frame that has native anonymous content.
> 
> So the frame should keep an array of native anonymous content, right?

Frames with anonymous content tend to already keep references to their anonymous nodes in member variables. The frame Destroy methods clean up that content.

> How can I find the generated content? Should I traverse all frame children, get
> their content and check its node names (nsGkAtoms::mozgeneratedcontentbefore or
> nsGkAtoms::mozgeneratedcontentafter)?

No. You should use nsLayoutUtils::GetBeforeFrame/GetAfterFrame and call GetContent on the returned frame.
Comment 24 alexander :surkov 2010-01-28 02:42:31 PST
Could we extend nsIDocument interface to add these methods? Would it be more nice than create new interface and XPCOM service for it?
Comment 25 alexander :surkov 2010-01-28 06:54:03 PST
(In reply to comment #24)
> Could we extend nsIDocument interface to add these methods?

I think nsIDocument should be a proper place (since it has methods to get XBL anonymous content already). Also I think it's enough to have GetAllChildren() method only. At least we can live without GetParent() now. Is GetAllChildren() fine name?
Comment 26 David Bolter [:davidb] 2010-01-28 07:03:13 PST
(In reply to comment #25)
> (In reply to comment #24)
> > Could we extend nsIDocument interface to add these methods?
> 
> I think nsIDocument should be a proper place (since it has methods to get XBL
> anonymous content already). Also I think it's enough to have GetAllChildren()
> method only. At least we can live without GetParent() now. Is GetAllChildren()
> fine name?

Sounds okay to me.
Comment 27 David Bolter [:davidb] 2010-01-28 07:41:55 PST
As per IRC, assigning to Alexander as he has started to dig in. Alexander let's dump our plans in this bug so Roc can correct them :)
Comment 28 alexander :surkov 2010-01-28 09:48:02 PST
Created attachment 424023 [details] [diff] [review]
patch
Comment 29 Olli Pettay [:smaug] 2010-01-28 12:21:10 PST
Comment on attachment 424023 [details] [diff] [review]
patch

>+   * Return children of the node, including explict, native anonymous, XBL
>+   * anonymous and genererated nodes.
>+   */
>+  virtual already_AddRefed<nsINodeList>
>+    GetAllChildren(nsIContent* aContent) = 0;
>+
Why you add the method to nsIDocument, and not to nsIContent?
You wouldn't need the aContent parameter if the method was in nsIContent interface.

>+  // Append explicit content if there is neither native nor XBL anonymous
>+  // content.
>+  if (!creator && !bindingList) {
I don't understand this.
An element may have native anonymous children, but yet have also
normal child nodes. For example <textarea> has native anonymous div
but it has normal #text child pretty often.

>+  nsINodeList* returnList = nsnull;
>+  list.forget(&returnList);
>+  return returnList;

I think you could just do
return returnlist.forget();

>+  if (mDisplayContent) {
>+    if (!aElements.AppendElement(mDisplayContent))
>+      return NS_ERROR_OUT_OF_MEMORY;
>+  }
All these would be simpler if you did 
if (mDisplayContent && !aElements.AppendElement(mDisplayContent)) {
  return NS_ERROR_OUT_OF_MEMORY;
}

or maybe even
NS_ENSURE_TRUE(!mDisplayContent || aElements.AppendElement(mDisplayContent),
               NS_ERROR_OUT_OF_MEMORY);

I know some people don't like using NS_ENSURE_XXX macros, but I do :)
But both ways are ok.

>   /**
>+   * Returns "native" anonymous content created by CreateAnonymousContent().
>+   */
>+  virtual nsresult GetAnonymousContent(nsTArray<nsIContent*>& aElements) = 0;
>+
I wonder if this could take nsBaseContentList* aList as a parameter.
The you wouldn't need to copy nodes first to an nsTArray before
adding them to the list.
Comment 30 alexander :surkov 2010-01-28 12:31:49 PST
(In reply to comment #29)
> (From update of attachment 424023 [details] [diff] [review])
> >+   * Return children of the node, including explict, native anonymous, XBL
> >+   * anonymous and genererated nodes.
> >+   */
> >+  virtual already_AddRefed<nsINodeList>
> >+    GetAllChildren(nsIContent* aContent) = 0;
> >+
> Why you add the method to nsIDocument, and not to nsIContent?
> You wouldn't need the aContent parameter if the method was in nsIContent
> interface.

I like this. Thanks.

> >+  // Append explicit content if there is neither native nor XBL anonymous
> >+  // content.
> >+  if (!creator && !bindingList) {
> I don't understand this.
> An element may have native anonymous children, but yet have also
> normal child nodes. For example <textarea> has native anonymous div
> but it has normal #text child pretty often.

Does textarea clones explict children into native anonymous content? Are there another examples? I suspected this case could be but I don't how to expose them in correct order. Where is insertion point in this case?

> >+  nsINodeList* returnList = nsnull;
> >+  list.forget(&returnList);
> >+  return returnList;
> 
> I think you could just do
> return returnlist.forget();

nope, this forget() returns already_AddRefed<nsBaseContentList> which cannot be casted to already_AddRefed<nsINodeList>.

> All these would be simpler if you did 
> if (mDisplayContent && !aElements.AppendElement(mDisplayContent)) {
>   return NS_ERROR_OUT_OF_MEMORY;
> }
> 
> or maybe even
> NS_ENSURE_TRUE(!mDisplayContent || aElements.AppendElement(mDisplayContent),
>                NS_ERROR_OUT_OF_MEMORY);

yes, I'll fix this.

> >   /**
> >+   * Returns "native" anonymous content created by CreateAnonymousContent().
> >+   */
> >+  virtual nsresult GetAnonymousContent(nsTArray<nsIContent*>& aElements) = 0;
> >+
> I wonder if this could take nsBaseContentList* aList as a parameter.
> The you wouldn't need to copy nodes first to an nsTArray before
> adding them to the list.

I'll check. Thanks!
Comment 31 Olli Pettay [:smaug] 2010-01-28 12:41:14 PST
(In reply to comment #30)
> Does textarea clones explict children into native anonymous content?
No. It takes the default value from the explicit child nodes.

> Are there
> another examples? I suspected this case could be but I don't how to expose them
> in correct order.
I think for example <video> may have explicit child nodes. And yet it has
also the native anonymous content for the controls.

And you could add elements under <input> element, even though input element does
have the native anonymous <div> etc.

> Where is insertion point in this case?
There may not be, in a way.
In those cases the native anonymous content is rendered
and explicit content isn't.


> nope, this forget() returns already_AddRefed<nsBaseContentList> which cannot be
> casted to already_AddRefed<nsINodeList>.
Ah, right
Comment 32 :Ehsan Akhgari (busy, don't ask for review please) 2010-01-28 15:10:53 PST
Will nsAccessible::CacheChildren use the GetAllChildren API once this patch lands?
Comment 33 alexander :surkov 2010-01-28 22:17:19 PST
(In reply to comment #32)
> Will nsAccessible::CacheChildren use the GetAllChildren API once this patch
> lands?

Yes, I'm going to fix this in bug 530081 immediately after this one.
Comment 34 alexander :surkov 2010-01-28 22:25:26 PST
(In reply to comment #31)

> > Are there
> > another examples? I suspected this case could be but I don't how to expose them
> > in correct order.
> I think for example <video> may have explicit child nodes. And yet it has
> also the native anonymous content for the controls.

> > Where is insertion point in this case?
> There may not be, in a way.
> In those cases the native anonymous content is rendered
> and explicit content isn't.

We don't care in this case: we'll expose rendered content only and that is what we want. However does video element allow put video control elements into video element so that they will be used instead default control elements? If so this is the case when the element contain both rendered explicit and native anonymous content.

I wonder how does it look when XBL binding is applied to the element having native anonymous content.
Comment 35 Olli Pettay [:smaug] 2010-01-29 02:18:12 PST
(In reply to comment #34)
> I wonder how does it look when XBL binding is applied to the element having
> native anonymous content.
Is that possible at all?
Comment 36 alexander :surkov 2010-01-29 02:52:09 PST
(In reply to comment #35)
> (In reply to comment #34)
> > I wonder how does it look when XBL binding is applied to the element having
> > native anonymous content.
> Is that possible at all?

Yes, that should been my question :) I just tried to apply XBL binding to input@type="file", XBL binding content was ignored.
Comment 37 alexander :surkov 2010-01-29 03:08:11 PST
(In reply to comment #31)

> I think for example <video> may have explicit child nodes. And yet it has
> also the native anonymous content for the controls.

If explicit content is ignored in general when native anonymous content is presented then I would guess explicit video controls should be a children of xul:videocontrols element which is native anonymous node.
Comment 38 alexander :surkov 2010-01-31 06:46:48 PST
(In reply to comment #29)

> >+  // Append explicit content if there is neither native nor XBL anonymous
> >+  // content.
> >+  if (!creator && !bindingList) {
> I don't understand this.
> An element may have native anonymous children, but yet have also
> normal child nodes. For example <textarea> has native anonymous div
> but it has normal #text child pretty often.

Possibly the method should be called GetRealChildren() to not confuse it doesn't return not used (ignored) children?
Comment 39 Olli Pettay [:smaug] 2010-01-31 07:46:38 PST
(In reply to comment #38)
> Possibly the method should be called GetRealChildren() to not confuse it
> doesn't return not used (ignored) children?
Those child nodes aren't ignored. Or are perhaps ignored by layout and a11y, but
not DOM itself.

How does a11y use these nodes? Does it always check that there is the
primary frame before using them? If it does check, returning those
extra nodes shouldn't cause any harm.
Comment 40 alexander :surkov 2010-01-31 07:55:59 PST
(In reply to comment #39)
> (In reply to comment #38)
> > Possibly the method should be called GetRealChildren() to not confuse it
> > doesn't return not used (ignored) children?
> Those child nodes aren't ignored. Or are perhaps ignored by layout and a11y,
> but
> not DOM itself.
> 
> How does a11y use these nodes? Does it always check that there is the
> primary frame before using them? If it does check, returning those
> extra nodes shouldn't cause any harm.

Yes, we always get the primary frame and check if it's not hidden before to create accessible object for the node.

Later we would like to break this rule but it requires a huge changes in a11y module so I'm not sure it will be really soon. At least now it doesn't make sense to expose these nodes since they will be ignored any way.
Comment 41 Olli Pettay [:smaug] 2010-01-31 08:03:33 PST
(In reply to comment #40)
> At least now it doesn't make
> sense to expose these nodes since they will be ignored any way.
Well, then you may need to call the method something quite different.
Perhaps GetVisibleChildren()
That would return the child nodes which have primary frame and which aren't hidden.
Comment 42 alexander :surkov 2010-01-31 08:10:55 PST
(In reply to comment #41)
> (In reply to comment #40)
> > At least now it doesn't make
> > sense to expose these nodes since they will be ignored any way.
> Well, then you may need to call the method something quite different.
> Perhaps GetVisibleChildren()
> That would return the child nodes which have primary frame and which aren't
> hidden.

This is the thing we probably want to keep inside of a11y.

Since GetAnonymousContent() is morphed to take nsBaseContentList* then this will force us to walk these children twice which is not good.
Comment 43 alexander :surkov 2010-02-01 05:15:38 PST
(In reply to comment #31)

> > Where is insertion point in this case?
> There may not be, in a way.
> In those cases the native anonymous content is rendered
> and explicit content isn't.

Ok. The element can have visible native anonymous content and explicit content both, for example, scrollable elements: they have scrollbar in native anonymous content. So, where is insertion point of native anonymous content? Are their frames appended after frames for explicit content?
Comment 44 alexander :surkov 2010-02-01 05:22:04 PST
Created attachment 424578 [details] [diff] [review]
patch2

comment #43 should be answered before the patch review.
Comment 45 alexander :surkov 2010-02-01 20:39:34 PST
Created attachment 424720 [details] [diff] [review]
patch3

1) rerequesting review from Olli since changes are big
2) native anon content is appended to the end, at least it breaks nothing, but I hope to get some details about native anon insertion point.
Comment 46 alexander :surkov 2010-02-02 04:15:44 PST
The patch makes xpcshell tests fail. I suspected nsBindingManger changes and therefore I rollback the changes of original GetContentListFor(). But I xpcshell fails in any way (http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1265104809.1265111195.10207.gz#err3). I really don't get how it might be related. The first failure is 

NEXT ERROR TEST-UNEXPECTED-FAIL | e:/builds/slave/win32-unit/mozilla/objdir/_tests/xpcshell/test_zipwriter/unit/test_asyncadd.js | 1265106791000 == 1265106791357 - See following stack:
JS frame :: e:\builds\slave\win32-unit\mozilla\testing\xpcshell\head.js :: do_throw :: line 202
JS frame :: e:\builds\slave\win32-unit\mozilla\testing\xpcshell\head.js :: do_check_eq :: line 232
JS frame :: e:/builds/slave/win32-unit/mozilla/objdir/_tests/xpcshell/test_zipwriter/unit/test_asyncadd.js :: anonymous :: line 81
TEST-INFO | (xpcshell/head.js) | exiting test
uncaught exception: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: e:/builds/slave/win32-unit/mozilla/objdir/_tests/xpcshell/test_zipwriter/unit/tail_zipwriter.js :: <TOP_LEVEL> :: line 47"  data: no]

This failure doesn't seem to be related with the changed code. But since other builds in try tinderbox are green then something wrong with my patch.

Do you have any idea what could be wrong?
Comment 47 David Bolter [:davidb] 2010-02-02 05:38:20 PST
Benjamin, any thoughts on what might trigger the failure in comment #46?

(In reply to comment #32)
> Will nsAccessible::CacheChildren use the GetAllChildren API once this patch
> lands?

Ehsan, do you have concerns about this? We won't be hanging on to frame pointers.
Comment 48 :Ehsan Akhgari (busy, don't ask for review please) 2010-02-02 07:27:44 PST
(In reply to comment #46)
> NEXT ERROR TEST-UNEXPECTED-FAIL |
> e:/builds/slave/win32-unit/mozilla/objdir/_tests/xpcshell/test_zipwriter/unit/test_asyncadd.js

I have seen these failures on the try server quite frequently, and they definitely look random (and they only happen on Windows boxes for me.)  I have landed patches which failed like this on m-c, and there is no such failure there.

I don't know what the process of tracking the random oranges on try is, but I just filed bug 543740 to track this.  For now, you can ignore this failure, I think.
Comment 49 :Ehsan Akhgari (busy, don't ask for review please) 2010-02-02 07:30:40 PST
(In reply to comment #47)
> (In reply to comment #32)
> > Will nsAccessible::CacheChildren use the GetAllChildren API once this patch
> > lands?
> 
> Ehsan, do you have concerns about this? We won't be hanging on to frame
> pointers.

With bug 542824 landed, I don't have any more concerns about this.

I assume you meant native anonymous content pointers.  If that is right, why not grab the value using the content API?  FWIW, when bug 534785 lands, this will be even simpler.
Comment 50 alexander :surkov 2010-02-02 09:15:40 PST
(In reply to comment #48)
> (In reply to comment #46)
> > NEXT ERROR TEST-UNEXPECTED-FAIL |
> > e:/builds/slave/win32-unit/mozilla/objdir/_tests/xpcshell/test_zipwriter/unit/test_asyncadd.js
> 
> I have seen these failures on the try server quite frequently, and they
> definitely look random (and they only happen on Windows boxes for me.)  I have
> landed patches which failed like this on m-c, and there is no such failure
> there.

There is a lot of failures - http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1265104809.1265111195.10207.gz#err3). I tried the try server build few times and these failures were persistent. I might be just unlucky guy but failures bother me.
Comment 51 :Ehsan Akhgari (busy, don't ask for review please) 2010-02-02 11:28:40 PST
(In reply to comment #50)
> (In reply to comment #48)
> > (In reply to comment #46)
> > > NEXT ERROR TEST-UNEXPECTED-FAIL |
> > > e:/builds/slave/win32-unit/mozilla/objdir/_tests/xpcshell/test_zipwriter/unit/test_asyncadd.js
> > 
> > I have seen these failures on the try server quite frequently, and they
> > definitely look random (and they only happen on Windows boxes for me.)  I have
> > landed patches which failed like this on m-c, and there is no such failure
> > there.
> 
> There is a lot of failures -
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1265104809.1265111195.10207.gz#err3).
> I tried the try server build few times and these failures were persistent. I
> might be just unlucky guy but failures bother me.

My comment only covered the test_asyncadd.js and test_sync.js failures.  The rest probably need investigation.  :-)
Comment 52 Johnny Stenback (:jst, jst@mozilla.com) 2010-02-03 17:00:07 PST
Moving this to the accessibility component for them to triage whether or not this needs to block 1.9.3
Comment 53 David Bolter [:davidb] 2010-02-03 19:08:54 PST
I'd say block. This is integral to some or our a11y stability work. Alex, Marco?
Comment 54 Marco Zehe (:MarcoZ) 2010-02-03 23:02:51 PST
I agree, block!
Comment 55 alexander :surkov 2010-02-04 09:42:53 PST
Created attachment 425226 [details] [diff] [review]
patch4

Ok, the reason was is I changed the CID of nsIContent. Possibly try server build doesn't make full rebuilt and this cause the failure. David said that Enn said that CID shouldn't be ever changed at all. So it's possibly ok I file the patch with unchanged CID. Now tests pass.
Comment 56 alexander :surkov 2010-02-05 01:30:56 PST
(In reply to comment #53)
> I'd say block. This is integral to some or our a11y stability work. Alex,
> Marco?

I'm sure we want to land it on 1.9.3 but I hope we'll be able to get all necessary reviews before 1.9.3 is branched.
Comment 57 Olli Pettay [:smaug] 2010-02-05 01:53:29 PST
Comment on attachment 425226 [details] [diff] [review]
patch4

>diff --git a/content/base/public/nsIContent.h b/content/base/public/nsIContent.h
>--- a/content/base/public/nsIContent.h
>+++ b/content/base/public/nsIContent.h
>@@ -146,16 +146,42 @@ public:
>    * DEPRECATED - Use GetCurrentDoc or GetOwnerDoc.
>    * Get the document for this content.
>    * @return the document
>    */
>   nsIDocument *GetDocument() const
>   {
>     return GetCurrentDoc();
>   }
>+
>+  enum {
>+    /**
>+     * All children will be return.
>+     *
>+     * @note the result children order is
>+     *   1. :before generated contnent
>+     *   2.1. XBL anonymous content together with altered explicit children
>+     *   2.2. otherwise explicit children
>+     *   3. native anonymous content
>+     *   3. :after generated content
>+     */
>+    eAllChildren = 0,
>+
>+    /**
>+     * All children but XBL anonymous content will be return.
>+     */
>+    eAllButXBL = 1
>+  };
>+
>+  /**
>+   * Return children of the node, including explict, native anonymous, XBL
>+   * anonymous and genererated nodes until the child type is given (see above).
>+   */
>+  virtual already_AddRefed<nsINodeList>
>+    GetAllChildren(PRInt32 aChildType = eAllChildren) = 0;
If this may in some cases not all the children, call the method something else.
GetChildren(PRInt32 aChildType) ?

Btw what is the use case for eAllButXBL? I guess a11y needs it, but I just don't
know for what.
Might be better to not have the default parameter, so that the caller
much think whether all the children are needed.



>+already_AddRefed<nsINodeList>
>+nsGenericElement::GetAllChildren(PRInt32 aChildType)
>+{
>+  if (!IsInDoc())
>+    return nsnull;
Why this? If you really want this behavior, then
the method name should be again something else:
GetChildrenIfInDocument()... that would be strange.


>+
>+  nsIFrame *frame = GetPrimaryFrame();
>+  if (!frame)
>+    return nsnull;
Maybe this was in the previous patch too, but actually this is strange too.
The method description says that all the child nodes should be returned.
It doesn't say anything about that there is the requirement for a layout object.
And either you do all the primaryFrame checks in a11y or you do them 
all in this method (in which case the method name should be something else).
In the earlier comment you said that a11y does the primary frame checks.


>+  // If XBL is bound to this node then append XBL anonymous content including
>+  // explict content altered by insertion point if we were requested for XBL
>+  // anonymous content, otherwise append explicit content with respect to
>+  // insertion point if any.
>+  nsINodeList *childList = nsnull;
>+  if (aChildType != eAllButXBL) {
>+    childList = GetOwnerDoc()->BindingManager()->GetXBLChildNodesFor(this);
Is it guaranteed that this method can't be called after the document is destroyed? Might be better to add a null check for GetOwnerDoc()
Comment 58 alexander :surkov 2010-02-05 03:26:55 PST
(In reply to comment #57)

> >+  virtual already_AddRefed<nsINodeList>
> >+    GetAllChildren(PRInt32 aChildType = eAllChildren) = 0;
> If this may in some cases not all the children, call the method something else.
> GetChildren(PRInt32 aChildType) ?

fine with me

> 
> Btw what is the use case for eAllButXBL? I guess a11y needs it, but I just
> don't
> know for what.

yes, a11y needs, some bindings deny accessibles for anonymous nodes.

> Might be better to not have the default parameter, so that the caller
> much think whether all the children are needed.

fine with me.

> 
> 
> >+already_AddRefed<nsINodeList>
> >+nsGenericElement::GetAllChildren(PRInt32 aChildType)
> >+{
> >+  if (!IsInDoc())
> >+    return nsnull;
> Why this? If you really want this behavior, then
> the method name should be again something else:
> GetChildrenIfInDocument()... that would be strange.

I think I can remove it.

> >+
> >+  nsIFrame *frame = GetPrimaryFrame();
> >+  if (!frame)
> >+    return nsnull;
> Maybe this was in the previous patch too, but actually this is strange too.
> The method description says that all the child nodes should be returned.
> It doesn't say anything about that there is the requirement for a layout
> object.
> And either you do all the primaryFrame checks in a11y or you do them 
> all in this method (in which case the method name should be something else).
> In the earlier comment you said that a11y does the primary frame checks.

this too

> >+  nsINodeList *childList = nsnull;
> >+  if (aChildType != eAllButXBL) {
> >+    childList = GetOwnerDoc()->BindingManager()->GetXBLChildNodesFor(this);
> Is it guaranteed that this method can't be called after the document is
> destroyed? Might be better to add a null check for GetOwnerDoc()

it was guaranteed by IsInDoc() check, I'll add GetOwnderDoc check here since I remove IsInDoc() check.
Comment 59 alexander :surkov 2010-02-05 12:08:12 PST
Created attachment 425520 [details] [diff] [review]
patch5

Olli's comments are addressed.
Comment 60 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-02-07 14:46:54 PST
> +     * All children will be return.

All children will be returned.

+     *   2.1. XBL anonymous content together with altered explicit children

What does "altered explicit children" mean? I think I'd just say

2. Explicit children and XBL anonymous children

+     *   3. :after generated content

This should be 4.

+     * All children but XBL anonymous content will be return.

  * All children but XBL anonymous children will be returned.

+      if (beforeFrame)
+        list->AppendElement(beforeContent);

{} around the if body

+      if (!childList)
+        childList = GetChildNodesList();
+

{} here

+    if (creator)
+      creator->GetAnonymousContent(list);

{} here

+      if (afterFrame)
+        list->AppendElement(afterContent);

{} here

+  if (mDisplayContent)
+    aElements->AppendElement(mDisplayContent);
+
+  if (mButtonContent)
+    aElements->AppendElement(mButtonContent);

{} here

+  if (mTextContent)
+    aElements->AppendElement(mTextContent);
+
+  if (mBrowse)
+    aElements->AppendElement(mBrowse);

and here

+  if (mTextContent)
+    aElements->AppendElement(mTextContent);
+
+  if (mBrowse)
+    aElements->AppendElement(mBrowse);

and here

+  if (mTextContent)
+    aElements->AppendElement(mTextContent);
+
+  if (mInputContent)
+    aElements->AppendElement(mInputContent);

and here

+  if (mAnonymousDiv)
+    aElements->AppendElement(mAnonymousDiv);

and here

+  if (mHScrollbarContent)
+    aElements->AppendElement(mHScrollbarContent);
+
+  if (mVScrollbarContent)
+    aElements->AppendElement(mVScrollbarContent);
+
+  if (mScrollCornerContent)
+    aElements->AppendElement(mScrollCornerContent);

and here

+  if (mPosterImage)
+    aElements->AppendElement(mPosterImage);
+
+  if (mVideoControls)
+    aElements->AppendElement(mVideoControls);

and here

... would it make sense to have nsBaseContentList::AppendElement just accept 'null' parameters and skip them?

+  if (clone)
+    aElements->AppendElement(clone);

{} here

+  if (mPopupgroupContent)
+    aElements->AppendElement(mPopupgroupContent);
+
+  if (mTooltipContent)
+    aElements->AppendElement(mTooltipContent);

{} here

What is eAllButXBL for?
Comment 61 alexander :surkov 2010-02-08 02:18:23 PST
(In reply to comment #60)
> > +     * All children will be return.
> 
> All children will be returned.
> 
> +     *   2.1. XBL anonymous content together with altered explicit children
> 
> What does "altered explicit children" mean? I think I'd just say
> 
> 2. Explicit children and XBL anonymous children

This means explicit children whose order is altered by insertion point. I'm fine with your wording. I'll change.

> 
> What is eAllButXBL for?

(from comment #58)
> 
> Btw what is the use case for eAllButXBL? I guess a11y needs it, but I just
> don't
> know for what.

yes, a11y needs, some bindings deny accessibles for anonymous nodes.
Comment 62 alexander :surkov 2010-02-09 07:51:20 PST
(In reply to comment #60)

> ... would it make sense to have nsBaseContentList::AppendElement just accept
> 'null' parameters and skip them?

I'm not sure but it would hide errors of some potential wrong AppendElement() usage.
Comment 63 alexander :surkov 2010-02-09 09:17:44 PST
Created attachment 426005 [details] [diff] [review]
patch6
Comment 64 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-02-09 11:57:45 PST
(In reply to comment #62)
> I'm not sure but it would hide errors of some potential wrong AppendElement()
> usage.

I think we should either make AppendElement ignore null, or create a helper function in nsLayoutUtils that calls AppendElement but skips nulls.
Comment 65 alexander :surkov 2010-02-09 12:09:52 PST
(In reply to comment #64)
> (In reply to comment #62)
> > I'm not sure but it would hide errors of some potential wrong AppendElement()
> > usage.
> 
> I think we should either make AppendElement ignore null, or create a helper
> function in nsLayoutUtils that calls AppendElement but skips nulls.

I'll go with any way you like.
Comment 66 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-02-09 12:23:37 PST
I suggest adding nsContentUtils::AppendAnonymousContent(nsContentList*, nsIContent*) near nsContentUtils::DestroyAnonymousContent.
Comment 67 alexander :surkov 2010-02-09 12:32:58 PST
(In reply to comment #66)
> I suggest adding nsContentUtils::AppendAnonymousContent(nsContentList*,
> nsIContent*) near nsContentUtils::DestroyAnonymousContent.

nsBaseContentList I assume?
Comment 68 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-02-09 13:06:58 PST
right
Comment 69 alexander :surkov 2010-02-10 01:20:14 PST
Created attachment 426202 [details] [diff] [review]
patch7
Comment 70 alexander :surkov 2010-02-11 05:30:27 PST
Olli, does the patch look fine?
Comment 71 Olli Pettay [:smaug] 2010-02-11 06:06:59 PST
Comment on attachment 426202 [details] [diff] [review]
patch7

>   /**
>+   * Appends anonymous content node to the content list.
>+   */
>+  static void AppendAnonymousContent(nsBaseContentList* aContentList,
>+                                     nsIContent* aContent)
>+  {
>+    if (aContent)
>+      aContentList->AppendElement(aContent);
>+  }
Could you rather add 
MaybeAppendElement method to nsBaseContentList and document that it
null checks.

nsContentUtils::AppendAnonymousContent doesn't have anything to do with
anonymous content. It just has the null check.


>+++ b/layout/generic/nsIAnonymousContentCreator.h
>@@ -67,16 +67,21 @@ public:
...
>+   * Returns "native" anonymous content created by CreateAnonymousContent().
>+   */
>+  virtual void GetAnonymousContent(nsBaseContentList* aElements) = 0;
Could this take nsBaseContentList& aElements as a parameter. That would
indicate that it is never null.
Then it would be safe to call in the implementations
aElements.MaybeAppendElement(mSome_native_anon_element);

With those, r=me.

I hope roc doesn't object my suggestions ;)
Comment 72 alexander :surkov 2010-02-11 06:13:55 PST
I'm fine with this, I hope too :)
Comment 73 alexander :surkov 2010-02-11 08:54:53 PST
Created attachment 426512 [details] [diff] [review]
patch8

Putting the patch if my reviewers would like to take a look before I land it.
Comment 74 alexander :surkov 2010-02-11 09:22:21 PST
Last patch change line ending type to Unix (LF) in nsFileControlControlFrame.cpp, I hope it's ok (my xcode editor change this automatically).
Comment 75 David Bolter [:davidb] 2010-02-11 09:39:48 PST
Alexander can you change the bug summary to better reflect the changes?
Comment 76 alexander :surkov 2010-02-11 09:40:39 PST
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/a0d63d34b3ae

Thank you for reviews!
Comment 77 alexander :surkov 2010-02-11 09:41:41 PST
(In reply to comment #75)
> Alexander can you change the bug summary to better reflect the changes?

"Accessibility needs new DOM traversal API"?
Comment 78 Joe Drew (not getting mail) 2010-02-11 14:07:47 PST
It looks very much like this checkin caused a 50% Txul regression; is that possible? Alexander, can you look into it?
Comment 79 alexander :surkov 2010-02-11 14:23:28 PST
I shouldn't be this bug because the patch changes nothing, it introduce nsIContent::GetChildren and nsIAnonymousContentCreator::GetAnonymousContent methods but these methods aren't used yet.

It has only one change that could affect on the tree, this is nsBindingManager code changes, but I believe the changed code is equivalent to existing one.
Comment 80 Boris Zbarsky [:bz] 2010-02-11 17:47:45 PST
So I have a number of issues with this patch:

1)  The child type enum and API documentation for GetChildren is completely
    opaque as to what it means by "children".  When is it DOM children?  When is
    it XBL flattened tree children?  When is it something else?

    The behavior implemented in the patch seems to be that if aChildType is
    eAllChildren then the children are the XBL flattened tree children (plus
    the native anonymous stuff) and if aChildType is eAllButXBL then the
    children are the children of the node after processing all bindings applied
    to the node's ancestors, but before processing whatever bindings might be
    applied to the node itself (plus the anonymous content stuff).  This is not
    exactly what the documentation says.
2)  In the face of DOM mutations, I suspect the eAllButXBL case will return
    bogus results; we don't maintain those child lists very well.  Not much you
    can do about that, though.
3)  I'm not sure what "until the child type is given" means.
4)  The documentation needs to make it very clear that mixing eAllButXBL and
    eAllChildren in the same traversal can easily end up with walking the same
    node multiple times.  I'm not sure whether this is desirable.  What _is_
    the eAllButXBL use case anyway?
5)  There are redundant null-checks of beforeFrame and afterFrame in
    nsGenericElement::GetChildren.  Why are they there?
6)  The new nsIAnonymousContentCreator.h doesn't document (ideally by its name)
    that the anonymous content should be _appended_ to aElements as opposed to
    replacing what is currently in aElements.
Comment 81 alexander :surkov 2010-02-11 22:19:00 PST
Thanks, Boris, I'll try to clean up the stuffs and address your comments.
Comment 82 alexander :surkov 2010-02-12 07:58:33 PST
(In reply to comment #80)

> 6)  The new nsIAnonymousContentCreator.h doesn't document (ideally by its name)
>     that the anonymous content should be _appended_ to aElements as opposed to
>     replacing what is currently in aElements.

Is AppendAnonymousContentTo() good?
Comment 83 Boris Zbarsky [:bz] 2010-02-12 08:43:55 PST
Yes.  And document that it only appends existing content without creating new content?
Comment 84 alexander :surkov 2010-02-12 08:54:35 PST
Sure.

Is

for (PRUInt32 idx = 0; idx < length; idx)
  nsIContent* child = GetChildAt(i);

quicker than

list = nsIContent::GetChildNodesList();
for (PRUint32 idx = 0; idx < length; idx++)
  nsIContent* child = list->GetNodeAt(idx);

assuming GetChildNodesList is called the first time for the given node? Or does GetChildNodesList() is computed lazy?
Comment 85 Boris Zbarsky [:bz] 2010-02-12 09:11:36 PST
GetChildAt is faster the first time around, since it doesn't have to create a separate object.  If you want _really_ fast, you should use the GetChildArray method.  I really doubt it matters which of those you use in practice, though.
Comment 86 alexander :surkov 2010-02-12 10:36:07 PST
(In reply to comment #80)

> 4)  The documentation needs to make it very clear that mixing eAllButXBL and
>     eAllChildren in the same traversal can easily end up with walking the same
>     node multiple times.  I'm not sure whether this is desirable.

Could you describe how it can happen please?

>  What _is_
>     the eAllButXBL use case anyway?

Sometimes accessibility wants to get explicit children only and it doesn't want XBL anonymous content. For example, it happens for xul:menu where we want to expose accessible object for explicit xul:menupopup but we don't want to expose accessible objects for anonymous xul:label.
Comment 87 alexander :surkov 2010-02-12 10:42:21 PST
Created attachment 426701 [details] [diff] [review]
bz patch

Boris, is it better?
Comment 88 Boris Zbarsky [:bz] 2010-02-12 10:47:29 PST
> Could you describe how it can happen please?

Say you have a node N with some kids.  N has a binding attached which looks like this:

  <xbl:content>
    <html:div><children/></html:div>
  </xbl:content>

Now if you ask for the kids of N with eAllButXBL and then later ask for the kids of that <html:div> with either flag, you'll see the same nodes twice.

> Sometimes accessibility wants to get explicit children only

Explicit children?  Or XBL-resolved children ignoring bindings on the node itself?  And in those cases do you still want native anonymous content and :before/:after?
Comment 89 Boris Zbarsky [:bz] 2010-02-12 10:49:21 PST
I'll look over the comments in that last patch once I'm sure I understand what the API goal is.  :before/:after should never have null content.
Comment 90 alexander :surkov 2010-02-12 11:01:52 PST
(In reply to comment #88)
> > Could you describe how it can happen please?
> 
> Say you have a node N with some kids.  N has a binding attached which looks
> like this:
> 
>   <xbl:content>
>     <html:div><children/></html:div>
>   </xbl:content>
> 
> Now if you ask for the kids of N with eAllButXBL and then later ask for the
> kids of that <html:div> with either flag, you'll see the same nodes twice.

I see, thanks.

> > Sometimes accessibility wants to get explicit children only
> 
> Explicit children?  Or XBL-resolved children ignoring bindings on the node
> itself?  

Sorry, I was unclear. We want to see everything but XBL anonymous content.

> And in those cases do you still want native anonymous content and
> :before/:after?

yes.
Comment 91 alexander :surkov 2010-02-12 11:07:03 PST
Created attachment 426707 [details] [diff] [review]
bz patch2
Comment 92 alexander :surkov 2010-02-12 11:12:43 PST
(In reply to comment #89)
> I'll look over the comments in that last patch once I'm sure I understand what
> the API goal is.

We want to make simpler DOM traversal in accessibility. We would like to have common way to navigate through all kinds of content.

  :before/:after should never have null content.

I'll remove null-check
Comment 93 Boris Zbarsky [:bz] 2010-02-12 12:04:29 PST
> +     * All children including explicit DOM children and anonymous children.

This is not quite true.  For example, there are "explicit DOM children" which are NOT returned by this function.

What this should probably say is more like:

  * All XBL flattened tree children of the node, as well as :before and :after
  * anonymous content and native anonymous children.

>+     *   2. child nodes as they appear under XBL bound element, including
>+     *     explicit DOM nodes altered by XBL insertion point and XBL anonymous
>+     *     nodes; if there is no XBL binding then explicit DOM child nodes

That's not what this method returns.  This item should just say "XBL flattened tree children of this node".

>+     * All children including explicit DOM children and anonymous but XBL
>+     * anonymous children.

This should say:

  * All XBL explicit children of the node, as well as :before and :after
  * anonymous content and native anonymous children.

If desired, point to the definition of "explicit children" at http://www.w3.org/TR/xbl/#explicit3

>+     *   2. explicit DOM child nodes altered by XBL insertion point; if there is
>+     *     no XBL binding then explicit DOM child nodes

This should just say "XBL explicit children of the node".

>+   * Return children of the node, including explict DOM children, native
>+   * anonymous, XBL anonymous or genererated chidren depending on the given
>+   * child type is given (see above).

"Return either the XBL explicit children of the node or the XBL flattened tree children of the node, depending on the child type, as well as any native anonymous children."

>+   * @note be aware to call this method with different child types during
>+   *   the children traversal to avoid the same nodes getting twice

Perhaps better:

  @note calling this method with eAllButXBL will return children that are also
        in the eAllButXBL and eAllChildren child lists of other descendants of
        this node in the tree, but those other nodes cannot be reached from the
        eAllButXBL child list.
Comment 94 alexander :surkov 2010-02-15 02:16:21 PST
Created attachment 426958 [details] [diff] [review]
bz patch3
Comment 95 Boris Zbarsky [:bz] 2010-02-16 12:12:41 PST
Comment on attachment 426958 [details] [diff] [review]
bz patch3

Looks good.  Thanks!
Comment 96 alexander :surkov 2010-02-20 17:00:02 PST
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/704b52b7a02c

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