Add a runtime flag for the column-fill property.

Add a runtime flag for the column-fill property.

Decouple column-fill support from the RegionBasedColumns flag.

The new multicol implementation is about to ship. All other multicol
properties are -webkit- prefixed, except for column-fill. Allowing
such a mix would very likely confuse authors, so better hide
column-fill away until we can unprefix all other multicol properties.

If column-fill isn't supported, the layout engine will behave as if it
were set to 'auto', which means that a multicol container with a
specified height won't have its columns balanced.

Discussed here:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/hhecvWUBJvg/nq4X3s8Jmk4J

R=jchaffraix@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195873

[Julien - ping for review, 2015-05-20 19:01:22]

jchaffraix@chromium.org changed reviewers:
+ esprehn@chromium.org

[Julien - ping for review, 2015-05-20 19:01:23]

I am concerned about the initial value change.

FWIW I was on board with either proposals (shipping the unprefixed column-fill
or adding this flag).

https://codereview.chromium.org/1145093002/diff/1/Source/core/style/ComputedS...
File Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1145093002/diff/1/Source/core/style/ComputedS...
Source/core/style/ComputedStyle.cpp:1646: if
(RuntimeEnabledFeatures::columnFillEnabled())
That means that we will check this flag for every ComputedStyle we create and
introduce a branch there.

https://codereview.chromium.org/1145093002/diff/1/Source/core/style/ComputedS...
Source/core/style/ComputedStyle.cpp:1652: // implementation, which has no
support for this property.
That comment is weird. The specification seems to have said to use 'balance' as
the initial value since 2011. Is that a bug in our old implementation?

https://codereview.chromium.org/1145093002/diff/1/Source/platform/RuntimeEnab...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1145093002/diff/1/Source/platform/RuntimeEnab...
Source/platform/RuntimeEnabledFeatures.in:32: ColumnFill status=experimental
The ColumnFill flag seems to me like it should be UnprefixMultiCol to make it
clear what the intent is. On top of it, we will probably want to rename it once
we start the unprefixing effort (we don't want 2 flags for that).

[mstensho (USE GERRIT), 2015-05-20 19:30:42]

No new patch, just comments. I want to settle this matter first. Should I:

A: Drop this patch, and ship what after all has been given a sufficient amount
of l-g-t-m?

B: Go with your UnprefixMultiCol flag suggestion?

C: Just back out the bad ComputedStyle change and do it in a better way?

Option A is going to cause some more mess and paperwork for us Blink developers.
The two others may confuse web developers until everything is unprefixed. More
paperwork for us is a negligible cost if it keeps web developers happy, of
course.

https://codereview.chromium.org/1145093002/diff/1/Source/core/style/ComputedS...
File Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1145093002/diff/1/Source/core/style/ComputedS...
Source/core/style/ComputedStyle.cpp:1646: if
(RuntimeEnabledFeatures::columnFillEnabled())
On 2015/05/20 19:01:22, Julien Chaffraix - PST wrote:
> That means that we will check this flag for every ComputedStyle we create and
> introduce a branch there.

Ouch, that's a good point. I don't wanna do it this way then.

I could fix it at a less hot spot in the layout engine instead.

https://codereview.chromium.org/1145093002/diff/1/Source/core/style/ComputedS...
Source/core/style/ComputedStyle.cpp:1652: // implementation, which has no
support for this property.
On 2015/05/20 19:01:22, Julien Chaffraix - PST wrote:
> That comment is weird. The specification seems to have said to use 'balance'
as
> the initial value since 2011. Is that a bug in our old implementation?

Well, a "bug" (or rather missing feature) in the old implementation, where
column-fill wasn't supported. The old implementation balances auto-height
multicol containers (what else could you do anyway), and doesn't balance
non-auto-height multicol containers. In other words, as if column-fill were
'auto'.

https://codereview.chromium.org/1145093002/diff/1/Source/platform/RuntimeEnab...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1145093002/diff/1/Source/platform/RuntimeEnab...
Source/platform/RuntimeEnabledFeatures.in:32: ColumnFill status=experimental
On 2015/05/20 19:01:23, Julien Chaffraix - PST wrote:
> The ColumnFill flag seems to me like it should be UnprefixMultiCol to make it
> clear what the intent is. On top of it, we will probably want to rename it
once
> we start the unprefixing effort (we don't want 2 flags for that).

I was planning to do unprefixing separately without flags. When we're ready to
unprefix, we can flip this ColumnFill flag to 'stable', and quickly unprefix
afterwards, before anyone gets confused. :) It seems weird to introduce support
for a brand new CSS property just because we unprefix, which is one of the
reasons why I decided against lumping the unprefixing stuff behind this flag.

It's been a while since I looked at the CSS property code, property alias stuff,
etc. Is it at all possible to unprefix using a flag?

[esprehn, 2015-05-20 20:26:55]

https://codereview.chromium.org/1145093002/diff/1/Source/core/style/ComputedS...
File Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1145093002/diff/1/Source/core/style/ComputedS...
Source/core/style/ComputedStyle.cpp:1652: // implementation, which has no
support for this property.
This is the wrong place to fix this, you should fix it in the getter that
getComputedStyle uses, not here.

[mstensho (USE GERRIT), 2015-05-21 07:08:55]

Correction to my confusing alphabet soup comment:

---

No new patch, just comments. I want to settle this matter first. Should I:

A: Drop this patch, and ship what after all has been given a sufficient amount
of l-g-t-m?

B: Go with your UnprefixMultiCol flag suggestion?

C: Just back out the bad ComputedStyle change and do it in a better way?

Option B and C are going to cause some more code mess and paperwork for us Blink
developers. Option A may confuse web developers until everything is unprefixed.
More paperwork for us is a  negligible cost if it keeps web developers happy, of
course.


On 2015/05/20 19:30:42, mstensho wrote:
> No new patch, just comments. I want to settle this matter first. Should I:
> 
> A: Drop this patch, and ship what after all has been given a sufficient amount
> of l-g-t-m?
> 
> B: Go with your UnprefixMultiCol flag suggestion?
> 
> C: Just back out the bad ComputedStyle change and do it in a better way?
> 
> Option A is going to cause some more mess and paperwork for us Blink
developers.
> The two others may confuse web developers until everything is unprefixed. More
> paperwork for us is a negligible cost if it keeps web developers happy, of
> course.

[Julien - ping for review, 2015-05-21 14:12:15]

On 2015/05/20 at 19:30:42, mstensho wrote:
> No new patch, just comments. I want to settle this matter first. Should I:
> 
> A: Drop this patch, and ship what after all has been given a sufficient amount
of l-g-t-m?
> 
> B: Go with your UnprefixMultiCol flag suggestion?
> 
> C: Just back out the bad ComputedStyle change and do it in a better way?
> 
> Option A is going to cause some more mess and paperwork for us Blink
developers. The two others may confuse web developers until everything is
unprefixed. More paperwork for us is a negligible cost if it keeps web
developers happy, of course.

I like option B as it doesn't confuse the web-authors (but fixing the extra
branching so it's kinda C too). It's also not a lot of paperwork for us.

I could totally live with A FWIW if we can't find a good way to make B work.

[Julien - ping for review, 2015-05-21 14:12:47]

https://codereview.chromium.org/1145093002/diff/1/Source/core/style/ComputedS...
File Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1145093002/diff/1/Source/core/style/ComputedS...
Source/core/style/ComputedStyle.cpp:1652: // implementation, which has no
support for this property.
On 2015/05/20 20:26:55, esprehn wrote:
> This is the wrong place to fix this, you should fix it in the getter that
> getComputedStyle uses, not here.

I don't agree, that's a super gross approach. It's a lot better to handle this
in the layout code (which I think is what Morten offered).

[mstensho (USE GERRIT), 2015-05-21 18:47:41]

On 2015/05/21 14:12:15, Julien Chaffraix - PST wrote:
> On 2015/05/20 at 19:30:42, mstensho wrote:
> > No new patch, just comments. I want to settle this matter first. Should I:
> > 
> > A: Drop this patch, and ship what after all has been given a sufficient
amount
> of l-g-t-m?
> > 
> > B: Go with your UnprefixMultiCol flag suggestion?
> > 
> > C: Just back out the bad ComputedStyle change and do it in a better way?
> > 
> > Option A is going to cause some more mess and paperwork for us Blink
> developers. The two others may confuse web developers until everything is
> unprefixed. More paperwork for us is a negligible cost if it keeps web
> developers happy, of course.
> 
> I like option B as it doesn't confuse the web-authors (but fixing the extra
> branching so it's kinda C too). It's also not a lot of paperwork for us.
> 
> I could totally live with A FWIW if we can't find a good way to make B work.

I'll try B. I meant B to also include C. Okay, enough confusing letters from me.
I'll start coding instead. :)

[mstensho (USE GERRIT), 2015-05-22 08:06:41]

New patch. Please have another look.

[mstensho (USE GERRIT), 2015-05-22 13:38:53]

On 2015/05/22 08:06:41, mstensho wrote:
> New patch. Please have another look.

I'm not sure about the UnprefixMulticol flag, though. Seems that I need to do a
lot of work to add support for unprefixing the existing multicol properties
behind a flag. Take a look at https://codereview.chromium.org/1150313002/ to see
what I mean. So maybe just name the flag ColumnFill after all, and do the
unprefixing when the implementation is good enough (without using runtime
flags)?

[Julien - ping for review, 2015-05-22 22:08:51]

On 2015/05/22 at 13:38:53, mstensho wrote:
> On 2015/05/22 08:06:41, mstensho wrote:
> > New patch. Please have another look.
> 
> I'm not sure about the UnprefixMulticol flag, though. Seems that I need to do
a lot of work to add support for unprefixing the existing multicol properties
behind a flag. Take a look at https://codereview.chromium.org/1150313002/ to see
what I mean. So maybe just name the flag ColumnFill after all, and do the
unprefixing when the implementation is good enough (without using runtime
flags)?

I thought we would gate all the unprefixed properties with the same flag
(UnprefixMulticol) so we could disable them if we had some compatibility issue.
If that's not the case, then it's better with a separate flag but I am concerned
about the de-coupling with unprefixing.

To be frank, it doesn't matter much what we pick up so you don't need to ask me
about it, just select one sensible option and let's stick to that.

[Julien - ping for review, 2015-05-22 22:09:18]

lgtm

https://codereview.chromium.org/1145093002/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutMultiColumnSet.cpp (right):

https://codereview.chromium.org/1145093002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutMultiColumnSet.cpp:141: if
(RuntimeEnabledFeatures::unprefixMulticolEnabled()) {
Comment about why we do that and what the behavior is, por favor! The comment
from the previous iteration was great.

[mstensho (USE GERRIT), 2015-05-25 18:47:20]

Renamed the flag back to ColumnFill, and landing.

https://codereview.chromium.org/1145093002/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutMultiColumnSet.cpp (right):

https://codereview.chromium.org/1145093002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutMultiColumnSet.cpp:141: if
(RuntimeEnabledFeatures::unprefixMulticolEnabled()) {
On 2015/05/22 22:09:18, Julien Chaffraix - PST wrote:
> Comment about why we do that and what the behavior is, por favor! The comment
> from the previous iteration was great.

Done.

[mstensho (USE GERRIT), 2015-05-25 18:47:24]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2015-05-25 18:47:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
jchaffraix@chromium.org
Link to the patchset: https://codereview.chromium.org/1145093002/#ps40001
(title: "Name the runtime flag ColumnFill after all, since that's all it's going
to control")

[commit-bot: I haz the power, 2015-05-25 18:47:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1145093002/40001

[commit-bot: I haz the power, 2015-05-25 20:01:15]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195873