If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

fixup copy and move ctors for children iterators

RESOLVED FIXED in mozilla34

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla34
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8468621 [details] [diff] [review]
fixup copy and move ctors for children iterators
Attachment #8468621 - Flags: review?(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.
Attachment #8468621 - Flags: review?(wchen) → review+
(Assignee)

Comment 3

3 years ago
> 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?
Yeah, it seems OK.
(Assignee)

Comment 5

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/59f193167901
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
Flags: needinfo?(trev.saunders)
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'
https://hg.mozilla.org/mozilla-central/rev/d046865456ff
Assignee: nobody → trev.saunders
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(Assignee)

Updated

3 years ago
Flags: needinfo?(trev.saunders)
You need to log in before you can comment on or make changes to this bug.