Parse the @-webkit-viewport entry on a stylesheet.
Created attachment 163702 [details] Patch
Comment on attachment 163702 [details] Patch Can you add a simple parsing test?
Created attachment 171012 [details] Patch
(In reply to comment #3) > Created an attachment (id=171012) [details] > Patch I reworked the patch as suggested and it is now adding the parsed rule to the rule list and can be tested via layout test.
Comment on attachment 171012 [details] Patch Attachment 171012 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14603462
Comment on attachment 171012 [details] Patch Attachment 171012 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14603464
Comment on attachment 171012 [details] Patch Attachment 171012 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14606228
Comment on attachment 171012 [details] Patch Attachment 171012 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14617139
Comment on attachment 171012 [details] Patch Attachment 171012 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14615171
Comment on attachment 171012 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171012&action=review > Source/WebCore/ChangeLog:11 > + Test: css3/device-adapt/viewport_at_rule_parsing.html don't we normally use - in tests instead of _? I would prefer that > Source/WebCore/ChangeLog:30 > + (WebCore::CSSParser::inViewportScope): > + These methods are needed by the next patch validating the properties. I would add a new line and indent this 4 chars > Source/WebCore/css/CSSGrammar.y.in:955 > + > +viewport: > + before_viewport_rule WEBKIT_VIEWPORT_RULE_SYM at_rule_header_end_maybe_space > + '{' at_rule_body_start maybe_space_before_declaration declaration_list closing_brace { > + $$ = parser->createViewportRule(); > + parser->markViewportScopeEnd(); > + } Is this grammar defined in the spec? would be nice if the changelog made aware of such things > LayoutTests/css3/device-adapt/viewport_at_rule_parsing.html:10 > + > + @-webkit-viewport { > + } > + Maybe some minor comments would be nice so we know what is supposed to be tested > LayoutTests/css3/device-adapt/viewport_at_rule_parsing.html:23 > + @media all { > + @-webkit-viewport { > + } > + } Like is this supposed to be valid or invalid, hard to know without reading the spec > LayoutTests/platform/efl/TestExpectations:1674 > +# CSS Device Adaptation is not enabled. > +webkit.org/b/95959 css3/device-adapt [ Skip ] Shouldn't it at least be tested on one platform? now it is ignored on all.
Comment on attachment 171012 [details] Patch Attachment 171012 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14616190
Created attachment 171086 [details] Patch Thanks for reviewing. I rebased the patch, fixed the issues and it is now enabled by default for EFL, the port that I'm watching the bots all the time.
Comment on attachment 171086 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171086&action=review > Source/WebCore/css/WebKitCSSViewportRule.h:45 > +class WebKitCSSViewportRule: public CSSRule { I don't think we need to prefix implementation classes. We can prefix it in JS level through IDL annotation.
(In reply to comment #13) > (From update of attachment 171086 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171086&action=review > > > Source/WebCore/css/WebKitCSSViewportRule.h:45 > > +class WebKitCSSViewportRule: public CSSRule { > > I don't think we need to prefix implementation classes. We can prefix it in JS level through IDL annotation. I did that to keep consistence with WebKitCSSRegionRule, WebKitCSSKeyframeRule, etc. Honestly I would prefer not prefixing the files and classes because it is likely to get renamed on the future and will make a "git blame" shadowy. In the other hand, it makes explicit that the feature is not yet compliant with the spec. Should I rename?
(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 171086 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=171086&action=review > > > > > Source/WebCore/css/WebKitCSSViewportRule.h:45 > > > +class WebKitCSSViewportRule: public CSSRule { > > > > I don't think we need to prefix implementation classes. We can prefix it in JS level through IDL annotation. > > I did that to keep consistence with WebKitCSSRegionRule, WebKitCSSKeyframeRule, etc. Honestly I would prefer not prefixing the files and classes because it is likely to get renamed on the future and will make a "git blame" shadowy. > > In the other hand, it makes explicit that the feature is not yet compliant with the spec. > > Should I rename? I think it is fine, but maybe it is better to rename the file names? because such renames affect all build systems?
Comment on attachment 171086 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171086&action=review >>>> Source/WebCore/css/WebKitCSSViewportRule.h:45 >>>> +class WebKitCSSViewportRule: public CSSRule { >>> >>> I don't think we need to prefix implementation classes. We can prefix it in JS level through IDL annotation. >> >> I did that to keep consistence with WebKitCSSRegionRule, WebKitCSSKeyframeRule, etc. Honestly I would prefer not prefixing the files and classes because it is likely to get renamed on the future and will make a "git blame" shadowy. >> >> In the other hand, it makes explicit that the feature is not yet compliant with the spec. >> >> Should I rename? > > I think it is fine, but maybe it is better to rename the file names? because such renames affect all build systems? Hmm you are right. Regions and keyframes are already doing this. OK, I'd be fine for either way.
Comment on attachment 171086 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171086&action=review > Source/WebCore/css/CSSParser.h:371 > + void markViewportScopeStart() { m_inViewportScope = true; } We have void markRuleBodyStart(); Shouldn't this be called markViewportRuleBodyStart() ? > Source/WebCore/css/CSSParser.h:524 > + bool inViewportScope() const { return m_inViewportScope; } bool inShorthand() const { return m_inParseShorthand; } Wouldn't inViewport() const { return m_inParseViewport; } not be more consistent? > LayoutTests/css3/device-adapt/viewport-at-rule-parsing.html:22 > + @-webkit-viewport { > + asdasd > + } what about adding an import rule inside a @-..webkit ?
Comment on attachment 171086 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171086&action=review >> LayoutTests/css3/device-adapt/viewport-at-rule-parsing.html:22 >> + } > > what about adding an import rule inside a @-..webkit ? Good idea. I'm also going to add a test for nested @-webkit-viewport rule.
Created attachment 171664 [details] Patch
Created attachment 171672 [details] Patch Rebased.
Comment on attachment 171672 [details] Patch Clearing flags on attachment: 171672 Committed r133084: <http://trac.webkit.org/changeset/133084>
All reviewed patches have been landed. Closing bug.