[SPInvalidation] Fix PrePaintTreeWalk for multicol spanner

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

 // Copyright 2015 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/layout/LayoutTestHelper.h"
 #include "core/layout/LayoutTreeAsText.h"
 #include "core/layout/api/LayoutViewItem.h"
 #include "core/paint/ObjectPaintProperties.h"
 #include "core/paint/PaintPropertyTreePrinter.h"
 #include "platform/graphics/paint/GeometryMapper.h"
@@ skipping 53 matching lines @@
     return frameView->contentClip();
   }
 
   const ScrollPaintPropertyNode* frameScroll() {
     FrameView* frameView = document().view();
     if (RuntimeEnabledFeatures::rootLayerScrollingEnabled())
       return frameView->layoutView()->objectPaintProperties()->scroll();
     return frameView->scroll();
   }
 
+  LayoutPoint paintOffset(const LayoutObject* object) {
+    return object->objectPaintProperties()
+        ->localBorderBoxProperties()
+        ->paintOffset;
+  }
+
  private:
   void SetUp() override {
     Settings::setMockScrollbarsEnabled(true);
 
     RenderingTest::SetUp();
     enableCompositing();
   }
 
   void TearDown() override {
     RenderingTest::TearDown();
@@ skipping 2648 matching lines @@
               MainThreadScrollingReason::kHasBackgroundAttachmentFixedObjects));
   EXPECT_FALSE(
       overflowB->layoutObject()
           ->objectPaintProperties()
           ->scroll()
           ->parent()
           ->hasMainThreadScrollingReasons(
               MainThreadScrollingReason::kHasBackgroundAttachmentFixedObjects));
 }
 
+TEST_P(PaintPropertyTreeBuilderTest, PaintOffsetUnderMultiColumn) {

[chrishtr, 2016/10/05 00:48:17]

@mstensho could you please review this test and verify Xianzhu's calculations as
well as his comments
about some of them being incorrect?

+  setBodyInnerHTML(
+      "<style>"
+      "  body { margin: 0; }"
+      "  .space { height: 30px; }"
+      "  .abs { position: absolute; width: 20px; height: 20px; }"
+      "</style>"
+      "<div style='columns:2; width: 200px; column-gap: 0'>"
+      "  <div style='position: relative'>"
+      "    <div id=space1 class=space></div>"
+      "    <div id=space2 class=space></div>"
+      "    <div id=spanner style='column-span: all'>"
+      "      <div id=normal style='height: 50px'>"

[mstensho (USE GERRIT), 2016/10/05 08:44:09]

Missing </div> ?

[Xianzhu, 2016/10/05 17:29:40]

Done.

+      "      <div id=top-left class=abs style='top: 0; left: 0'></div>"
+      "      <div id=bottom-right class=abs style='bottom: 0; right: 0'></div>"
+      "    </div>"
+      "    <div id=space3 class=space></div>"

[mstensho (USE GERRIT), 2016/10/05 08:44:09]

I don't think you intended for this to be inside the spanner? At least that's
what the indentation suggests.

[Xianzhu, 2016/10/05 17:29:40]

Right.

+      "    <div id=space4 class=space></div>"
+      "  </div>"
+      "</div>");
+
+  // Above the spanner.
+  // Column 1.
+  EXPECT_EQ(LayoutPoint(), paintOffset(getLayoutObjectByElementId("space1")));
+  // Column 2. TODO(crbug.com/648274): This is incorrect. Should be (50, 0).

[mstensho (USE GERRIT), 2016/10/05 08:44:09]

What should it be relative to? LayoutPoint(0, 30) suggests that it's relative to
the flow thread. As far as I can read from the documentation, it should be
relative to "the propertyTreeState's transform space". Does that mean the
viewport in this case, since we have no transforms?

If it should be something else, I don't think (50, 0) is right. Perhaps (100,
0). Column width is 100px, after all.

[Xianzhu, 2016/10/05 17:29:40]

Changed the comment to (100, 0).

Made the mistake (as well as other should-be's) using 50px column width.

+  EXPECT_EQ(LayoutPoint(0, 30),
+            paintOffset(getLayoutObjectByElementId("space2")));
+
+  // The spanner's normal flow.
+  EXPECT_EQ(LayoutPoint(0, 30),
+            paintOffset(getLayoutObjectByElementId("spanner")));
+  EXPECT_EQ(LayoutPoint(0, 30),
+            paintOffset(getLayoutObjectByElementId("normal")));
+
+  // Below the spanner.
+  // Column 1.
+  EXPECT_EQ(LayoutPoint(0, 80),
+            paintOffset(getLayoutObjectByElementId("space3")));
+  // Column 2. TODO(crbug.com/648274): This is incorrect. Should be (50, 80).

[mstensho (USE GERRIT), 2016/10/05 08:44:09]

(100, 80), perhaps?

[Xianzhu, 2016/10/05 17:29:40]

Done.

+  EXPECT_EQ(LayoutPoint(0, 110),
+            paintOffset(getLayoutObjectByElementId("space4")));
+
+  // Out-of-flow positioned descendants of the spanner. They are laid out in
+  // the relative-position container.
+  // This should be aligned to the top-left corner of space1.
+  EXPECT_EQ(LayoutPoint(0, 30),

[mstensho (USE GERRIT), 2016/10/05 08:44:09]

Shouldn't this be (0,0) then?

+            paintOffset(getLayoutObjectByElementId("top-left")));
+
+  // This should be aligned to the bottom-right corner of space4.
+  // TODO(crbug.com/648274): This is incorrect. Should be (80, 90).

[mstensho (USE GERRIT), 2016/10/05 08:44:09]

Probably (180, 90) then.

[Xianzhu, 2016/10/05 17:29:40]

Done.

+  EXPECT_EQ(LayoutPoint(80, 40),
+            paintOffset(getLayoutObjectByElementId("bottom-right")));
+}
+
 }  // namespace blink