Bug 67106

Summary: Implement CSSPropertyWebkitColumns in CSSStyleApplyProperty.
Product: WebKit Reporter: Luke Macpherson <macpherson>
Component: CSSAssignee: Luke Macpherson <macpherson>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, levin, macpherson, mitz, rhodovan.u-szeged, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 67188    
Bug Blocks: 67685    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Luke Macpherson 2011-08-28 23:25:03 PDT
Implement CSSPropertyWebkitColumns in CSSStyleApplyProperty.
Comment 1 Luke Macpherson 2011-08-28 23:28:38 PDT
Created attachment 105462 [details]
Patch
Comment 2 Darin Adler 2011-08-29 11:00:10 PDT
Comment on attachment 105462 [details]
Patch

Are there tests covering inheriting and initial for these properties? Since the new way of doing this is so different from the old way, I was wondering how we were checking that behavior is still the same?

If there aren’t tests, it seems it would be really straightforward to make them as we go.
Comment 3 Darin Adler 2011-08-29 11:00:34 PDT
The thing to keep in mind is that the bar is higher for test coverage when we are doing a refactoring.
Comment 4 Luke Macpherson 2011-08-29 17:29:43 PDT
Created attachment 105552 [details]
Patch
Comment 5 Luke Macpherson 2011-08-29 17:30:27 PDT
Added tests for inherit and initial cases.
Comment 6 Darin Adler 2011-08-29 17:45:16 PDT
Comment on attachment 105552 [details]
Patch

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

> LayoutTests/fast/multicol/initial-column-values.html:8
> +<div style="-webkit-columns: initial;">

This test would be even better if it used computed style and dumpAsText so it work without platform-specific results.
Comment 7 WebKit Review Bot 2011-08-29 18:48:16 PDT
Comment on attachment 105552 [details]
Patch

Clearing flags on attachment: 105552

Committed r94037: <http://trac.webkit.org/changeset/94037>
Comment 8 WebKit Review Bot 2011-08-29 18:48:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 David Levin 2011-08-29 22:31:44 PDT
(In reply to comment #6)
> (From update of attachment 105552 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105552&action=review
> 
> > LayoutTests/fast/multicol/initial-column-values.html:8
> > +<div style="-webkit-columns: initial;">
> 
> This test would be even better if it used computed style and dumpAsText so it work without platform-specific results.

Luke, please consider this strongly. Platform specific results are a huge maintenance burden on the project.
Comment 10 Luke Macpherson 2011-08-29 23:06:41 PDT
Yeah, will do. I was surprised it got the cq+ with that request, which seemed a good idea to me.
Comment 11 Renata Hodovan 2011-08-30 02:30:27 PDT
The new tests are failed on Qt Linux Release bot.
The bug is filed in:
https://bugs.webkit.org/show_bug.cgi?id=67188
Comment 12 Darin Adler 2011-08-30 07:50:18 PDT
(In reply to comment #10)
> Yeah, will do. I was surprised it got the cq+ with that request, which seemed a good idea to me.

You are more than welcome to follow up by fixing the test.
Comment 13 Luke Macpherson 2011-09-04 21:53:32 PDT
Created attachment 106304 [details]
Patch
Comment 14 Eric Seidel (no email) 2011-09-06 15:45:13 PDT
Comment on attachment 106304 [details]
Patch

Cleared review? from attachment 106304 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).