Bug 130770 - [CSS Grid Layout] Prevent issues with checkLayout() in grid items
Summary: [CSS Grid Layout] Prevent issues with checkLayout() in grid items
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-26 01:27 PDT by Manuel Rego Casasnovas
Modified: 2014-03-26 04:12 PDT (History)
3 users (show)

See Also:


Attachments
Patch (22.45 KB, patch)
2014-03-26 01:34 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (4.58 KB, patch)
2014-03-26 03:12 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (4.62 KB, patch)
2014-03-26 03:40 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2014-03-26 01:27:33 PDT
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/
Comment 1 Manuel Rego Casasnovas 2014-03-26 01:34:37 PDT
Created attachment 227835 [details]
Patch
Comment 2 Sergio Villar Senin 2014-03-26 01:59:56 PDT
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?
Comment 3 Manuel Rego Casasnovas 2014-03-26 03:12:12 PDT
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.
Comment 4 Sergio Villar Senin 2014-03-26 03:27:51 PDT
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.
Comment 5 Manuel Rego Casasnovas 2014-03-26 03:40:18 PDT
Created attachment 227843 [details]
Patch

Patch for landing.
Comment 6 WebKit Commit Bot 2014-03-26 04:12:20 PDT
Comment on attachment 227843 [details]
Patch

Clearing flags on attachment: 227843

Committed r166290: <http://trac.webkit.org/changeset/166290>
Comment 7 WebKit Commit Bot 2014-03-26 04:12:23 PDT
All reviewed patches have been landed.  Closing bug.