Bug 89542 – Add runtime flag to enable/disable CSS variables (in addition to existing compile-time flag).

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Luke Macpherson]

Add runtime flag to enable/disable CSS variables (in addition to existing compile-time flag).

[Luke Macpherson]

Created attachment 148483 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Luke Macpherson]

Hi Dimitri, could you please take a look and let me know if this is heading in direction you wanted?

[Dimitri Glazkov (Google)]

Comment on attachment 148483 [details]
Patch

yes, I think this looks right.

[WebKit Review Bot]

Comment on attachment 148483 [details]
Patch

Attachment 148483 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13001076

[WebKit Review Bot]

Comment on attachment 148483 [details]
Patch

Attachment 148483 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13009081

[WebKit Review Bot]

Comment on attachment 148483 [details]
Patch

Attachment 148483 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13003115

[Luke Macpherson]

I'm going to expose the runtime flag to JS for testing and modify the tests to turn it on at runtime. Expect another patch with those changes soon.

[Luke Macpherson]

Created attachment 148729 [details]
Patch

[WebKit Review Bot]

Comment on attachment 148729 [details]
Patch

Attachment 148729 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13036019

[Luke Macpherson]

Created attachment 148933 [details]
Patch

[Luke Macpherson]

Created attachment 148951 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 148951 [details]
Patch

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

Looks good. Please take care of the comments before landing.

> Source/WebCore/ChangeLog:8
> +        No new tests.

Actually, you _are_ making changes to tests, so perhaps a more accurate way to say it would be "Tests adapted to use this functionality."

> Source/WebCore/ChangeLog:16
> +        (WebCore::CSSParser::detectDashToken):
> +        (WebCore::CSSParser::lex):

I don't see where you're making changes to these two functions in the patch.

> Source/WebCore/css/CSSParser.cpp:1027
> +bool CSSParser::parseValue(StylePropertySet* declaration, CSSPropertyID propertyID, const String& string, bool important, Document* document)

I am not sure what this function is doing here in this patch.

[Luke Macpherson]

(In reply to comment #13)
> (From update of attachment 148951 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148951&action=review
> 
> Looks good. Please take care of the comments before landing.
>
> > Source/WebCore/ChangeLog:8
> > +        No new tests.
> 
> Actually, you _are_ making changes to tests, so perhaps a more accurate way to say it would be "Tests adapted to use this functionality."

Changelog updated.

> > Source/WebCore/ChangeLog:16
> > +        (WebCore::CSSParser::detectDashToken):
> > +        (WebCore::CSSParser::lex):
> 
> I don't see where you're making changes to these two functions in the patch.

Line 8552 (in detectDashToken) now checks the runtime flag.
Line 8894 (in lex) now checks the runtime flag.
 
> > Source/WebCore/css/CSSParser.cpp:1027
> > +bool CSSParser::parseValue(StylePropertySet* declaration, CSSPropertyID propertyID, const String& string, bool important, Document* document)
> 
> I am not sure what this function is doing here in this patch.

This is a static function that needs to construct a parser context. Because the enable css variables runtime flag is set at the document settings level we need to pass it in here in order to construct a parser context with the right set of flags enabled. That the existing static function doesn't do this could be considered a bug because it means that other flags like enable css regions could also be ignored.

[Luke Macpherson]

Created attachment 149208 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 149208 [details]
Patch for landing

Clearing flags on attachment: 149208

Committed r121129: <http://trac.webkit.org/changeset/121129>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hin-Chung Lam]

Reverted r121129 for reason:

Chromium ASan failure: crbug.com/134402

Committed r121187: <http://trac.webkit.org/changeset/121187>

[Luke Macpherson]

Created attachment 149403 [details]
Patch for landing

[Luke Macpherson]

Second patch for landing is identical except that it doesn't flip the compile-time enable flag on for chrome yet. That can happen in another patch when we're ready for it.

[WebKit Review Bot]

Comment on attachment 149403 [details]
Patch for landing

Clearing flags on attachment: 149403

Committed r121229: <http://trac.webkit.org/changeset/121229>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Luke Macpherson]

Comment on attachment 149403 [details]
Patch for landing

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

> Source/WebCore/page/Settings.h:336
> +        bool cssVariablesEnabled() const { return true; }

Oh dear, this shouldn't always return true. I'll upload a patch to fix promptly.