Bug 157134

Created attachment 277615 [details] Patch

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.

(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 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 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 on attachment 277615 [details] Patch I think bugzilla erased smfr's r+ when I commented.

Created attachment 277643 [details] Patch

Created attachment 277647 [details] =Patch for landing

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 on attachment 277647 [details] =Patch for landing Clearing flags on attachment: 277647 Committed r200215: <http://trac.webkit.org/changeset/200215>

All reviewed patches have been landed. Closing bug.

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

Sure, that time is coming soon. We will remove both the build flag and the runtime switch when we reach that point.