1227927 - Remove nsIFrame::GetFirstPrincipalChild()

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Per bug 1226875 comment 5, mats suggests removing GetFirstPrincipalChild() as well.

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Part 1 - Make nsIFrame::PrincipalChildList a const function. r=bz

Since nsIFrame::GetChildList() is a const function, PrincipalChildList()
should be a const function as well.

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Part 2 - Remove nsIFrame::GetFirstPrincipalChild(). r=mats

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Part 3 - Use ranged-based for-loop to rewrite some simple loops in part 2. r=mats

Comment 4

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8712139 [details] [diff] [review]
Part 1 - Make nsIFrame::PrincipalChildList a const function. r=bz

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

This patch was reviewed by bz in Bug 591737. Since it's needed by this bug as well, I carry bz's r+ here.

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8712141 [details] [diff] [review]
Part 3 - Use ranged-based for-loop to rewrite some simple loops in part 2. r=mats

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

::: layout/printing/nsPrintEngine.cpp
@@ +2237,5 @@
>      if (po->mPresContext && po->mPresContext->IsRootPaginatedDocument()) {
>        nsIPageSequenceFrame* pageSequence = po->mPresShell->GetPageSequenceFrame();
>        nsIFrame * seqFrame = do_QueryFrame(pageSequence);
>        if (seqFrame) {
> +        aNumPages += seqFrame->PrincipalChildList().GetLength();

This is the only place where the loop can be totally eliminated.

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e46f4ace766

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 8712140 [details] [diff] [review]
Part 2 - Remove nsIFrame::GetFirstPrincipalChild(). r=mats

r=mats

>layout/base/nsCSSFrameConstructor.cpp
>@@ -10718,17 +10718,17 @@ nsCSSFrameConstructor::InsertFirstLineFrames(
>           }
> 
>           nsLineFrame* nextLineFrame = (nsLineFrame*) lineFrame;
>           for (;;) {
>             nextLineFrame = nextLineFrame->GetNextInFlow();
>             if (!nextLineFrame) {
>               break;
>             }
>-            nsIFrame* kids = nextLineFrame->GetFirstPrincipalChild();
>+            nsIFrame* kids = nextLineFrame->PrincipalChildList().FirstChild();
>           }
>         }

This whole chunk of code seems pointless.  After a quick look
at the surrounding code I can't tell if we can just remove it
or if this loop actually *should* do something but that code
is missing.

Please file a follow-up bug on this and add a XXX comment here
with the bug number, something like:
// XXX bug NNNNN is this loop supposed to do something or what?


>layout/tables/nsCellMap.cpp

FYI, you will likely get merge conflicts here.
Sorry about that.

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 8712141 [details] [diff] [review]
Part 3 - Use ranged-based for-loop to rewrite some simple loops in part 2. r=mats

r=mats, thanks for cleaning this up!

Comment 9

[Mats Palmgren (inactive)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #5)
> This is the only place where the loop can be totally eliminated.

I think nsFrameList::GetLength() was DEBUG-only at some point because
it's O(n) and we didn't want people to use by mistake if they were
unaware of that.  (I think it's ok to use it here though instead of
of the loop.)

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Re comment 7: I found that nsCSSFrameConstructor::InsertFirstLineFrames() is actually no-op. Filed bug 1243635 for investigation.

Re comment 9: Should be better add a comment to nsFrameList::GetLength() to warn that it's O(n). I'll add a patch for this.

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Part 4 - Add comment to nsFrameList::GetLength() to warn it's O(n). r=mats

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 8713025 [details] [diff] [review]
Part 4 - Add comment to nsFrameList::GetLength() to warn it's O(n). r=mats

Good idea.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/934e3fb97f3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca70ac026805
https://hg.mozilla.org/integration/mozilla-inbound/rev/e500c5a05778
https://hg.mozilla.org/integration/mozilla-inbound/rev/854dbd810267

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/934e3fb97f3e
https://hg.mozilla.org/mozilla-central/rev/ca70ac026805
https://hg.mozilla.org/mozilla-central/rev/e500c5a05778
https://hg.mozilla.org/mozilla-central/rev/854dbd810267