Bug 69826 - [Chromium] Implement font shaping with font-feature-settings on Mac
: [Chromium] Implement font shaping with font-feature-settings on Mac
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 67933 79336 88332
: 63796
  Show dependency treegraph
 
Reported: 2011-10-11 03:26 PST by
Modified: 2012-06-12 21:32 PST (History)


Attachments
Patch (65.55 KB, patch)
2012-01-27 01:06 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (just fixed a typo and a style error) (65.60 KB, patch)
2012-01-29 18:07 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (60.99 KB, patch)
2012-02-29 19:36 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch (82.47 KB, patch)
2012-03-07 20:24 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
rebased to ToT (61.04 KB, patch)
2012-04-17 11:34 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (60.95 KB, patch)
2012-06-04 18:32 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (45.29 KB, patch)
2012-06-12 16:07 PST, Kenichi Ishibashi
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-10-11 03:26:24 PST
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.
------- Comment #1 From 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.
------- Comment #2 From 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.
------- Comment #3 From 2012-01-27 01:06:04 PST -------
Created an attachment (id=124271) [details]
Patch
------- Comment #4 From 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.
------- Comment #5 From 2012-01-27 01:09:34 PST -------
(In reply to comment #3)
> Created an attachment (id=124271) [details] [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.
------- Comment #6 From 2012-01-27 01:21:04 PST -------
*** Bug 67933 has been marked as a duplicate of this bug. ***
------- Comment #7 From 2012-01-27 01:21:52 PST -------
*** Bug 64930 has been marked as a duplicate of this bug. ***
------- Comment #8 From 2012-01-27 11:26:36 PST -------
(From update of attachment 124271 [details])
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?
------- Comment #9 From 2012-01-29 18:07:24 PST -------
Created an attachment (id=124480) [details]
Patch (just fixed a typo and a style error)
------- Comment #10 From 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.
------- Comment #11 From 2012-01-29 18:11:58 PST -------
(From update of attachment 124271 [details])
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?
------- Comment #12 From 2012-01-31 16:21:04 PST -------
Any suggestions/comments?
------- Comment #13 From 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?
------- Comment #14 From 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.
------- Comment #15 From 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...
------- Comment #16 From 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...
------- Comment #17 From 2012-02-21 15:39:53 PST -------
Hi Evan,

Could you please take another look?
------- Comment #18 From 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
------- Comment #19 From 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.
------- Comment #20 From 2012-02-29 19:36:48 PST -------
Created an attachment (id=129617) [details]
Patch
------- Comment #21 From 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.
------- Comment #22 From 2012-02-29 19:57:07 PST -------
(In reply to comment #20)
> Created an attachment (id=129617) [details] [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 #23 From 2012-03-01 10:41:29 PST -------
(From update of attachment 129617 [details])
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 #24 From 2012-03-01 18:53:07 PST -------
(From update of attachment 129617 [details])
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.
------- Comment #25 From 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?
------- Comment #26 From 2012-03-05 17:02:28 PST -------
How much does HarfBuzzShaper overlap with the ComplexTextController implementation in ComplexTextControllerHarfBuzz?
------- Comment #27 From 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.
------- Comment #28 From 2012-03-07 20:24:24 PST -------
Created an attachment (id=130756) [details]
Patch
------- Comment #29 From 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.
------- Comment #30 From 2012-03-07 20:32:37 PST -------
(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.
------- Comment #31 From 2012-03-08 23:17:12 PST -------
(In reply to comment #30)
> (In reply to comment #28)
> > Created an attachment (id=130756) [details] [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].
------- Comment #32 From 2012-03-08 23:31:47 PST -------
(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, but I'd like to add this feature and I couldn't come up with other ideas.
------- Comment #33 From 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] [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].

> 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?
------- Comment #34 From 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] [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.
------- Comment #35 From 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] [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] [details] patch wasn't good
> > 
> > I didn’t object to attachment 129617 [details] [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.
------- Comment #36 From 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.
------- Comment #37 From 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.
------- Comment #38 From 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.
------- Comment #39 From 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.
------- Comment #40 From 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] [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,
------- Comment #41 From 2012-03-25 16:52:37 PST -------
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] [details] (though I am not the right person to review it), if you would like to go that way.

Any updates on this?
------- Comment #42 From 2012-04-03 13:44:02 PST -------
Also curious about progress on this. Any updates?
------- Comment #43 From 2012-04-17 11:34:31 PST -------
Created an attachment (id=137568) [details]
rebased to ToT
------- Comment #44 From 2012-04-17 11:37:35 PST -------
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?
------- Comment #45 From 2012-04-17 11:39:13 PST -------
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 #46 From 2012-05-17 09:52:05 PST -------
(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.
------- Comment #47 From 2012-06-04 18:31:41 PST -------
(In reply to comment #46)
> (From update of attachment 137568 [details] [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.
------- Comment #48 From 2012-06-04 18:32:41 PST -------
Created an attachment (id=145674) [details]
Patch for landing
------- Comment #49 From 2012-06-05 01:15:59 PST -------
(From update of attachment 145674 [details])
Clearing flags on attachment: 145674

Committed r119467: <http://trac.webkit.org/changeset/119467>
------- Comment #50 From 2012-06-05 01:16:07 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #51 From 2012-06-05 08:14:00 PST -------
Re-opened since this is blocked by 88332
------- Comment #52 From 2012-06-05 08:40:04 PST -------
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
------- Comment #54 From 2012-06-05 08:58:01 PST -------
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.
------- Comment #55 From 2012-06-05 10:24:19 PST -------
(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?
------- Comment #56 From 2012-06-05 15:50:03 PST -------
(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.
------- Comment #57 From 2012-06-06 16:50:43 PST -------
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?
------- Comment #58 From 2012-06-06 17:09:50 PST -------
(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
------- Comment #59 From 2012-06-12 16:07:07 PST -------
Created an attachment (id=147183) [details]
Patch for landing
------- Comment #60 From 2012-06-12 21:32:46 PST -------
(From update of attachment 147183 [details])
Clearing flags on attachment: 147183

Committed r120156: <http://trac.webkit.org/changeset/120156>
------- Comment #61 From 2012-06-12 21:32:55 PST -------
All reviewed patches have been landed.  Closing bug.