Bug 161537 - Add support for media query parsing using new CSS Parser
Summary: Add support for media query parsing using new CSS Parser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks: 46591
  Show dependency treegraph
 
Reported: 2016-09-02 11:04 PDT by Dave Hyatt
Modified: 2016-09-22 13:37 PDT (History)
1 user (show)

See Also:


Attachments
Patch (46.53 KB, patch)
2016-09-02 11:04 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (47.74 KB, patch)
2016-09-02 11:11 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (51.11 KB, patch)
2016-09-02 12:06 PDT, Dave Hyatt
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2016-09-02 11:04:14 PDT
Add support for media query parsing using new CSS Parser.
Comment 1 Dave Hyatt 2016-09-02 11:04:55 PDT
Created attachment 287780 [details]
Patch
Comment 2 WebKit Commit Bot 2016-09-02 11:06:38 PDT
Attachment 287780 [details] did not pass style-queue:


ERROR: Source/WebCore/css/parser/MediaQueryParser.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/css/parser/MediaQueryParser.h:93:  'parseImpl' is incorrectly named. It should be named 'protector' or 'protectedCSSParserTokenRange'.  [readability/naming/protected] [4]
ERROR: Source/WebCore/css/parser/MediaQueryParser.cpp:131:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/parser/MediaQueryParser.cpp:156:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/parser/MediaQueryParser.cpp:158:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/parser/MediaQueryParser.cpp:176:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/parser/MediaQueryParser.cpp:193:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/parser/MediaQueryParser.cpp:213:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/css/MediaQueryExp.cpp:68:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/css/MediaQueryExp.cpp:291:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/css/MediaQueryExp.cpp:311:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/MediaQueryExp.cpp:312:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 12 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Dave Hyatt 2016-09-02 11:11:48 PDT
Created attachment 287786 [details]
Patch
Comment 4 WebKit Commit Bot 2016-09-02 11:14:14 PDT
Attachment 287786 [details] did not pass style-queue:


ERROR: Source/WebCore/css/parser/MediaQueryParser.h:93:  'parseImpl' is incorrectly named. It should be named 'protector' or 'protectedCSSParserTokenRange'.  [readability/naming/protected] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Dave Hyatt 2016-09-02 12:06:47 PDT
Created attachment 287793 [details]
Patch
Comment 6 WebKit Commit Bot 2016-09-02 12:08:50 PDT
Attachment 287793 [details] did not pass style-queue:


ERROR: Source/WebCore/css/parser/MediaQueryParser.h:93:  'parseInternal' is incorrectly named. It should be named 'protector' or 'protectedCSSParserTokenRange'.  [readability/naming/protected] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Dean Jackson 2016-09-02 12:24:45 PDT
Comment on attachment 287793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287793&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:3355
> -		946D37401D6CE3C20077084F /* CSSParserToken.h in Headers */ = {isa = PBXBuildFile; fileRef = 946D373D1D6CE31A0077084F /* CSSParserToken.h */; };
> +		946D37401D6CE3C20077084F /* CSSParserToken.h in Headers */ = {isa = PBXBuildFile; fileRef = 946D373D1D6CE31A0077084F /* CSSParserToken.h */; settings = {ATTRIBUTES = (Private, ); }; };

This looks unintentional.

> Source/WebCore/css/MediaQueryExp.cpp:71
> +    
> +    

Nit: extra blank line

> Source/WebCore/css/MediaQueryExp.cpp:321
> +        // TODO(timloh): <ratio> is supposed to allow whitespace around the '/'

Did you want to leave this in?

> Source/WebCore/css/MediaQueryExp.h:37
> -
> +    

oops

> Source/WebCore/css/parser/CSSParserIdioms.cpp:39
> +template<typename CharacterType> ALWAYS_INLINE static void convertToASCIILowercaseInPlace(CharacterType* characters, unsigned length)
> +{
> +    for (unsigned i = 0; i < length; ++i)
> +        characters[i] = toASCIILower(characters[i]);
> +}

You have this code in CSSParserToken.cpp too.

> Source/WebCore/css/parser/MediaQueryBlockWatcher.cpp:39
> +MediaQueryBlockWatcher::MediaQueryBlockWatcher()
> +{
> +}

Can you just put this in the header? Or leave it out?

> Source/WebCore/css/parser/MediaQueryBlockWatcher.cpp:45
> +    if (token.getBlockType() == CSSParserToken::BlockStart) {
> +        ++m_blockLevel;
> +    } else if (token.getBlockType() == CSSParserToken::BlockEnd) {

Nit: Single line conditional.

> Source/WebCore/css/parser/MediaQueryParser.cpp:85
> +// State machine member functions start here

Remove?

> Source/WebCore/css/parser/MediaQueryParser.cpp:124
> +        } else if (m_state == ReadRestrictor && equalIgnoringASCIICase(token.value(), "only")) {
> +            setStateAndRestrict(ReadMediaType, MediaQuery::Only);
> +        } else if (m_mediaQueryData.restrictor() != MediaQuery::None
> +            && isRestrictorOrLogicalOperator(token)) {
> +            m_state = SkipUntilComma;
> +        } else {
> +            StringView stringView = token.value();

Single line conditions here.

> Source/WebCore/css/parser/MediaQueryParser.cpp:151
> +    if (type == IdentToken && equalIgnoringASCIICase(token.value(), "and")) {
> +        m_state = ReadFeatureStart;
> +    } else if (type == CommaToken && m_parserType != MediaConditionParser) {

ANd here.

> Source/WebCore/css/parser/MediaQueryParser.h:55
> +private:
> +    MediaQuery::Restrictor m_restrictor;
> +    String m_mediaType;
> +    Vector<MediaQueryExpression> m_expressions;
> +    String m_mediaFeature;
> +    Vector<CSSParserToken, 4> m_valueList;
> +    bool m_mediaTypeSet;
> +
> +public:

Why is private before public here?
Comment 8 Sam Weinig 2016-09-02 12:25:09 PDT
Comment on attachment 287793 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287793&action=review

> Source/WebCore/css/MediaQueryExp.cpp:71
> +    

Extra newline.

> Source/WebCore/css/parser/MediaQueryBlockWatcher.h:31
> +#ifndef MediaQueryBlockWatcher_h
> +#define MediaQueryBlockWatcher_h

We are doing #pragma once now, so you can replace this and the #endif with it.

> Source/WebCore/css/parser/MediaQueryParser.h:31
> +#ifndef MediaQueryParser_h
> +#define MediaQueryParser_h

We are doing #pragma once now, so you can replace this and the #endif with it.

> Source/WebCore/css/parser/MediaQueryParser.h:136
> +
> +};

Extra newline.
Comment 9 Dave Hyatt 2016-09-02 13:51:33 PDT
Landed in r205368.