Bug 157134 – [css-grid] Add CSS Grid Layout runtime flag

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Manuel Rego Casasnovas]

As announced on the mailing list, we're adding a runtime flag to CSS Grid Layout.

In a next patch we'll be removing the prefixes.

More info at: https://lists.webkit.org/pipermail/webkit-dev/2016-April/028192.html

[Manuel Rego Casasnovas]

Created attachment 277615 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 277615 [details]
Patch

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

I was tempted to r+ it but I'll let a third party do it instead.

> Source/WebCore/ChangeLog:14
> +        Test: fast/css-grid-layout/grid-disable.html

Can't you use fast/css/getComputedStyle/computed-style.html for adding the new tests?

> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:116
> +#if ENABLE(CSS_GRID_LAYOUT)

We could remove the build flag in the same patch. Not a strong opinion though.

> Source/WebCore/css/CSSParser.cpp:2801
> +        if (!cssGridLayoutEnabled())

isCSSGridLayoutEnabled() sounds better for me but I concede that you could find examples of both in the current code.

[Manuel Rego Casasnovas]

(In reply to comment #2)
> I was tempted to r+ it but I'll let a third party do it instead.

Yeah it'd be nice if someone else can take a look.
I'm adding more reviewers on CC.

> > Source/WebCore/ChangeLog:14
> > +        Test: fast/css-grid-layout/grid-disable.html
> 
> Can't you use fast/css/getComputedStyle/computed-style.html for adding the
> new tests?

I don't get what you mean, that test is not disabling any runtime feature like I'm doing.
The goal of my test is to check that we can disable grid layout turning of the runtime flag.

> > Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:116
> > +#if ENABLE(CSS_GRID_LAYOUT)
> 
> We could remove the build flag in the same patch. Not a strong opinion
> though.

Dunno, I think I prefer to do it in separated steps.

> > Source/WebCore/css/CSSParser.cpp:2801
> > +        if (!cssGridLayoutEnabled())
> 
> isCSSGridLayoutEnabled() sounds better for me but I concede that you could
> find examples of both in the current code.

The other CSS runtime flags in this file use this format, so I'm for keeping it but I don't have strong opinion.

[Simon Fraser (smfr)]

Comment on attachment 277615 [details]
Patch

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

What about CSS OM stuff?

> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:246
> +    bool cssGridLayoutEnabled() const { return m_isCSSGridLayoutEnabled; }

I would call this isCSSGridLayoutEnabled()

> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:363
> +    bool m_isCSSGridLayoutEnabled;

I would call this m_cssGridLayoutEnabled

[Dean Jackson]

Comment on attachment 277615 [details]
Patch

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

>> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:116
>> +#if ENABLE(CSS_GRID_LAYOUT)
> 
> We could remove the build flag in the same patch. Not a strong opinion though.

No, please keep the build flag for now. For experimental features we want both a build flag and a runtime switch. That way if a shipping browser wants to disable it, they don't need to even build the code (reducing binary size).

> Source/WebKit2/Shared/WebPreferencesDefinitions.h:233
> +    macro(CSSGridLayoutEnabled, cssGridLayoutEnabled, Bool, bool, true, "", "") \

I think this is a good case of something we could put in the new Experimental Features section. This would allow a browser to query WebKit to dynamically build a menu of features to enable and disable.

[Dean Jackson]

Comment on attachment 277615 [details]
Patch

I think bugzilla erased smfr's r+ when I commented.

[Manuel Rego Casasnovas]

Created attachment 277643 [details]
Patch

[Manuel Rego Casasnovas]

Created attachment 277647 [details]
=Patch for landing

[Manuel Rego Casasnovas]

Thanks for the quick reviews!

(In reply to comment #4)
> Comment on attachment 277615 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277615&action=review
> 
> What about CSS OM stuff?

This is an old issue that happens for all the runtime flags (see bug #128271).

> > Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:246
> > +    bool cssGridLayoutEnabled() const { return m_isCSSGridLayoutEnabled; }
> 
> I would call this isCSSGridLayoutEnabled()
> 
> > Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:363
> > +    bool m_isCSSGridLayoutEnabled;
> 
> I would call this m_cssGridLayoutEnabled

I've changed this and also the other similar methods like in CSSParser as Sergio suggested before.


(In reply to comment #5)
> Comment on attachment 277615 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277615&action=review
> 
> >> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:116
> >> +#if ENABLE(CSS_GRID_LAYOUT)
> > 
> > We could remove the build flag in the same patch. Not a strong opinion though.
> 
> No, please keep the build flag for now. For experimental features we want
> both a build flag and a runtime switch. That way if a shipping browser wants
> to disable it, they don't need to even build the code (reducing binary size).

Good reason, we'll keep it.
Once CSS Grid Layout is shipped by default and the implementation is stable enough we could get rid of the build flag.

> > Source/WebKit2/Shared/WebPreferencesDefinitions.h:233
> > +    macro(CSSGridLayoutEnabled, cssGridLayoutEnabled, Bool, bool, true, "", "") \
> 
> I think this is a good case of something we could put in the new
> Experimental Features section. This would allow a browser to query WebKit to
> dynamically build a menu of features to enable and disable.

Moved to the experimental features section.

[WebKit Commit Bot]

Comment on attachment 277647 [details]
=Patch for landing

Clearing flags on attachment: 277647

Committed r200215: <http://trac.webkit.org/changeset/200215>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sergio Villar Senin]

(In reply to comment #5)
> Comment on attachment 277615 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277615&action=review
> 
> >> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:116
> >> +#if ENABLE(CSS_GRID_LAYOUT)
> > 
> > We could remove the build flag in the same patch. Not a strong opinion though.
> 
> No, please keep the build flag for now. For experimental features we want
> both a build flag and a runtime switch. That way if a shipping browser wants
> to disable it, they don't need to even build the code (reducing binary size).

I have to disagree with this. I concede is experimental in the sense that we are changing it constantly, but that's just because the specs are changing like mad (even thought they're supposed to be quite close the CR).

My point is that CSS Grid Layout will be a basic feature of every single web engine, so disabling it would be like disabling tables, something we definitely do not want to permit.

[Darin Adler]

Sure, that time is coming soon.

We will remove both the build flag and the runtime switch when we reach that point.