Bug 78525

Summary: CSS regions enabled by default
Product: WebKit Reporter: James Robinson <jamesr>
Component: CSSAssignee: Mihnea Ovidenie <mihnea>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, fishd, hyatt, mihnea, paulirish, peter, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 78960    
Bug Blocks: 57312    
Attachments:
Description Flags
Patch
none
Patch 2 none

Description James Robinson 2012-02-13 13:42:31 PST
CSS regions are enabled by default today, but we aren't quite ready to be exposing these to the web yet in shipping browsers (at least in Chromium).  I think we should add guards back for now while the feature is still under active development and the CSSWG is debating how this area will look.

Options:

1.) #ifdef ENABLE(). Upside: Easy, works in all platforms. Downside: this prevents the bots from running the tests by default, far more annoying for developers, greater risk of compile breakage from developers not working directly on the feature.

2.) Runtime preference. Upside: Fairly easy, allows for feature to by on in DumpRenderTree and off in shipping browsers and allows for developers working on the feature to simply flip a flag to test without requiring rebuilds. Prevents compile breakages from developers not working directly on the feature. Downside: No way to have runtime guards for the JSC bindings.
Comment 1 Mihnea Ovidenie 2012-02-14 02:04:19 PST
I would prefer to use a runtime flag for the feature instead of a compile time flag.
Comment 2 James Robinson 2012-02-14 10:49:56 PST
I agree that would be more convenient.  In IRC Dave Hyatt agreed that a runtime guard is probably the best way to go here.

Do you have some cycles to try to make this happen, Mihnea?  I probably won't be able to take this on this week.  In order to make the next Chromium release train we would want something in place in the next 3 weeks.
Comment 3 Mihnea Ovidenie 2012-02-15 01:31:04 PST
Working on it.
Comment 4 Mihnea Ovidenie 2012-02-16 10:06:06 PST
Created attachment 127400 [details]
Patch

First version of patch. It disables css regions functionality at runtime. The preference key is "WebKitCSSRegionsEnabled".
In DRT, use layoutTestController.overridePreference("WebKitCSSRegionsEnabled", "1") to enable the css regions functionality.
The css regions tests in fast/repaint will be moved to fast/regions in another patch.
The css regions tests in fast/regions are skipped and will be enabled in another patch.
Comment 5 Mihnea Ovidenie 2012-02-17 07:04:56 PST
Created attachment 127584 [details]
Patch 2

Another patch, this time the css regions are enabled by default.
Comment 6 Dave Hyatt 2012-02-17 10:44:20 PST
Comment on attachment 127584 [details]
Patch 2

r=me
Comment 7 WebKit Review Bot 2012-02-17 12:44:19 PST
Comment on attachment 127584 [details]
Patch 2

Clearing flags on attachment: 127584

Committed r108108: <http://trac.webkit.org/changeset/108108>
Comment 8 WebKit Review Bot 2012-02-17 12:44:24 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Peter Beverloo 2012-02-20 09:29:54 PST
(In reply to comment #8)
> All reviewed patches have been landed.  Closing bug.

Toggling regions during run-time will disable working of the CSS properties, but since DOM properties[1] (and perhaps element.style.webkitRegionOverflow et al as well, haven't tested that) will still be exposed this will break feature detection. Take the following script:

var element = document.createElement('div');
if (typeof(x.webkitRegionOverflow) !== 'undefined') {
    // supported!
}

While larger scripts such as Modernizr haven't yet picked up on detecting CSS Region support (supposedly since it is enabled by default and the DOM implementation is still rather new), this doesn't seem a desirable effect. Have you thought about this?

[1] http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.idl#L137
Comment 10 James Robinson 2012-02-20 12:54:57 PST
The DOM features should be guarded by V8EnabledAtRuntime:

http://trac.webkit.org/wiki/WebKitIDL#V8EnabledAtRuntime


There's no way to guard at runtime for the JSC bindings, but I think we're OK there due to ship schedules.