Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1955523002:
Fallback on system locale for font selection (Closed)
|
Created:
4 years, 7 months ago by qi1988.yang Modified:
4 years, 6 months ago CC:
chromium-reviews, krit, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, vmpstr+blinkwatch_chromium.org, dshwang, jbroman, Justin Novosad, blink-reviews-platform-graphics_chromium.org, Rik, Stephen Chennney, blink-reviews, f(malita), danakj+watch_chromium.org, kinuko+watch, h.joshi Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFallback on system locale for font selection
If two character have the same unicode, Font will depend on the language attribute.
But if Font do not get locale from net response and Html tag, it should be defaultLanguage.
It should not only be EN.
BUG=609043
Committed: https://crrev.com/7ae70b716aa5221221412e35796d07b5cb852292
Cr-Commit-Position: refs/heads/master@{#394093}
Patch Set 1 #Patch Set 2 : Fallback on system locale for font selection #Patch Set 3 : add testcase #
Total comments: 1
Patch Set 4 : modify testcase #
Total comments: 1
Patch Set 5 : add a doctype in the testcase #Patch Set 6 : #Patch Set 7 : add defalut locale of PPapi platform #Patch Set 8 : add default locale of PPAPI Platform #Patch Set 9 : #
Messages
Total messages: 57 (24 generated)
qi1988.yang@samsung.com changed reviewers: + eae@chromium.org, pxinchao@gmail.com
Change itself makes sense but please add a test and reword the title to something like "Fallback on system locale for font selection".
On 2016/05/05 13:18:10, eae wrote: > Change itself makes sense but please add a test and reword the title to > something like "Fallback on system locale for font selection". I also wonder if there is some overlap with crbug.com/179331? Seems like this implementation is proposed there.
Description was changed from ========== The defaultlocale of Font from system platform If two character have the same unicode, Font will depend on the langauge attribute. But if Font do not get locale from net response and Html tag, it should be defaultLanguage. It should not only be EN. BUG=609043 ========== to ========== The defaultlocale of Font from system platform If two character have the same unicode, Font will depend on the langauge attribute. But if Font do not get locale from net response and Html tag, it should be defaultLanguage. It should not only be EN. BUG=609043 ==========
On 2016/05/05 13:18:10, eae wrote: > Change itself makes sense but please add a test and reword the title to > something like "Fallback on system locale for font selection". In the testcase of https://codereview.chromium.org/276573010 , झ have two different character in Hindi and JHA . It show according by the lang attribute . But if the tag do not have the lang attribute , I think that it also show right charater when user viste the page in the Hindi and JHA.
Description was changed from ========== The defaultlocale of Font from system platform If two character have the same unicode, Font will depend on the langauge attribute. But if Font do not get locale from net response and Html tag, it should be defaultLanguage. It should not only be EN. BUG=609043 ========== to ========== Fallback on system locale for font selection If two character have the same unicode, Font will depend on the langauge attribute. But if Font do not get locale from net response and Html tag, it should be defaultLanguage. It should not only be EN. BUG=609043 ==========
On 2016/05/06 03:31:31, pxinchao wrote: > On 2016/05/05 13:18:10, eae wrote: > > Change itself makes sense but please add a test and reword the title to > > something like "Fallback on system locale for font selection". > > In the testcase of https://codereview.chromium.org/276573010 , > झ have two different character in Hindi and JHA . > It show according by the lang attribute . > But if the tag do not have the lang attribute , I think that it also show > right charater when user viste the page in the Hindi and JHA. For example <html> ... <p style="font-family: NotoX">Hindi:JHA झ </p><br> </html> In Hindi , browser should show "झ". In Nepali, browser should show other character.
On 2016/05/06 05:25:10, pxinchao wrote: > On 2016/05/06 03:31:31, pxinchao wrote: > > On 2016/05/05 13:18:10, eae wrote: > > > Change itself makes sense but please add a test and reword the title to > > > something like "Fallback on system locale for font selection". > > > > In the testcase of https://codereview.chromium.org/276573010 , > > झ have two different character in Hindi and JHA . > > It show according by the lang attribute . > > But if the tag do not have the lang attribute , I think that it also show > > right charater when user viste the page in the Hindi and JHA. > > For example > <html> > ... > <p style="font-family: NotoX">Hindi:JHA झ </p><br> > </html> > In Hindi , browser should show "झ". > In Nepali, browser should show other character. @pxinchao that is great, but some layout test has to be added, probably to LayoutTests/fast/text, and probably it should be a reference test.
Description was changed from ========== Fallback on system locale for font selection If two character have the same unicode, Font will depend on the langauge attribute. But if Font do not get locale from net response and Html tag, it should be defaultLanguage. It should not only be EN. BUG=609043 ========== to ========== Fallback on system locale for font selection If two character have the same unicode, Font will depend on the language attribute. But if Font do not get locale from net response and Html tag, it should be defaultLanguage. It should not only be EN. BUG=609043 ==========
rob.buis@samsung.com changed reviewers: + rob.buis@samsung.com
Thanks for adding the tests, I spotted one mistake. Don't you want to add one for chinese too? https://codereview.chromium.org/1955523002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/text/same-script-different-lang-in-Nepali-expected.html (right): https://codereview.chromium.org/1955523002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/text/same-script-different-lang-in-Nepali-expected.html:15: <p lang="hi" style="font-family: NotoX">JHA झ </p><br> Looks like it should be "ne": http://www.w3schools.com/tags/ref_language_codes.asp
On 2016/05/09 19:44:31, rwlbuis wrote: > Thanks for adding the tests, I spotted one mistake. Don't you want to add one > for chinese too? I have a little trouble of adding testcase for chinese . I don't have the suitable web font file of chinese . And i think the testcase has been enough. So i don't have plan to add it now. > https://codereview.chromium.org/1955523002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/text/same-script-different-lang-in-Nepali-expected.html:15: > <p lang="hi" style="font-family: NotoX">JHA झ </p><br> > Looks like it should be "ne": > http://www.w3schools.com/tags/ref_language_codes.asp Thanks for your help . You are right . I immediate motify the same-script-different-lang-in-Nepali-expected.html.
On 2016/05/10 01:31:24, pxinchao wrote: > I have a little trouble of adding testcase for chinese . I don't have the > suitable web font file of chinese . > And i think the testcase has been enough. So i don't have plan to add it now. Ok, I'll leave that one to eae since he can decide better if it is a needed testcase for this CL and if so maybe has a suggestion how to fix above problem. > Thanks for your help . You are right . I immediate motify the > same-script-different-lang-in-Nepali-expected.html. Thanks! Peer looks good to me.
On 2016/05/11 02:02:50, pxinchao wrote: hi , eae , How about you about the patch?
On 2016/05/11 02:07:21, pxinchao wrote: > On 2016/05/11 02:02:50, pxinchao wrote: > > hi , eae , How about you about the patch? pxinchao, we usually do that like this: :) ping eae
LGTM w/nit https://codereview.chromium.org/1955523002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/text/same-script-different-lang-in-Hindi.html (right): https://codereview.chromium.org/1955523002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/text/same-script-different-lang-in-Hindi.html:1: <html> Please add a doctype to prevent quirks mode. <!DOCTYPE html>
On 2016/05/12 18:28:28, eae wrote: > LGTM w/nit > > https://codereview.chromium.org/1955523002/diff/60001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/fast/text/same-script-different-lang-in-Hindi.html > (right): > > https://codereview.chromium.org/1955523002/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/text/same-script-different-lang-in-Hindi.html:1: > <html> > Please add a doctype to prevent quirks mode. > > <!DOCTYPE html> Thank you. I will add a doctype . Please check and review Path Set 5. Thanks !
The CQ bit was checked by qi1988.yang@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/1955523002/#ps80001 (title: "add a doctype in the testcase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955523002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955523002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by qi1988.yang@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/1955523002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955523002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955523002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by qi1988.yang@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/1955523002/#ps120001 (title: "add defalut locale of PPapi platform")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955523002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955523002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
qi1988.yang@samsung.com changed reviewers: + piman@chromium.org
hi , piman. I add the default "en" in the local of PPAPI platform. It will fix the faile of OutOfProcessPPAPITest.BrowserFont. [1:1:0515/192636:21206662454:FATAL:ppapi_blink_platform_impl.cc(203)] Check failed: false. #0 0x000000d6cfee base::debug::StackTrace::StackTrace() #1 0x000000d7b57a logging::LogMessage::~LogMessage() #2 0x000003f7fb79 content::PpapiBlinkPlatformImpl::defaultLocale() #3 0x000001df0b3c blink::platformLanguage() #4 0x000001df0a59 blink::defaultLanguage() #5 0x000001dcdb2e blink::FontDescription::locale() #6 0x0000043a3e9a blink::HarfBuzzShaper::shapeResult() #7 0x0000043a0556 blink::CachingWordShapeIterator::shapeWordWithoutSpacing() #8 0x0000043a01a2 blink::CachingWordShapeIterator::shapeWord() #9 0x0000043a0262 blink::CachingWordShapeIterator::shapeToEndIndex() #10 0x00000439f927 blink::CachingWordShapeIterator::next() #11 0x00000439f79b blink::CachingWordShaper::width() #12 0x0000043990ac blink::Font::width() #13 0x000004394a18 blink::WebFont::calculateWidth() #14 0x00000373e37c content::BrowserFontResource_Trusted::MeasureText() #15 0x000000c99b75 ppapi::thunk::(anonymous namespace)::MeasureText()
lgtm lgtm
lgtm
The CQ bit was checked by qi1988.yang@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/1955523002/#ps140001 (title: "add default locale of PPAPI Platform")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955523002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955523002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
hi , piman. I motified the function PpapiBlinkPlatformImpl::defaultLocale(), Please review it againt . Thank you
lgtm
The CQ bit was checked by qi1988.yang@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/1955523002/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955523002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955523002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by qi1988.yang@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1955523002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1955523002/160001
Message was sent while issue was closed.
Description was changed from ========== Fallback on system locale for font selection If two character have the same unicode, Font will depend on the language attribute. But if Font do not get locale from net response and Html tag, it should be defaultLanguage. It should not only be EN. BUG=609043 ========== to ========== Fallback on system locale for font selection If two character have the same unicode, Font will depend on the language attribute. But if Font do not get locale from net response and Html tag, it should be defaultLanguage. It should not only be EN. BUG=609043 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Fallback on system locale for font selection If two character have the same unicode, Font will depend on the language attribute. But if Font do not get locale from net response and Html tag, it should be defaultLanguage. It should not only be EN. BUG=609043 ========== to ========== Fallback on system locale for font selection If two character have the same unicode, Font will depend on the language attribute. But if Font do not get locale from net response and Html tag, it should be defaultLanguage. It should not only be EN. BUG=609043 Committed: https://crrev.com/7ae70b716aa5221221412e35796d07b5cb852292 Cr-Commit-Position: refs/heads/master@{#394093} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/7ae70b716aa5221221412e35796d07b5cb852292 Cr-Commit-Position: refs/heads/master@{#394093} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
