Invalidate row/section for backgrounds when cell's geometry changes

third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "core/paint/TableCellPaintInvalidator.h"
+
+#include "core/layout/LayoutTable.h"
+#include "core/layout/LayoutTableCell.h"
+#include "core/layout/LayoutTableCol.h"
+#include "core/layout/LayoutTableRow.h"
+#include "core/layout/LayoutTableSection.h"
+#include "core/paint/BlockPaintInvalidator.h"
+#include "core/paint/ObjectPaintInvalidator.h"
+#include "core/paint/PaintInvalidator.h"
+#include "core/paint/PaintLayer.h"
+
+namespace blink {
+
+PaintInvalidationReason TableCellPaintInvalidator::InvalidatePaintIfNeeded() {

[wkorman, 2017/04/27 22:30:23]

Can we add a unit test for this new class/logic? I know it may be a hassle since
we probably don't have great mocks/fakes for context and so forth.

[Xianzhu, 2017/04/28 00:31:06]

I'm not sure how a unit test would benefit us for this case. To test, I need to
create a table containing at least one column, row, section and cell which meet
the conditions here, call the function, then check if the layers are set
needsRepaint and the display item clients are invalidated. The test would
basically repeat the code in another form. If there is any error (e.g. we
invalidated the wrong layer or wrong display item client) preventing this code
from working, the unit test can hardly discover the error.

A layout test reproducing the bug can easily discover errors in this code.

[wkorman, 2017/04/28 02:24:54]

Well, unit tests are faster, less flaky, and are conceptually capable of testing
this logic, so I don't agree that a unit test can't find errors (presuming we
take on the hassle of adding some way to track and scrutinize invalidations,
display item clients, etc.). Agree that it would be sort of the inverse of this
class logic, but it would live on in that state even if this class is rewritten
and ensure future changes don't break it.

Given your point that this is sort of an internal class used only by
LayoutTableCell, a unit test would maybe make more sense within
LayoutTableCellTest.

In any case I am ok with just the layout test but I think it's worth keeping in
mind ongoing to consider expanding where/when we write unit tests. Today it
feels like we still sometimes rely on heavier weight layout tests. Invalidation
state specifically seems like something we could cover in unit tests as well at
some point.

+  // The cell's containing row and section paint backgrounds behind the cell.
+  // If the cell's geometry changed, invalidate the background display items.
+  if (context_.old_location != context_.new_location ||
+      cell_.Size() != cell_.PreviousSize()) {
+    const auto& row = *cell_.Row();
+    if (row.GetPaintInvalidationReason() == kPaintInvalidationNone &&
+        row.StyleRef().HasBackground()) {
+      context_.parent_context->painting_layer->SetNeedsRepaint();
+      row.InvalidateDisplayItemClients(kPaintInvalidationForcedByLayout);
+    }
+
+    const auto& section = *row.Section();
+    if (section.GetPaintInvalidationReason() == kPaintInvalidationNone) {
+      bool section_paints_background = section.StyleRef().HasBackground();
+      if (!section_paints_background) {
+        auto col_and_colgroup = section.Table()->ColElementAtAbsoluteColumn(
+            cell_.AbsoluteColumnIndex());
+        if ((col_and_colgroup.col &&
+             col_and_colgroup.col->StyleRef().HasBackground()) ||
+            (col_and_colgroup.colgroup &&
+             col_and_colgroup.colgroup->StyleRef().HasBackground()))
+          section_paints_background = true;
+      }
+      if (section_paints_background) {
+        context_.parent_context->parent_context->painting_layer
+            ->SetNeedsRepaint();
+        section.InvalidateDisplayItemClients(kPaintInvalidationForcedByLayout);
+      }
+    }
+  }
+
+  return BlockPaintInvalidator(cell_).InvalidatePaintIfNeeded(context_);
+}
+
+}  // namespace blink