481932 - use do_QueryFrame instead of IS_TABLE_CELL + cast

Status: NEW

(Core :: Layout: Tables, defect)

Description

[Arpad Borsos [:Swatinem]]

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*.

Comment 1

[Arpad Borsos [:Swatinem]]

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].

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Weird. Is that code hit in our tests?

Comment 3

[Arpad Borsos [:Swatinem]]

Attachment: patch [pushed: comment 10]

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.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Arpad Borsos [:Swatinem]]

Attachment: Tinderbox waterfall print preview Firefox 3.0.7

A large table with a lot of spanning cells. The Tinderbox waterfall seems to be the perfect testcase for this.

Comment 7

[Arpad Borsos [:Swatinem]]

Attachment: Tinderbox waterfall print preview Minefield + patch

If I see it correctly, The splitted cells just disappear in Firefox 3 but do continue on the new page with this patch.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Great! If you wanted to be really awesome you could write a reftest for this using reftest-print :-).

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(I don't mean to imply that you are not already really awesome!)

Comment 10

[Arpad Borsos [:Swatinem]]

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.

Comment 11

[Arpad Borsos [:Swatinem]]

Attachment: reftest

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.

Comment 12

[Arpad Borsos [:Swatinem]]

Attachment: Tinderbox waterfall printed to pdf, unpatched+patched (zip)

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.

Comment 13

[cmtalbert]

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.

Comment 14

[Bernd]

>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

Comment 15

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.