Remove helper-class nsTableIterator

RESOLVED FIXED in Firefox 41

Status

()

Core
Layout: Tables
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: jfkthame)

Tracking

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment)

Looks like attachment 8624178 [details] [diff] [review] (on bug 1174700) is removing all the interesting parts of nsTableIterator (the "interesting parts" being special cases to handle RTL tables), because now we're going to be supporting RTL tables more directly via logical coordinates, passing around WritingMode structs, etc.

I suspect this means we can & should get rid of nsTableIterator entirely, since it won't really be doing anything special/interesting anymore (I think).  We can probably replace its usages with iteration over a nsFrameList.
(Assignee)

Comment 1

3 years ago
Created attachment 8625345 [details] [diff] [review]
Remove the (largely gutted) nsTableIterator class, and replace with simple frame-list iteration
Attachment #8625345 - Flags: review?(dholbert)
(Assignee)

Updated

3 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8625345 [details] [diff] [review]
Remove the (largely gutted) nsTableIterator class, and replace with simple frame-list iteration

r=me with the below:

>+++ b/layout/tables/nsTableFrame.cpp
>@@ -1424,28 +1424,24 @@ nsTableFrame::SetColumnDimensions(nscoor
>                                   nscoord aContainerWidth)
> {
[...]
>-    nsTableIterator iterCol(*colGroupFrame);
>-    for (nsIFrame* colFrame = iterCol.First(); colFrame;
>-         colFrame = iterCol.Next()) {
>+    for (nsIFrame* colFrame : colGroupFrame->PrincipalChildList()) {
[...]
>-    for (nsIFrame* colFrame = iterCol.First(); colFrame;
>-         colFrame = iterCol.Next()) {
>+    for (nsIFrame* colFrame : colGroupFrame->PrincipalChildList()) {


Here, rather than calling colGroupFrame->PrincipalChildList() twice, just call it once and cache that in a local 'const nsFrameList' variable (and use that in each of these loops).

(The function PrincipalChildList() involves a virtual function call, to GetChildList().  Might as well avoid introducing a new virtual function call as part of this cleanup. (unless there was a risk that one of these loops actually modified the list; but neither of the loops seem to have any risk of that.)

>+++ b/layout/tables/nsTableRowFrame.cpp
[...]
> void
> nsTableRowFrame::DidResize()
> {
>   // Resize and re-align the cell frames based on our row bsize
>   nsTableFrame* tableFrame = GetTableFrame();
>-  nsTableIterator iter(*this);
>-  nsIFrame* childFrame = iter.First();
[...]
>-  while (childFrame) {
>+  for (nsIFrame* childFrame : PrincipalChildList()) {

Let's just use "mFrames" here.  "PrincipalChildList()" is unnecessarily abstract, for accessing our own child list.

This applies to pretty much all of the chunks in this file (nsTableRowFrame.cpp)
Attachment #8625345 - Flags: review?(dholbert) → review+
Summary: Consider removing class nsTableIterator → Remove helper-class nsTableIterator
https://hg.mozilla.org/mozilla-central/rev/e79763059bef
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.