Bug 95961 - Added viewport at-rule to the CSS parser and tokenizer
: Added viewport at-rule to the CSS parser and tokenizer
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 95959
  Show dependency treegraph
 
Reported: 2012-09-06 02:57 PST by
Modified: 2012-10-31 15:22 PST (History)


Attachments
Patch (7.05 KB, patch)
2012-09-12 14:47 PST, Thiago Marcos P. Santos
no flags Review Patch | Details | Formatted Diff | Diff
Patch (40.44 KB, patch)
2012-10-26 14:31 PST, Thiago Marcos P. Santos
no flags Review Patch | Details | Formatted Diff | Diff
Patch (43.68 KB, patch)
2012-10-27 04:16 PST, Thiago Marcos P. Santos
no flags Review Patch | Details | Formatted Diff | Diff
Patch (44.36 KB, patch)
2012-10-31 08:18 PST, Thiago Marcos P. Santos
no flags Review Patch | Details | Formatted Diff | Diff
Patch (44.70 KB, patch)
2012-10-31 09:16 PST, Thiago Marcos P. Santos
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-09-06 02:57:56 PST
Parse the @-webkit-viewport entry on a stylesheet.
------- Comment #1 From 2012-09-12 14:47:02 PST -------
Created an attachment (id=163702) [details]
Patch
------- Comment #2 From 2012-10-25 06:54:50 PST -------
(From update of attachment 163702 [details])
Can you add a simple parsing test?
------- Comment #3 From 2012-10-26 14:31:34 PST -------
Created an attachment (id=171012) [details]
Patch
------- Comment #4 From 2012-10-26 14:34:22 PST -------
(In reply to comment #3)
> Created an attachment (id=171012) [details] [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 #5 From 2012-10-26 14:37:47 PST -------
(From update of attachment 171012 [details])
Attachment 171012 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14603462
------- Comment #6 From 2012-10-26 14:37:50 PST -------
(From update of attachment 171012 [details])
Attachment 171012 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14603464
------- Comment #7 From 2012-10-26 14:52:31 PST -------
(From update of attachment 171012 [details])
Attachment 171012 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14606228
------- Comment #8 From 2012-10-26 15:05:15 PST -------
(From update of attachment 171012 [details])
Attachment 171012 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14617139
------- Comment #9 From 2012-10-26 15:27:33 PST -------
(From update of attachment 171012 [details])
Attachment 171012 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14615171
------- Comment #10 From 2012-10-26 15:38:21 PST -------
(From update of attachment 171012 [details])
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 #11 From 2012-10-26 15:51:57 PST -------
(From update of attachment 171012 [details])
Attachment 171012 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14616190
------- Comment #12 From 2012-10-27 04:16:48 PST -------
Created an attachment (id=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 #13 From 2012-10-29 04:15:07 PST -------
(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.
------- Comment #14 From 2012-10-29 06:31:06 PST -------
(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?
------- Comment #15 From 2012-10-30 06:07:29 PST -------
(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 171086 [details] [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 #16 From 2012-10-30 06:32:43 PST -------
(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?
> 
> 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 #17 From 2012-10-30 16:33:23 PST -------
(From update of attachment 171086 [details])
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 #18 From 2012-10-31 08:11:24 PST -------
(From update of attachment 171086 [details])
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.
------- Comment #19 From 2012-10-31 08:18:02 PST -------
Created an attachment (id=171664) [details]
Patch
------- Comment #20 From 2012-10-31 09:16:51 PST -------
Created an attachment (id=171672) [details]
Patch

Rebased.
------- Comment #21 From 2012-10-31 15:22:30 PST -------
(From update of attachment 171672 [details])
Clearing flags on attachment: 171672

Committed r133084: <http://trac.webkit.org/changeset/133084>
------- Comment #22 From 2012-10-31 15:22:37 PST -------
All reviewed patches have been landed.  Closing bug.