1049758 - fixup copy and move ctors for children iterators

Status: RESOLVED FIXED

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

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: fixup copy and move ctors for children iterators

Comment 2

[William Chen [:wchen]]

Comment on attachment 8468621 [details] [diff] [review]
fixup copy and move ctors for children iterators

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

nit: The local style in this file is for the members in the initialization list to line up with the other members, not with the colon.

r+ ignoring AllChildrenIterator because it's not in the tree.

::: content/base/src/ChildIterator.h
@@ +131,5 @@
>  
> +  explicit FlattenedChildIterator(FlattenedChildIterator&& aOther)
> +    : ExplicitChildIterator(Move(aOther)), mXBLInvolved(aOther.mXBLInvolved) {}
> +
> +  FlattenedChildIterator(const FlattenedChildIterator& aOther)

According to the style guide, it's not necessary to mark copy and move constructors as explicit. I'm not sure if we gain anything here, but if you are going to do it, this constructor should also be marked explicit (unless we can't for some other reason). I would prefer if none of these constructors were marked as explicit.

@@ +168,5 @@
>      FlattenedChildIterator(aNode, (aFlags & nsIContent::eAllButXBL)),
>      mOriginalContent(aNode), mFlags(aFlags),
>      mPhase(eNeedBeforeKid) {}
>  
> +  explicit AllChildrenIterator(AllChildrenIterator&& aOther)

This looks new.

Comment 3

[Trevor Saunders (:tbsaunde)]

> r+ ignoring AllChildrenIterator because it's not in the tree.

it's in the tree now I landed the patch adding it a couple days ago.

> ::: content/base/src/ChildIterator.h
> @@ +131,5 @@
> >  
> > +  explicit FlattenedChildIterator(FlattenedChildIterator&& aOther)
> > +    : ExplicitChildIterator(Move(aOther)), mXBLInvolved(aOther.mXBLInvolved) {}
> > +
> > +  FlattenedChildIterator(const FlattenedChildIterator& aOther)
> 
> According to the style guide, it's not necessary to mark copy and move
> constructors as explicit. I'm not sure if we gain anything here, but if you
> are going to do it, this constructor should also be marked explicit (unless
> we can't for some other reason). I would prefer if none of these
> constructors were marked as explicit.

I'll just remove all the explicits, I think it may be useful, but I forget too.

> @@ +168,5 @@
> >      FlattenedChildIterator(aNode, (aFlags & nsIContent::eAllButXBL)),
> >      mOriginalContent(aNode), mFlags(aFlags),
> >      mPhase(eNeedBeforeKid) {}
> >  
> > +  explicit AllChildrenIterator(AllChildrenIterator&& aOther)
> 
> This looks new.

true, does it seem ok?

Comment 4

[William Chen [:wchen]]

Yeah, it seems OK.

Comment 5

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59f193167901

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for build bustage everywhere: https://hg.mozilla.org/integration/mozilla-inbound/rev/156f1bee4139

https://tbpl.mozilla.org/php/getParsedLog.php?id=46713847&tree=Mozilla-Inbound

Comment 7

[David Baron :dbaron:]

Looks like the build bustage was on all non-DEBUG builds:

../../dist/include/mozilla/dom/ChildIterator.h:179:30: error: class 'mozilla::dom::AllChildrenIterator' does not have any field named 'mMutationGuard'
../../dist/include/mozilla/dom/ChildIterator.h:179:52: error: 'class mozilla::dom::AllChildrenIterator' has no member named 'mMutationGuard'

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d046865456ff