Use appropriate background object visual rect for composited table cell backgrounds.

third_party/WebKit/Source/core/layout/LayoutTableCell.cpp

Index: third_party/WebKit/Source/core/layout/LayoutTableCell.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp b/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp
index b842de0d1adf12959601e2381e614de54c9f9d27..ffe9eae5de16e3672934304e5d278ef58a7ac8e7 100644
--- a/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp
@@ -46,7 +46,11 @@ using namespace HTMLNames;
 struct SameSizeAsLayoutTableCell : public LayoutBlockFlow {
   unsigned bitfields;
   int paddings[2];
-  void* pointer;
+  void* pointer1;
+  void* pointer2;
+  void* pointer3;
+  void* pointer4;
+  void* pointer5;
 };
 
 static_assert(sizeof(LayoutTableCell) == sizeof(SameSizeAsLayoutTableCell),
@@ -463,6 +467,44 @@ int LayoutTableCell::cellBaselinePosition() const {
   return (borderBefore() + paddingBefore() + contentLogicalHeight()).toInt();
 }
 
+void LayoutTableCell::createCompositedCellDisplayItemClients() {
+  if (!usesCompositedCellDisplayItemClients())
+    return;
+
+  if (row()->styleRef().hasBackground() && !m_rowBackgroundDisplayItemClient) {
+    m_rowBackgroundDisplayItemClient =
+        wrapUnique(new LayoutTableCell::RowBackgroundDisplayItemClient(*row()));
+  }
+
+  LayoutTable::ColAndColGroup colAndColGroup =
+      table()->colElementAtAbsoluteColumn(absoluteColumnIndex());
+  LayoutTableCol* column = colAndColGroup.col;
+  LayoutTableCol* columnGroup = colAndColGroup.colgroup;
+  if (column && column->styleRef().hasBackground() &&
+      !m_colBackgroundDisplayItemClient) {
+    m_colBackgroundDisplayItemClient = wrapUnique(
+        new LayoutTableCell::ColBackgroundDisplayItemClient(*column));
+  }
+  if (columnGroup && columnGroup->styleRef().hasBackground() &&
+      !m_colGroupBackgroundDisplayItemClient) {
+    m_colGroupBackgroundDisplayItemClient = wrapUnique(
+        new LayoutTableCell::ColBackgroundDisplayItemClient(*columnGroup));
+  }
+  if (section()->styleRef().hasBackground() &&
+      !m_sectionBackgroundDisplayItemClient) {
+    m_sectionBackgroundDisplayItemClient = wrapUnique(
+        new LayoutTableCell::SectionBackgroundDisplayItemClient(*section()));
+  }
+}
+
+void LayoutTableCell::ensureIsReadyForPaintInvalidation() {
+  LayoutBlockFlow::ensureIsReadyForPaintInvalidation();
+  // Below handles the case where a page starts out in a state where the table

[wkorman, 2016/11/15 01:05:33]

I ended up keeping this as it handles one case (described in this comment) which
is not handled by the logic added to TablePaintInvalidator. TPI currently walks
through looking for any background style changes, doesn't see any, and bails
out. We could add a cell-walk to do this, but using this method for what it's
intended on initial paint seems more appropriate.

+  // cell compositing state and its container styling is such that one or more
+  // special display item clients are needed.
+  createCompositedCellDisplayItemClients();
+}
+
 void LayoutTableCell::styleDidChange(StyleDifference diff,
                                      const ComputedStyle* oldStyle) {
   DCHECK_EQ(style()->display(), EDisplay::TableCell);
@@ -1416,21 +1458,84 @@ bool LayoutTableCell::backgroundIsKnownToBeOpaqueInRect(
   return LayoutBlockFlow::backgroundIsKnownToBeOpaqueInRect(localRect);
 }
 
-bool LayoutTableCell::usesTableAsAdditionalDisplayItemClient() const {
-  // In certain cases such as collapsed borders for composited table cells we
-  // paint content for the cell into the table graphics layer backing and so
-  // must use the table's visual rect.
+LayoutTableCell::RowBackgroundDisplayItemClient::RowBackgroundDisplayItemClient(
+    const LayoutTableRow& layoutTableRow)
+    : m_layoutTableRow(layoutTableRow) {}
+
+String LayoutTableCell::RowBackgroundDisplayItemClient::debugName() const {
+  return "RowBackground";
+}
+
+LayoutRect LayoutTableCell::RowBackgroundDisplayItemClient::visualRect() const {
+  return m_layoutTableRow.visualRect();
+}
+
+LayoutTableCell::ColBackgroundDisplayItemClient::ColBackgroundDisplayItemClient(
+    const LayoutTableCol& layoutTableCol)
+    : m_layoutTableCol(layoutTableCol) {}
+
+String LayoutTableCell::ColBackgroundDisplayItemClient::debugName() const {
+  return "ColBackground";
+}
+
+LayoutRect LayoutTableCell::ColBackgroundDisplayItemClient::visualRect() const {
+  return m_layoutTableCol.visualRect();
+}
+
+LayoutTableCell::SectionBackgroundDisplayItemClient::
+    SectionBackgroundDisplayItemClient(
+        const LayoutTableSection& layoutTableSection)
+    : m_layoutTableSection(layoutTableSection) {}
+
+String LayoutTableCell::SectionBackgroundDisplayItemClient::debugName() const {
+  return "SectionBackground";
+}
+
+LayoutRect LayoutTableCell::SectionBackgroundDisplayItemClient::visualRect()
+    const {
+  return m_layoutTableSection.visualRect();
+}
+
+bool LayoutTableCell::usesCompositedCellDisplayItemClients() const {
+  // Composited table cells are painted in special ways that require the use of
+  // custom display item clients:
+  //
+  // 1. When painting row background we paint content for the cell into the
+  // table row graphics layer backing and so must use the row's visual rect.
+  //
+  // 2. When painting section, column or column group backgrounds we paint
+  // content for the cell into the table section graphics layer backing and must
+  // use the visual rect for the respective background object
   return (hasLayer() && layer()->compositingState() != NotComposited) ||
          RuntimeEnabledFeatures::slimmingPaintV2Enabled();

[Xianzhu, 2016/11/15 02:58:23]

This misses the case that the row is composited but the cell is not, and the
section has background. In the case, we should create section display item
clients in cells for section backgrounds painted on the parent layer of the row.
Another similar case is that a section is composited and a col has background.

At first I thought of for simplicity that we could just embed the display item
clients in DisplayItemClients. Then we could replace the "*cell"s in
TablePaintInvalidator with the special display item clients without adding more
code there.

Now I think to make the CL small enough for merging we could just fix the case
that a composited cell in non-composited row and section first (which is
basically one of the previous patch sets) and merge it, then investigate
painting all backgrounds for a row or section in one display item to fix all the
cases. Wdyt?

[wkorman, 2016/11/15 19:46:02]

Ah. I will look at adding tests for these cases (even though this change may be
tabled for now).
Can you elaborate on this, I am not sure what you mean by "embed the display
item
clients in DisplayItemClients". Maybe you mean, essentially, what this change
currently does -- by adding add'l display item clients within LayoutTableCell?
Sounds ok to me, I am flexible, I will look to prepare separate change minimized
to that.

I may have misunderstood, from your comment in
https://codereview.chromium.org/2469903002#msg16 I thought we'd decided to
pursue this path. Just to make sure I understand, is this a result of:
- concern re: the addition of add'l pointers to LayoutTableCell
- concern re: the general complexity, potential performance/memory impact
- on reconsideration, the appeal/desire to reduce the number of display
items/display item clients for performance purposes

I had come away from your previous comments thinking you were unsure whether one
display item approach would be feasible correctness-wise. While I have not
investigated in depth yet myself or read the W3C table spec in detail, from
discussion and code reading to date it sounds like there is considerable
flexibility and correctness challenges around backgrounds, shadows and opacity
(which has some TODOs noted in code and sounds like it may not currently work in
all cases, if at all) particularly given I believe they can be applied in one or
multiple of table, section, col, cell, and should stack/layer in a particular
manner.

We should also consider, before embarking on single display item for
backgrounds, how much add'l work to invest in tables as I believe focus for
further work is shifting to Grid. I believe Table is in updated re-spec to
document current behavior for improved cross-browser compatibility, and may not
evolve heavily from here.

[Xianzhu, 2016/11/15 20:16:58]

Sorry, "DisplayItemClients" should be "LayoutTableCells". I meant to just put
the display item clients as direct data members of LayoutTableCell instead of
pointers, so that we always use them for backgrounds from containers. However,
this would cause rasterization regressions in normal cases so it's not a good
way.
Let's land a simple CL for the most common case (composited cell in
non-composited row) first. It should also be able to correctly handle the case
that a row is composited but the intersecting col and section don't have
background.

For other cases, I can run cluster-telemetry to see how rare they are in real
world. I think we can keep the regressions as-is in M55 for the very rare cases,
and fix them on trunk only with a more complete CL.
My concern is mainly about complexity which may lead to risks of
functional/performance regressions, 
 
I think it will be fine just to fix and avoid regressions about tables. I would
put the existing bugs in the lowest priority even if we want to fix them.

 }
 
 void LayoutTableCell::invalidateDisplayItemClients(
     PaintInvalidationReason reason) const {
-  if (m_collapsedBorderValues && usesTableAsAdditionalDisplayItemClient()) {
-    ObjectPaintInvalidator(*this).invalidateDisplayItemClient(
-        *m_collapsedBorderValues, reason);
-  }
   LayoutBlockFlow::invalidateDisplayItemClients(reason);
+
+  if (!usesCompositedCellDisplayItemClients())
+    return;
+
+  ObjectPaintInvalidator invalidator(*this);
+  if (m_collapsedBorderValues)
+    invalidator.invalidateDisplayItemClient(*m_collapsedBorderValues, reason);
+  if (m_rowBackgroundDisplayItemClient) {
+    invalidator.invalidateDisplayItemClient(*m_rowBackgroundDisplayItemClient,
+                                            reason);
+  }
+  if (m_colBackgroundDisplayItemClient) {
+    invalidator.invalidateDisplayItemClient(*m_colBackgroundDisplayItemClient,
+                                            reason);
+  }
+  if (m_colGroupBackgroundDisplayItemClient) {
+    invalidator.invalidateDisplayItemClient(
+        *m_colGroupBackgroundDisplayItemClient, reason);
+  }
+  if (m_sectionBackgroundDisplayItemClient) {
+    invalidator.invalidateDisplayItemClient(
+        *m_sectionBackgroundDisplayItemClient, reason);
+  }
 }
 
 // TODO(lunalu): Deliberately dump the "inner" box of table cells, since that