Bug 139928 – Canonicalization of :lang() should preserve the :lang()'s arguments representations

• Component: New Bugs
• Status: RESOLVED FIXED

[Dhi Aurrahman]

Always canonicalize :lang() to string instead of identifier

[Dhi Aurrahman]

Created attachment 243716 [details]
Patch

[Dhi Aurrahman]

Created attachment 243717 [details]
Patch

[Build Bot]

Comment on attachment 243717 [details]
Patch

Attachment 243717 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5597974700752896

New failing tests:
fast/css/parsing-css-lang.html
fast/css/css-set-selector-text.html
fast/dom/css-selectorText.html

[Build Bot]

Created attachment 243721 [details]
Archive of layout-test-results from ews100 for mac-mountainlion

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

[Build Bot]

Comment on attachment 243717 [details]
Patch

Attachment 243717 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6658914972073984

New failing tests:
fast/css/parsing-css-lang.html
fast/css/css-set-selector-text.html
fast/dom/css-selectorText.html

[Build Bot]

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

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

[Dhi Aurrahman]

Created attachment 243723 [details]
Patch

[Benjamin Poulain]

Comment on attachment 243723 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        The canonicalization of :lang()'s arguments is expected to be always represented as string.

Before moving forward with this, I'll have a look at the parser and CSSSelector.

If it is not too messy, it would be nice to serialize identifier and strings as their original types.

> Source/WebCore/ChangeLog:9
> +        For example, :lang(foo, \*-bar) will be canonicalized as :lang("foo", "bar").

"bar" -> "*-bar"

[Dhi Aurrahman]

Comment on attachment 243723 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        The canonicalization of :lang()'s arguments is expected to be always represented as string.
> 
> Before moving forward with this, I'll have a look at the parser and CSSSelector.
> 
> If it is not too messy, it would be nice to serialize identifier and strings as their original types.

I'm a bit confused. By preserving original types, does that mean IDENTs stay IDENTs and STRINGs stay STRINGs? Or are you talking about anything other than :lang()? :)

>> Source/WebCore/ChangeLog:9
>> +        For example, :lang(foo, \*-bar) will be canonicalized as :lang("foo", "bar").
> 
> "bar" -> "*-bar"

Thank you

[Benjamin Poulain]

(In reply to comment #9)
> Comment on attachment 243723 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243723&action=review
> 
> >> Source/WebCore/ChangeLog:8
> >> +        The canonicalization of :lang()'s arguments is expected to be always represented as string.
> > 
> > Before moving forward with this, I'll have a look at the parser and CSSSelector.
> > 
> > If it is not too messy, it would be nice to serialize identifier and strings as their original types.
> 
> I'm a bit confused. By preserving original types, does that mean IDENTs stay
> IDENTs and STRINGs stay STRINGs? Or are you talking about anything other
> than :lang()? :)

I am very sorry for the delay. I had tons of stuff on my mind and I kept pushing this back.

I think we should try to preserve the representation of IDENT and STRING when serializing the string. It would be cleaner.
Ideally, a selector like:
    :lang(foo,"bar"   ,baz)
would serialize as
    :lang(foo, "bar", baz)

This creates some challenges on our side to do that. We will have to record the type when parsing and extend CSSSelector to keep the string and its type. But overall I believe it would be better.

What do you think?

[Dhi Aurrahman]

Created attachment 244416 [details]
Patch

[Build Bot]

Comment on attachment 244416 [details]
Patch

Attachment 244416 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5543898512359424

New failing tests:
fast/css/css-lang-selector-with-string-arguments-text.html

[Build Bot]

Created attachment 244417 [details]
Archive of layout-test-results from ews101 for mac-mountainlion

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

[Build Bot]

Comment on attachment 244416 [details]
Patch

Attachment 244416 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5911193780748288

New failing tests:
fast/css/css-lang-selector-with-string-arguments-text.html

[Build Bot]

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

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

[Dhi Aurrahman]

Created attachment 244420 [details]
Patch

[Dhi Aurrahman]

Comment on attachment 243723 [details]
Patch

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

>>>> Source/WebCore/ChangeLog:8
>>>> +        The canonicalization of :lang()'s arguments is expected to be always represented as string.
>>> 
>>> Before moving forward with this, I'll have a look at the parser and CSSSelector.
>>> 
>>> If it is not too messy, it would be nice to serialize identifier and strings as their original types.
>> 
>> I'm a bit confused. By preserving original types, does that mean IDENTs stay IDENTs and STRINGs stay STRINGs? Or are you talking about anything other than :lang()? :)
> 
> I am very sorry for the delay. I had tons of stuff on my mind and I kept pushing this back.
> 
> I think we should try to preserve the representation of IDENT and STRING when serializing the string. It would be cleaner.
> Ideally, a selector like:
>     :lang(foo,"bar"   ,baz)
> would serialize as
>     :lang(foo, "bar", baz)
> 
> This creates some challenges on our side to do that. We will have to record the type when parsing and extend CSSSelector to keep the string and its type. But overall I believe it would be better.
> 
> What do you think?

Sounds great. I'll update the patch.

[Dhi Aurrahman]

Created attachment 244421 [details]
Patch

[Dhi Aurrahman]

Comment on attachment 244421 [details]
Patch

Updated to be consistent on renaming some related methods.

[Benjamin Poulain]

Comment on attachment 244421 [details]
Patch

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

I was expecting this fix to be much bigger for some reason...this is pretty cool

> Source/WebCore/css/CSSSelector.h:226
> -        const Vector<AtomicString>* argumentList() const { return m_hasRareData ? m_data.m_rareData->m_argumentList.get() : nullptr; }
> +        const Vector<AtomicString>* langArgumentList() const { return m_hasRareData ? m_data.m_rareData->m_langArgumentList.get() : nullptr; }
> +        const Vector<bool>* langArgumentParsedFromStringFlagList() const { return langArgumentList() ? m_data.m_rareData->m_langArgumentParsedFromStringFlagList.get() : nullptr; }

I think it would be cleaner to store the string and its type together.

something like:

enum class TokenType {
    AtomicString
    Identifier
}
struct LanguageArgument {
    AtomicString languageRange;
    TokenType type;
}

...

std::unique_ptr<Vector< LanguageArgument>> m_langArgumentList;

[Dhi Aurrahman]

Created attachment 244508 [details]
Patch

[Dhi Aurrahman]

Created attachment 244511 [details]
Patch

[Dhi Aurrahman]

Created attachment 244556 [details]
Patch

[Benjamin Poulain]

Comment on attachment 244556 [details]
Patch

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

Some style nitpick below. Everything looks correct otherwise.

> Source/WebCore/css/CSSParserValues.cpp:265
> +void CSSParserSelector::setLangArgumentList(Vector<CSSParserString>& stringVector)

Could the "stringVector" be const?

> Source/WebCore/css/CSSParserValues.cpp:270
> +    argumentList->reserveInitialCapacity(stringVectorSize);

Having "auto argumentList = std::make_unique<Vector<LanguageArgument>>();" between "size_t stringVectorSize = stringVector.size();" and "argumentList->reserveInitialCapacity(stringVectorSize);" will add register pressure.

First, "size_t stringVectorSize = stringVector.size();" will put "stringVectorSize" into a register. Then we have "auto argumentList = std::make_unique<Vector<LanguageArgument>>();", that will likely cause a function call, which will spill all registers to the stack.

By moving "size_t stringVectorSize = stringVector.size();" one line down, you would avoid this.

> Source/WebCore/css/CSSParserValues.cpp:272
> +    for (size_t stringVectorIndex = 0; stringVectorIndex < stringVectorSize; ++stringVectorIndex) {
> +        const CSSParserString& string = stringVector[stringVectorIndex];

You can use a C++ 11 range based for here.

> Source/WebCore/css/CSSParserValues.h:39
> +enum class TokenType {
> +    IdentifierToken = 0,
> +    AtomicStringToken
> +};

I would define this in CSSSelector.h to reverse the dependencies.

> Source/WebCore/css/CSSSelector.cpp:30
> +#include "CSSParserValues.h"

Having the CSSSelector implementation relying on the parser is unfortunate.

But with TokenType on CSSSelector.h, we would not need this.

[Dhi Aurrahman]

Created attachment 244649 [details]
Patch

[Benjamin Poulain]

Comment on attachment 244649 [details]
Patch

Great work. Let's land!

[WebKit Commit Bot]

Comment on attachment 244649 [details]
Patch

Clearing flags on attachment: 244649

Committed r178532: <http://trac.webkit.org/changeset/178532>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This broke fast/css/parsing-css-lang.html on Mac Yosemite Debug:

https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r178579%20(1470)/fast/css/parsing-css-lang-diff.txt

-PASS document.getElementById('style-container').sheet.cssRules[0].selectorText is ":lang(fr-CH)"
+FAIL document.getElementById('style-container').sheet.cssRules[0].selectorText should be :lang(fr-CH). Was :lang("fr-CH").
...
-PASS document.getElementById('style-container').sheet.cssRules[0].selectorText is ":lang(rm-CH)"
+FAIL document.getElementById('style-container').sheet.cssRules[0].selectorText should be :lang(rm-CH). Was :lang("rm-CH").
...

Not yet sure how this is possible; will roll out if I cannot figure it out quickly.

[Alexey Proskuryakov]

This test has the same expectations for all platforms, and doesn't have TestExpectations, other than for Gtk. 

Smells like memory corruption. It may or may not be new for this patch (cf. bug 139353), but it seems right to investigate it before re-landing the patch that exposes it.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 140548

[Dhi Aurrahman]

Created attachment 244827 [details]
Patch

[Dhi Aurrahman]

Comment on attachment 244827 [details]
Patch

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

> Source/WebCore/css/CSSGrammar.y.in:1108
> +lang_range: 

I found that the token type of the parsed CSSParserString should be explicitly defined after it's initialized. Do we have preference of this tokenType? 

We can also put e.g. 

tokenType = TokenType::IdentifierToken; inside each init() in CSSParserString.

[Benjamin Poulain]

(In reply to comment #33)
> Comment on attachment 244827 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244827&action=review
> 
> > Source/WebCore/css/CSSGrammar.y.in:1108
> > +lang_range: 
> 
> I found that the token type of the parsed CSSParserString should be
> explicitly defined after it's initialized. Do we have preference of this
> tokenType? 
> 
> We can also put e.g. 
> 
> tokenType = TokenType::IdentifierToken; inside each init() in
> CSSParserString.

Arg. It is unfortunate that EWS did not catch the mistake.

IMHO, you should have the initialization in each init() and clear() to ensure CSSParserString is always in a valid state.

[Dhi Aurrahman]

Created attachment 244867 [details]
Patch

[Dhi Aurrahman]

Comment on attachment 244867 [details]
Patch

I'm not sure what's wrong with the red-label on windows EWS build bot status. I hope it is just a false alarm.

[Benjamin Poulain]

Comment on attachment 244867 [details]
Patch

Let's try again.

[WebKit Commit Bot]

Comment on attachment 244867 [details]
Patch

Clearing flags on attachment: 244867

Committed r178675: <http://trac.webkit.org/changeset/178675>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.