Remove nsIFrame::GetFirstPrincipalChild()

RESOLVED FIXED in Firefox 47

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: TYLin, Assigned: TYLin)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

3 years ago
Per bug 1226875 comment 5, mats suggests removing GetFirstPrincipalChild() as well.
(Assignee)

Comment 1

3 years ago
Created attachment 8712139 [details] [diff] [review]
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.
(Assignee)

Comment 2

3 years ago
Created attachment 8712140 [details] [diff] [review]
Part 2 - Remove nsIFrame::GetFirstPrincipalChild(). r=mats
(Assignee)

Comment 3

3 years ago
Created attachment 8712141 [details] [diff] [review]
Part 3 - Use ranged-based for-loop to rewrite some simple loops in part 2. r=mats
(Assignee)

Comment 4

3 years ago
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+
(Assignee)

Updated

3 years ago
Attachment #8712140 - Flags: review?(mats)
(Assignee)

Comment 5

3 years ago
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.)
(Assignee)

Comment 10

3 years ago
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.
(Assignee)

Comment 11

3 years ago
Created attachment 8713025 [details] [diff] [review]
Part 4 - Add comment to nsFrameList::GetLength() to warn it's O(n). r=mats
Attachment #8713025 - Flags: review?(mats)
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+

Comment 14

3 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.