Closed Bug 1227927 Opened 4 years ago Closed 4 years ago

Remove nsIFrame::GetFirstPrincipalChild()

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

Details

Attachments

(4 files)

Per bug 1226875 comment 5, mats suggests removing GetFirstPrincipalChild() as well.
Since nsIFrame::GetChildList() is a const function, PrincipalChildList()
should be a const function as well.
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.
Attachment #8712139 - Flags: review+
Attachment #8712140 - Flags: review?(mats)
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.
Attachment #8712141 - Flags: review?(mats)
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.
Attachment #8712140 - Flags: review?(mats) → review+
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!
Attachment #8712141 - Flags: review?(mats) → review+
(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.)
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 on attachment 8713025 [details] [diff] [review]
Part 4 - Add comment to nsFrameList::GetLength() to warn it's O(n). r=mats

Good idea.
Attachment #8713025 - Flags: review?(mats) → review+
You need to log in before you can comment on or make changes to this bug.