If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[FIX]OrderRowGroups should be typesafe

RESOLVED FIXED in mozilla1.9alpha6

Status

()

Core
Layout: Tables
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla1.9alpha6
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

 
Target Milestone: --- → mozilla1.9alpha
Created attachment 252357 [details] [diff] [review]
Cleanup stuff

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?

Comment 3

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

Comment 4

11 years ago
+  // 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];|?
Created attachment 267190 [details] [diff] [review]
Updated to comments

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)

Updated

11 years ago
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
Last Resolved: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED

Updated

10 years ago
Depends on: 385880
Depends on: 387344
You need to log in before you can comment on or make changes to this bug.