Closed
Bug 1227927
Opened 9 years ago
Closed 8 years ago
Remove nsIFrame::GetFirstPrincipalChild()
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: TYLin, Assigned: TYLin)
References
Details
Attachments
(4 files)
1.15 KB,
patch
|
TYLin
:
review+
|
Details | Diff | Splinter Review |
178.79 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
45.75 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
872 bytes,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
Per bug 1226875 comment 5, mats suggests removing GetFirstPrincipalChild() as well.
Assignee | ||
Comment 1•8 years ago
|
||
Since nsIFrame::GetChildList() is a const function, PrincipalChildList() should be a const function as well.
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 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•8 years ago
|
Attachment #8712140 -
Flags: review?(mats)
Assignee | ||
Comment 5•8 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)
Assignee | ||
Comment 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e46f4ace766
Assignee: nobody → tlin
Comment 7•8 years ago
|
||
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 8•8 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 r=mats, thanks for cleaning this up!
Attachment #8712141 -
Flags: review?(mats) → review+
Comment 9•8 years ago
|
||
(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•8 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•8 years ago
|
||
Attachment #8713025 -
Flags: review?(mats)
Comment 12•8 years ago
|
||
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 13•8 years ago
|
||
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•8 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
Closed: 8 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.
Description
•