[css-grid] Use order-modified document order for m_gridItemsIndexesMap (issue 2438253003 by rego@igalia.com)

[re...@igalia.com]

Reviewers: svillar, jfernandez, cbiesinger
CL: https://codereview.chromium.org/2438253003/

Message:
This is small refactoring as @jfernandez will need to use
this hash map for the grid baseline patch.

Description:
[css-grid] Use order-modified document order for m_gridItemsIndexesMap

We were storing the index in the DOM for the grid items
in m_gridItemsIndexesMap. However, we can already store the index
following the order-modified document order.

This allows us to simplify the sorter in painting,
as we don't need to check the order property now,
just simply use the stored index.

No new tests, as it's already covered by current tests.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Affected files (+12, -19 lines):
M third_party/WebKit/Source/core/layout/LayoutGrid.cpp
M third_party/WebKit/Source/core/paint/GridPainter.cpp


Index: third_party/WebKit/Source/core/layout/LayoutGrid.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
index 26b4afd42c9f6a66f70e7a0e1a2aeccb0f5cc909..47d8a7f295e4360e46ee4756e91a1e8bf0f3bcf3 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -1922,7 +1922,6 @@ void LayoutGrid::placeItemsOnGrid() {
return;

DCHECK(m_gridItemArea.isEmpty());
- DCHECK(m_gridItemsIndexesMap.isEmpty());

populateExplicitGridAndOrderIterator();

@@ -1931,12 +1930,16 @@ void LayoutGrid::placeItemsOnGrid() {

Vector<LayoutBox*> autoMajorAxisAutoGridItems;
Vector<LayoutBox*> specifiedMajorAxisAutoGridItems;
+ DCHECK(m_gridItemsIndexesMap.isEmpty());
+ size_t childIndex = 0;
m_hasAnyOrthogonalChildren = false;
for (LayoutBox* child = m_orderIterator.first(); child;
child = m_orderIterator.next()) {
if (child->isOutOfFlowPositioned())
continue;

+ m_gridItemsIndexesMap.set(child, childIndex++);
+
m_hasAnyOrthogonalChildren =
m_hasAnyOrthogonalChildren || isOrthogonalChild(*child);

@@ -1997,12 +2000,9 @@ void LayoutGrid::populateExplicitGridAndOrderIterator() {
size_t maximumColumnIndex = GridPositionsResolver::explicitGridColumnCount(
*style(), m_autoRepeatColumns);

- ASSERT(m_gridItemsIndexesMap.isEmpty());
- size_t childIndex = 0;
for (LayoutBox* child = firstInFlowChildBox(); child;
child = child->nextInFlowSiblingBox()) {
populator.collectChild(child);
- m_gridItemsIndexesMap.set(child, childIndex++);

// This function bypasses the cache (cachedGridArea()) as it is used to
// build it.
Index: third_party/WebKit/Source/core/paint/GridPainter.cpp
diff --git a/third_party/WebKit/Source/core/paint/GridPainter.cpp b/third_party/WebKit/Source/core/paint/GridPainter.cpp
index a5bc679deb796aa8c847effa46cf3f1aaa5a6f86..066b901e33083de3ba4111804c5872735d21cdf7 100644
--- a/third_party/WebKit/Source/core/paint/GridPainter.cpp
+++ b/third_party/WebKit/Source/core/paint/GridPainter.cpp
@@ -36,18 +36,13 @@ static GridSpan dirtiedGridAreas(const Vector<LayoutUnit>& coordinates,
endGridAreaIndex + 1);
}

-class GridItemsSorter {
- public:
- bool operator()(const std::pair<LayoutBox*, size_t>& firstChild,
- const std::pair<LayoutBox*, size_t>& secondChild) const {
- if (firstChild.first->style()->order() !=
- secondChild.first->style()->order())
- return firstChild.first->style()->order() <
- secondChild.first->style()->order();
-
- return firstChild.second < secondChild.second;
- }
-};
+// Helper for the sorting of grid items following order-modified document order.
+// See http://www.w3.org/TR/css-flexbox/#order-modified-document-order
+static inline bool compareOrderModifiedDocumentOrder(
+ const std::pair<LayoutBox*, size_t>& firstItem,
+ const std::pair<LayoutBox*, size_t>& secondItem) {
+ return firstItem.second < secondItem.second;
+}

void GridPainter::paintChildren(const PaintInfo& paintInfo,
const LayoutPoint& paintOffset) {
@@ -103,10 +98,8 @@ void GridPainter::paintChildren(const PaintInfo& paintInfo,
std::make_pair(item, m_layoutGrid.paintIndexForGridItem(item)));
}

- // Sort grid items following order-modified document order.
- // See http://www.w3.org/TR/css-flexbox/#order-modified-document-order
std::stable_sort(gridItemsToBePainted.begin(), gridItemsToBePainted.end(),
- GridItemsSorter());
+ compareOrderModifiedDocumentOrder);

LayoutBox* previous = 0;
for (const auto& gridItemAndPaintIndex : gridItemsToBePainted) {

[cbies...@chromium.org]

lgtm

https://codereview.chromium.org/2438253003/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2438253003/1


https://codereview.chromium.org/2438253003/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

Try jobs failed on following builders:
linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/321718)

https://codereview.chromium.org/2438253003/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

Committed patchset #1 (id:1)

https://codereview.chromium.org/2438253003/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

Patchset 1 (id:??) landed as
https://crrev.com/bb7071018e1fd8bf223b8ffff660883b7d17278d
Cr-Commit-Position: refs/heads/master@{#426989}

https://codereview.chromium.org/2438253003/