Closed Bug 1247364 Opened 4 years ago Closed 4 years ago

add AllChildrenIterator::Seek

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: surkov, Unassigned)

Details

(Whiteboard: dom-triaged)

Attachments

(3 files, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
I'm not sure whether it's better to virtualize GetNextChild. Also not sure whether it's good to ensure the iterator is at the start before seeking the node or maybe we just should assert if it's not.
Attachment #8718019 - Flags: review?(bzbarsky)
Attached patch a11y partSplinter Review
Attachment #8718020 - Flags: review?(dbolter)
Comment on attachment 8718020 [details] [diff] [review]
a11y part

Review of attachment 8718020 [details] [diff] [review]:
-----------------------------------------------------------------

If the DOM part passes this part seems fine to me.
Attachment #8718020 - Flags: review?(dbolter) → review+
Whiteboard: dom-triaged
Comment on attachment 8718019 [details] [diff] [review]
patch

So...  ExplicitChildIterator::Seek seeks from the current position to the child.  If not found it puts the iterator in the "iterated all the kids" state.

What this new Seek method will do, as far as I can tell, is the following, starting with an iterator at the beginning:

1)  If aChildToFind is the before content, it will set the ExplicitChildIterator state all the way to the end, then find the before kid.  Then stepping it will no longer see any of the explicit kids and go from before kid directly to anon kids.

2)  If passed anything else it will behave reasonably, I think.

This seems like a pretty weird API.  Unless you're only meant to pass certain kinds of nodes in to it, in which case it should at least be documented?
Attachment #8718019 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky [:bz] from comment #3)

> 1)  If aChildToFind is the before content, it will set the
> ExplicitChildIterator state all the way to the end, then find the before
> kid.  Then stepping it will no longer see any of the explicit kids and go
> from before kid directly to anon kids.

right, I missed it.

> 2)  If passed anything else it will behave reasonably, I think.
> 
> This seems like a pretty weird API.

the most confusing thing about this API for me is it's looking from current pos, but that's probably ok.
Attached patch content partSplinter Review
does it look better?
Attachment #8718019 - Attachment is obsolete: true
Attachment #8718218 - Flags: review?(bzbarsky)
Comment on attachment 8718218 [details] [diff] [review]
content part

Review of attachment 8718218 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/ChildIterator.cpp
@@ +326,5 @@
> +    }
> +  }
> +
> +  if (ExplicitChildIterator::Seek(aChildToFind)) {
> +    mPhase = eNeedExplicitKids;

I guess I should move this part to if () statement above, in case if Seek() called twice for a before node.
Comment on attachment 8718218 [details] [diff] [review]
content part

>+AllChildrenIterator::Seek(nsIContent* aChildToFind)
>+{
>+  if (mPhase == eNeedBeforeKid) {

Then need to immediately set mPhase to eNeedExplicitKids, no?  Otherwise the next GetNextChild() call will find the before kid again.

>+  if (ExplicitChildIterator::Seek(aChildToFind)) {

If this tests false, need to set mPhase to eNeedAnonKids, no?

r=me with those two fixed, I think, but please double-check the logic _very_ carefully...
Attachment #8718218 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #7)
> Comment on attachment 8718218 [details] [diff] [review]
> content part
> 
> >+AllChildrenIterator::Seek(nsIContent* aChildToFind)
> >+{
> >+  if (mPhase == eNeedBeforeKid) {
> 
> Then need to immediately set mPhase to eNeedExplicitKids, no?  Otherwise the
> next GetNextChild() call will find the before kid again.

yes, I noticed right after I filed the patch

> >+  if (ExplicitChildIterator::Seek(aChildToFind)) 

> If this tests false, need to set mPhase to eNeedAnonKids, no?

GetNextChild() should handle that, but agree, having mPhase set to eNeedAnonKids gets rid of some unnecessary call. I'll make a change of course.

> r=me with those two fixed, I think, but please double-check the logic _very_
> carefully...

right. What's good is that I'm going to use this method more, so a11y test suite should show problems if something is unnoticed.
Attached patch content part2Splinter Review
this one would probably more correct
https://hg.mozilla.org/mozilla-central/rev/2fd81451b842
https://hg.mozilla.org/mozilla-central/rev/d158f3f5ebca
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.