Bug 157134 - [css-grid] Add CSS Grid Layout runtime flag
Summary: [css-grid] Add CSS Grid Layout runtime flag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on:
Blocks: 60731 157137
  Show dependency treegraph
 
Reported: 2016-04-28 05:46 PDT by Manuel Rego Casasnovas
Modified: 2016-04-29 22:15 PDT (History)
7 users (show)

See Also:


Attachments
Patch (32.25 KB, patch)
2016-04-28 06:18 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (32.37 KB, patch)
2016-04-28 13:10 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
=Patch for landing (32.37 KB, patch)
2016-04-28 14:01 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2016-04-28 05:46:07 PDT
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
Comment 1 Manuel Rego Casasnovas 2016-04-28 06:18:41 PDT
Created attachment 277615 [details]
Patch
Comment 2 Sergio Villar Senin 2016-04-28 07:42:25 PDT
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.
Comment 3 Manuel Rego Casasnovas 2016-04-28 12:12:22 PDT
(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.
Comment 4 Simon Fraser (smfr) 2016-04-28 12:20:43 PDT
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
Comment 5 Dean Jackson 2016-04-28 12:27:02 PDT
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.
Comment 6 Dean Jackson 2016-04-28 12:41:50 PDT
Comment on attachment 277615 [details]
Patch

I think bugzilla erased smfr's r+ when I commented.
Comment 7 Manuel Rego Casasnovas 2016-04-28 13:10:23 PDT
Created attachment 277643 [details]
Patch
Comment 8 Manuel Rego Casasnovas 2016-04-28 14:01:14 PDT
Created attachment 277647 [details]
=Patch for landing
Comment 9 Manuel Rego Casasnovas 2016-04-28 14:01:54 PDT
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.
Comment 10 WebKit Commit Bot 2016-04-28 16:13:30 PDT
Comment on attachment 277647 [details]
=Patch for landing

Clearing flags on attachment: 277647

Committed r200215: <http://trac.webkit.org/changeset/200215>
Comment 11 WebKit Commit Bot 2016-04-28 16:13:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Sergio Villar Senin 2016-04-29 00:36:45 PDT
(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.
Comment 13 Darin Adler 2016-04-29 22:15:20 PDT
Sure, that time is coming soon.

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