1176105 - Remove helper-class nsTableIterator

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Remove the (largely gutted) nsTableIterator class, and replace with simple frame-list iteration

Comment 2

[Daniel Holbert [:dholbert]]

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)

Comment 3

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e79763059bef

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e79763059bef