RESOLVED FIXED 89542
Add runtime flag to enable/disable CSS variables (in addition to existing compile-time flag).
https://bugs.webkit.org/show_bug.cgi?id=89542
Summary Add runtime flag to enable/disable CSS variables (in addition to existing com...
Luke Macpherson
Reported 2012-06-19 18:59:16 PDT
Add runtime flag to enable/disable CSS variables (in addition to existing compile-time flag).
Attachments
Patch (10.35 KB, patch)
2012-06-19 19:00 PDT, Luke Macpherson
no flags
Patch (24.63 KB, patch)
2012-06-20 22:17 PDT, Luke Macpherson
no flags
Patch (24.64 KB, patch)
2012-06-21 18:20 PDT, Luke Macpherson
no flags
Patch (23.84 KB, patch)
2012-06-21 20:32 PDT, Luke Macpherson
no flags
Patch for landing (24.03 KB, patch)
2012-06-24 17:00 PDT, Luke Macpherson
no flags
Patch for landing (23.39 KB, patch)
2012-06-25 17:19 PDT, Luke Macpherson
no flags
Luke Macpherson
Comment 1 2012-06-19 19:00:06 PDT
WebKit Review Bot
Comment 2 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.
Luke Macpherson
Comment 3 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?
Dimitri Glazkov (Google)
Comment 4 2012-06-19 19:29:04 PDT
Comment on attachment 148483 [details] Patch yes, I think this looks right.
WebKit Review Bot
Comment 5 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
WebKit Review Bot
Comment 6 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
WebKit Review Bot
Comment 7 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
Luke Macpherson
Comment 8 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.
Luke Macpherson
Comment 9 2012-06-20 22:17:18 PDT
WebKit Review Bot
Comment 10 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
Luke Macpherson
Comment 11 2012-06-21 18:20:15 PDT
Luke Macpherson
Comment 12 2012-06-21 20:32:04 PDT
Dimitri Glazkov (Google)
Comment 13 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.
Luke Macpherson
Comment 14 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.
Luke Macpherson
Comment 15 2012-06-24 17:00:57 PDT
Created attachment 149208 [details] Patch for landing
WebKit Review Bot
Comment 16 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>
WebKit Review Bot
Comment 17 2012-06-24 19:04:10 PDT
All reviewed patches have been landed. Closing bug.
Hin-Chung Lam
Comment 18 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>
Luke Macpherson
Comment 19 2012-06-25 17:19:12 PDT
Created attachment 149403 [details] Patch for landing
Luke Macpherson
Comment 20 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.
WebKit Review Bot
Comment 21 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>
WebKit Review Bot
Comment 22 2012-06-25 22:24:20 PDT
All reviewed patches have been landed. Closing bug.
Luke Macpherson
Comment 23 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.
Note You need to log in before you can comment on or make changes to this bug.