Bug 157961 – Clean up / Modernize the CSS Parser

• Component: CSS
• Status: RESOLVED FIXED

[Chris Dumez]

Clean up / Modernize the CSS Parser

[Chris Dumez]

Created attachment 279519 [details]
Patch

[Chris Dumez]

Created attachment 279520 [details]
Patch

[Build Bot]

Comment on attachment 279520 [details]
Patch

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

New failing tests:
http/tests/css/shared-stylesheet-mutation.html
http/tests/css/shared-stylesheet-mutation-preconstruct.html
tables/mozilla_expected_failures/bugs/bug89315.html
printing/page-rule-css-text.html

[Build Bot]

Created attachment 279528 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 279520 [details]
Patch

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

New failing tests:
tables/mozilla_expected_failures/bugs/bug89315.html
http/tests/css/shared-stylesheet-mutation.html
printing/page-rule-css-text.html
http/tests/css/shared-stylesheet-mutation-preconstruct.html

[Build Bot]

Created attachment 279529 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 279520 [details]
Patch

Attachment 279520 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1356215

New failing tests:
http/tests/css/shared-stylesheet-mutation.html
http/tests/css/shared-stylesheet-mutation-preconstruct.html
tables/mozilla_expected_failures/bugs/bug89315.html

[Build Bot]

Created attachment 279531 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 279520 [details]
Patch

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

New failing tests:
http/tests/css/shared-stylesheet-mutation.html
printing/page-rule-css-text.html
tables/mozilla_expected_failures/bugs/bug89315.html
http/tests/css/shared-stylesheet-mutation-preconstruct.html

[Build Bot]

Created attachment 279533 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Chris Dumez]

Created attachment 279534 [details]
Patch

[Alex Christensen]

Comment on attachment 279534 [details]
Patch

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

partial review

> Source/WebCore/ChangeLog:59
> +        * css/CSSGrammar.y.in:

There's also a few places in this file where we use the std::unique_ptr constructor instead of std::make_unique, such as line 1783:
f->args = std::unique_ptr<CSSParserValueList>(new CSSParserValueList);
I've been wanting to fix that.

> Source/WebCore/css/BasicShapeFunctions.cpp:245
> -        const Vector<RefPtr<CSSPrimitiveValue>>& values = polygonValue.values();
> +        auto& values = polygonValue.values();

const auto&

> Source/WebCore/css/CSSBasicShapes.h:94
> +        updateShapeSize4Values(value1.copyRef(), value1.copyRef(), value1.copyRef(), WTFMove(value1));

This is such a cool idiom.

> Source/WebCore/css/CSSBasicShapes.h:97
> +    void updateShapeSize2Values(Ref<CSSPrimitiveValue>&& value1,  Ref<CSSPrimitiveValue>&& value2)

two spaces

> Source/WebCore/css/CSSBasicShapes.h:194
> +    CSSPrimitiveValue& getXAt(unsigned i) { return m_values[i * 2].get(); }
> +    CSSPrimitiveValue& getYAt(unsigned i) { return m_values[i * 2 + 1].get(); }
> +    const CSSPrimitiveValue& getXAt(unsigned i) const { return m_values[i * 2].get(); }
> +    const CSSPrimitiveValue& getYAt(unsigned i) const { return m_values[i * 2 + 1].get(); }

I think these are unused.  They should take a size_t if they are used, but I think we can just remove them.  I wonder what else we can remove.

> Source/WebCore/css/CSSImageValue.cpp:69
> +StyleImage& CSSImageValue::cachedOrPendingImage() const
>  {
>      if (!m_image)
> -        m_image = StylePendingImage::create(this);
> +        m_image = StylePendingImage::create(const_cast<CSSImageValue*>(this));

I think adding const and a const_cast is not an improvement.  Could StylePendingImage::create be made to take a const pointer?

> Source/WebCore/css/CSSImageValue.h:66
> -    RefPtr<StyleImage> m_image;
> +    mutable RefPtr<StyleImage> m_image;

Ditto.  Unless there's a good reason to make this mutable, I think this is abusing const.

> Source/WebCore/css/CSSKeyframesRule.cpp:51
> -    , m_keyframes(o.m_keyframes)
>      , m_name(o.m_name)
>  {
> +    m_keyframes.reserveInitialCapacity(o.keyframes().size());
> +    for (auto& keyframe : o.keyframes())
> +        m_keyframes.uncheckedAppend(keyframe.copyRef());

What does this improve?

> Source/WebCore/css/CSSKeyframesRule.cpp:156
>      int i = m_keyframesRule->findKeyframeIndex(s);

findKeyframeIndex should be made to return a size_t, then this and item could be consolidated.

[Chris Dumez]

Comment on attachment 279534 [details]
Patch

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

>> Source/WebCore/ChangeLog:59
>> +        * css/CSSGrammar.y.in:
> 
> There's also a few places in this file where we use the std::unique_ptr constructor instead of std::make_unique, such as line 1783:
> f->args = std::unique_ptr<CSSParserValueList>(new CSSParserValueList);
> I've been wanting to fix that.

Ok, I found a couple. I'll fix them.

>> Source/WebCore/css/BasicShapeFunctions.cpp:245
>> +        auto& values = polygonValue.values();
> 
> const auto&

Darin and I prefer auto& in this case. The const does not bring much value and is longer.

>> Source/WebCore/css/CSSBasicShapes.h:94
>> +        updateShapeSize4Values(value1.copyRef(), value1.copyRef(), value1.copyRef(), WTFMove(value1));
> 
> This is such a cool idiom.

Not sure what you mean.

>> Source/WebCore/css/CSSBasicShapes.h:97
>> +    void updateShapeSize2Values(Ref<CSSPrimitiveValue>&& value1,  Ref<CSSPrimitiveValue>&& value2)
> 
> two spaces

OK, will fix.

>> Source/WebCore/css/CSSBasicShapes.h:194
>> +    const CSSPrimitiveValue& getYAt(unsigned i) const { return m_values[i * 2 + 1].get(); }
> 
> I think these are unused.  They should take a size_t if they are used, but I think we can just remove them.  I wonder what else we can remove.

It looks like they are unused. I can drop them.

>> Source/WebCore/css/CSSImageValue.cpp:69
>> +        m_image = StylePendingImage::create(const_cast<CSSImageValue*>(this));
> 
> I think adding const and a const_cast is not an improvement.  Could StylePendingImage::create be made to take a const pointer?

No, it cannot take a const CSSImageValue* unfortunately. I had to make this const because I switch from RefPtr to Ref and Ref requires more const correctness. I think I will need a const_cast but it does not have to be here, it could be at the call site.

>> Source/WebCore/css/CSSKeyframesRule.cpp:51
>> +        m_keyframes.uncheckedAppend(keyframe.copyRef());
> 
> What does this improve?

Well, this is because Ref<> does not have a copy constructor. I made m_keyframes a Vector of Ref instead of a Vector of RefPtr. What it improves is that we now guarantee that this vector does not contain any null pointers.
The unfortunate effect is that I cannot just copy the vector using a simple assignment.

>> Source/WebCore/css/CSSKeyframesRule.cpp:156
>>      int i = m_keyframesRule->findKeyframeIndex(s);
> 
> findKeyframeIndex should be made to return a size_t, then this and item could be consolidated.

I have just looked and findKeyframeIndex() can return -1 so using size_t seems a no-go? Or did I misunderstand your comment?

[Chris Dumez]

Comment on attachment 279534 [details]
Patch

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

>>> Source/WebCore/css/CSSKeyframesRule.cpp:51
>>> +        m_keyframes.uncheckedAppend(keyframe.copyRef());
>> 
>> What does this improve?
> 
> Well, this is because Ref<> does not have a copy constructor. I made m_keyframes a Vector of Ref instead of a Vector of RefPtr. What it improves is that we now guarantee that this vector does not contain any null pointers.
> The unfortunate effect is that I cannot just copy the vector using a simple assignment.

Honestly, I wish Ref<> has a copy constructor. It is meant to be used with RefCounted types so making a copy is fine and relatively cheap. The fact that Ref<> both does not have a copy constructor, that its get() method is const and that it does not have an operator==() makes it less convenient to use than RefPtr<> unfortunately :/ I like the guarantee that something cannot be null but I just wish we had implemented it more like RefPtr<> to make the transition a lot easier.

[Chris Dumez]

Comment on attachment 279534 [details]
Patch

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

>>> Source/WebCore/css/CSSImageValue.cpp:69
>>> +        m_image = StylePendingImage::create(const_cast<CSSImageValue*>(this));
>> 
>> I think adding const and a const_cast is not an improvement.  Could StylePendingImage::create be made to take a const pointer?
> 
> No, it cannot take a const CSSImageValue* unfortunately. I had to make this const because I switch from RefPtr to Ref and Ref requires more const correctness. I think I will need a const_cast but it does not have to be here, it could be at the call site.

Looks like I may be able to drop the const (and mutable member) by dropping to making several calling functions non-const (subimageIsPending() and several isPending() method on CSS images classes). It is kind of sad though because you couldn't expect something like isPending() to be non-const. I personally thought using mutable / const makes sense for caching use cases which is the case here, but anyway.

[Chris Dumez]

Created attachment 279548 [details]
Patch

[Alex Christensen]

(In reply to comment #13)
> >> Source/WebCore/css/CSSKeyframesRule.cpp:156
> >>      int i = m_keyframesRule->findKeyframeIndex(s);
> > 
> > findKeyframeIndex should be made to return a size_t, then this and item could be consolidated.
> 
> I have just looked and findKeyframeIndex() can return -1 so using size_t
> seems a no-go? Or did I misunderstand your comment?
It should return something like WTF::notFound

[Alex Christensen]

Comment on attachment 279548 [details]
Patch

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

> Source/WebCore/css/CSSImageGeneratorValue.cpp:170
> -bool CSSImageGeneratorValue::isPending() const
> +bool CSSImageGeneratorValue::isPending()

Yes, this is disappointing, but I think this is better than adding const_cast.

> Source/WebCore/css/CSSParser.cpp:1864
> +    for (auto& property : m_parsedProperties) {
> +        if (property.id() == propID)

There might be a better algorithm to avoid searching, but not in this patch.

> Source/WebCore/css/CSSParser.cpp:2849
> +            ASSERT(val2);

I still think this might be worth a null check.

> Source/WebCore/css/CSSParser.cpp:12892
> +    Ref<CSSRuleSourceData> rule = *popRuleData();

What guarantees popRuleData in this case?

> Source/WebCore/css/CSSParser.h:455
> +    SourceRange m_propertyRange { UINT_MAX, UINT_MAX };

std::numeric_limits

[Chris Dumez]

Comment on attachment 279548 [details]
Patch

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

>> Source/WebCore/css/CSSParser.cpp:12892
>> +    Ref<CSSRuleSourceData> rule = *popRuleData();
> 
> What guarantees popRuleData in this case?

The isExtractingSourceData() check above. Note that the code would have already crashed before my change if popRuleData() returned null because fixUnparsedPropertyRanges() dereferences its input argument without null check.

[Chris Dumez]

Created attachment 279567 [details]
Patch

[Chris Dumez]

Comment on attachment 279567 [details]
Patch

Clearing flags on attachment: 279567

Committed r201290: <http://trac.webkit.org/changeset/201290>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Konstantin Tokarev]

Comment on attachment 279567 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:5464
> +    return WTFMove(values);

I don't think this is a correct idiom. In C++11 return value is either subject to (N)RVO, or moved, and explicit move is unnecessary and will prevent RVO when it is possible. 

For example, see here https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-3956e82276f3/entry/RVO_V_S_std_move?lang=en

[Chris Dumez]

Comment on attachment 279567 [details]
Patch

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

>> Source/WebCore/css/CSSParser.cpp:5464
>> +    return WTFMove(values);
> 
> I don't think this is a correct idiom. In C++11 return value is either subject to (N)RVO, or moved, and explicit move is unnecessary and will prevent RVO when it is possible. 
> 
> For example, see here https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-3956e82276f3/entry/RVO_V_S_std_move?lang=en

It is needed because values is a Ref<> and this function returns a RefPtr<>.

[Darin Adler]

Comment on attachment 279567 [details]
Patch

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

"not guaranteed"

> Source/WebCore/css/CSSBasicShapes.h:94
> +        updateShapeSize4Values(value1.copyRef(), value1.copyRef(), value1.copyRef(), WTFMove(value1));

Order of evaluation of function arguments is no guaranteed. Some of the value1.copyRef() could be null. We need to write this differently.

> Source/WebCore/css/CSSBasicShapes.h:99
> +        updateShapeSize4Values(value1.copyRef(), value2.copyRef(), WTFMove(value1), WTFMove(value2));

Same issue here.

> Source/WebCore/css/CSSBasicShapes.h:104
> +        updateShapeSize4Values(WTFMove(value1), value2.copyRef(), WTFMove(value3), WTFMove(value2));

Ditto.

[Chris Dumez]

(In reply to comment #25)
> Comment on attachment 279567 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279567&action=review
> 
> "not guaranteed"
> 
> > Source/WebCore/css/CSSBasicShapes.h:94
> > +        updateShapeSize4Values(value1.copyRef(), value1.copyRef(), value1.copyRef(), WTFMove(value1));
> 
> Order of evaluation of function arguments is no guaranteed. Some of the
> value1.copyRef() could be null. We need to write this differently.
> 
> > Source/WebCore/css/CSSBasicShapes.h:99
> > +        updateShapeSize4Values(value1.copyRef(), value2.copyRef(), WTFMove(value1), WTFMove(value2));
> 
> Same issue here.
> 
> > Source/WebCore/css/CSSBasicShapes.h:104
> > +        updateShapeSize4Values(WTFMove(value1), value2.copyRef(), WTFMove(value3), WTFMove(value2));
> 
> Ditto.

Oh, I did not know about this! I'll fix shortly. Thanks for letting me know.

[Chris Dumez]

Created attachment 279602 [details]
Follow-up to fix undefined behavior

[Chris Dumez]

Reopen to fix the undefined behavior.

[WebKit Commit Bot]

Attachment 279602 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Committed r201303: <http://trac.webkit.org/changeset/201303>