Bug 127055

Summary: [CSS Grid Layout] Simplify grid-columns-rows-get-set{-multiple}.html tests
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: CSSAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: kling, koivisto, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 127057    
Attachments:
Description Flags
Patch
none
Patch kling: review+

Sergio Villar Senin
Reported 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.
Attachments
Patch (77.77 KB, patch)
2014-01-15 10:08 PST, Sergio Villar Senin 		no flags
DetailsFormatted DiffDiff
Patch (98.59 KB, patch)
2014-01-16 08:22 PST, Sergio Villar Senin 		kling: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1 2014-01-15 10:08:37 PST
Created attachment 221280 [details] Patch
Sergio Villar Senin
Comment 2 2014-01-16 08:22:44 PST
Created attachment 221379 [details] Patch Also refactored named-grid-line-get-set.html
Sergio Villar Senin
Comment 3 2014-01-16 08:24:09 PST
$ git show --shortstat 9 files changed, 332 insertions(+), 692 deletions(-) I'd say goal achieved :)
Xabier Rodríguez Calvar
Comment 4 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.
Sergio Villar Senin
Comment 5 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.
Sergio Villar Senin
Comment 6 2014-01-20 08:29:54 PST
Committed r162346: <http://trac.webkit.org/changeset/162346>
Note You need to log in before you can comment on or make changes to this bug.