Skip to content

Commit

Permalink
Revert of [css-grid] Remove a duplicated auto repeat computation for …
Browse files Browse the repository at this point in the history
…intrinsic sizes (patchset #4 id:60001 of https://codereview.chromium.org/2287533002/ )

Reason for revert: Caused crash on Mac. See http://crbug.com/662016

We're reverting only the code change as we want to keep the tests,
that will be fixed after the refactoring happening in bug #627812
is done.
So, we need to mark 2 tests as failure in TestExpectations file.

BUG=662016,666688,627812

Original issue's description:
> [css-grid] Remove a duplicated auto repeat computation for intrinsic sizes
>
> After crrev.com/414446 we can safely remove the temporary fix
> (crrev.com/405529) we had to avoid crashes and recursive calls when
> computing the auto repeat tracks count for grids with intrinsic sizes.
>
> This change unveiled a couple of bugs in the computation of auto repeat
> tracks for intrinsic sizes. The first one is that we were not considering the
> existence of definite minimum sizes. In those cases, instead of just returning 1
> repetition, we should return the minimum number of repetitions that fulfill the
> minimum size requirement.
>
> The second bug was that grids were not properly recomputing the number of
> auto repeat tracks when the min|max-content contributions of grid items
> changed (whenever the grid container had an intrinsic width).
>
> One Mozilla test had to be updated too because it was wrong.
> BUG=621517,633474
>
> Review-Url: https://codereview.chromium.org/2287533002
> Cr-Commit-Position: refs/heads/master@{#425958}

Review-Url: https://codereview.chromium.org/2510393002
Cr-Commit-Position: refs/heads/master@{#433183}
(cherry picked from commit b9e7d1d)

Review URL: https://codereview.chromium.org/2521573002 .

Cr-Commit-Position: refs/branch-heads/2924@{#24}
Cr-Branched-From: 3a87aec-refs/heads/master@{#433059}
  • Loading branch information
mrego committed Nov 21, 2016
1 parent e8e59dd commit ab661a1
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 23 deletions.
4 changes: 4 additions & 0 deletions third_party/WebKit/LayoutTests/TestExpectations
Original file line number Diff line number Diff line change
Expand Up @@ -1593,6 +1593,10 @@ crbug.com/657968 storage/indexeddb/idbdatabase-deleteObjectStore-exception-order
crbug.com/659610 fast/css-grid-layout/grid-baseline.html [ Failure ]
crbug.com/659610 fast/css-grid-layout/grid-baseline-margins.html [ Failure ]

# Due to a temporal revert these tests are failing:
crbug.com/666688 fast/css-grid-layout/grid-change-intrinsic-size-with-auto-repeat-tracks.html [ Failure ]
crbug.com/666688 fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-001.html [ Failure ]

# TODO(chrishall): this is a temporary mediation step as part of the P0 issue crbug.com/657646
# this is not meant to be here for more than a few days (from 2016-11-03 SYD)
crbug.com/657646 [ Win ] fast/text/font-weight.html [ Failure Pass ]
Expand Down
39 changes: 18 additions & 21 deletions third_party/WebKit/Source/core/layout/LayoutGrid.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -510,8 +510,14 @@ void LayoutGrid::layoutBlock(bool relayoutChildren) {

TextAutosizer::LayoutScope textAutosizerLayoutScope(this, &layoutScope);

updateAutoRepeatTracksAndSetDirtyIfNeeded(TrackSizing);
placeItemsOnGrid();
// TODO(svillar): we won't need to do this once the intrinsic width
// computation is isolated from the LayoutGrid object state (it should not
// touch any attribute) (see crbug.com/627812)
if (m_autoRepeatColumns &&
m_autoRepeatColumns !=
computeAutoRepeatTracksCount(ForColumns, TrackSizing))
dirtyGrid();
placeItemsOnGrid(TrackSizing);

GridSizingData sizingData(gridColumnCount(), gridRowCount());

Expand Down Expand Up @@ -687,9 +693,7 @@ LayoutUnit LayoutGrid::guttersSize(GridTrackSizingDirection direction,
void LayoutGrid::computeIntrinsicLogicalWidths(
LayoutUnit& minLogicalWidth,
LayoutUnit& maxLogicalWidth) const {
const_cast<LayoutGrid*>(this)->updateAutoRepeatTracksAndSetDirtyIfNeeded(
IntrinsicSizeComputation);
const_cast<LayoutGrid*>(this)->placeItemsOnGrid();
const_cast<LayoutGrid*>(this)->placeItemsOnGrid(IntrinsicSizeComputation);

GridSizingData sizingData(gridColumnCount(), gridRowCount());
computeTrackSizesForIndefiniteSize(ForColumns, sizingData, minLogicalWidth,
Expand Down Expand Up @@ -1827,21 +1831,6 @@ void LayoutGrid::insertItemIntoGrid(LayoutBox& child, const GridArea& area) {
}
}

void LayoutGrid::updateAutoRepeatTracksAndSetDirtyIfNeeded(
SizingOperation sizingOperation) {
size_t newAutoRepeatColumns =
computeAutoRepeatTracksCount(ForColumns, sizingOperation);
size_t newAutoRepeatRows =
computeAutoRepeatTracksCount(ForRows, sizingOperation);

if (m_autoRepeatColumns != newAutoRepeatColumns ||
m_autoRepeatRows != newAutoRepeatRows)
dirtyGrid();

m_autoRepeatColumns = newAutoRepeatColumns;
m_autoRepeatRows = newAutoRepeatRows;
}

size_t LayoutGrid::computeAutoRepeatTracksCount(
GridTrackSizingDirection direction,
SizingOperation sizingOperation) const {
Expand Down Expand Up @@ -1982,12 +1971,20 @@ LayoutGrid::computeEmptyTracksForAutoRepeat(
return emptyTrackIndexes;
}

void LayoutGrid::placeItemsOnGrid() {
void LayoutGrid::placeItemsOnGrid(SizingOperation sizingOperation) {
if (!m_gridIsDirty)
return;

DCHECK(m_gridItemArea.isEmpty());

if (sizingOperation == IntrinsicSizeComputation) {
m_autoRepeatColumns = styleRef().gridAutoRepeatColumns().size();
} else {
m_autoRepeatColumns =
computeAutoRepeatTracksCount(ForColumns, sizingOperation);
}
m_autoRepeatRows = computeAutoRepeatTracksCount(ForRows, sizingOperation);

populateExplicitGridAndOrderIterator();

// We clear the dirty bit here as the grid sizes have been updated.
Expand Down
3 changes: 1 addition & 2 deletions third_party/WebKit/Source/core/layout/LayoutGrid.h
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ class LayoutGrid final : public LayoutBlock {
void ensureGridSize(size_t maximumRowSize, size_t maximumColumnSize);
void insertItemIntoGrid(LayoutBox&, const GridArea&);

void updateAutoRepeatTracksAndSetDirtyIfNeeded(SizingOperation);
size_t computeAutoRepeatTracksCount(GridTrackSizingDirection,
SizingOperation) const;

Expand All @@ -154,7 +153,7 @@ class LayoutGrid final : public LayoutBlock {
bool isEmptyAutoRepeatTrack(GridTrackSizingDirection,
size_t lineNumber) const;

void placeItemsOnGrid();
void placeItemsOnGrid(SizingOperation);
void populateExplicitGridAndOrderIterator();
std::unique_ptr<GridArea> createEmptyGridAreaAtSpecifiedPositionsOutsideGrid(
const LayoutBox&,
Expand Down

0 comments on commit ab661a1

Please sign in to comment.