Bug 130770 – [CSS Grid Layout] Prevent issues with checkLayout() in grid items

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

When we use checkLayout() on a grid item, the results (PASS or FAIL messages) are added as new auto-placed items in the grid.

This might cause some unexpected behavior in the tests. In the current code all of them are passing anyway, but depending on the specific test some problems could arise.

It seems a better idea to modify the related layout tests to use a different container for the test results. Avoiding any kind of interference with the grid code.

This has been already fixed in Blink: https://codereview.chromium.org/208133003/

[Manuel Rego Casasnovas]

Created attachment 227835 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 227835 [details]
Patch

There is an issue in the tests indeed, but moving the output to a single place worsens the expectations as it becomes more difficult to know which tests could be failing. Could it be possible to fix the issue without changing the expectations?

[Manuel Rego Casasnovas]

Created attachment 227842 [details]
Patch

I modified the patch to keep the same expectations. On top of that I realized that the the patch contained some uneeded changes, becaues of checkLayout for a grid is not a problem as results will be added to the parent and not the grid itself.

[Sergio Villar Senin]

Comment on attachment 227842 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227842&action=review

Much better indeed.

> LayoutTests/ChangeLog:18
> +

Nit: could you please try to keep columns <80 before landing? Readability of the ChangeLog is improved a lot.

[Manuel Rego Casasnovas]

Created attachment 227843 [details]
Patch

Patch for landing.

[WebKit Commit Bot]

Comment on attachment 227843 [details]
Patch

Clearing flags on attachment: 227843

Committed r166290: <http://trac.webkit.org/changeset/166290>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.