Remove CSS_REGIONS compile flag from Blink.

Remove CSS_REGIONS compile flag from Blink.

The regions functionality will be enabled/disabled at runtime.

BUG=232294
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149599

[eseidel, 2013-04-18 05:36:06]

Unfortunately until we can disable CSS properties at runtime, we can't land
this. :(  I expect that support to land early next week.

[eseidel, 2013-04-18 06:39:30]

Actually, I should have been more clear.  As I noted on blink-dev this
afternoon, since we're blocked on disabling CSS properties at runtime until
https://code.google.com/p/chromium/issues/detail?id=232181 is resolved, we
should continue using prefixes for in-development CSS properties for the time
being.  But the rest of this change should still be guarded behind runtime flags
before removing this #ifdef.

[eseidel, 2013-04-27 03:40:50]

lgtm

It would be ideal if we added tests to LayoutTests/webexposed which validated
all the entry points we know about.  I'll soon be adding a "virtual test suite"
which will run all of "webexposed" a second time with the runtime flags which
are on in "stable".  That will be able to validate that regions is off in
stable.

In the meanwhile you could test those same tests in a build of
chromium/content_shell to make sure regions is not exposed there.

In any case lgtm.  Please consider further testing, as we'll all be sad if we
leak this to a release build before we've made a conscious decision to. :)

https://codereview.chromium.org/14178009/diff/4001/Source/core/css/CSSParser.cpp
File Source/core/css/CSSParser.cpp (left):

https://codereview.chromium.org/14178009/diff/4001/Source/core/css/CSSParser....
Source/core/css/CSSParser.cpp:8695: bool CSSParser::cssRegionsEnabled() const
What is this for?  Doesn't RuntimeEnabledFeatures do this?

https://codereview.chromium.org/14178009/diff/4001/Source/core/css/CSSRule.h
File Source/core/css/CSSRule.h (left):

https://codereview.chromium.org/14178009/diff/4001/Source/core/css/CSSRule.h#...
Source/core/css/CSSRule.h:58: WEBKIT_REGION_RULE = 16,
I'm not sure we're guarding rules at runtime.  We'll need to do some testing.

[mihnea, 2013-04-29 21:50:45]

On 2013/04/27 03:40:50, Eric Seidel (Google) wrote:
> lgtm
> 
> It would be ideal if we added tests to LayoutTests/webexposed which validated
> all the entry points we know about.  I'll soon be adding a "virtual test
suite"
> which will run all of "webexposed" a second time with the runtime flags which
> are on in "stable".  That will be able to validate that regions is off in
> stable.
> 
> In the meanwhile you could test those same tests in a build of
> chromium/content_shell to make sure regions is not exposed there.
> 
> In any case lgtm.  Please consider further testing, as we'll all be sad if we
> leak this to a release build before we've made a conscious decision to. :)
> 
>
https://codereview.chromium.org/14178009/diff/4001/Source/core/css/CSSParser.cpp
> File Source/core/css/CSSParser.cpp (left):
> 
>
https://codereview.chromium.org/14178009/diff/4001/Source/core/css/CSSParser....
> Source/core/css/CSSParser.cpp:8695: bool CSSParser::cssRegionsEnabled() const
> What is this for?  Doesn't RuntimeEnabledFeatures do this?
> 

The flag from RuntimeEnabledFeatures is propagated in different classes, like
Document, CSSParser and so on. I would rather keep it like this instead of
putting the call to RuntimeEnabledFeatures everywhere.

> https://codereview.chromium.org/14178009/diff/4001/Source/core/css/CSSRule.h
> File Source/core/css/CSSRule.h (left):
> 
>
https://codereview.chromium.org/14178009/diff/4001/Source/core/css/CSSRule.h#...
> Source/core/css/CSSRule.h:58: WEBKIT_REGION_RULE = 16,
> I'm not sure we're guarding rules at runtime.  We'll need to do some testing.

[eseidel, 2013-04-30 00:11:09]

lgtm

Thank you.  We're going to need more work here to make sure we aren't leaking
this at runtime.  Some of this I'm actively working on making easier, and some
of this we just need to carefully ASSERT, etc.  Please see nits below.

https://codereview.chromium.org/14178009/diff/8001/Source/core/css/CSSParserM...
File Source/core/css/CSSParserMode.h (left):

https://codereview.chromium.org/14178009/diff/8001/Source/core/css/CSSParserM...
Source/core/css/CSSParserMode.h:70: bool isCSSCompositingEnabled;
I didn't know these even existed.  Interesting.

https://codereview.chromium.org/14178009/diff/8001/Source/core/css/RuleSet.cpp
File Source/core/css/RuleSet.cpp (left):

https://codereview.chromium.org/14178009/diff/8001/Source/core/css/RuleSet.cp...
Source/core/css/RuleSet.cpp:335: else if (rule->isRegionRule() && resolver) {
I wonder if we need to runtime guard this?

https://codereview.chromium.org/14178009/diff/8001/Source/core/css/StyleBuild...
File Source/core/css/StyleBuilder.cpp (left):

https://codereview.chromium.org/14178009/diff/8001/Source/core/css/StyleBuild...
Source/core/css/StyleBuilder.cpp:2249:
setPropertyHandler(CSSPropertyWebkitRegionBreakAfter,
ApplyPropertyDefault<EPageBreak, &RenderStyle::regionBreakAfter, EPageBreak,
&RenderStyle::setRegionBreakAfter, EPageBreak,
&RenderStyle::initialPageBreak>::createHandler());
Do these need to be runtime guarded?  I'm not sure how these are enforced?

https://codereview.chromium.org/14178009/diff/8001/Source/core/css/StyleResol...
File Source/core/css/StyleResolver.cpp (left):

https://codereview.chromium.org/14178009/diff/8001/Source/core/css/StyleResol...
Source/core/css/StyleResolver.cpp:3240: case CSSPropertyWebkitRegionBreakAfter:
Does StyleResolver::applyProperty check (or ASSERT)
RuntimeCSSEnabled::isCSSPropertyEnabled()?

https://codereview.chromium.org/14178009/diff/8001/Source/core/css/StyleRule.cpp
File Source/core/css/StyleRule.cpp (left):

https://codereview.chromium.org/14178009/diff/8001/Source/core/css/StyleRule....
Source/core/css/StyleRule.cpp:230: rule =
WebKitCSSRegionRule::create(static_cast<StyleRuleRegion*>(self), parentSheet);
The constructor for WebKitCSSRegionRule should likely
ASSERT(RuntimeEnabledFeatures::isCSSRegionsEnabled()) to make sure we don't
create one of these when we don't mean to.

https://codereview.chromium.org/14178009/diff/8001/Source/core/css/WebKitCSSR...
File Source/core/css/WebKitCSSRegionRule.cpp (left):

https://codereview.chromium.org/14178009/diff/8001/Source/core/css/WebKitCSSR...
Source/core/css/WebKitCSSRegionRule.cpp:44:
WebKitCSSRegionRule::WebKitCSSRegionRule(StyleRuleRegion* regionRule,
CSSStyleSheet* parent)
We'll want to remove the "WebKit" from these in a later patch.

[mihnea, 2013-04-30 20:35:19]

On 2013/04/30 00:11:09, Eric Seidel (Google) wrote:
> lgtm
> 
> Thank you.  We're going to need more work here to make sure we aren't leaking
> this at runtime.  Some of this I'm actively working on making easier, and some
> of this we just need to carefully ASSERT, etc.  Please see nits below.
> 
>
https://codereview.chromium.org/14178009/diff/8001/Source/core/css/CSSParserM...
> File Source/core/css/CSSParserMode.h (left):
> 
>
https://codereview.chromium.org/14178009/diff/8001/Source/core/css/CSSParserM...
> Source/core/css/CSSParserMode.h:70: bool isCSSCompositingEnabled;
> I didn't know these even existed.  Interesting.
> 
> https://codereview.chromium.org/14178009/diff/8001/Source/core/css/RuleSet.cpp
> File Source/core/css/RuleSet.cpp (left):
> 
>
https://codereview.chromium.org/14178009/diff/8001/Source/core/css/RuleSet.cp...
> Source/core/css/RuleSet.cpp:335: else if (rule->isRegionRule() && resolver) {
> I wonder if we need to runtime guard this?

No, because if the regions are not enabled the rules inside @region are not
parsed.

> 
>
https://codereview.chromium.org/14178009/diff/8001/Source/core/css/StyleBuild...
> File Source/core/css/StyleBuilder.cpp (left):
> 
>
https://codereview.chromium.org/14178009/diff/8001/Source/core/css/StyleBuild...
> Source/core/css/StyleBuilder.cpp:2249:
> setPropertyHandler(CSSPropertyWebkitRegionBreakAfter,
> ApplyPropertyDefault<EPageBreak, &RenderStyle::regionBreakAfter, EPageBreak,
> &RenderStyle::setRegionBreakAfter, EPageBreak,
> &RenderStyle::initialPageBreak>::createHandler());
> Do these need to be runtime guarded?  I'm not sure how these are enforced?

I would not runtime guard them. When the properties are not exposed to the
outside world, the handlers for them are not used at all.

> 
>
https://codereview.chromium.org/14178009/diff/8001/Source/core/css/StyleResol...
> File Source/core/css/StyleResolver.cpp (left):
> 
>
https://codereview.chromium.org/14178009/diff/8001/Source/core/css/StyleResol...
> Source/core/css/StyleResolver.cpp:3240: case
CSSPropertyWebkitRegionBreakAfter:
> Does StyleResolver::applyProperty check (or ASSERT)
> RuntimeCSSEnabled::isCSSPropertyEnabled()?

I think this code path is used to assert that the properties used in
StyleBuilder are not hit in this place. If you think we should add more general
checks for runtime enabled properties, maybe we can do that in other bug?

> 
>
https://codereview.chromium.org/14178009/diff/8001/Source/core/css/StyleRule.cpp
> File Source/core/css/StyleRule.cpp (left):
> 
>
https://codereview.chromium.org/14178009/diff/8001/Source/core/css/StyleRule....
> Source/core/css/StyleRule.cpp:230: rule =
> WebKitCSSRegionRule::create(static_cast<StyleRuleRegion*>(self), parentSheet);
> The constructor for WebKitCSSRegionRule should likely
> ASSERT(RuntimeEnabledFeatures::isCSSRegionsEnabled()) to make sure we don't
> create one of these when we don't mean to.

I have added such assertions to other classes too.

> 
>
https://codereview.chromium.org/14178009/diff/8001/Source/core/css/WebKitCSSR...
> File Source/core/css/WebKitCSSRegionRule.cpp (left):
> 
>
https://codereview.chromium.org/14178009/diff/8001/Source/core/css/WebKitCSSR...
> Source/core/css/WebKitCSSRegionRule.cpp:44:
> WebKitCSSRegionRule::WebKitCSSRegionRule(StyleRuleRegion* regionRule,
> CSSStyleSheet* parent)
> We'll want to remove the "WebKit" from these in a later patch.

Ok.

[eseidel, 2013-04-30 21:08:55]

lgtm

[commit-bot: I haz the power, 2013-04-30 21:09:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihnea@adobe.com/14178009/8001

[commit-bot: I haz the power, 2013-04-30 21:09:16]

Failed to apply patch for Source/core/dom/Document.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/dom/Document.cpp
  Hunk #1 FAILED at 1087.
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/dom/Document.cpp.rej

Patch:       Source/core/dom/Document.cpp
Index: Source/core/dom/Document.cpp
diff --git a/Source/core/dom/Document.cpp b/Source/core/dom/Document.cpp
index
e5fb9d1ba1cad97415af88843fd382318160bd0d..696b70ac77128791158bfbd210d9542352cb237c
100644
--- a/Source/core/dom/Document.cpp
+++ b/Source/core/dom/Document.cpp
@@ -1087,11 +1087,6 @@ bool Document::cssStickyPositionEnabled() const
     return settings() && settings()->cssStickyPositionEnabled(); 
 }
 
-bool Document::cssRegionsEnabled() const
-{
-    return RuntimeEnabledFeatures::cssRegionsEnabled(); 
-}
-
 bool Document::cssCompositingEnabled() const
 {
     return RuntimeEnabledFeatures::cssCompositingEnabled();
@@ -1102,11 +1097,9 @@ bool Document::cssGridLayoutEnabled() const
     return settings() && settings()->cssGridLayoutEnabled();
 }
 
-#if ENABLE(CSS_REGIONS)
-
 PassRefPtr<DOMNamedFlowCollection> Document::webkitGetNamedFlows()
 {
-    if (!cssRegionsEnabled() || !renderer())
+    if (!RuntimeEnabledFeatures::cssRegionsEnabled() || !renderer())
         return 0;
 
     updateStyleIfNeeded();
@@ -1114,8 +1107,6 @@ PassRefPtr<DOMNamedFlowCollection>
Document::webkitGetNamedFlows()
     return namedFlows()->createCSSOMSnapshot();
 }
 
-#endif
-
 NamedFlowCollection* Document::namedFlows()
 {
     if (!m_namedFlows)

[commit-bot: I haz the power, 2013-05-02 16:44:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihnea@adobe.com/14178009/21001

[commit-bot: I haz the power, 2013-05-02 16:46:37]

Change committed as 149599