Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1135003004:
Make small-caps work correctly with tr locale (Closed)
|
Created:
5 years, 7 months ago by rwlbuis Modified:
5 years, 6 months ago CC:
blink-reviews, krit, Rik, dshwang, jbroman, danakj, pdr+graphicswatchlist_chromium.org, f(malita), aandrey+blink_chromium.org, Stephen Chennney, Mikhail, blink-reviews-wtf_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionMake small-caps work correctly with tr locale
We correctly support tr upper/lower casing rules
for Strings through StringImpl::lower/upper methods
that take a locale parameter. However for upper/lowering
case for single Char32, which the small-caps code
path uses, there is no such overload, so add that to wtf.
Behavior matches Firefox.
BUG=395369
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196325
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196432
Patch Set 1 #Patch Set 2 : Add NeedsRebaseline #Patch Set 3 : Add virtual/slimmingpaint test result #Patch Set 4 : Rebase against ToT #Patch Set 5 : Remove rawLocale #Patch Set 6 : Add boolean param #
Total comments: 1
Patch Set 7 : Patch for landing #
Total comments: 12
Patch Set 8 : Address review comments #
Total comments: 1
Patch Set 9 : Fix TODOs #
Total comments: 1
Patch Set 10 : Patch for landing #Patch Set 11 : another try #Patch Set 12 : Remove characters8 change #Patch Set 13 : add back TestExpectations change #Messages
Total messages: 80 (43 generated)
Patchset #2 (id:20001) has been deleted
Patchset #4 (id:80001) has been deleted
rob.buis@samsung.com changed reviewers: + eae@chromium.org
PTAL. Sadly I think we still need pixel tests here...
On 2015/05/19 15:59:02, rwlbuis wrote: > PTAL. Sadly I think we still need pixel tests here... ping?:)
What's rawLocale and how is it different from locale?
On 2015/05/26 22:41:17, eae wrote: > What's rawLocale and how is it different from locale? It does not return the default locale when none is set like locale method does. Basically in toUpper I am trying to avoid comparisons with tr/az for the default case, but it feels a bit hacky to compare to the default (but not exposed) en locale there. This is probably not critical for small-caps code path but it may become so for other (future) code paths. Let me know what you think :)
On 2015/05/26 23:27:25, rwlbuis wrote: > On 2015/05/26 22:41:17, eae wrote: > > What's rawLocale and how is it different from locale? > > It does not return the default locale when none is set like locale method does. > > Basically in toUpper I am trying to avoid comparisons with tr/az for the default > case, but it feels a bit hacky to compare to the default (but not exposed) en > locale there. > > This is probably not critical for small-caps code path but it may become so for > other (future) code paths. Let me know what you think :) ok, how about modifying locale to take a "bool includeDefault = true" param instead?
On 2015/05/27 00:17:33, eae wrote: > On 2015/05/26 23:27:25, rwlbuis wrote: > > On 2015/05/26 22:41:17, eae wrote: > > > What's rawLocale and how is it different from locale? > > > > It does not return the default locale when none is set like locale method > does. > > > > Basically in toUpper I am trying to avoid comparisons with tr/az for the > default > > case, but it feels a bit hacky to compare to the default (but not exposed) en > > locale there. > > > > This is probably not critical for small-caps code path but it may become so > for > > other (future) code paths. Let me know what you think :) > > ok, how about modifying locale to take a "bool includeDefault = true" param > instead? Sounds good, I modified it in my latest patch, PTAL. BTW I wonder why this defaulting is needed, I don't really like it. But I guess one of the code paths needs it.
LGTM w/nit https://codereview.chromium.org/1135003004/diff/140001/Source/platform/fonts/... File Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/1135003004/diff/140001/Source/platform/fonts/... Source/platform/fonts/Font.cpp:430: UChar32 upperC = toUpper(c, m_fontDescription.locale(false)); bool includeDefault = false; UChar32 upperC = toUpper(c, m_fontDescription.locale(includeDefault));
The CQ bit was checked by rob.buis@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/1135003004/#ps160001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135003004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)
rob.buis@samsung.com changed reviewers: + tkent@chromium.org
Adding tkent for the wtf changes, PTAL
Patchset #7 (id:160001) has been deleted
https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... File Source/wtf/text/StringImpl.cpp (left): https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... Source/wtf/text/StringImpl.cpp:533: data8[i] = static_cast<LChar>(Unicode::toLower(characters8()[i])); I prefer keeping Unicode::. We have Unicode::toLower, StringImpl::lower, Unicode::toUpper, StringImpl::upper, StringImpl::toUpper. They are confusing. https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... File Source/wtf/text/StringImpl.cpp (right): https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... Source/wtf/text/StringImpl.cpp:2103: return 0x130; // Latin capital letter i with dot above Please add latinCapitalLetterIWithDotAbove to wtf/unicode/CharacterNames.h. https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... Source/wtf/text/StringImpl.cpp:2104: if (c == 0x131) // Latin small letter dotless i Please add latinSmallLetterDotlessI to wtf/unicode/CharacterNames.h. https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... Source/wtf/text/StringImpl.cpp:2107: // FIXME FIXME -> TODO(rob.buis): blah blah https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... File Source/wtf/text/StringImpl.h (right): https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... Source/wtf/text/StringImpl.h:759: WTF_EXPORT UChar32 toUpper(UChar32, const AtomicString& localeIdentifier); This function needs a documentation. StringImpl.{h,cpp} shouldn't have this function, which is unrelated to StringImpl. I'm not sure where is the best place. A candidate is wtf/unicode/. But adding AtomicString dependency to wtf/unicode/ is also weird.
Patchset #8 (id:200001) has been deleted
PTAL :) https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... File Source/wtf/text/StringImpl.cpp (left): https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... Source/wtf/text/StringImpl.cpp:533: data8[i] = static_cast<LChar>(Unicode::toLower(characters8()[i])); On 2015/05/28 00:38:26, tkent wrote: > I prefer keeping Unicode::. We have Unicode::toLower, StringImpl::lower, > Unicode::toUpper, StringImpl::upper, StringImpl::toUpper. They are confusing. Yes, I should not have included it. My idea was to make it consistent. Fixed. https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... File Source/wtf/text/StringImpl.cpp (right): https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... Source/wtf/text/StringImpl.cpp:2103: return 0x130; // Latin capital letter i with dot above On 2015/05/28 00:38:26, tkent wrote: > Please add latinCapitalLetterIWithDotAbove to wtf/unicode/CharacterNames.h. Done. https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... Source/wtf/text/StringImpl.cpp:2104: if (c == 0x131) // Latin small letter dotless i On 2015/05/28 00:38:26, tkent wrote: > Please add latinSmallLetterDotlessI to wtf/unicode/CharacterNames.h. Done. https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... Source/wtf/text/StringImpl.cpp:2107: // FIXME On 2015/05/28 00:38:26, tkent wrote: > FIXME -> TODO(rob.buis): blah blah In this case, I don't think I want to put my name there. I would not mind working on it, but I don't know the language and don't know of a bug/testcase for it. It is more a reminder that we'll have to do something based on StringImpl::upper. https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... File Source/wtf/text/StringImpl.h (right): https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... Source/wtf/text/StringImpl.h:759: WTF_EXPORT UChar32 toUpper(UChar32, const AtomicString& localeIdentifier); On 2015/05/28 00:38:26, tkent wrote: > This function needs a documentation. > > StringImpl.{h,cpp} shouldn't have this function, which is unrelated to > StringImpl. > > I'm not sure where is the best place. A candidate is wtf/unicode/. But adding > AtomicString dependency to wtf/unicode/ is also weird. Unfortunately I didn't find a place. It is especially hard because: 1. it should be inline 2. needs AtomicString
Patchset #8 (id:220001) has been deleted
https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... File Source/wtf/text/StringImpl.cpp (right): https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... Source/wtf/text/StringImpl.cpp:2107: // FIXME On 2015/05/28 23:03:07, rwlbuis wrote: > On 2015/05/28 00:38:26, tkent wrote: > > FIXME -> TODO(rob.buis): blah blah > > In this case, I don't think I want to put my name there. I would not mind > working on it, but I don't know the language and don't know of a bug/testcase > for it. It is more a reminder that we'll have to do something based on > StringImpl::upper. Please add TODO(rub.buis). It doesn't mean you'll fix it. Please refer to the discussion on blink-dev. https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... File Source/wtf/text/StringImpl.h (right): https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... Source/wtf/text/StringImpl.h:759: WTF_EXPORT UChar32 toUpper(UChar32, const AtomicString& localeIdentifier); Let's put it here, and add TODO(rub.buis) about finding the best location, and add a document of this functions.
Patchset #9 (id:260001) has been deleted
On 2015/05/29 05:49:51, tkent wrote: > https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... > File Source/wtf/text/StringImpl.cpp (right): > > https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... > Source/wtf/text/StringImpl.cpp:2107: // FIXME > On 2015/05/28 23:03:07, rwlbuis wrote: > > On 2015/05/28 00:38:26, tkent wrote: > > > FIXME -> TODO(rob.buis): blah blah > > > > In this case, I don't think I want to put my name there. I would not mind > > working on it, but I don't know the language and don't know of a bug/testcase > > for it. It is more a reminder that we'll have to do something based on > > StringImpl::upper. > > Please add TODO(rub.buis). > It doesn't mean you'll fix it. Please refer to the discussion on blink-dev. Ah, I missed that, thanks! Done. https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... > File Source/wtf/text/StringImpl.h (right): > > https://codereview.chromium.org/1135003004/diff/180001/Source/wtf/text/String... > Source/wtf/text/StringImpl.h:759: WTF_EXPORT UChar32 toUpper(UChar32, const > AtomicString& localeIdentifier); > Let's put it here, and add TODO(rub.buis) about finding the best location, and > add a document of this functions. Done.
lgtm https://codereview.chromium.org/1135003004/diff/280001/Source/wtf/text/String... File Source/wtf/text/StringImpl.cpp (right): https://codereview.chromium.org/1135003004/diff/280001/Source/wtf/text/String... Source/wtf/text/StringImpl.cpp:2106: // TODO(rob.buis) Please add information about what we need.
The CQ bit was checked by rob.buis@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org, tkent@chromium.org Link to the patchset: https://codereview.chromium.org/1135003004/#ps300001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135003004/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/64586)
The CQ bit was unchecked by rob.buis@samsung.com
The CQ bit was checked by rob.buis@samsung.com
The CQ bit was checked by rob.buis@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135003004/300001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for LayoutTests/TestExpectations: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file LayoutTests/TestExpectations Hunk #1 FAILED at 4143. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/TestExpectations.rej Patch: LayoutTests/TestExpectations Index: LayoutTests/TestExpectations diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations index e9e31a52f92f75a9e1d7ec08f1ceea422a9df5df..a995e44f7bc7a413db20602c3e51890a2331efd9 100644 --- a/LayoutTests/TestExpectations +++ b/LayoutTests/TestExpectations @@ -4143,3 +4143,6 @@ crbug.com/321237 [ Mac ] virtual/antialiasedtext/fast/text/international/thai-ba crbug.com/321237 [ Mac ] fast/text/international/complex-text-leading-space-wrapping.html [ ImageOnlyFailure ] crbug.com/321237 [ Mac ] virtual/antialiasedtext/fast/text/international/complex-text-leading-space-wrapping.html [ ImageOnlyFailure ] crbug.com/321237 [ Win ] virtual/antialiasedtext/fast/text/international/complex-text-leading-space-wrapping.html [ ImageOnlyFailure ] + +crbug.com/395369 fast/text/small-caps-turkish.html [ NeedsRebaseline ] +crbug.com/395369 virtual/antialiasedtext/fast/text/small-caps-turkish.html [ NeedsRebaseline ]
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by rob.buis@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/1135003004/#ps320001 (title: "Another try")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135003004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)
Patchset #11 (id:320001) has been deleted
The CQ bit was checked by rob.buis@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/1135003004/#ps340001 (title: "Another try")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135003004/340001
Patchset #11 (id:340001) has been deleted
The CQ bit was checked by rob.buis@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/1135003004/#ps360001 (title: "Another try")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135003004/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/57286)
Patchset #11 (id:360001) has been deleted
The CQ bit was checked by rob.buis@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/1135003004/#ps380001 (title: "Another try")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135003004/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)
Patchset #11 (id:380001) has been deleted
The CQ bit was checked by rob.buis@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/1135003004/#ps400001 (title: "another try")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135003004/400001
Message was sent while issue was closed.
Committed patchset #11 (id:400001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196325
Message was sent while issue was closed.
On 2015/06/02 16:13:19, commit-bot: I haz the power wrote: > Committed patchset #11 (id:400001) as > https://src.chromium.org/viewvc/blink?view=rev&revision=196325 This patch has ASAN errors: https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_ASA... The layout tests are flaky (dot above 'I' does not always show).
Message was sent while issue was closed.
On 2015/06/02 20:34:07, Justin Novosad wrote: > On 2015/06/02 16:13:19, commit-bot: I haz the power wrote: > > Committed patchset #11 (id:400001) as > > https://src.chromium.org/viewvc/blink?view=rev&revision=196325 > > This patch has ASAN errors: > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_ASA... > The layout tests are flaky (dot above 'I' does not always show). reverted: https://src.chromium.org/viewvc/blink?view=rev&revision=196344
Message was sent while issue was closed.
https://codereview.chromium.org/1135003004/diff/240001/Source/platform/fonts/... File Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/1135003004/diff/240001/Source/platform/fonts/... Source/platform/fonts/Font.cpp:431: UChar32 upperC = toUpper(c, m_fontDescription.locale(includeDefault).characters8()); Why did you add .characters8()? It's wrong.
On 2015/06/02 22:42:20, tkent wrote: > https://codereview.chromium.org/1135003004/diff/240001/Source/platform/fonts/... > File Source/platform/fonts/Font.cpp (right): > > https://codereview.chromium.org/1135003004/diff/240001/Source/platform/fonts/... > Source/platform/fonts/Font.cpp:431: UChar32 upperC = toUpper(c, > m_fontDescription.locale(includeDefault).characters8()); > Why did you add .characters8()? It's wrong. Ouch, I think I applied an old incorrect local patchset after #7. Will fix, thanks for pointing it out.
The CQ bit was checked by rob.buis@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/1135003004/#ps420001 (title: "Remove characters8 change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135003004/420001
The CQ bit was unchecked by rob.buis@samsung.com
The CQ bit was checked by rob.buis@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/1135003004/#ps440001 (title: "add back TestExpectations change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135003004/440001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)
The CQ bit was checked by rob.buis@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135003004/440001
Message was sent while issue was closed.
Committed patchset #13 (id:440001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196432 |
