Bug 127055 - [CSS Grid Layout] Simplify grid-columns-rows-get-set{-multiple}.html tests
Summary: [CSS Grid Layout] Simplify grid-columns-rows-get-set{-multiple}.html tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on:
Blocks: 127057
  Show dependency treegraph
 
Reported: 2014-01-15 10:02 PST by Sergio Villar Senin
Modified: 2014-01-20 08:29 PST (History)
3 users (show)

See Also:


Attachments
Patch (77.77 KB, patch)
2014-01-15 10:08 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (98.59 KB, patch)
2014-01-16 08:22 PST, Sergio Villar Senin
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2014-01-15 10:02:39 PST
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.
Comment 1 Sergio Villar Senin 2014-01-15 10:08:37 PST
Created attachment 221280 [details]
Patch
Comment 2 Sergio Villar Senin 2014-01-16 08:22:44 PST
Created attachment 221379 [details]
Patch

Also refactored named-grid-line-get-set.html
Comment 3 Sergio Villar Senin 2014-01-16 08:24:09 PST
$ git show --shortstat
 9 files changed, 332 insertions(+), 692 deletions(-)

I'd say goal achieved :)
Comment 4 Xabier Rodríguez Calvar 2014-01-20 03:31:56 PST
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.
Comment 5 Sergio Villar Senin 2014-01-20 03:50:22 PST
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.
Comment 6 Sergio Villar Senin 2014-01-20 08:29:54 PST
Committed r162346: <http://trac.webkit.org/changeset/162346>