Bug 139340

Summary: Extend :lang()'s selector checker to handle ranges with '*' properly and perform matching within the ASCII range
Product: WebKit Reporter: Dhi Aurrahman <diorahman>
Component: New BugsAssignee: Dhi Aurrahman <diorahman>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, commit-queue, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews105 for mac-mountainlion-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Dhi Aurrahman 2014-12-06 05:27:01 PST
Consider asterisk containing range as input for the extended filtering of :lang()'s selector checker
Comment 1 Dhi Aurrahman 2014-12-06 05:44:21 PST
Created attachment 242713 [details]
Patch
Comment 2 Dhi Aurrahman 2014-12-06 06:01:10 PST
Created attachment 242714 [details]
Patch
Comment 3 Dhi Aurrahman 2014-12-09 00:33:26 PST
Created attachment 242889 [details]
Patch
Comment 4 Dhi Aurrahman 2014-12-09 00:50:38 PST
Created attachment 242892 [details]
Patch
Comment 5 Dhi Aurrahman 2014-12-09 21:18:18 PST
Created attachment 242987 [details]
Patch
Comment 6 Build Bot 2014-12-09 21:49:56 PST
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
Comment 7 Build Bot 2014-12-09 21:49:58 PST
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
Comment 8 Dhi Aurrahman 2014-12-10 01:26:32 PST
Created attachment 242999 [details]
Patch
Comment 9 Darin Adler 2014-12-10 10:18:24 PST
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.
Comment 10 Dhi Aurrahman 2014-12-10 13:54:41 PST
Created attachment 243063 [details]
Patch
Comment 11 Dhi Aurrahman 2014-12-10 14:19:07 PST
Created attachment 243064 [details]
Patch
Comment 12 Dhi Aurrahman 2014-12-10 14:33:04 PST
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.
Comment 13 Dhi Aurrahman 2014-12-10 14:36:47 PST
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.
Comment 14 Dhi Aurrahman 2014-12-10 14:43:28 PST
Sorry, for multiple posts, I was connected to bad internet connection.
Comment 15 Darin Adler 2014-12-10 17:15:17 PST
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?
Comment 16 Dhi Aurrahman 2014-12-10 19:39:22 PST
Created attachment 243089 [details]
Patch
Comment 17 Benjamin Poulain 2014-12-11 18:20:36 PST
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 :)
Comment 18 Dhi Aurrahman 2014-12-11 18:37:26 PST
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?
Comment 19 Benjamin Poulain 2014-12-11 19:54:26 PST
(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É"
Comment 20 Dhi Aurrahman 2014-12-11 20:31:25 PST
Created attachment 243174 [details]
Patch
Comment 21 Benjamin Poulain 2014-12-11 21:17:47 PST
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.
Comment 22 Dhi Aurrahman 2014-12-11 22:10:35 PST
Created attachment 243177 [details]
Patch
Comment 23 Benjamin Poulain 2014-12-12 15:46:33 PST
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?
Comment 24 Benjamin Poulain 2014-12-12 15:47:49 PST
(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 :)
Comment 25 Dhi Aurrahman 2014-12-15 16:49:14 PST
Comment on attachment 243177 [details]
Patch

Put it on review
Comment 26 Dhi Aurrahman 2014-12-15 17:11:05 PST
Created attachment 243332 [details]
Patch
Comment 27 Benjamin Poulain 2014-12-15 21:20:45 PST
Comment on attachment 243332 [details]
Patch

Everything looks correct to me. The test coverage is excellent. Let's land.
Comment 28 WebKit Commit Bot 2014-12-15 21:55:55 PST
Comment on attachment 243332 [details]
Patch

Clearing flags on attachment: 243332

Committed r177333: <http://trac.webkit.org/changeset/177333>
Comment 29 WebKit Commit Bot 2014-12-15 21:55:59 PST
All reviewed patches have been landed.  Closing bug.