Support font-feature-settings CSS property. This bug entry is for Chromium mac port. Chromium mac port shares FontPlatformData class with other mac ports and it uses CoreText to shape text. A difficult point is that CoreText doesn't provide interfaces to use OpenType features. To support the property, I'm planning to introduce Harfbuzz as a complex text shaper. Chromium Linux port uses Harfbuzz so we can reuse or merge the existing code. Firefox also uses Harfbuzz. I think there are three options (Using Skia is premised): 1. add Harfbuzz as an optional shaper 2. replace CoreText with Harfbuzz 3. fork FontPlatformData and use Harfbuzz as complex text shaper In #1, I'm planning to use Harfbuzz if and only if font-feature-settings properties are set. This approach minimizes the impact of layout result. This will require to modify FontComplexTextMac.cpp. This might be undesirable for other ports which don't want to add an extra dependency or won't want to support the property. If we take option #2, I will implement methods of Font class for complex text (e.g. Font::drawComplexText) into FontSkia.cpp by Harfbuzz and stop linking FontComplexTextMac.cpp. This might require only a slight change in FontPlatformData to hold information about Harfbuzz. This approach also has an additional advantage that we can integrate FontSkia.cpp with FontLinux.cpp partially (we can't integrate them completely because they use different FontPlatformData implementation). A disadvantage of this approach is rendering results for complex text would differ from the current implementation. The advantage of #3 is that we can completely share font related code between Chromium Linux and Chromium Mac Skia ports. However this will require a lot of things to implement and could impact rendering results. Comments and suggestions are welcome.
You're going to need CoreText when dealing with AAT fonts for Indic, I don't think OSX ships with OpenType fonts for Indic.
(In reply to comment #1) > You're going to need CoreText when dealing with AAT fonts for Indic, I don't think OSX ships with OpenType fonts for Indic. Thank you for the useful information. I didn't know that. I understand that we need keep CoreText in any cases.
Created attachment 124271 [details] Patch
Attachment 124271 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzCoreText.cpp:33: You should add a blank line after implementation file's own header. [build/include_order] [4] Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.h:39: hb_face_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.h:41: hb_font_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 3 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #3) > Created an attachment (id=124271) [details] > Patch According to comment #1, only option we can take is adding HarfBuzz-ng as a secondary shaper and use it only for -webkit-font-feature-settings is specified. I uploaded a proposed patch. This implementation would be eventually used on Chromium Linux. However, for now, I don't want to use it on Linux because there are some rendering and performance issues when drawing complex text. (That's an another reason why we use HarfBuzz-ng just only for supporting -webkit-font-feature-settings) This patch won't build for now. I uploaded the patch set which is required to build chromium with this patch at: https://chromiumcodereview.appspot.com/9223010.
*** Bug 67933 has been marked as a duplicate of this bug. ***
*** Bug 64930 has been marked as a duplicate of this bug. ***
Comment on attachment 124271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124271&action=review It appears that AAT are just TrueType extensions. Would it be simpler to implement the AAT extensions in Harfbuzz-NG instead? > Source/WebCore/ChangeLog:7 > + Chromium mac port uses it as secondary text shaper to support OpenType features. typo: one space before OpenType > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.cpp:49 > + DEFINE_STATIC_LOCAL(HarfBuzzFaceCache, s_harfbuzzFaceCache, ()); Does this cache ever get smaller? (That is, does this slowly leak memory?) >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.h:39 >> +typedef _hb_face_t hb_face_t; > > hb_face_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] I think this is ok, since you're forward-declaring. > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:150 > + // for characters like '\n' otherwise. Isn't this code copy and pasted from other code? Can we somehow refactor?
Created attachment 124480 [details] Patch (just fixed a typo and a style error)
Attachment 124480 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.h:39: hb_face_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.h:41: hb_font_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 124271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124271&action=review > Would it be simpler to implement the AAT extensions in Harfbuzz-NG instead? I know next to nothing about AAT format so I can't estimate how difficult to add AAT extension to HarfBuzz-ng. According to <http://fontforge.sourceforge.net/gposgsub.html>, some advanced layout features are interchangeable between OpenType and AAT, but others have essentially no common ground. FireFox doesn't use HarfBuzz when the font has 'mort' table or 'morx' table and the font doesn't have 'GSUB' nor 'GPOS'. I guess adding AAT extension isn't so easy. Behdad, do you have any ideas? >> Source/WebCore/ChangeLog:7 >> + Chromium mac port uses it as secondary text shaper to support OpenType features. > > typo: one space before OpenType Done. >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzCoreText.cpp:33 >> +#include "CoreText/CoreText.h" > > You should add a blank line after implementation file's own header. [build/include_order] [4] Done. (and renamed the file to HarfBuzzFaceCoreText.cpp) >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.cpp:49 >> + DEFINE_STATIC_LOCAL(HarfBuzzFaceCache, s_harfbuzzFaceCache, ()); > > Does this cache ever get smaller? (That is, does this slowly leak memory?) An hb_face_t object in this cache will be destroyed (and the cache get smaller) when all FontPlatformData objects which are associated with the hb_face_object are removed from FontCache. The FontCache class controls number of FontPlatformData in the cache. This means memory leak doesn't occur as long as FontCache handles the FontPlatformData cache appropriately. >>> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.h:39 >>> +typedef _hb_face_t hb_face_t; >> >> hb_face_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] > > I think this is ok, since you're forward-declaring. Left it as is. >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.h:41 >> +typedef _hb_font_t hb_font_t; > > hb_font_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Ditto. >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:150 >> + // for characters like '\n' otherwise. > > Isn't this code copy and pasted from other code? Can we somehow refactor? Actually, this code comes from getNormalizedTextRun() which lives in ComplexTextControllerHarfBuzz.cpp, but I made some minor change: - Don't create TextRun. I think we only need normalized buffer here. - Don't convert mirrored characters. HarfBuzz-ng handles mirrored characters itself. I just copied the code because (1) the code is a bit different, (2) there is no good place to share the code, (3) ComplexTextControllerHarfBuzz.cpp should be deprecated eventually. Should I share the code between ComplexTextControllerHarfBuzz.cpp and HarfBuzzShaper.cpp?
Any suggestions/comments?
I am still anxious about how much code was cut'n'pasted. Is the plan to manually copy any further changes made to this code into the other file as well?
Thanks for the comment. (In reply to comment #13) > I am still anxious about how much code was cut'n'pasted. Is the plan to manually copy any further changes made to this code into the other file as well? HarfBuzzShaper.cpp is based on ComplexTextControllerHarfBuzz.cpp and following methods are almost the same (but a bit different): - normalizeSpaces() and normalizeSpacesAndMirrorChars() - HarfBuzzShaper::setNormalizedBuffer() and ComplexTextController::getNormalizedTextRun() - HarfBuzzShaper::isWordEnd() and ComplexTextController::isWordBreak() - HarfBuzzShaper::setPadding() and ComplexTextController::setPadding(() - HarfBuzzShaper::determineWordBreakSpacing() and ComplexTextController::determineWordBreakSpacing() No further code won't be copy and pasted.
FWIW I do plan to eventually add AAT morx support to HarfBuzz. But "eventually" is the key word here... I'll take a look to get a better estimate of how much work is involved...
(In reply to comment #15) > FWIW I do plan to eventually add AAT morx support to HarfBuzz. But "eventually" is the key word here... I'll take a look to get a better estimate of how much work is involved... Thanks for comments. That's great to hear. I'm waiting for review or further comments...
Hi Evan, Could you please take another look?
My concern is that a bug fix in ComplexTextControllerHarfbuzz will need to be manually copied over. Like the code in setNormalizedBuffer() has seen a number of changes over time and probably will see more in the future. But if you think this way is best, it LGTM
Thank you for review. (In reply to comment #18) > My concern is that a bug fix in ComplexTextControllerHarfbuzz will need to be manually copied over. Like the code in setNormalizedBuffer() has seen a number of changes over time and probably will see more in the future. > > But if you think this way is best, it LGTM I'll think about whether these functions can be shared.
Created attachment 129617 [details] Patch
Attachment 129617 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.h:39: hb_face_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.h:41: hb_font_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #20) > Created an attachment (id=129617) [details] > Patch I added HarfBuzzShaperBase class to share functions. Revised the patch to follow the change. Evan, could you take another look if possible? Tony, could you tell me if Evan can't review the patch?
Comment on attachment 129617 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129617&action=review > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.cpp:35 > +#include "hb.h" Should we use <hb.h> to make it more clear that this header is not part of webkit? That seems to be what we do for icu includes. > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.cpp:44 > +typedef pair<hb_face_t*, unsigned> FaceCacheEntry; Using a struct would make the code less confusing. > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.cpp:63 > + ++(result.get()->second.second); I think you can remove the .get() since operator-> is defined on the iterator. > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.cpp:75 > + ASSERT(result.get()->second.second > 0); > + --(result.get()->second.second); > + if (!(result.get()->second.second)) { > + hb_face_destroy(result.get()->second.first); Ditto. > Source/WebCore/platform/graphics/mac/FontComplexTextMac.cpp:57 > FloatRect Font::selectionRectForComplexText(const TextRun& run, const FloatPoint& point, int h, > int from, int to) const > { > +#if PLATFORM(CHROMIUM) > + if (preferHarfBuzz(this)) { This feels really invasive. Maybe mitz or hyatt have an idea on how to make this less invasive.
Comment on attachment 129617 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129617&action=review >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.cpp:35 >> +#include "hb.h" > > Should we use <hb.h> to make it more clear that this header is not part of webkit? That seems to be what we do for icu includes. Will do. >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.cpp:44 >> +typedef pair<hb_face_t*, unsigned> FaceCacheEntry; > > Using a struct would make the code less confusing. Will use a struct. >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.cpp:63 >> + ++(result.get()->second.second); > > I think you can remove the .get() since operator-> is defined on the iterator. Will do. >> Source/WebCore/platform/graphics/mac/FontComplexTextMac.cpp:57 >> + if (preferHarfBuzz(this)) { > > This feels really invasive. Maybe mitz or hyatt have an idea on how to make this less invasive. Agree, but I couldn't come up with an idea to avoid ifdefs without code duplications. Suggestions are really welcomed.
> > Source/WebCore/platform/graphics/mac/FontComplexTextMac.cpp:57 > > FloatRect Font::selectionRectForComplexText(const TextRun& run, const FloatPoint& point, int h, > > int from, int to) const > > { > > +#if PLATFORM(CHROMIUM) > > + if (preferHarfBuzz(this)) { > > This feels really invasive. Maybe mitz or hyatt have an idea on how to make this less invasive. Any suggestions?
How much does HarfBuzzShaper overlap with the ComplexTextController implementation in ComplexTextControllerHarfBuzz?
(In reply to comment #26) > How much does HarfBuzzShaper overlap with the ComplexTextController implementation in ComplexTextControllerHarfBuzz? Their function is the same, but ComplexTextControllerHarfBuzz uses old HarfBuzz, while HarfBuzzShaper uses new HarfBuzz (so called HarfBuzz-ng). The common code lives in HarfBuzzShaperBase class.
Created attachment 130756 [details] Patch
Attachment 130756 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.h:39: hb_face_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.h:41: hb_font_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #28) > Created an attachment (id=130756) [details] > Patch To reduce #ifdefs, I added a new file called FontComplexTextCommon.cpp and moved some methods of Font class into the file. The name of moved methods are added "Common" suffix. On mac port, original methods just call xxxCommon(). How about this approach? There is extra function calls, but I think it unlikely affects performance.
(In reply to comment #30) > (In reply to comment #28) > > Created an attachment (id=130756) [details] [details] > > Patch > > To reduce #ifdefs, I added a new file called FontComplexTextCommon.cpp and moved some methods of Font class into the file. The name of moved methods are added "Common" suffix. On mac port, original methods just call xxxCommon(). How about this approach? There is extra function calls, but I think it unlikely affects performance. This looks to me much worse than attachment 129617 [details].
(In reply to comment #31) > This looks to me much worse than attachment 129617 [details]. Ok. I drop the patch. Do you have any ideas to make the patch less invasive without forking? I would really appreciate if I could hear suggestions. I agree that attachment 129617 [details] patch wasn't good, but I'd like to add this feature and I couldn't come up with other ideas.
(In reply to comment #32) > (In reply to comment #31) > > This looks to me much worse than attachment 129617 [details] [details]. > > Ok. I drop the patch. Do you have any ideas to make the patch less invasive without forking? I would really appreciate if I could hear suggestions. I agree that attachment 129617 [details] patch wasn't good I didn’t object to attachment 129617 [details]. > but I'd like to add this feature and I couldn't come up with other ideas. Have you considered using Core Text, which is OS X’s text engine, rather than using HarfBuzz?
(In reply to comment #33) > (In reply to comment #32) > > (In reply to comment #31) > > > This looks to me much worse than attachment 129617 [details] [details] [details]. > > > > Ok. I drop the patch. Do you have any ideas to make the patch less invasive without forking? I would really appreciate if I could hear suggestions. I agree that attachment 129617 [details] [details] patch wasn't good > > I didn’t object to attachment 129617 [details]. I see. Thank you for comments! > > but I'd like to add this feature and I couldn't come up with other ideas. > > Have you considered using Core Text, which is OS X’s text engine, rather than using HarfBuzz? Does CoreText support OpenType features? If so, I'm happy to try it. Using CoreText should have less impact for the current implementation.
(In reply to comment #34) > (In reply to comment #33) > > (In reply to comment #32) > > > (In reply to comment #31) > > > > This looks to me much worse than attachment 129617 [details] [details] [details] [details]. > > > > > > Ok. I drop the patch. Do you have any ideas to make the patch less invasive without forking? I would really appreciate if I could hear suggestions. I agree that attachment 129617 [details] [details] [details] patch wasn't good > > > > I didn’t object to attachment 129617 [details] [details]. > > I see. Thank you for comments! > > > > but I'd like to add this feature and I couldn't come up with other ideas. > > > > Have you considered using Core Text, which is OS X’s text engine, rather than using HarfBuzz? > > Does CoreText support OpenType features? If so, I'm happy to try it. Using CoreText should have less impact for the current implementation. Core Text’s API is in terms of AAT features, which it maps internally to OpenType features. Check out <ATS/SFNTLayoutTypes.h> to see what’s supported. I believe that it’s possible to access even some features beyond what’s included in that header (anything the standard Typography panel in OS X can do). I can try asking the appropriate people at Apple about specifics if you’re interested.
(In reply to comment #35) > Core Text’s API is in terms of AAT features, which it maps internally to OpenType features. Check out <ATS/SFNTLayoutTypes.h> to see what’s supported. I believe that it’s possible to access even some features beyond what’s included in that header (anything the standard Typography panel in OS X can do). I can try asking the appropriate people at Apple about specifics if you’re interested. Thank you so much for the information! If my understanding correct, we need to have reverse-mapping(OpenType-AAT mapping) because page authors can specify arbitrary OpenType feature to font-feature-settings. Is it possible to have such mapping or an API which converts an OpenType feature to corresponding AAT feature? We can guess such OpenType-AAT mapping for some features (like http://en.wikipedia.org/wiki/List_of_typographic_features), but such guess is likely to fail. If WebKit can have OpenType-AAT mapping, I'll try to implement this feature with CoreText.
(In reply to comment #36) > If my understanding correct, we need to have reverse-mapping(OpenType-AAT mapping) because page authors can specify arbitrary OpenType feature to font-feature-settings. True (it is kind of unfortunate, in my opinion, that this CSS feature is tailored specifically for OpenType). > Is it possible to have such mapping or an API which converts an OpenType feature to corresponding AAT feature? I don’t think OS X has this sort of API. We would have to put this knowledge in WebCore. > We can guess such OpenType-AAT mapping for some features (like http://en.wikipedia.org/wiki/List_of_typographic_features), but such guess is likely to fail. If WebKit can have OpenType-AAT mapping, I'll try to implement this feature with CoreText. I am going to ask a Core Text expert for advice here, so that we don’t have to make bad guesses.
(In reply to comment #37) > (In reply to comment #36) > > If my understanding correct, we need to have reverse-mapping(OpenType-AAT mapping) because page authors can specify arbitrary OpenType feature to font-feature-settings. > > True (it is kind of unfortunate, in my opinion, that this CSS feature is tailored specifically for OpenType). Agreed, but as the spec says, the property provides low-level feature settings control and OpenType looks reasonable choice to me because major OS/platform can handle it. > > Is it possible to have such mapping or an API which converts an OpenType feature to corresponding AAT feature? > > I don’t think OS X has this sort of API. We would have to put this knowledge in WebCore. I see. > > We can guess such OpenType-AAT mapping for some features (like http://en.wikipedia.org/wiki/List_of_typographic_features), but such guess is likely to fail. If WebKit can have OpenType-AAT mapping, I'll try to implement this feature with CoreText. > > I am going to ask a Core Text expert for advice here, so that we don’t have to make bad guesses. Thank you so much for your help. I'd appreciate if the CoreText expert could give us advice.
(In reply to comment #38) > (In reply to comment #37) > > (In reply to comment #36) > > > If my understanding correct, we need to have reverse-mapping(OpenType-AAT mapping) because page authors can specify arbitrary OpenType feature to font-feature-settings. > > > > True (it is kind of unfortunate, in my opinion, that this CSS feature is tailored specifically for OpenType). > > Agreed, but as the spec says, the property provides low-level feature settings control and OpenType looks reasonable choice to me because major OS/platform can handle it. I’d also like us to see implementing more of the higher-level properties (I’ve started on kerning and ligatures in <http://trac.webkit.org/r104696> and <http://trac.webkit.org/r104786>). I think they are more author-friendly. > > > > Is it possible to have such mapping or an API which converts an OpenType feature to corresponding AAT feature? > > > > I don’t think OS X has this sort of API. We would have to put this knowledge in WebCore. > > I see. > > > > We can guess such OpenType-AAT mapping for some features (like http://en.wikipedia.org/wiki/List_of_typographic_features), but such guess is likely to fail. If WebKit can have OpenType-AAT mapping, I'll try to implement this feature with CoreText. > > > > I am going to ask a Core Text expert for advice here, so that we don’t have to make bad guesses. > > Thank you so much for your help. I'd appreciate if the CoreText expert could give us advice. I just learned that that person might not be available for a week. While I think it would be great to do this with Core Text (which would also benefit Apple’s OS X WebKit), I want to repeat that I have no objection to attachment 129617 [details] (though I am not the right person to review it), if you would like to go that way.
(In reply to comment #39) > (In reply to comment #38) > > (In reply to comment #37) > > > (In reply to comment #36) > > > > If my understanding correct, we need to have reverse-mapping(OpenType-AAT mapping) because page authors can specify arbitrary OpenType feature to font-feature-settings. > > > > > > True (it is kind of unfortunate, in my opinion, that this CSS feature is tailored specifically for OpenType). > > > > Agreed, but as the spec says, the property provides low-level feature settings control and OpenType looks reasonable choice to me because major OS/platform can handle it. > > I’d also like us to see implementing more of the higher-level properties (I’ve started on kerning and ligatures in <http://trac.webkit.org/r104696> and <http://trac.webkit.org/r104786>). I think they are more author-friendly. That's great to hear. Thank you for heads-up. My original plan was implementing low-level property first, then implementing the higher-level properties. If there are some areas which I can do, I'm happy to do it. > > > > > > > Is it possible to have such mapping or an API which converts an OpenType feature to corresponding AAT feature? > > > > > > I don’t think OS X has this sort of API. We would have to put this knowledge in WebCore. > > > > I see. > > > > > > We can guess such OpenType-AAT mapping for some features (like http://en.wikipedia.org/wiki/List_of_typographic_features), but such guess is likely to fail. If WebKit can have OpenType-AAT mapping, I'll try to implement this feature with CoreText. > > > > > > I am going to ask a Core Text expert for advice here, so that we don’t have to make bad guesses. > > > > Thank you so much for your help. I'd appreciate if the CoreText expert could give us advice. > > I just learned that that person might not be available for a week. While I think it would be great to do this with Core Text (which would also benefit Apple’s OS X WebKit), I want to repeat that I have no objection to attachment 129617 [details] (though I am not the right person to review it), if you would like to go that way. I'm in no hurry. I'd like to wait and see whether I can implement the property with CoreText. I may go this way if using CoreText looks difficult to me, though. Thanks,
Hi mitz, (In reply to comment #39) > I just learned that that person might not be available for a week. While I think it would be great to do this with Core Text (which would also benefit Apple’s OS X WebKit), I want to repeat that I have no objection to attachment 129617 [details] (though I am not the right person to review it), if you would like to go that way. Any updates on this?
Also curious about progress on this. Any updates?
Created attachment 137568 [details] rebased to ToT
Hi Tony, It seems that we can't get information about the mapping in near future, so I'd like to use HarfBuzz-ng for now. Could you take another look for the patch?
Attachment 137568 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.h:39: hb_face_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzFace.h:41: hb_font_t is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 137568 [details] rebased to ToT The changes to FontComplexTextMac.cpp are unfortunate, but I can't think of a better way to do this. If a future version of CoreText supports these OpenType features, I'm sure bashi would be happy to convert this to CoreText so Apple Mac will also benefit. bashi: Please watch the page cyclers when you land. In particular, watch the intl1 and intl2 bots.
(In reply to comment #46) > (From update of attachment 137568 [details]) > The changes to FontComplexTextMac.cpp are unfortunate, but I can't think of a better way to do this. If a future version of CoreText supports these OpenType features, I'm sure bashi would be happy to convert this to CoreText so Apple Mac will also benefit. > > bashi: Please watch the page cyclers when you land. In particular, watch the intl1 and intl2 bots. Thanks. I'm going to land the patch. I'll watch the page cyclers. I'm happy to replace this with CoreText if we can implement the feature with a future version of CoreText.
Created attachment 145674 [details] Patch for landing
Comment on attachment 145674 [details] Patch for landing Clearing flags on attachment: 145674 Committed r119467: <http://trac.webkit.org/changeset/119467>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 88332
http://build.chromium.org/p/chromium/buildstatus?builder=Mac&number=14990 RESULT Chromium: Chromium= 8728 bytes RESULT Chromium-__TEXT: __TEXT= 4096 bytes RESULT Chromium-__DATA: __DATA= 4096 bytes RESULT Chromium-__OBJC: __OBJC= 0 bytes RESULT ChromiumFramework: ChromiumFramework= 64977912 bytes RESULT ChromiumFramework-__TEXT: __TEXT= 61120512 bytes RESULT ChromiumFramework-__DATA: __DATA= 1794048 bytes RESULT ChromiumFramework-__OBJC: __OBJC= 131072 bytes RESULT Chromium.app: Chromium.app= 91017216 bytes RESULT chrome-si: initializers= 28 files program finished with exit code 0 elapsedTime=0.193812 http://build.chromium.org/p/chromium/builders/Mac/builds/14989/steps/sizes/logs/stdio RESULT Chromium: Chromium= 8728 bytes RESULT Chromium-__TEXT: __TEXT= 4096 bytes RESULT Chromium-__DATA: __DATA= 4096 bytes RESULT Chromium-__OBJC: __OBJC= 0 bytes RESULT ChromiumFramework: ChromiumFramework= 64875308 bytes RESULT ChromiumFramework-__TEXT: __TEXT= 61018112 bytes RESULT ChromiumFramework-__DATA: __DATA= 1794048 bytes RESULT ChromiumFramework-__OBJC: __OBJC= 131072 bytes RESULT Chromium.app: Chromium.app= 90914816 bytes RESULT chrome-si: initializers= 20 files program finished with exit code 0 elapsedTime=0.066987
http://build.chromium.org/f/chromium/perf/mac-release/sizes/report.html?history=150&rev=-1&graph=chrome-si
I'll work on this upstream today to reduce those. Would one be tolerated? Though, perhaps I can make most of the initializers conditional on HB_NO_MT, such that NO_MT case does not use initializers.
(In reply to comment #54) > I'll work on this upstream today to reduce those. Would one be tolerated? Can you give an example of something you can't put in a static function using a macro like DEFINE_STATIC_LOCAL?
(In reply to comment #55) > (In reply to comment #54) > > I'll work on this upstream today to reduce those. Would one be tolerated? > > Can you give an example of something you can't put in a static function using a macro like DEFINE_STATIC_LOCAL? For various reasons HarfBuzz cannot link to libstdc++. Static local variables with initializers need libstdc++ to do the thread-safe initialization. No worries though, I fixed all but one already, and fixing the last one. Tomorrow bashi can do a new roll that has zero initializers, lazy or not.
Behdad fixed the problem on upstream so I'd like to check whether the patch doesn't increase the number of static initializers. Does anyone know how can I do it?
(In reply to comment #57) > Behdad fixed the problem on upstream so I'd like to check whether the patch doesn't increase the number of static initializers. Does anyone know how can I do it? The script for getting the number of static initializers is at: http://src.chromium.org/viewvc/chrome/trunk/tools/build/scripts/slave/chromium/sizes.py?view=log You can run that on the chromium binary. The bots step running the command is: http://build.chromium.org/p/chromium/builders/Mac/builds/15045/steps/sizes/logs/stdio
Created attachment 147183 [details] Patch for landing
Comment on attachment 147183 [details] Patch for landing Clearing flags on attachment: 147183 Committed r120156: <http://trac.webkit.org/changeset/120156>