[Webkit-unassigned] [Bug 181716] Parse calc() in CSS media queries

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 19 08:24:17 PST 2018


https://bugs.webkit.org/show_bug.cgi?id=181716

--- Comment #33 from Chris Nardi <christopherncarmel at hotmail.com> ---
Comment on attachment 331681
  --> https://bugs.webkit.org/attachment.cgi?id=331681
Patch

Apologies for the mess of emails; I really need to fix my local build (bug 181639).

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

>> LayoutTests/ChangeLog:14
>> +        * fast/media/w3c/test_media_queries-expected.txt: Removed.
> 
> Could you explain in the ChangeLog what you are doing with the tests? (Importing new WPTs and removing old ones I guess)

Done.

>> Source/WebCore/ChangeLog:19
>> +        (WebCore::featureWithZeroOrOne):
> 
> You should add some function level ChangeLog comments explaining the code changes you are making.

Done.

>> Source/WebCore/css/MediaQueryExpression.cpp:55
>> +static inline bool featureWithValidDensity(const String& mediaFeature, const RefPtr<CSSPrimitiveValue> value)
> 
> - We use Ref<> for non-null things. You are not null testing anywhere so value is expected to be non-null.
> - Using Ref/RefPtr here achieves nothing and causes ref churn. I think what you really want here is 'const CSSPrimitiveValue& value'.

Thanks, I fixed that.

>> Source/WebCore/css/MediaQueryExpression.cpp:173
>> +    RefPtr<CSSPrimitiveValue> value = CSSPropertyParserHelpers::consumeInteger(range, 0);
> 
> A more descriptive name than 'value' would be better, especially since this function also deals with m_value member, firstValue or something

Changed to firstValue.

>> Source/WebCore/css/MediaQueryExpression.cpp:180
>> +        value = CSSPropertyParserHelpers::consumeResolution(range);
> 
> This might read better as a helper function that does early returns:
> 
> RefPtr<CSSPrimitiveValue> value = consumeFirstValue(...)

I added a helper function in this format.

>> Source/WebCore/css/MediaQueryExpression.cpp:202
>> +            m_value = CSSPrimitiveValue::create(value->doubleValue(), (CSSPrimitiveValue::UnitType) value->primitiveType());
> 
> Isn't this the same as just assigning m_value = value?

Yep; I was getting a test failure but I realize now that's just due to the rem unit being miscalculated which is preexisting. Somehow this way must avoid that bug but I'm not sure why... I'll just use what you suggest though since that makes more sense.

>> Source/WebCore/css/MediaQueryExpression.cpp:211
>> +        } else
> 
> WebKit prefers early return style. All these else ifs can be replaced with plain ifs and returns at the end:
> 
> if (something) {
>    ...
>    return;
> }
> if (somethingelse) {
>    ...
>    return;
> }
> 
> The code will read better

I changed it around; it does look a lot better.

>> Source/WebCore/css/MediaQueryExpression.cpp:223
>>      }
> 
> This does nothing.

Changed per above.

>> Source/WebCore/css/parser/MediaQueryParser.cpp:240
>> +void MediaQueryParser::processToken(const CSSParserToken& token, CSSParserTokenRange& range)
> 
> token is just range.peek() so providing it separately seems unnecessary. 
> 
> Since this function now consumes tokens maybe it should be renamed consumeToken? Actually, it can now consume multiple tokens...

Yeah, I thought this was awkward but I wasn't sure the best way to go about it.

>> Source/WebCore/css/parser/MediaQueryParser.cpp:248
>> +    }
> 
> It is pretty awkward that this knows readFeatureValue does its own consuming and has special case for it. Can we avoid special case, for example by making all these state functions do their own consuming?

True, although we would still need a special case for handleBlocks/blockWatcher as calc functions would appear to be another block and fail out due to this check. Also, all other functions do their own consuming so we'd just be adding ~10 range.consume() statements total... I'm not sure what would be cleanest.

>> Source/WebCore/css/parser/MediaQueryParser.cpp:263
>> +        processToken(CSSParserToken(EOFToken), range);
> 
> There is this special case where token is not range.peek(). Maybe this could be handled better?

Yeah, I'll try to play around with this to make it better.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180119/ab909c9d/attachment.html>


More information about the webkit-unassigned mailing list