Bug 127055 – [CSS Grid Layout] Simplify grid-columns-rows-get-set{-multiple}.html tests

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sergio Villar Senin]

Both fast/css-grid-layout/resources/grid-columns-rows-get-set.js and fast/css-grid-layout/resources/grid-columns-rows-get-set-multiple.js (the helpers used by the two tests mentioned in the title) are full of boilerplate. Most of the code can be easily refactored in a couple of helper functions reducing the size of those files significantly.

This will also simplify the patch for the bug 127033, as I'll have to change the all the computed values to pixels.

[Sergio Villar Senin]

Created attachment 221280 [details]
Patch

[Sergio Villar Senin]

Created attachment 221379 [details]
Patch

Also refactored named-grid-line-get-set.html

[Sergio Villar Senin]

$ git show --shortstat
 9 files changed, 332 insertions(+), 692 deletions(-)

I'd say goal achieved :)

[Xabier Rodríguez Calvar]

Comment on attachment 221379 [details]
Patch

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

Without knowing too much about the subject, I suggested some cosmetic changes.

> LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt:39
> +PASS getComputedStyle(element, '').getPropertyValue('-webkit-grid-definition-columns') is "18px"

Isn't it possible to use ' instead of " for the output? Because either I am missing something or would not be polluting the diff with some of these changes.

> LayoutTests/fast/css-grid-layout/resources/grid-definitions-parsing-utils.js:9
> +function testGridDefinitionsSetJSValues(columnValue, rowValue, computedColumnValue, computedRowValue)

I would rework this three functions in a testDefinitionsSetJSValues taking a final parameter useGrid and checking for it to be undefined, considering for example true as default parameter.

[Sergio Villar Senin]

Comment on attachment 221379 [details]
Patch

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

Thanks for your comments.

>> LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set-expected.txt:39
>> +PASS getComputedStyle(element, '').getPropertyValue('-webkit-grid-definition-columns') is "18px"
> 
> Isn't it possible to use ' instead of " for the output? Because either I am missing something or would not be polluting the diff with some of these changes.

That's the direct output of JSON.stringify() (used by shouldBeEqualToString which gives us an additional type checking over the old code). The old code used shouldBe() that does not perform any kind of "stringifycation"

>> LayoutTests/fast/css-grid-layout/resources/grid-definitions-parsing-utils.js:9
>> +function testGridDefinitionsSetJSValues(columnValue, rowValue, computedColumnValue, computedRowValue)
> 
> I would rework this three functions in a testDefinitionsSetJSValues taking a final parameter useGrid and checking for it to be undefined, considering for example true as default parameter.

The reason why it cannot be done this way, it's because some of the parametters (like the computed values) are already optional.

[Sergio Villar Senin]

Committed r162346: <http://trac.webkit.org/changeset/162346>