Bug 67106 – Implement CSSPropertyWebkitColumns in CSSStyleApplyProperty.

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

[Luke Macpherson]

Implement CSSPropertyWebkitColumns in CSSStyleApplyProperty.

[Luke Macpherson]

Created attachment 105462 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

The thing to keep in mind is that the bar is higher for test coverage when we are doing a refactoring.

[Luke Macpherson]

Created attachment 105552 [details]
Patch

[Luke Macpherson]

Added tests for inherit and initial cases.

[Darin Adler]

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.

[WebKit Review Bot]

Comment on attachment 105552 [details]
Patch

Clearing flags on attachment: 105552

Committed r94037: <http://trac.webkit.org/changeset/94037>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Levin]

(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.

[Luke Macpherson]

Yeah, will do. I was surprised it got the cq+ with that request, which seemed a good idea to me.

[Renata Hodovan]

The new tests are failed on Qt Linux Release bot.
The bug is filed in:
https://bugs.webkit.org/show_bug.cgi?id=67188

[Darin Adler]

(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.

[Luke Macpherson]

Created attachment 106304 [details]
Patch

[Eric Seidel (no email)]

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).