New issue
Advanced search Search tips

Issue 666688 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 627812

Blocking:
issue 79180



Sign in to add a comment

[css-grid] Some tests are failing after the revert of r425958

Project Member Reported by r...@igalia.com, Nov 18 2016

Issue description


Because of bug #662016, we're reverting r425958:
https://codereview.chromium.org/2287533002/

But we decided to just revert the code and keep the tests,
as they're right.
So we're marking a few tests are failing until the refactoring
performed in  bug #627812  is completed.

The failing tests are:
* fast/css-grid-layout/grid-change-intrinsic-size-with-auto-repeat-tracks.html
* fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-001.html

Then we should re-apply r425958 and then verify that the tests are passing.
Don't forget to remove them from LayoutTests/TestExpectations.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64

commit b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64
Author: rego <rego@igalia.com>
Date: Fri Nov 18 12:31:52 2016

Revert of [css-grid] Remove a duplicated auto repeat computation for 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}

[modify] https://crrev.com/b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64/third_party/WebKit/Source/core/layout/LayoutGrid.h

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 21 2016

Labels: merge-merged-2924
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ab661a19616ff4cd7ff522cef71354971c4d5694

commit ab661a19616ff4cd7ff522cef71354971c4d5694
Author: Manuel Rego Casasnovas <rego@igalia.com>
Date: Mon Nov 21 11:38:41 2016

Revert of [css-grid] Remove a duplicated auto repeat computation for 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 b9e7d1d1cc0bcef66b23b320d5c645c97bf4fc64)

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

Cr-Commit-Position: refs/branch-heads/2924@{#24}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/ab661a19616ff4cd7ff522cef71354971c4d5694/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/ab661a19616ff4cd7ff522cef71354971c4d5694/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/ab661a19616ff4cd7ff522cef71354971c4d5694/third_party/WebKit/Source/core/layout/LayoutGrid.h

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b34fbbccb46eae5f12faec3ea00ed3c3bfb2692b

commit b34fbbccb46eae5f12faec3ea00ed3c3bfb2692b
Author: svillar <svillar@igalia.com>
Date: Tue Dec 27 15:12:58 2016

[css-grid] Isolate intrinsic size computation from layout

This is the last patch of the items placement data refactoring. By using
a different Grid instance in computeIntrinsicLogicalWidths we effectively
isolate the intrinsic size computation from the layout. They are now using
different data structures so they don't interfere each other.

This also means that we no longer reuse the placement of items done in the
intrinsic size computation. That shouldn't be a big issue because we do save
the position of items between layouts.

Last but not least, this patch finally removes the ugly const_cast's we had
in computeIntrinsicLogicalWidths() as we no longer modify the internal state
of LayoutGrid.

BUG= 627812 , 666688 

Review-Url: https://codereview.chromium.org/2589143002
Cr-Commit-Position: refs/heads/master@{#440755}

[modify] https://crrev.com/b34fbbccb46eae5f12faec3ea00ed3c3bfb2692b/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/b34fbbccb46eae5f12faec3ea00ed3c3bfb2692b/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-change-intrinsic-size-with-auto-repeat-tracks.html
[modify] https://crrev.com/b34fbbccb46eae5f12faec3ea00ed3c3bfb2692b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/b34fbbccb46eae5f12faec3ea00ed3c3bfb2692b/third_party/WebKit/Source/core/layout/LayoutGrid.h

Comment 4 by r...@igalia.com, Jan 9 2017

Status: Fixed (was: Available)

Sign in to add a comment