Consider asterisk containing range as input for the extended filtering of :lang()'s selector checker
Created attachment 242713 [details] Patch
Created attachment 242714 [details] Patch
Created attachment 242889 [details] Patch
Created attachment 242892 [details] Patch
Created attachment 242987 [details] Patch
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
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
Created attachment 242999 [details] Patch
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.
Created attachment 243063 [details] Patch
Created attachment 243064 [details] Patch
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 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.
Sorry, for multiple posts, I was connected to bad internet connection.
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?
Created attachment 243089 [details] Patch
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 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?
(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É"
Created attachment 243174 [details] Patch
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.
Created attachment 243177 [details] Patch
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?
(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 on attachment 243177 [details] Patch Put it on review
Created attachment 243332 [details] Patch
Comment on attachment 243332 [details] Patch Everything looks correct to me. The test coverage is excellent. Let's land.
Comment on attachment 243332 [details] Patch Clearing flags on attachment: 243332 Committed r177333: <http://trac.webkit.org/changeset/177333>
All reviewed patches have been landed. Closing bug.