Closed Bug 367706 Opened 15 years ago Closed 14 years ago

[FIX]OrderRowGroups should be typesafe

Categories

(Core :: Layout: Tables, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha6

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file, 1 obsolete file)

 
Target Milestone: --- → mozilla1.9alpha
Attached patch Cleanup stuff (obsolete) — Splinter Review
Please note the XXX comments I added... They worry me.  ;)
Attachment #252357 - Flags: superreview?(roc)
Attachment #252357 - Flags: review?(bernd_mozilla)
Attachment #252357 - Flags: superreview?(roc) → superreview+
I'm not sure that the code in nsTableFrame::CalcDesiredHeight is right either.  What should happen with scrollable rowgroups there?
OMG, I am sitting on a review request for 8 weeks. Sorry. This will however not happen this week :-( .
+  // 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.






> 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];|?
I filed bug 383196 and bug 383193 on the remaining issues.
Attachment #252357 - Attachment is obsolete: true
Attachment #267190 - Flags: review?(bernd_mozilla)
Attachment #252357 - Flags: review?(bernd_mozilla)
Attachment #267190 - Flags: review?(bernd_mozilla) → review+
Target Milestone: mozilla1.9alpha1 → mozilla1.9alpha6
Checked in.  This is a code-level change that should not change behavior, so no regression tests to be had...
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Depends on: 385880
Depends on: 387344
You need to log in before you can comment on or make changes to this bug.