WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Luke Macpherson
Comment 1
2012-06-19 19:00:06 PDT
Created
attachment 148483
[details]
Patch
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
Created
attachment 148729
[details]
Patch
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
Created
attachment 148933
[details]
Patch
Luke Macpherson
Comment 12
2012-06-21 20:32:04 PDT
Created
attachment 148951
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug