Bug 69826 - [Chromium] Implement font shaping with font-feature-settings on Mac
Summary: [Chromium] Implement font shaping with font-feature-settings on Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenichi Ishibashi
URL:
Keywords:
: 64930 67933 (view as bug list)
Depends on: 67933 79336 88332
Blocks: 63796
  Show dependency treegraph
 
Reported: 2011-10-11 03:26 PDT by Kenichi Ishibashi
Modified: 2012-06-12 21:32 PDT (History)
18 users (show)

See Also:


Attachments
Patch (65.55 KB, patch)
2012-01-27 01:06 PST, Kenichi Ishibashi
no flags 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 Details | Formatted Diff | Diff
Patch (60.99 KB, patch)
2012-02-29 19:36 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (82.47 KB, patch)
2012-03-07 20:24 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
rebased to ToT (61.04 KB, patch)
2012-04-17 11:34 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (60.95 KB, patch)
2012-06-04 18:32 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (45.29 KB, patch)
2012-06-12 16:07 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 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.
Comment 1 John Daggett 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 Kenichi Ishibashi 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 Kenichi Ishibashi 2012-01-27 01:06:04 PST
Created attachment 124271 [details]
Patch
Comment 4 WebKit Review Bot 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 Kenichi Ishibashi 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.
Comment 6 Kenichi Ishibashi 2012-01-27 01:21:04 PST
*** Bug 67933 has been marked as a duplicate of this bug. ***
Comment 7 Kenichi Ishibashi 2012-01-27 01:21:52 PST
*** Bug 64930 has been marked as a duplicate of this bug. ***
Comment 8 Evan Martin 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?
Comment 9 Kenichi Ishibashi 2012-01-29 18:07:24 PST
Created attachment 124480 [details]
Patch (just fixed a typo and a style error)
Comment 10 WebKit Review Bot 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 Kenichi Ishibashi 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?
Comment 12 Kenichi Ishibashi 2012-01-31 16:21:04 PST
Any suggestions/comments?
Comment 13 Evan Martin 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 Kenichi Ishibashi 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 Behdad Esfahbod 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 Kenichi Ishibashi 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 Kenichi Ishibashi 2012-02-21 15:39:53 PST
Hi Evan,

Could you please take another look?
Comment 18 Evan Martin 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 Kenichi Ishibashi 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 Kenichi Ishibashi 2012-02-29 19:36:48 PST
Created attachment 129617 [details]
Patch
Comment 21 WebKit Review Bot 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 Kenichi Ishibashi 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?
Comment 23 Tony Chang 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.
Comment 24 Kenichi Ishibashi 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.
Comment 25 Kenichi Ishibashi 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 mitz 2012-03-05 17:02:28 PST
How much does HarfBuzzShaper overlap with the ComplexTextController implementation in ComplexTextControllerHarfBuzz?
Comment 27 Kenichi Ishibashi 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 Kenichi Ishibashi 2012-03-07 20:24:24 PST
Created attachment 130756 [details]
Patch
Comment 29 WebKit Review Bot 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 Kenichi Ishibashi 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.
Comment 31 mitz 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].
Comment 32 Kenichi Ishibashi 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.
Comment 33 mitz 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?
Comment 34 Kenichi Ishibashi 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.
Comment 35 mitz 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.
Comment 36 Kenichi Ishibashi 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 mitz 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 Kenichi Ishibashi 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 mitz 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 Kenichi Ishibashi 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,
Comment 41 Kenichi Ishibashi 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?
Comment 42 Ian Storm Taylor 2012-04-03 13:44:02 PDT
Also curious about progress on this. Any updates?
Comment 43 Kenichi Ishibashi 2012-04-17 11:34:31 PDT
Created attachment 137568 [details]
rebased to ToT
Comment 44 Kenichi Ishibashi 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?
Comment 45 WebKit Review Bot 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.
Comment 46 Tony Chang 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.
Comment 47 Kenichi Ishibashi 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.
Comment 48 Kenichi Ishibashi 2012-06-04 18:32:41 PDT
Created attachment 145674 [details]
Patch for landing
Comment 49 WebKit Review Bot 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>
Comment 50 WebKit Review Bot 2012-06-05 01:16:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 51 WebKit Review Bot 2012-06-05 08:14:00 PDT
Re-opened since this is blocked by 88332
Comment 52 Ryosuke Niwa 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
Comment 54 Behdad Esfahbod 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.
Comment 55 Tony Chang 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?
Comment 56 Behdad Esfahbod 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.
Comment 57 Kenichi Ishibashi 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?
Comment 58 Tony Chang 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
Comment 59 Kenichi Ishibashi 2012-06-12 16:07:07 PDT
Created attachment 147183 [details]
Patch for landing
Comment 60 WebKit Review Bot 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>
Comment 61 WebKit Review Bot 2012-06-12 21:32:55 PDT
All reviewed patches have been landed.  Closing bug.