Bug 89542

Created attachment 148483 [details] Patch

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.

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

Comment on attachment 148483 [details] Patch yes, I think this looks right.

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

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

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

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.

Created attachment 148729 [details] Patch

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

Created attachment 148933 [details] Patch

Created attachment 148951 [details] Patch

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.

(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.

Created attachment 149208 [details] Patch for landing

Comment on attachment 149208 [details] Patch for landing Clearing flags on attachment: 149208 Committed r121129: <http://trac.webkit.org/changeset/121129>

All reviewed patches have been landed. Closing bug.

Reverted r121129 for reason: Chromium ASan failure: crbug.com/134402 Committed r121187: <http://trac.webkit.org/changeset/121187>

Created attachment 149403 [details] Patch for landing

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 on attachment 149403 [details] Patch for landing Clearing flags on attachment: 149403 Committed r121229: <http://trac.webkit.org/changeset/121229>

All reviewed patches have been landed. Closing bug.

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.