Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
276573010:
Adding Locale (language attribute) information to font and using the (Closed)
|
Created:
6 years, 7 months ago by h.joshi Modified:
6 years, 6 months ago CC:
blink-reviews, jamesr, krit, danakj, dsinclair, ed+blinkwatch_opera.com, jbroman, ojan, dglazkov+blink, Rik, apavlov+blink_chromium.org, darktears, Stephen Chennney, rune+blink, pdr., rwlbuis, behdad, jungshik at Google, eseidel, bashi, behdad_google_chromium.org, zkov_chromium.org, del_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdding Locale (language attribute) information to font and using the
same while shaping Complex text. More than one langauge can point to
single Script and single font can have support for different languages.
BUG=372502
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175124
Patch Set 1 #
Total comments: 13
Patch Set 2 : Patch with comment fixes and moving NotoSans ttf to new patch #Patch Set 3 : Changing font name in test case as per comment in 267423005 code review #
Total comments: 2
Patch Set 4 : Comment Fix #Patch Set 5 : Comment fix #
Total comments: 18
Patch Set 6 : Comment fix and reverting to Old code #
Total comments: 21
Patch Set 7 : Comment fixes #Patch Set 8 : Updating test case #Patch Set 9 : Updating TestExpectation file #Patch Set 10 : Fixing Test Expectation Issue #Patch Set 11 : Updating Test Expectation file #Patch Set 12 : Test Expectation Update #Patch Set 13 : Updating Test Expectation file as Bots took more time and Test Expection files were outdated #Patch Set 14 : Updating Test Expectation file due to recent commits #Messages
Total messages: 66 (0 generated)
PTAL
lgtm This looks great! Ideally we shouldn't keep language in the font. But I think we should commit this and refactor later.
Thank you, will commit this and then start with your comment to refactor code.
The CQ bit was checked by h.joshi@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/h.joshi@samsung.com/276573010/1
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
I am getting "No LGTM from a valid reviewer yet" error when trying to commit changes.
On 2014/05/13 17:19:23, h.joshi wrote: > I am getting "No LGTM from a valid reviewer yet" error when trying to commit > changes. Wait for eae@.
You could add the test case from this older WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=37984 (Serbian vs. regular cyrillic forms depending on lang.) Do you need the extra font? If so, please put it in /LayoutTests/third_party/NotoSans/ with license and README for justification. Otherwise, looks good to me, too. - Agree with Behdad on refactoring where to keep lang info.
Took test case from Behdad site (same is mentioned in bug as well) Using extra font was done so that we do not depend on platform. In old Webkit bug they are using "font-family:'DejaVu Serif';", this may work for Linux distributions. To upload font, will make another patch and add "dpranke" as reviewer (taken from comment in "https://codereview.chromium.org/263363005/") Pls suggest, should I start making another patch to upload font or should I use test case from Old Webkit bug?
On 2014/05/14 07:21:43, h.joshi wrote: > Took test case from Behdad site (same is mentioned in bug as well) > Using extra font was done so that we do not depend on platform. In old Webkit > bug they are using "font-family:'DejaVu Serif';", this may work for Linux > distributions. If it works on all relevant platform bots without an extra font and you can remove it from your test case and the patch. Dirk mentioned he would like to keep the number of fonts we include in the repo minimal. He commented on my patches earlier, that we should avoid adding a font for every feature we test. But it may be that we can't avoid it in this case, your call. I should have mentioned this in my first review: It would be good to put a description of the expected visual result in the test case, e.g. "The glyph for JHA should look different depending on Nepali and Hindi, depending on the lang attribute." so that someone debugging a potential failure can get an idea of the expected result.
Sorry for being nagging, but some comments. https://codereview.chromium.org/276573010/diff/1/LayoutTests/fast/text/shapin... File LayoutTests/fast/text/shaping/same_script_different_lang.html (right): https://codereview.chromium.org/276573010/diff/1/LayoutTests/fast/text/shapin... LayoutTests/fast/text/shaping/same_script_different_lang.html:1: <!DOCTYPE HTML> nit: please-use-hyphen-for-layout-tests like this instead of using underscores https://codereview.chromium.org/276573010/diff/1/LayoutTests/fast/text/shapin... LayoutTests/fast/text/shaping/same_script_different_lang.html:7: font-family: NotoX; nit: 4 spaces for indent https://codereview.chromium.org/276573010/diff/1/Source/platform/fonts/FontDe... File Source/platform/fonts/FontDescription.h (right): https://codereview.chromium.org/276573010/diff/1/Source/platform/fonts/FontDe... Source/platform/fonts/FontDescription.h:84: , m_locale(String("en")) Assuming lang=en is OK? Also, I'm not sure we can add a String into this class, as some members are bit fields, which indicates that there might be a lot of instances of this class and adding a field might impact memory consumption. https://codereview.chromium.org/276573010/diff/1/Source/platform/fonts/FontDe... Source/platform/fonts/FontDescription.h:120: String locale() const { return static_cast<String>(m_locale); } Why static_cast<>? https://codereview.chromium.org/276573010/diff/1/Source/platform/fonts/harfbu... File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right): https://codereview.chromium.org/276573010/diff/1/Source/platform/fonts/harfbu... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:809: hb_buffer_set_language(harfBuzzBuffer.get(), hb_language_from_string((const char*)(static_cast<const LChar *>(localeStr.characters8())), localeStr.length())); What happens when localeStr contains invalid value? Won't that happen?
@Dominik: Will make changes to HTML test case and add suggested comment to it.
Will start with the suggested changes and upload new patch. https://codereview.chromium.org/276573010/diff/1/LayoutTests/fast/text/shapin... File LayoutTests/fast/text/shaping/same_script_different_lang.html (right): https://codereview.chromium.org/276573010/diff/1/LayoutTests/fast/text/shapin... LayoutTests/fast/text/shaping/same_script_different_lang.html:1: <!DOCTYPE HTML> Okey, will use hyphen for test case name On 2014/05/14 10:00:01, bashi1 wrote: > nit: please-use-hyphen-for-layout-tests like this instead of using underscores https://codereview.chromium.org/276573010/diff/1/LayoutTests/fast/text/shapin... LayoutTests/fast/text/shaping/same_script_different_lang.html:7: font-family: NotoX; okey, will correct indentation On 2014/05/14 10:00:01, bashi1 wrote: > nit: 4 spaces for indent https://codereview.chromium.org/276573010/diff/1/Source/platform/fonts/FontDe... File Source/platform/fonts/FontDescription.h (right): https://codereview.chromium.org/276573010/diff/1/Source/platform/fonts/FontDe... Source/platform/fonts/FontDescription.h:84: , m_locale(String("en")) As commented by Behdad, once this patch is committed we need to make changes to move String member for language out of this class. I will be starting on that work. On 2014/05/14 10:00:01, bashi1 wrote: > Assuming lang=en is OK? Also, I'm not sure we can add a String into this class, > as some members are bit fields, which indicates that there might be a lot of > instances of this class and adding a field might impact memory consumption. https://codereview.chromium.org/276573010/diff/1/Source/platform/fonts/FontDe... Source/platform/fonts/FontDescription.h:120: String locale() const { return static_cast<String>(m_locale); } This method is "const", so need to type cast. On 2014/05/14 10:00:01, bashi1 wrote: > Why static_cast<>? https://codereview.chromium.org/276573010/diff/1/Source/platform/fonts/harfbu... File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right): https://codereview.chromium.org/276573010/diff/1/Source/platform/fonts/harfbu... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:809: hb_buffer_set_language(harfBuzzBuffer.get(), hb_language_from_string((const char*)(static_cast<const LChar *>(localeStr.characters8())), localeStr.length())); As by default its assigned to "en", so I feel it should not contain invalid value. On 2014/05/14 10:00:01, bashi1 wrote: > What happens when localeStr contains invalid value? Won't that happen?
https://codereview.chromium.org/276573010/diff/1/Source/platform/fonts/FontDe... File Source/platform/fonts/FontDescription.h (right): https://codereview.chromium.org/276573010/diff/1/Source/platform/fonts/FontDe... Source/platform/fonts/FontDescription.h:84: , m_locale(String("en")) On 2014/05/14 10:50:47, h.joshi wrote: > As commented by Behdad, once this patch is committed we need to make changes to > move String member for language out of this class. I will be starting on that > work. > > On 2014/05/14 10:00:01, bashi1 wrote: > > Assuming lang=en is OK? Also, I'm not sure we can add a String into this > class, > > as some members are bit fields, which indicates that there might be a lot of > > instances of this class and adding a field might impact memory consumption. > I'm still not convinced, but I don't have a strong objection. https://codereview.chromium.org/276573010/diff/1/Source/platform/fonts/FontDe... Source/platform/fonts/FontDescription.h:120: String locale() const { return static_cast<String>(m_locale); } On 2014/05/14 10:50:47, h.joshi wrote: > This method is "const", so need to type cast. > On 2014/05/14 10:00:01, bashi1 wrote: > > Why static_cast<>? > Then, it should return const String& https://codereview.chromium.org/276573010/diff/1/Source/platform/fonts/harfbu... File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right): https://codereview.chromium.org/276573010/diff/1/Source/platform/fonts/harfbu... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:809: hb_buffer_set_language(harfBuzzBuffer.get(), hb_language_from_string((const char*)(static_cast<const LChar *>(localeStr.characters8())), localeStr.length())); On 2014/05/14 10:50:47, h.joshi wrote: > As by default its assigned to "en", so I feel it should not contain invalid > value. > > On 2014/05/14 10:00:01, bashi1 wrote: > > What happens when localeStr contains invalid value? Won't that happen? > What happens when the lang attribute is set and it contains malformed value(e.g. invalid surrogate pair)? I want to make sure such values are stripped at this point.
The FontDescription changes l-g-t-m. Would like eae to review the rest.
thank you Eric, Patch "https://codereview.chromium.org/267423005/" is added for NotoSans font file.
On 2014/05/14 07:38:23, Dominik Röttsches wrote: > On 2014/05/14 07:21:43, h.joshi wrote: > > Took test case from Behdad site (same is mentioned in bug as well) > > Using extra font was done so that we do not depend on platform. In old Webkit > > bug they are using "font-family:'DejaVu Serif';", this may work for Linux > > distributions. > > If it works on all relevant platform bots without an extra font and you can > remove it from your test case and the patch. Dirk mentioned he would like to > keep the number of fonts we include in the repo minimal. He commented on my > patches earlier, that we should avoid adding a font for every feature we test. > But it may be that we can't avoid it in this case, your call. I like to see us go the other direction: test as many tests as possible using small web fonts. That would free us from depending on platform fonts. Note that for example, for this case, I subset the NotoSansDevanagari-Regular font to the bare minimum needed for this test case to work. So it only has 3 glyphs instead of the original font's 797, and is 1.2kb instead of 130kb. So, I think with a tools like pyftsubset [1] it's possibly to keep a font for every test in a practical way. [1] https://github.com/behdad/fonttools
On 2014/05/14 17:29:38, behdad_google wrote: > On 2014/05/14 07:38:23, Dominik Röttsches wrote: > > On 2014/05/14 07:21:43, h.joshi wrote: > > > Took test case from Behdad site (same is mentioned in bug as well) > > > Using extra font was done so that we do not depend on platform. In old > Webkit > > > bug they are using "font-family:'DejaVu Serif';", this may work for Linux > > > distributions. > > > > If it works on all relevant platform bots without an extra font and you can > > remove it from your test case and the patch. Dirk mentioned he would like to > > keep the number of fonts we include in the repo minimal. He commented on my > > patches earlier, that we should avoid adding a font for every feature we test. > > But it may be that we can't avoid it in this case, your call. > > I like to see us go the other direction: test as many tests as possible using > small web fonts. That would free us from depending on platform fonts. Note > that for example, for this case, I subset the NotoSansDevanagari-Regular font to > the bare minimum needed for this test case to work. So it only has 3 glyphs > instead of the original font's 797, and is 1.2kb instead of 130kb. So, I think > with a tools like pyftsubset [1] it's possibly to keep a font for every test in > a practical way. > > [1] https://github.com/behdad/fonttools Okay - sounds like a viable approach to me and I agree it's useful for testing a variety of special cases, font rendering and shaping. Let's try to go that way. I was mostly advocating dpranke's concerns regarding making license auditing less painful. I will move this to blink-dev to align with him if you don't mind. Maybe we can have a LayoutTests/third-party/font-subsets folder, where we generally put those? His main interest was to have a clear origin and license info for fonts that we add, additionally, he would like to keep the repo size small. I think these points can be addressed.
Cool. If you're going to land some nice font subsets, 313806 needs a font with ipa tone letter ccmp ligature substitutions.
Waiting for update from dpranke on blink-dev (https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/PoxWa75ex3k) Going with subset font is good option
https://codereview.chromium.org/276573010/diff/40001/Source/platform/fonts/Fo... File Source/platform/fonts/FontDescription.h (right): https://codereview.chromium.org/276573010/diff/40001/Source/platform/fonts/Fo... Source/platform/fonts/FontDescription.h:84: , m_locale(String("en")) How about storing the hb_language_t instead of the locale string and then initialize it to either HB_LANGUAGE_INVALID or hb_language_get_default()?
https://codereview.chromium.org/276573010/diff/40001/Source/platform/fonts/Fo... File Source/platform/fonts/FontDescription.h (right): https://codereview.chromium.org/276573010/diff/40001/Source/platform/fonts/Fo... Source/platform/fonts/FontDescription.h:84: , m_locale(String("en")) Okey, will start working on this suggestion and share new patch.
I have a doubt here, all Harfbuzz related call etc are done from file present under "platform/fonts/harfbuzz/". "FontDescription.h" is present under "platform/fonts/", will it be good to call/include Harfbuzz related stuff from this folder. Pls suggest
On 2014/05/20 04:41:41, h.joshi wrote: > I have a doubt here, all Harfbuzz related call etc are done from file present > under "platform/fonts/harfbuzz/". > "FontDescription.h" is present under "platform/fonts/", will it be good to > call/include Harfbuzz related stuff > from this folder. > Pls suggest harfbuzz is used on all platforms now (and the fonts/harfbuzz directory is going away before long). Don't use it outside of platforms/fonts.
Okey, thank you for clarifying my doubt. Will start with make code changes as per comments.
New patch with comment fix, had to make changes to "gyp" files to create dependency for "harfbuzz-ng" while Blink files are compiled.
https://codereview.chromium.org/276573010/diff/80001/LayoutTests/fast/text/sh... File LayoutTests/fast/text/shaping/same-script-different-lang.html (right): https://codereview.chromium.org/276573010/diff/80001/LayoutTests/fast/text/sh... LayoutTests/fast/text/shaping/same-script-different-lang.html:1: <!DOCTYPE HTML> lowercase "html" https://codereview.chromium.org/276573010/diff/80001/LayoutTests/fast/text/sh... LayoutTests/fast/text/shaping/same-script-different-lang.html:13: <h3>The glyph for JHA should look different depending on Nepali and Hindi, depending on the lang attribute.</h3> "The glyph for JHA should look different for Nepali and Hindi, depending on the lang attribute." https://codereview.chromium.org/276573010/diff/80001/Source/core/core.gyp File Source/core/core.gyp (right): https://codereview.chromium.org/276573010/diff/80001/Source/core/core.gyp#new... Source/core/core.gyp:233: '<(DEPTH)/third_party/harfbuzz-ng/harfbuzz.gyp:harfbuzz-ng', Alphabetical order please https://codereview.chromium.org/276573010/diff/80001/Source/core/core.gyp#new... Source/core/core.gyp:385: '<(DEPTH)/third_party/harfbuzz-ng/harfbuzz.gyp:harfbuzz-ng', ditto https://codereview.chromium.org/276573010/diff/80001/Source/core/core.gyp#new... Source/core/core.gyp:405: '<(DEPTH)/third_party/harfbuzz-ng/harfbuzz.gyp:harfbuzz-ng', ditto https://codereview.chromium.org/276573010/diff/80001/Source/core/core.gyp#new... Source/core/core.gyp:750: '<(DEPTH)/third_party/harfbuzz-ng/harfbuzz.gyp:harfbuzz-ng', ditto https://codereview.chromium.org/276573010/diff/80001/Source/platform/blink_pl... File Source/platform/blink_platform.gyp (right): https://codereview.chromium.org/276573010/diff/80001/Source/platform/blink_pl... Source/platform/blink_platform.gyp:197: '<(DEPTH)/third_party/harfbuzz-ng/harfbuzz.gyp:harfbuzz-ng', Alphabetical order please https://codereview.chromium.org/276573010/diff/80001/Source/platform/blink_pl... Source/platform/blink_platform.gyp:262: ['include', 'fonts/FontDescription\\.(cpp|h)$'], ditto https://codereview.chromium.org/276573010/diff/80001/Source/platform/fonts/Fo... File Source/platform/fonts/FontDescription.cpp (right): https://codereview.chromium.org/276573010/diff/80001/Source/platform/fonts/Fo... Source/platform/fonts/FontDescription.cpp:46: hb_language_t locale; Ditto, please move this up after the RefPtr. https://codereview.chromium.org/276573010/diff/80001/Source/platform/fonts/Fo... File Source/platform/fonts/FontDescription.h (right): https://codereview.chromium.org/276573010/diff/80001/Source/platform/fonts/Fo... Source/platform/fonts/FontDescription.h:121: const hb_language_t locale() const { return m_locale; } Not sure this needs to return a const. https://codereview.chromium.org/276573010/diff/80001/Source/platform/fonts/Fo... Source/platform/fonts/FontDescription.h:160: void setLocale(String localeStr) { m_locale = hb_language_from_string((const char*)(static_cast<const LChar *>(localeStr.characters8())), localeStr.length()); } - Please pass the String argument by const reference. - No abbreviations: -> localeString - Also, no C-Style casting. - Also WTFString::characters8() will assert if the input string is not 8-bit. You likely want something like: CString locale = localeString.latin1(); m_locale = hb_language_from_string(locale.data(), locale.length()); https://codereview.chromium.org/276573010/diff/80001/Source/platform/fonts/Fo... Source/platform/fonts/FontDescription.h:226: hb_language_t m_locale; It looks like hb_language_t is a typedef to a pointer, please move this up before the bitfield (e.g. after m_featureSettings). https://codereview.chromium.org/276573010/diff/80001/Source/platform/fonts/ha... File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right): https://codereview.chromium.org/276573010/diff/80001/Source/platform/fonts/ha... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:797: const hb_language_t localeStr = m_font->fontDescription().locale(); no longer needs to be const if you update the getter. "localeStr" is not a great name for 2 reasons: - We don't use abbreviations in Blink - This is not a String. Maybe just use "locale" ? https://codereview.chromium.org/276573010/diff/80001/Source/web/web.gyp File Source/web/web.gyp (right): https://codereview.chromium.org/276573010/diff/80001/Source/web/web.gyp#newco... Source/web/web.gyp:58: '<(DEPTH)/third_party/harfbuzz-ng/harfbuzz.gyp:harfbuzz-ng', alphabetical order. https://codereview.chromium.org/276573010/diff/80001/Source/web/web.gyp#newco... Source/web/web.gyp:65: '<(DEPTH)/third_party/harfbuzz-ng/harfbuzz.gyp:harfbuzz-ng', ditto https://codereview.chromium.org/276573010/diff/80001/Source/web/web.gyp#newco... Source/web/web.gyp:100: '<(DEPTH)/third_party/harfbuzz-ng/harfbuzz.gyp:harfbuzz-ng', ditto https://codereview.chromium.org/276573010/diff/80001/Source/web/web_tests.gyp File Source/web/web_tests.gyp (right): https://codereview.chromium.org/276573010/diff/80001/Source/web/web_tests.gyp... Source/web/web_tests.gyp:51: '<(DEPTH)/third_party/harfbuzz-ng/harfbuzz.gyp:harfbuzz-ng', alphabetical order https://codereview.chromium.org/276573010/diff/80001/Source/web/web_tests.gyp... Source/web/web_tests.gyp:101: '<(DEPTH)/third_party/harfbuzz-ng/harfbuzz.gyp:harfbuzz-ng', ditto
We really don't want to add a harfbuzz dependency to modules, blink_common or core. blink_platform is fine. Perhaps we need a typedef (or worse case go back to the stirng approach if we can't contain it). Sorry about making you do all this extra work.
New patch submitted with comment fix and reverting back to String approach. Tried removing dependency from module and webcore but compile issues were faced.
LGTM
not lgtm for now. When you reupload a patch, please take into consideration *all* the previous review comments or explain why you did not. https://codereview.chromium.org/276573010/diff/100001/LayoutTests/fast/text/s... File LayoutTests/fast/text/shaping/same-script-different-lang.html (right): https://codereview.chromium.org/276573010/diff/100001/LayoutTests/fast/text/s... LayoutTests/fast/text/shaping/same-script-different-lang.html:1: <!DOCTYPE HTML> nit: lowercase html https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/F... File Source/platform/fonts/FontDescription.cpp (right): https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/F... Source/platform/fonts/FontDescription.cpp:46: String locale; Please move this up after m_featureSettings https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/F... File Source/platform/fonts/FontDescription.h (right): https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/F... Source/platform/fonts/FontDescription.h:84: , m_locale(String("en")) nit: unnecessary String(). https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/F... Source/platform/fonts/FontDescription.h:120: String locale() const { return m_locale; } nit: could return a const ref. https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/F... Source/platform/fonts/FontDescription.h:225: String m_locale; Please move this up before the bitfield, preferably after m_featureSettings. https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/h... File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right): https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/h... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:89: CachedShapingResults(hb_buffer_t* harfBuzzBuffer, const Font* runFont, hb_direction_t runDir, CString newLocale); You really don't want to copy a CString (passed by value). Please pass by const reference. Also, I think we should use a String to avoid copying raw strings. https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/h... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:95: CString locale; String https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/h... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:106: CachedShapingResults::CachedShapingResults(hb_buffer_t* harfBuzzBuffer, const Font* fontData, hb_direction_t dirData, CString newLocale) const String& https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/h... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:797: CString locale = m_font->fontDescription().locale().latin1(); You should probably store m_font->fontDescription().locale() in a variable since you are going to use it several times below. https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/h... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:823: if (cachedResults->dir == props.direction && cachedResults->font == *m_font && cachedResults->locale == locale) { Compare Strings here. https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/h... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:859: runCache.insert(key, new CachedShapingResults(harfBuzzBuffer.get(), m_font, props.direction, locale)); Pass String here.
Will make suggested changes. https://codereview.chromium.org/276573010/diff/100001/LayoutTests/fast/text/s... File LayoutTests/fast/text/shaping/same-script-different-lang.html (right): https://codereview.chromium.org/276573010/diff/100001/LayoutTests/fast/text/s... LayoutTests/fast/text/shaping/same-script-different-lang.html:1: <!DOCTYPE HTML> Forgot to change this, will make changes. https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/F... File Source/platform/fonts/FontDescription.cpp (right): https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/F... Source/platform/fonts/FontDescription.cpp:46: String locale; Okey, it is now String object that was the reason I did not changed object position. Will make changes. https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/F... File Source/platform/fonts/FontDescription.h (right): https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/F... Source/platform/fonts/FontDescription.h:84: , m_locale(String("en")) We need m_locale to have some default value else we may need to add conditional check in HarfbuzzShaper while using it. https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/F... Source/platform/fonts/FontDescription.h:120: String locale() const { return m_locale; } Okey, will make changes. In previous comment "Not sure this needs to return a const." was suggested, so that was the reason I removed const https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/F... Source/platform/fonts/FontDescription.h:225: String m_locale; Okey, it is now String object that was the reason I did not changed object position. Will make changes. https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/h... File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right): https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/h... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:89: CachedShapingResults(hb_buffer_t* harfBuzzBuffer, const Font* runFont, hb_direction_t runDir, CString newLocale); Okey, did that after the previous comment related to use of CString. Will make changes and share new code. https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/h... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:95: CString locale; ditto https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/h... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:106: CachedShapingResults::CachedShapingResults(hb_buffer_t* harfBuzzBuffer, const Font* fontData, hb_direction_t dirData, CString newLocale) ditto https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/h... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:797: CString locale = m_font->fontDescription().locale().latin1(); Okey, will be making String related changes. https://codereview.chromium.org/276573010/diff/100001/Source/platform/fonts/h... Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:859: runCache.insert(key, new CachedShapingResults(harfBuzzBuffer.get(), m_font, props.direction, locale)); ditto
@Chris: new patch with suggested changes, pls review
lgtm, thanks.
Thank you Chris
The CQ bit was checked by h.joshi@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/h.joshi@samsung.com/276573010/180001
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 1225. 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 22d4c903016685e5edc1a3223cd37b4b2302bacb..e10efa9e5a35a43a95a823770e3d0022ae507082 100644 --- a/LayoutTests/TestExpectations +++ b/LayoutTests/TestExpectations @@ -1225,3 +1225,6 @@ crbug.com/376534 http/tests/inspector/network/network-xhr-redirect-method.html [ crbug.com/377013 [ Mac Debug ] fast/repaint/4776765.html [ Pass Failure ] crbug.com/239722 http/tests/websocket/connection-throttling.html [ Failure ] + +# Following test case needs to be rebased after https://codereview.chromium.org/276573010. +crbug.com/372502 fast/text/shaping/same-script-different-lang.html [ NeedsRebaseline ]
The CQ bit was checked by h.joshi@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/h.joshi@samsung.com/276573010/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9444)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9444)
Thank you for working with Chris, he's a very experianced Blink contributor and can help you navigate the process. Emil and Behdad are the experts in this code and it looks like they like it. rslgtm if you need it.
The CQ bit was checked by h.joshi@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/h.joshi@samsung.com/276573010/220001
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 #2 FAILED at 1265. 1 out of 2 hunks FAILED -- saving rejects to file LayoutTests/TestExpectations.rej Patch: LayoutTests/TestExpectations Index: LayoutTests/TestExpectations diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations index 6e9f1b3158b2f98a9dfc0258d41675b6b3896ba7..3b06185dfd6f06a390d4e2a0b919f7aabd223625 100644 --- a/LayoutTests/TestExpectations +++ b/LayoutTests/TestExpectations @@ -87,7 +87,6 @@ crbug.com/371916 [ Debug ] http/tests/security/contentSecurityPolicy/xsl-allowed # Pixel tests for RTL iframe scrollbar is erroneous. Cannot observe on actual browser. crbug.com/338794 crbug.com/192172 compositing/rtl/rtl-iframe-absolute-overflow.html [ Pass Failure ] crbug.com/338794 compositing/rtl/rtl-iframe-absolute-overflow-scrolled.html [ Pass Failure ] -crbug.com/338794 compositing/rtl/rtl-iframe-fixed-overflow.html [ Pass Failure ] crbug.com/338794 crbug.com/192172 compositing/rtl/rtl-iframe-fixed-overflow-scrolled.html [ Pass Failure ] crbug.com/338794 virtual/softwarecompositing/rtl/rtl-iframe-absolute-overflow-scrolled.html [ Pass Failure ] crbug.com/338794 crbug.com/192172 virtual/softwarecompositing/rtl/rtl-iframe-fixed-overflow.html [ Pass Failure ] @@ -1266,3 +1265,9 @@ crbug.com/239722 http/tests/websocket/connection-throttling.html [ Failure ] crbug.com/376194 inspector/console/console-format-es6.html [ Skip ] crbug.com/376194 inspector/console/console-format-es6-symbols-error.html [ Skip ] + +# Following test case needs to be rebased after https://codereview.chromium.org/276573010. +crbug.com/372502 fast/text/shaping/same-script-different-lang.html [ NeedsRebaseline ] +crbug.com/372502 virtual/windows-directwrite/fast/text/shaping/same-script-different-lang.html [ NeedsRebaseline ] +crbug.com/372502 compositing/rtl/rtl-iframe-fixed-overflow.html [ NeedsRebaseline ] +
The CQ bit was checked by h.joshi@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/h.joshi@samsung.com/276573010/240001
Thank you Eric, Yes will try to gather more information on Blink process from Chris and keep work on Font/Text code with Emil ad Behdad.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9912)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/9410)
The CQ bit was checked by h.joshi@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/h.joshi@samsung.com/276573010/260001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9770)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9775)
The CQ bit was checked by h.joshi@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/h.joshi@samsung.com/276573010/280001
Message was sent while issue was closed.
Change committed as 175124 |
