367706 - [FIX]OrderRowGroups should be typesafe

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Cleanup stuff

Please note the XXX comments I added... They worry me.  ;)

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm not sure that the code in nsTableFrame::CalcDesiredHeight is right either.  What should happen with scrollable rowgroups there?

Comment 3

[Bernd]

OMG, I am sitting on a review request for 8 weeks. Sorry. This will however not happen this week :-( .

Comment 4

[Bernd]

+  // put the children frames in the display order (e.g. thead before tbody
+  // before tfoot). This will handle calling GetRowGroupFrame() on the

please mention that it should be the first thead and the first tfoot

+  //
+  // XXXbz why do we really care about the non-rowgroup kids?

I took it it fro granted all the times. I guess this is from the times when tables pseudos where not very reliable. In principle I believe we should assert if we encounter such a non rowgroup  kid. Most of them are probably a result of a frame construction error.


+    // XXXbz why bother with the non-rowgroup frames?  It's not like we ever
+    // reflow them...

But we need to do something with them otherwise we will loose them during pagination.

+nsTableFrame::OrderRowGroups(RowGroupArray& aChildren) const
+{
+....
+      default:
+        // Just ignore it
+        break;
Assert here (aka notreached, how could we ever end here.

+            // XXXbz what if lastChild is a scrollable tfoot?  Bogus!!
+            // dbaron is right -- this should be using thead/tfoot!

Mats should know this, he wrote this.

if (childX == RowGroupArray::NoIndex) {
+          // XXXbz can this happen?
+          childX = numRowGroups;
         }

If not why not make noise about it.

+    if (rowCount > 0) { // XXXbz and if it's not we just keep looping?  What's
+                        // the point?

Rule 1.: BC is a hack ( repeat after me)
Rule 2.: the point here is if rowgroup does not have rows it can't have cells with a border. So the function tries to find the first "sane" rowgroup.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> please mention that it should be the first thead and the first tfoot

Done.

> In principle I believe we should assert if we encounter such a non rowgroup 
> kid.

Done.

> But we need to do something with them

Documented that.

> Assert here (aka notreached, how could we ever end here.

Done.

> Mats should know this, he wrote this.

Er... no.  karnaze did.  I think I'll just change it in a followup patch; it's pretty definitely wrong, and I'm surprised it hasn't crashed us before...

> If not why not make noise about it.

The problem is that I can't tell whether it can or not.

> So the function tries to find the first "sane" rowgroup.

I was having trouble reconciling that with my XXX comment about mRowGroup vs rgX.  Unless I'm right, and it should be doing |mRowGroup = mRowGroups[rgX];|?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

I filed bug 383196 and bug 383193 on the remaining issues.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Checked in.  This is a code-level change that should not change behavior, so no regression tests to be had...