Bug 98221

Created attachment 166791 [details] Patch

I'm open to other ways of doing this. I suspect this is just getting back a "regression" from http://trac.webkit.org/changeset/97691.

Comment on attachment 166791 [details] Patch Attachment 166791 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14131463

Comment on attachment 166791 [details] Patch Attachment 166791 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14137401

Comment on attachment 166791 [details] Patch Attachment 166791 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14130522

Comment on attachment 166791 [details] Patch Updating now.

Created attachment 166794 [details] Patch

Comment on attachment 166794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166794&action=review > Source/WebCore/ChangeLog:17 > + It also looks like we could consider removing m_hasHTMLTableCellElement since that appears > + to have been a previous attempt at such an optimization from http://trac.webkit.org/changeset/97691. m_hasHTMLTableCellElement is basically determining if we are an HTML table cell's renderer as CSS doesn't define spanning rows / columns. With your optimization, I think it should be removed, folded into m_hasColSpan and m_hasRowSpan. > Source/WebCore/rendering/RenderTableCell.cpp:56 > + , m_hasColSpan(false) > + , m_hasRowSpan(false) Not a huge fan of those names, how about m_hasColumnSpanSet or m_hasColumnSpanAttribute. > Source/WebCore/rendering/RenderTableCell.cpp:221 > + // The vast majority of table cells do not have a colspan or rowspan, > + // so we keep a bool to know if we need to bother reading from the DOM. > + m_hasColSpan = parseColSpanFromDOM() != 1; > + m_hasRowSpan = parseRowSpanFromDOM() != 1; You should do it on colSpanOrRowSpanChanged instead of forcing this cost for every layout. > Source/WebCore/rendering/RenderTableCell.h:245 > + unsigned m_column : 28; > bool m_cellWidthChanged : 1; > bool m_hasHTMLTableCellElement : 1; > + bool m_hasColSpan: 1; > + bool m_hasRowSpan: 1; If we want those fields to be packed on Windows, they should have the same type. See https://lists.webkit.org/pipermail/webkit-dev/2012-March/020134.html (we probably want to keep table cells small and add a COMPILE_ASSERT).

Comment on attachment 166794 [details] Patch Attachment 166794 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14129449 New failing tests: css1/cascade/important.html css1/basic/class_as_selector.html css1/box_properties/border_color.html css1/box_properties/border_bottom.html css1/basic/grouping.html css1/box_properties/border_bottom_width.html css1/box_properties/border_bottom_width_inline.html css1/classification/list_style_position.html css1/basic/id_as_selector.html css1/basic/inheritance.html css1/box_properties/border_color_inline.html css1/box_properties/border.html css1/color_and_background/background_position.html WebFrameTest.FindInPageMatchRects css1/color_and_background/background_attachment.html css1/classification/list_style_image.html css1/cascade/cascade_order.html css1/color_and_background/background.html css1/classification/list_style.html css1/conformance/forward_compatible_parsing.html css1/box_properties/border_inline.html css1/color_and_background/background_color.html css1/color_and_background/background_image.html css1/box_properties/border_bottom_inline.html css1/basic/containment.html css1/basic/contextual_selectors.html css1/basic/comments.html css1/color_and_background/background_repeat.html css1/classification/white_space.html css1/classification/display.html css1/classification/list_style_type.html

Comment on attachment 166794 [details] Patch Attachment 166794 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14124784 New failing tests: css1/cascade/important.html css1/basic/class_as_selector.html css1/box_properties/border_color.html css1/box_properties/border_bottom.html css1/basic/grouping.html css1/box_properties/border_bottom_width.html css1/box_properties/border_bottom_width_inline.html css1/classification/list_style_position.html css1/basic/id_as_selector.html css1/basic/inheritance.html css1/box_properties/border_color_inline.html css1/box_properties/border.html css1/color_and_background/background_position.html css1/color_and_background/background_attachment.html css1/classification/list_style_image.html css1/cascade/cascade_order.html css1/color_and_background/background.html css1/classification/list_style.html css1/conformance/forward_compatible_parsing.html css1/box_properties/border_inline.html css1/color_and_background/background_color.html css1/color_and_background/background_image.html css1/box_properties/border_bottom_inline.html css1/basic/containment.html css1/basic/contextual_selectors.html css1/basic/comments.html css1/classification/white_space.html css1/font_properties/font.html css1/classification/display.html css1/classification/list_style_type.html

Created attachment 167134 [details] Addressed Julien's feedback, but still fails tests

Created attachment 167186 [details] Patch

I may need to change bool to unsigned or the compile may fail on Windows. If it does, I'll fix it.

Comment on attachment 167186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167186&action=review > Source/WebCore/ChangeLog:9 > + so I stole another 2 bits from RenderTableCell::m_column to avoid you stole 1 bit actually ;) > Source/WebCore/rendering/RenderTableCell.cpp:55 > +COMPILE_ASSERT(sizeof(RenderTableCell) == sizeof(SameSizeAsRenderTableCell), RenderTableCell_should_stay_small); As you pointed out, this will likely fail on Windows due to the different member's type. > Source/WebCore/rendering/RenderTableCell.cpp:81 > + if (!node()) > + return 1; This could be moved around the call to updateColAndRowSpanFlags() in the constructor and removed from both parse function. You are guaranteed to have the right type if you get called from colSpanOrRowSpanChanged. > Source/WebCore/rendering/RenderTableCell.cpp:123 > + // FIXME: I suspect that we could return early here if !m_hasColSpan && !m_hasRowSpan. I don't think this is true. You could probably bail out if the old / new values of rowSpan and colSpan were identical though but we don't have enough information in this function to check that.

Comment on attachment 167186 [details] Patch Attachment 167186 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14168470

Created attachment 167359 [details] ready

I'll mark cq+ once the bots like it.

Comment on attachment 167359 [details] ready Clearing flags on attachment: 167359 Committed r130548: <http://trac.webkit.org/changeset/130548>

All reviewed patches have been landed. Closing bug.