Bug 89542 - Add runtime flag to enable/disable CSS variables (in addition to existing compile-time flag).
Summary: Add runtime flag to enable/disable CSS variables (in addition to existing com...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Luke Macpherson
URL:
Keywords:
Depends on:
Blocks: 90040
  Show dependency treegraph
 
Reported: 2012-06-19 18:59 PDT by Luke Macpherson
Modified: 2012-06-26 20:14 PDT (History)
10 users (show)

See Also:


Attachments
Patch (10.35 KB, patch)
2012-06-19 19:00 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (24.63 KB, patch)
2012-06-20 22:17 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (24.64 KB, patch)
2012-06-21 18:20 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (23.84 KB, patch)
2012-06-21 20:32 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch for landing (24.03 KB, patch)
2012-06-24 17:00 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch for landing (23.39 KB, patch)
2012-06-25 17:19 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2012-06-19 18:59:16 PDT
Add runtime flag to enable/disable CSS variables (in addition to existing compile-time flag).
Comment 1 Luke Macpherson 2012-06-19 19:00:06 PDT
Created attachment 148483 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-19 19:01:36 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Luke Macpherson 2012-06-19 19:01:48 PDT
Hi Dimitri, could you please take a look and let me know if this is heading in direction you wanted?
Comment 4 Dimitri Glazkov (Google) 2012-06-19 19:29:04 PDT
Comment on attachment 148483 [details]
Patch

yes, I think this looks right.
Comment 5 WebKit Review Bot 2012-06-19 21:22:11 PDT
Comment on attachment 148483 [details]
Patch

Attachment 148483 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13001076
Comment 6 WebKit Review Bot 2012-06-19 22:31:10 PDT
Comment on attachment 148483 [details]
Patch

Attachment 148483 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13009081
Comment 7 WebKit Review Bot 2012-06-19 23:34:25 PDT
Comment on attachment 148483 [details]
Patch

Attachment 148483 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13003115
Comment 8 Luke Macpherson 2012-06-20 17:08:19 PDT
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.
Comment 9 Luke Macpherson 2012-06-20 22:17:18 PDT
Created attachment 148729 [details]
Patch
Comment 10 WebKit Review Bot 2012-06-20 23:41:06 PDT
Comment on attachment 148729 [details]
Patch

Attachment 148729 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13036019
Comment 11 Luke Macpherson 2012-06-21 18:20:15 PDT
Created attachment 148933 [details]
Patch
Comment 12 Luke Macpherson 2012-06-21 20:32:04 PDT
Created attachment 148951 [details]
Patch
Comment 13 Dimitri Glazkov (Google) 2012-06-22 09:29:30 PDT
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.
Comment 14 Luke Macpherson 2012-06-24 16:59:13 PDT
(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.
Comment 15 Luke Macpherson 2012-06-24 17:00:57 PDT
Created attachment 149208 [details]
Patch for landing
Comment 16 WebKit Review Bot 2012-06-24 19:04:04 PDT
Comment on attachment 149208 [details]
Patch for landing

Clearing flags on attachment: 149208

Committed r121129: <http://trac.webkit.org/changeset/121129>
Comment 17 WebKit Review Bot 2012-06-24 19:04:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Hin-Chung Lam 2012-06-25 15:19:27 PDT
Reverted r121129 for reason:

Chromium ASan failure: crbug.com/134402

Committed r121187: <http://trac.webkit.org/changeset/121187>
Comment 19 Luke Macpherson 2012-06-25 17:19:12 PDT
Created attachment 149403 [details]
Patch for landing
Comment 20 Luke Macpherson 2012-06-25 17:26:46 PDT
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.
Comment 21 WebKit Review Bot 2012-06-25 22:24:13 PDT
Comment on attachment 149403 [details]
Patch for landing

Clearing flags on attachment: 149403

Committed r121229: <http://trac.webkit.org/changeset/121229>
Comment 22 WebKit Review Bot 2012-06-25 22:24:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Luke Macpherson 2012-06-26 18:19:49 PDT
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.