Bug 95961 – Added viewport at-rule to the CSS parser and tokenizer

• Component: CSS
• Status: RESOLVED FIXED

[Thiago Marcos P. Santos]

Parse the @-webkit-viewport entry on a stylesheet.

[Thiago Marcos P. Santos]

Created attachment 163702 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 163702 [details]
Patch

Can you add a simple parsing test?

[Thiago Marcos P. Santos]

Created attachment 171012 [details]
Patch

[Thiago Marcos P. Santos]

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

[Early Warning System Bot]

Comment on attachment 171012 [details]
Patch

Attachment 171012 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14603462

[Early Warning System Bot]

Comment on attachment 171012 [details]
Patch

Attachment 171012 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14603464

[Build Bot]

Comment on attachment 171012 [details]
Patch

Attachment 171012 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14606228

[Build Bot]

Comment on attachment 171012 [details]
Patch

Attachment 171012 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14617139

[WebKit Review Bot]

Comment on attachment 171012 [details]
Patch

Attachment 171012 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14615171

[Kenneth Rohde Christiansen]

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.

[Peter Beverloo (cr-android ews)]

Comment on attachment 171012 [details]
Patch

Attachment 171012 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14616190

[Thiago Marcos P. Santos]

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.

[Hajime Morrita]

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.

[Thiago Marcos P. Santos]

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

[Kenneth Rohde Christiansen]

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

[Hajime Morrita]

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.

[Kenneth Rohde Christiansen]

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 ?

[Thiago Marcos P. Santos]

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.

[Thiago Marcos P. Santos]

Created attachment 171664 [details]
Patch

[Thiago Marcos P. Santos]

Created attachment 171672 [details]
Patch

Rebased.

[WebKit Review Bot]

Comment on attachment 171672 [details]
Patch

Clearing flags on attachment: 171672

Committed r133084: <http://trac.webkit.org/changeset/133084>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.