Bug 69826

Summary: [Chromium] Implement font shaping with font-feature-settings on Mac
Product: WebKit Reporter: Kenichi Ishibashi <bashi>
Component: PlatformAssignee: Kenichi Ishibashi <bashi>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, behdad, caryclark, cc-bugs, efidler, eoconnor, evan, hyatt, ian, jamesr, jdaggett, mike, mitz, ned, rniwa, simon.fraser, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 67933, 79336, 88332    
Bug Blocks: 63796    
Attachments:
Description Flags
Patch
none
Patch (just fixed a typo and a style error)
none
Patch
none
Patch
none
rebased to ToT
none
Patch for landing
none
Patch for landing none

Kenichi Ishibashi
Reported 2011-10-11 03:26:24 PDT
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.
Attachments
Patch (65.55 KB, patch)
2012-01-27 01:06 PST, Kenichi Ishibashi 		no flags
DetailsFormatted DiffDiff
Patch (just fixed a typo and a style error) (65.60 KB, patch)
2012-01-29 18:07 PST, Kenichi Ishibashi 		no flags
DetailsFormatted DiffDiff
Patch (60.99 KB, patch)
2012-02-29 19:36 PST, Kenichi Ishibashi 		no flags
DetailsFormatted DiffDiff
Patch (82.47 KB, patch)
2012-03-07 20:24 PST, Kenichi Ishibashi 		no flags
DetailsFormatted DiffDiff
rebased to ToT (61.04 KB, patch)
2012-04-17 11:34 PDT, Kenichi Ishibashi 		no flags
DetailsFormatted DiffDiff
Patch for landing (60.95 KB, patch)
2012-06-04 18:32 PDT, Kenichi Ishibashi 		no flags
DetailsFormatted DiffDiff
Patch for landing (45.29 KB, patch)
2012-06-12 16:07 PDT, Kenichi Ishibashi 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
John Daggett
Comment 1 2012-01-09 22:35:05 PST
You're going to need CoreText when dealing with AAT fonts for Indic, I don't think OSX ships with OpenType fonts for Indic.
Kenichi Ishibashi
Comment 2 2012-01-09 22:48:52 PST
(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.
Kenichi Ishibashi
Comment 3 2012-01-27 01:06:04 PST
Created attachment 124271 [details] Patch
WebKit Review Bot
Comment 4 2012-01-27 01:07:48 PST
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.
Kenichi Ishibashi
Comment 5 2012-01-27 01:09:34 PST
(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.
Kenichi Ishibashi
Comment 6 2012-01-27 01:21:04 PST
*** Bug 67933 has been marked as a duplicate of this bug. ***
Kenichi Ishibashi
Comment 7 2012-01-27 01:21:52 PST
*** Bug 64930 has been marked as a duplicate of this bug. ***
Evan Martin
Comment 8 2012-01-27 11:26:36 PST
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?
Kenichi Ishibashi
Comment 9 2012-01-29 18:07:24 PST
Created attachment 124480 [details] Patch (just fixed a typo and a style error)
WebKit Review Bot
Comment 10 2012-01-29 18:09:38 PST
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.
Kenichi Ishibashi
Comment 11 2012-01-29 18:11:58 PST
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?
Kenichi Ishibashi
Comment 12 2012-01-31 16:21:04 PST
Any suggestions/comments?
Evan Martin
Comment 13 2012-01-31 16:27:27 PST
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?
Kenichi Ishibashi
Comment 14 2012-01-31 16:59:28 PST
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.
Behdad Esfahbod
Comment 15 2012-02-07 09:50:00 PST
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...
Kenichi Ishibashi
Comment 16 2012-02-07 16:19:42 PST
(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...
Kenichi Ishibashi
Comment 17 2012-02-21 15:39:53 PST
Hi Evan, Could you please take another look?
Evan Martin
Comment 18 2012-02-21 15:44:13 PST
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
Kenichi Ishibashi
Comment 19 2012-02-21 15:57:45 PST
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.
Kenichi Ishibashi
Comment 20 2012-02-29 19:36:48 PST
Created attachment 129617 [details] Patch
WebKit Review Bot
Comment 21 2012-02-29 19:43:26 PST
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.
Kenichi Ishibashi
Comment 22 2012-02-29 19:57:07 PST
(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?
Tony Chang
Comment 23 2012-03-01 10:41:29 PST
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.
Kenichi Ishibashi
Comment 24 2012-03-01 18:53:07 PST
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.
Kenichi Ishibashi
Comment 25 2012-03-05 16:24:29 PST
> > 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?
mitz
Comment 26 2012-03-05 17:02:28 PST
How much does HarfBuzzShaper overlap with the ComplexTextController implementation in ComplexTextControllerHarfBuzz?
Kenichi Ishibashi
Comment 27 2012-03-05 17:23:37 PST
(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.
Kenichi Ishibashi
Comment 28 2012-03-07 20:24:24 PST
Created attachment 130756 [details] Patch
WebKit Review Bot
Comment 29 2012-03-07 20:27:56 PST
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.
Kenichi Ishibashi
Comment 30 2012-03-07 20:32:37 PST
(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.
mitz
Comment 31 2012-03-08 23:17:12 PST
(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].
Kenichi Ishibashi
Comment 32 2012-03-08 23:31:47 PST
(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.
mitz
Comment 33 2012-03-08 23:34:18 PST
(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?
Kenichi Ishibashi
Comment 34 2012-03-08 23:56:37 PST
(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.
mitz
Comment 35 2012-03-09 00:08:52 PST
(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.
Kenichi Ishibashi
Comment 36 2012-03-09 14:07:50 PST
(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.
mitz
Comment 37 2012-03-09 15:21:58 PST
(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.
Kenichi Ishibashi
Comment 38 2012-03-09 16:12:11 PST
(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.
mitz
Comment 39 2012-03-09 16:54:14 PST
(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.
Kenichi Ishibashi
Comment 40 2012-03-09 17:22:22 PST
(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,
Kenichi Ishibashi
Comment 41 2012-03-25 16:52:37 PDT
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?
Ian Storm Taylor
Comment 42 2012-04-03 13:44:02 PDT
Also curious about progress on this. Any updates?
Kenichi Ishibashi
Comment 43 2012-04-17 11:34:31 PDT
Created attachment 137568 [details] rebased to ToT
Kenichi Ishibashi
Comment 44 2012-04-17 11:37:35 PDT
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?
WebKit Review Bot
Comment 45 2012-04-17 11:39:13 PDT
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.
Tony Chang
Comment 46 2012-05-17 09:52:05 PDT
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.
Kenichi Ishibashi
Comment 47 2012-06-04 18:31:41 PDT
(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.
Kenichi Ishibashi
Comment 48 2012-06-04 18:32:41 PDT
Created attachment 145674 [details] Patch for landing
WebKit Review Bot
Comment 49 2012-06-05 01:15:59 PDT
Comment on attachment 145674 [details] Patch for landing Clearing flags on attachment: 145674 Committed r119467: <http://trac.webkit.org/changeset/119467>
WebKit Review Bot
Comment 50 2012-06-05 01:16:07 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 51 2012-06-05 08:14:00 PDT
Re-opened since this is blocked by 88332
Ryosuke Niwa
Comment 52 2012-06-05 08:40:04 PDT
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
Behdad Esfahbod
Comment 54 2012-06-05 08:58:01 PDT
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.
Tony Chang
Comment 55 2012-06-05 10:24:19 PDT
(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?
Behdad Esfahbod
Comment 56 2012-06-05 15:50:03 PDT
(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.
Kenichi Ishibashi
Comment 57 2012-06-06 16:50:43 PDT
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?
Tony Chang
Comment 58 2012-06-06 17:09:50 PDT
(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
Kenichi Ishibashi
Comment 59 2012-06-12 16:07:07 PDT
Created attachment 147183 [details] Patch for landing
WebKit Review Bot
Comment 60 2012-06-12 21:32:46 PDT
Comment on attachment 147183 [details] Patch for landing Clearing flags on attachment: 147183 Committed r120156: <http://trac.webkit.org/changeset/120156>
WebKit Review Bot
Comment 61 2012-06-12 21:32:55 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.