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
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Thiago Marcos P. Santos
:
Depends on:
Blocks: 95959
  Show dependency treegraph
 
Reported: 2012-09-06 02:57 PDT by Thiago Marcos P. Santos
Modified: 2012-10-31 15:22 PDT (History)
20 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago Marcos P. Santos 2012-09-06 02:57:56 PDT
Parse the @-webkit-viewport entry on a stylesheet.
Comment 1 Thiago Marcos P. Santos 2012-09-12 14:47:02 PDT
Created attachment 163702 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-10-25 06:54:50 PDT
Comment on attachment 163702 [details]
Patch

Can you add a simple parsing test?
Comment 3 Thiago Marcos P. Santos 2012-10-26 14:31:34 PDT
Created attachment 171012 [details]
Patch
Comment 4 Thiago Marcos P. Santos 2012-10-26 14:34:22 PDT
(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 5 Early Warning System Bot 2012-10-26 14:37:47 PDT
Comment on attachment 171012 [details]
Patch

Attachment 171012 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14603462
Comment 6 Early Warning System Bot 2012-10-26 14:37:50 PDT
Comment on attachment 171012 [details]
Patch

Attachment 171012 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14603464
Comment 7 Build Bot 2012-10-26 14:52:31 PDT
Comment on attachment 171012 [details]
Patch

Attachment 171012 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14606228
Comment 8 Build Bot 2012-10-26 15:05:15 PDT
Comment on attachment 171012 [details]
Patch

Attachment 171012 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14617139
Comment 9 WebKit Review Bot 2012-10-26 15:27:33 PDT
Comment on attachment 171012 [details]
Patch

Attachment 171012 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14615171
Comment 10 Kenneth Rohde Christiansen 2012-10-26 15:38:21 PDT
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 11 Peter Beverloo (cr-android ews) 2012-10-26 15:51:57 PDT
Comment on attachment 171012 [details]
Patch

Attachment 171012 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14616190
Comment 12 Thiago Marcos P. Santos 2012-10-27 04:16:48 PDT
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 13 Hajime Morrita 2012-10-29 04:15:07 PDT
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.
Comment 14 Thiago Marcos P. Santos 2012-10-29 06:31:06 PDT
(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?
Comment 15 Kenneth Rohde Christiansen 2012-10-30 06:07:29 PDT
(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 16 Hajime Morrita 2012-10-30 06:32:43 PDT
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 17 Kenneth Rohde Christiansen 2012-10-30 16:33:23 PDT
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 18 Thiago Marcos P. Santos 2012-10-31 08:11:24 PDT
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.
Comment 19 Thiago Marcos P. Santos 2012-10-31 08:18:02 PDT
Created attachment 171664 [details]
Patch
Comment 20 Thiago Marcos P. Santos 2012-10-31 09:16:51 PDT
Created attachment 171672 [details]
Patch

Rebased.
Comment 21 WebKit Review Bot 2012-10-31 15:22:30 PDT
Comment on attachment 171672 [details]
Patch

Clearing flags on attachment: 171672

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