Bug 65904 – [Chromium] Implement font shaping with font-feature-settings on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Kenichi Ishibashi]

Implement text shaping with font-feature-settings CSS property.  This bug entry is for Chromium Windows port.

[Kenichi Ishibashi]

Created attachment 103754 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #1)
> Created an attachment (id=103754) [details]
> Patch

This is an WIP patch and is not for review.

Using Uniscribe OpenType APIs for complex text shaping to implement font feature settings.

For now, I enclose the modified part by USE_UNISCRIBE_OPENTYPE macro so that I can handily switch over.
I'd like to get some feedback from someone who are familiar with this area before moving forward on.  Cc'ing people who I can see thier name on ChangeLog and git blame.  Please let me know if there are person who are well suited for review this patch.

[Kenichi Ishibashi]

Created attachment 106109 [details]
Patch V1

[Kenichi Ishibashi]

(In reply to comment #3)
> Created an attachment (id=106109) [details]
> Patch V1

Revised the patch.  Could you guys take a look?

BTW, I couldn't create the test expectation for css3/font-feature-settings-rendering.html on my Windows machine, even run_webkit_tests.sh said "Writing new expected result ...".  There was no files on the relevant folder...

[Kenichi Ishibashi]

Created attachment 106375 [details]
Patch V2

[Kenichi Ishibashi]

(In reply to comment #5)
> Created an attachment (id=106375) [details]
> Patch V2

Added expectations. I'd appreciate if someone could review this patch.

[Kent Tamura]

Comment on attachment 106375 [details]
Patch V2

View in context: https://bugs.webkit.org/attachment.cgi?id=106375&action=review

> LayoutTests/ChangeLog:12
> +        * platform/chromium/test_expectations.txt: VISTA and WIN7 should pass css3/font-feature-settings-rendering.html.

Why won't it pass on XP?

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:66
> +static ScriptItemizeOpenTypeFunc sScriptItemizeOpenTypeFunc = 0;
> +static ScriptShapeOpenTypeFunc sScriptShapeOpenTypeFunc = 0;
> +static bool sOpenTypeFunctionsLoaded = false;

The sFooBar naming rule for file-scope variables is not common in WebKit.
Though we don't have the standard rule for it, some files use "gFooBar" for file-scope variables,
or making them to static data members of a class and use "s_fooBar".

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:750
> +            memcpy(&shaping.m_visualAttributes[i], &glyphProps[i].sva,
> +                   sizeof(SCRIPT_VISATTR));

I don't think we need to fold this line.  It's not so long.

[Kenichi Ishibashi]

Created attachment 106396 [details]
Patch V3

[Kenichi Ishibashi]

Comment on attachment 106375 [details]
Patch V2

View in context: https://bugs.webkit.org/attachment.cgi?id=106375&action=review

Hi Kent-san,

Thank you so much for taking a look.  Revised the patch.

>> LayoutTests/ChangeLog:12
>> +        * platform/chromium/test_expectations.txt: VISTA and WIN7 should pass css3/font-feature-settings-rendering.html.
> 
> Why won't it pass on XP?

I should have added more detailed description here.  According to the MSDN, Uniscribe OpenType APIs are available on usp10.dll version 1.600 or greater, which is not installed by default on XP. I added description and added PASS to the expectation since usp10.dll version 1.600 or greater can be installed on XP.

>> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:66
>> +static bool sOpenTypeFunctionsLoaded = false;
> 
> The sFooBar naming rule for file-scope variables is not common in WebKit.
> Though we don't have the standard rule for it, some files use "gFooBar" for file-scope variables,
> or making them to static data members of a class and use "s_fooBar".

Changed from sFooBar to gFooBar style.

>> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:750
>> +                   sizeof(SCRIPT_VISATTR));
> 
> I don't think we need to fold this line.  It's not so long.

Done.

[Kent Tamura]

Comment on attachment 106375 [details]
Patch V2

View in context: https://bugs.webkit.org/attachment.cgi?id=106375&action=review

>>> LayoutTests/ChangeLog:12
>>> +        * platform/chromium/test_expectations.txt: VISTA and WIN7 should pass css3/font-feature-settings-rendering.html.
>> 
>> Why won't it pass on XP?
> 
> I should have added more detailed description here.  According to the MSDN, Uniscribe OpenType APIs are available on usp10.dll version 1.600 or greater, which is not installed by default on XP. I added description and added PASS to the expectation since usp10.dll version 1.600 or greater can be installed on XP.

Ok, so we should have a separated entry for XP with the WONTFIX keyword, and put it in the "WONTFIX TESTS" section of test_expectations.txt.

[Kent Tamura]

Comment on attachment 106396 [details]
Patch V3

View in context: https://bugs.webkit.org/attachment.cgi?id=106396&action=review

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.h:52
> +#define UNISCRIBE_HELPER_FEATURES 4

We'd like to avoid to use preprocessor macros for constant values.

[Kenichi Ishibashi]

Created attachment 106551 [details]
Patch V4

[Kenichi Ishibashi]

Comment on attachment 106375 [details]
Patch V2

View in context: https://bugs.webkit.org/attachment.cgi?id=106375&action=review

>>>> LayoutTests/ChangeLog:12
>>>> +        * platform/chromium/test_expectations.txt: VISTA and WIN7 should pass css3/font-feature-settings-rendering.html.
>>> 
>>> Why won't it pass on XP?
>> 
>> I should have added more detailed description here.  According to the MSDN, Uniscribe OpenType APIs are available on usp10.dll version 1.600 or greater, which is not installed by default on XP. I added description and added PASS to the expectation since usp10.dll version 1.600 or greater can be installed on XP.
> 
> Ok, so we should have a separated entry for XP with the WONTFIX keyword, and put it in the "WONTFIX TESTS" section of test_expectations.txt.

Done. Thank you for letting me know appropriate way to update expectations.

[Kenichi Ishibashi]

Comment on attachment 106396 [details]
Patch V3

View in context: https://bugs.webkit.org/attachment.cgi?id=106396&action=review

>> Source/WebCore/platform/graphics/chromium/UniscribeHelper.h:52
>> +#define UNISCRIBE_HELPER_FEATURES 4
> 
> We'd like to avoid to use preprocessor macros for constant values.

Changed to use constant variables.

[Kenichi Ishibashi]

Could someone please review this? Or could you advice me to find the right person for review?

[Hajime Morrita]

(In reply to comment #15)
> Could someone please review this? Or could you advice me to find the right person for review?
Who wrote the original code? If he/she read gives an unofficial review, I'd be happy to rubber-stamp this.

[Kenneth Russell]

Comment on attachment 106551 [details]
Patch V4

View in context: https://bugs.webkit.org/attachment.cgi?id=106551&action=review

I'm not really qualified to review this code but had one question while looking it over. Perhaps reed could do an unofficial review? If so I would be happy to r+ it.

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:529
> +                            m_scriptTags.remove(i + 1, j);

Is this behavior of removing runs that were tagged SCRIPT_TAG_UNKNOWN specified anywhere? If so, why leave one run in place with that tag? From the documentation it seems that the run can contain glyphs that have shapes (i.e., punctuation) so simply dropping them seems wrong.

[Kenichi Ishibashi]

Hi Morrita-san and Kenneth,

Thank you so much offering rubber-stamp r+. I'd appreciate if reed could review the patch.

(In reply to comment #17)
> > Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:529
> > +                            m_scriptTags.remove(i + 1, j);
> 
> Is this behavior of removing runs that were tagged SCRIPT_TAG_UNKNOWN specified anywhere? If so, why leave one run in place with that tag? From the documentation it seems that the run can contain glyphs that have shapes (i.e., punctuation) so simply dropping them seems wrong.

No, this compacts more than two successive runs which were tagged SCRIPT_TAG_UNKNOWN into one run which is tagged SCRIPT_TAG_UNKNOWN, too. Glyphs contained these runs aren't discarded.

This is needed for support some OpenType features like "frac" and "afrc" (See http://dev.w3.org/csswg/css3-fonts/#propdef-font-variant-numeric).  As far as I checked, ScriptItemizeOpenType() divides "1/3" into 3 SCRIPT_TAG_UNKNOWN runs.  Since OpenType features are applied for each run, "frac" doesn't affect when "1/3" divides 3 runs so I added this logic.

[Mike Reed]

Looks fine, though I'm not familiar with these new APIs yet.

[Kenneth Russell]

(In reply to comment #18)
> Hi Morrita-san and Kenneth,
> 
> Thank you so much offering rubber-stamp r+. I'd appreciate if reed could review the patch.
> 
> (In reply to comment #17)
> > > Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:529
> > > +                            m_scriptTags.remove(i + 1, j);
> > 
> > Is this behavior of removing runs that were tagged SCRIPT_TAG_UNKNOWN specified anywhere? If so, why leave one run in place with that tag? From the documentation it seems that the run can contain glyphs that have shapes (i.e., punctuation) so simply dropping them seems wrong.
> 
> No, this compacts more than two successive runs which were tagged SCRIPT_TAG_UNKNOWN into one run which is tagged SCRIPT_TAG_UNKNOWN, too. Glyphs contained these runs aren't discarded.

I see. The SCRIPT_ITEMs only contain the starting position of the run.

> This is needed for support some OpenType features like "frac" and "afrc" (See http://dev.w3.org/csswg/css3-fonts/#propdef-font-variant-numeric).  As far as I checked, ScriptItemizeOpenType() divides "1/3" into 3 SCRIPT_TAG_UNKNOWN runs.  Since OpenType features are applied for each run, "frac" doesn't affect when "1/3" divides 3 runs so I added this logic.

OK.

Based on this and reed's review, fine with me assuming it's been tested. r=me

[Kenichi Ishibashi]

(In reply to comment #20)
> OK.
> 
> Based on this and reed's review, fine with me assuming it's been tested. r=me

Thank you for r+. The patch passed trybots(win,win_layout). However, when I tested the patch on WinXP by hand, I saw the text weren't rendered correctly. I'm investigating the cause and will revise the patch if needed.

[Kenichi Ishibashi]

(In reply to comment #21)
> I saw the text weren't rendered correctly. I'm investigating the cause and will revise the patch if needed.

The problem I found has no relation with this patch. I can reproduce the problem with Chrome canary on WinXP without applying this patch. I filed the bug (http://crbug.com/98855).

I think the patch has no problem for now. I'll revise the patch for land.

[Kenichi Ishibashi]

Created attachment 109592 [details]
Patch for land

[Kenichi Ishibashi]

Created attachment 109595 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 109595 [details]
Patch for landing

Rejecting attachment 109595 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
aa34921e6d114d98bc0b887e8db48ffa8658d226
r96579 = 4aaff3436823dc2e71b83a7140d7fee690ff859c
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9935487

[Kenichi Ishibashi]

Created attachment 109600 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 109600 [details]
Patch for landing

Clearing flags on attachment: 109600

Committed r96582: <http://trac.webkit.org/changeset/96582>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

This patch cause this test to change behavior:

http://trac.webkit.org/changeset/96602

If that change is a problem, please let me know.