Bug 139340 – Extend :lang()'s selector checker to handle ranges with '*' properly and perform matching within the ASCII range

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dhi Aurrahman]

Consider asterisk containing range as input for the extended filtering of :lang()'s selector checker

[Dhi Aurrahman]

Created attachment 242713 [details]
Patch

[Dhi Aurrahman]

Created attachment 242714 [details]
Patch

[Dhi Aurrahman]

Created attachment 242889 [details]
Patch

[Dhi Aurrahman]

Created attachment 242892 [details]
Patch

[Dhi Aurrahman]

Created attachment 242987 [details]
Patch

[Build Bot]

Comment on attachment 242987 [details]
Patch

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

New failing tests:
fast/canvas/webgl/tex-image-and-sub-image-2d-with-canvas-rgb565.html

[Build Bot]

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

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

[Dhi Aurrahman]

Created attachment 242999 [details]
Patch

[Darin Adler]

Comment on attachment 242999 [details]
Patch

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

Good basic approach. A couple small mistakes.

> Source/WebCore/css/SelectorCheckerTestFunctions.h:124
> +inline bool equalIgnoringCaseWithinASCIIRange(const AtomicString& a, const AtomicString& b)

This should take const String&, not const AtomicString&. The caller doesn’t always have an AtomicString and making one is expensive.

> Source/WebCore/css/SelectorCheckerTestFunctions.h:126
> +    size_t length = a.length() > b.length() ? a.length() : b.length();

This isn’t right. If the lengths don’t match then we should return false, not compare only characters based on the length of the shorter of the two strings.

This shows we have insufficient test coverage: You should add a test case that shows that this code was incorrect, where the prefix matches but there are extra characters that cause a mismatch.

I suggest naming this function equalIgnoringASCIICase. While “case within ASCII range” might be more precise but it’s harder to understand since “range” is often used to mean a substring rather than a range of Unicode values.

[Dhi Aurrahman]

Created attachment 243063 [details]
Patch

[Dhi Aurrahman]

Created attachment 243064 [details]
Patch

[Dhi Aurrahman]

Comment on attachment 243064 [details]
Patch

Added some tests e.g. 

shouldBe('document.querySelectorAll(":lang(FOöÉ-BÁr)").length', '1');
shouldBe('document.querySelectorAll(":lang(FOöÉ-BÁ)").length', '0');
shouldBe('document.querySelectorAll(":lang(FOöÉ-B)").length', '0');

shouldBe('document.querySelectorAll(":lang(f-BÁr)").length', '0');
shouldBe('document.querySelectorAll(":lang(fO-BÁr)").length', '0');
shouldBe('document.querySelectorAll(":lang(fOö-BÁr)").length', '0');
shouldBe('document.querySelectorAll(":lang(FOöÉ-BÁr1)").length', '0');
shouldBe('document.querySelectorAll(":lang(FOöÉ-BÁr12)").length', '0');
shouldBe('document.querySelectorAll(":lang(FOöÉ-BÁr123)").length', '0');

And it caught previous mistake.

[Dhi Aurrahman]

Comment on attachment 243064 [details]
Patch

Added some tests e.g. 

given 

<div lang="fooÉ"></div>
<div lang="foöÉ-bÁr"></div>
 <div lang="foöÉbÁr"></div>

shouldBe('document.querySelectorAll(":lang(FOöÉ-BÁr)").length', '1');
shouldBe('document.querySelectorAll(":lang(FOöÉ-BÁ)").length', '0');
shouldBe('document.querySelectorAll(":lang(FOöÉ-B)").length', '0');

shouldBe('document.querySelectorAll(":lang(f-BÁr)").length', '0');
shouldBe('document.querySelectorAll(":lang(fO-BÁr)").length', '0');
shouldBe('document.querySelectorAll(":lang(fOö-BÁr)").length', '0');
shouldBe('document.querySelectorAll(":lang(FOöÉ-BÁr1)").length', '0');
shouldBe('document.querySelectorAll(":lang(FOöÉ-BÁr12)").length', '0');
shouldBe('document.querySelectorAll(":lang(FOöÉ-BÁr123)").length', '0');

And it caught previous mistake.

[Dhi Aurrahman]

Sorry, for multiple posts, I was connected to bad internet connection.

[Darin Adler]

Comment on attachment 243064 [details]
Patch

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

> LayoutTests/fast/selectors/lang-extended-filtering-expected.txt:82
> +FAIL document.querySelectorAll(":lang(\\*)").length should be 34. Was 35.

We expect FAIL?

[Dhi Aurrahman]

Created attachment 243089 [details]
Patch

[Benjamin Poulain]

Comment on attachment 243089 [details]
Patch

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

The patch looks great.

I cq- because I would like a new test showing equalIgnoringASCIICase() is correct.

> LayoutTests/ChangeLog:11
> +        * fast/selectors/lang-extended-filtering-expected.txt: Updated.
> +        * fast/selectors/lang-extended-filtering.html: Updated.
> +        * fast/selectors/lang-valid-extended-filtering-expected.txt: Added.
> +        * fast/selectors/lang-valid-extended-filtering.html: Added.

Can you please add one little test for the ASCII Case Insensitive aspect of the algorithm?

It does not have to be big, one positive and one negative case would be enough. It is just to have a specific test for this aspect.

> LayoutTests/fast/selectors/lang-valid-extended-filtering.html:107
> +    "i-klingon",

hehe, I did not think of this use case :)

[Dhi Aurrahman]

Comment on attachment 243089 [details]
Patch

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

> LayoutTests/ChangeLog:12
> +

Do you think, given <div lang="de-Latn-DE" class="de"></div>

Test it with:
    shouldBe('document.querySelectorAll(":lang(de-Latn-DE)").length', '1');
    shouldBe('document.querySelectorAll(":lang(DE-LATN-DE)").length', '1');
    shouldBe('document.querySelectorAll(":lang(de-latn-de)").length', '1');
    shouldBe('document.querySelectorAll(":lang(DE-LATN-DF)").length', '0');
    shouldBe('document.querySelectorAll(":lang(DE-LATN-DÈ)").length', '0');
    shouldBe('document.querySelectorAll(":lang(de-Latn-DÈ)").length', '0');

will be enough?

[Benjamin Poulain]

(In reply to comment #18)
> Comment on attachment 243089 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243089&action=review
> 
> > LayoutTests/ChangeLog:12
> > +
> 
> Do you think, given <div lang="de-Latn-DE" class="de"></div>
> 
> Test it with:
>     shouldBe('document.querySelectorAll(":lang(de-Latn-DE)").length', '1');
>     shouldBe('document.querySelectorAll(":lang(DE-LATN-DE)").length', '1');
>     shouldBe('document.querySelectorAll(":lang(de-latn-de)").length', '1');
>     shouldBe('document.querySelectorAll(":lang(DE-LATN-DF)").length', '0');
>     shouldBe('document.querySelectorAll(":lang(DE-LATN-DÈ)").length', '0');
>     shouldBe('document.querySelectorAll(":lang(de-Latn-DÈ)").length', '0');
> 
> will be enough?

I would like a case making sure uppercase and lowercase non-ASCII are not equal. Any of "é" "É", "ç" "Ç", "ø" "Ø", etc.

Something as simple as:
    "Eé" == "eé"
    "Eé" != "EÉ"
    "Eé" != "eÉ"

[Dhi Aurrahman]

Created attachment 243174 [details]
Patch

[Benjamin Poulain]

I would prefer a separate test than extending the other ones. There is value in having small targeted tests: they are easier to analyze if they ever start failing.

[Dhi Aurrahman]

Created attachment 243177 [details]
Patch

[Benjamin Poulain]

Comment on attachment 243177 [details]
Patch

I am very very confused by lang-equal-ignoring-ascii-case.html
Is the review tool messing up the encoding?

[Benjamin Poulain]

(In reply to comment #23)
> Comment on attachment 243177 [details]
> Patch
> 
> I am very very confused by lang-equal-ignoring-ascii-case.html
> Is the review tool messing up the encoding?

Ok, yes it is :)

[Dhi Aurrahman]

Comment on attachment 243177 [details]
Patch

Put it on review

[Dhi Aurrahman]

Created attachment 243332 [details]
Patch

[Benjamin Poulain]

Comment on attachment 243332 [details]
Patch

Everything looks correct to me. The test coverage is excellent. Let's land.

[WebKit Commit Bot]

Comment on attachment 243332 [details]
Patch

Clearing flags on attachment: 243332

Committed r177333: <http://trac.webkit.org/changeset/177333>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.