Bug 130013 - [CSS Grid Layout] Enable runtime feature if feature flag is enabled
Summary: [CSS Grid Layout] Enable runtime feature if feature flag is enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-10 03:28 PDT by Manuel Rego Casasnovas
Modified: 2014-04-01 14:57 PDT (History)
8 users (show)

See Also:

Attachments
Patch (2.04 KB, patch)
2014-03-10 03:31 PDT, Manuel Rego Casasnovas 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (468.32 KB, application/zip)
2014-03-10 04:34 PDT, Build Bot 		no flags Details
Patch (4.30 KB, patch)
2014-03-10 05:15 PDT, Manuel Rego Casasnovas 		no flags Details | Formatted Diff | Diff
Patch (5.92 KB, patch)
2014-03-11 03:39 PDT, Manuel Rego Casasnovas 		no flags Details | Formatted Diff | Diff
Patch (7.69 KB, patch)
2014-03-17 13:49 PDT, Manuel Rego Casasnovas 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2014-03-10 03:28:24 PDT 
Currently in development builds ENABLE_CSS_GRID_LAYOUT is true, however the runtime feature is still false so you cannot test it without doing a manual build.

It would be nice to enable the runtime flag when ENABLE_CSS_GRID_LAYOUT is true, otherwise it doesn't have too much sense to compile the core related to CSS Grid Layout if  you're not going to be able to use it.
Comment 1 Manuel Rego Casasnovas 2014-03-10 03:31:39 PDT 
Created attachment 226292 [details]
Patch
Comment 2 Build Bot 2014-03-10 04:34:18 PDT 
Comment on attachment 226292 [details]
Patch

Attachment 226292 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5520739989979136

New failing tests:
fast/css-grid-layout/grid-disabled-by-default.html
Comment 3 Build Bot 2014-03-10 04:34:20 PDT 
Created attachment 226294 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 4 Manuel Rego Casasnovas 2014-03-10 05:15:51 PDT 
Created attachment 226299 [details]
Patch

Removing fast/css-grid-layout/grid-disabled-by-default.html as it fails with this patch.
Comment 5 Manuel Rego Casasnovas 2014-03-11 03:39:56 PDT 
Created attachment 226413 [details]
Patch

Finally keeping layout test but checking that the runtime feature can be disabled.
Comment 6 Simon Fraser (smfr) 2014-03-17 13:22:02 PDT 
Comment on attachment 226413 [details]
Patch

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

> Source/WebKit2/Shared/WebPreferencesStore.h:105
> +#if ENABLE(CSS_GRID_LAYOUT)
> +#define DEFAULT_CSS_GRID_LAYOUT_ENABLED true
> +#else
> +#define DEFAULT_CSS_GRID_LAYOUT_ENABLED false
> +#endif

This is only changing WebKit2. What about WebKit1?
Comment 7 Manuel Rego Casasnovas 2014-03-17 13:49:28 PDT 
Created attachment 226957 [details]
Patch

Changing WK1 code too.
Comment 8 Manuel Rego Casasnovas 2014-03-17 13:52:01 PDT 
(In reply to comment #7)
> Created an attachment (id=226957) [details]
> Patch
> 
> Changing WK1 code too.

Anyway, maybe it would be a better idea to make the runtime feature true directly in WebCore/page/Settings.in.

So if you compile with ENBALE_CSS_GRID_LAYOUT the feature will be enabled by default (and you have still the chance to disable the runtime feature if needed). And if you don't compile with ENBALE_CSS_GRID_LAYOUT the feature won't work as expected.

If you prefer this kind of change, I can change the patch and do something like that. What do you think?
Comment 9 WebKit Commit Bot 2014-04-01 14:57:38 PDT 
Comment on attachment 226957 [details]
Patch

Clearing flags on attachment: 226957

Committed r166614: <http://trac.webkit.org/changeset/166614>
Comment 10 WebKit Commit Bot 2014-04-01 14:57:43 PDT 
All reviewed patches have been landed.  Closing bug.