Open Bug 481932 Opened 11 years ago Updated Last year

use do_QueryFrame instead of IS_TABLE_CELL + cast

Categories

(Core :: Layout: Tables, defect)

x86
Linux
defect
Not set

Tracking

()

People

(Reporter: Swatinem, Unassigned)

References

()

Details

Attachments

(5 files)

As per bug 474369 comment 24:
> A nice followup bug would be to replace IS_TABLE_CELL and the cast with
> do_QueryFrame.
> 
> Similar for the cast to nsTableRowFrame*.
What is this?
http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableRowFrame.cpp#1327
So the code is casting the nsIFrame to nsTableCellFrame when it is NOT a table cell.
I'm confused.
I've tracked that code down to bug 59280 and attachment 30628 [details] [diff] [review].
Weird. Is that code hit in our tests?
Just out of curiosity, I changed the meaning of the if statement and that didn't change my reftest results.
I also pushed the patch to the try server to see if any other tests are affected.
Attachment #369164 - Flags: superreview?(roc)
Attachment #369164 - Flags: review?(roc)
Comment on attachment 369164 [details] [diff] [review]
patch [pushed: comment 10]

Looks good.

That bad code you found seems to only be reachable via nsTableRowGroupFrame::SplitSpanningCells. To trigger the bad if, we'd need a testcase containing
a) a table with many rows
b) a cell (not in the first column) spanning many rows
c) table splitting, so viewing the table in print or print preview
d) where the table is split inside the rows spanned by the cell
e) and the cell contents need to split across that page boundary too
Attachment #369164 - Flags: superreview?(roc)
Attachment #369164 - Flags: superreview+
Attachment #369164 - Flags: review?(roc)
Attachment #369164 - Flags: review+
I suspect the effect of the bug you found is that the cell contents on the page after the split would show up in the first column instead of the column the cell is supposed to be in.
A large table with a lot of spanning cells. The Tinderbox waterfall seems to be the perfect testcase for this.
If I see it correctly, The splitted cells just disappear in Firefox 3 but do continue on the new page with this patch.
Great! If you wanted to be really awesome you could write a reftest for this using reftest-print :-).
(I don't mean to imply that you are not already really awesome!)
Comment on attachment 369164 [details] [diff] [review]
patch [pushed: comment 10]

http://hg.mozilla.org/mozilla-central/rev/931671e83e44

Leaving the bug open for a reftest.
Attachment #369164 - Attachment description: patch → patch [pushed: comment 10]
Attached patch reftestSplinter Review
I can't get this reftest to fail without this patch.
Searching for similar bugs, I found bug 400149 and bug 467444. But those are not fixed by this patch.
The only thing I can confirm that is fixed by this patch is printing the tinderbox waterfall.
On the end of page 2, the plinux-trunk07 column should continue on page 3 with the old graph links. Thats broken with the unpatched build but does work with the patched one.
An uneducated comment: Is there any way we can get the HTML output of what will be printed and then minimize that into a reftest? Is there a breakpoint that can be set anywhere to grab print data serialized to HTML before it is serialized into whatever it is that transforms it into PDF?

I'm also setting in-testsuite-? so we don't lose track of the test we need here.  Mark as + once you have a test.
Flags: in-testsuite?
>So the code is casting the nsIFrame to nsTableCellFrame when it is NOT a table
>cell.
This IIRC required as there are two types of tables cells nsTableCellFrames and nsBCTableCellFrame and thats what the code had in mind
Assignee: arpad.borsos → nobody
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
No assignee, updating the status.
You need to log in before you can comment on or make changes to this bug.