Bug 65904 - [Chromium] Implement font shaping with font-feature-settings on Windows
Summary: [Chromium] Implement font shaping with font-feature-settings on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenichi Ishibashi
URL:
Keywords:
Depends on:
Blocks: 63796
  Show dependency treegraph
 
Reported: 2011-08-09 02:57 PDT by Kenichi Ishibashi
Modified: 2011-10-04 09:53 PDT (History)
10 users (show)

See Also:


Attachments
Patch (13.29 KB, patch)
2011-08-12 02:11 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V1 (15.11 KB, patch)
2011-09-02 03:05 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V2 (21.94 KB, patch)
2011-09-05 18:57 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V3 (22.01 KB, patch)
2011-09-06 00:50 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V4 (26.24 KB, patch)
2011-09-07 00:27 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for land (26.45 KB, patch)
2011-10-04 01:39 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (26.45 KB, patch)
2011-10-04 01:49 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (26.46 KB, patch)
2011-10-04 03:00 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-08-09 02:57:43 PDT
Implement text shaping with font-feature-settings CSS property.  This bug entry is for Chromium Windows port.
Comment 1 Kenichi Ishibashi 2011-08-12 02:11:44 PDT
Created attachment 103754 [details]
Patch
Comment 2 Kenichi Ishibashi 2011-08-12 02:14:38 PDT
(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.
Comment 3 Kenichi Ishibashi 2011-09-02 03:05:38 PDT
Created attachment 106109 [details]
Patch V1
Comment 4 Kenichi Ishibashi 2011-09-02 03:16:23 PDT
(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...
Comment 5 Kenichi Ishibashi 2011-09-05 18:57:40 PDT
Created attachment 106375 [details]
Patch V2
Comment 6 Kenichi Ishibashi 2011-09-05 19:02:09 PDT
(In reply to comment #5)
> Created an attachment (id=106375) [details]
> Patch V2

Added expectations. I'd appreciate if someone could review this patch.
Comment 7 Kent Tamura 2011-09-05 23:43:38 PDT
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.
Comment 8 Kenichi Ishibashi 2011-09-06 00:50:41 PDT
Created attachment 106396 [details]
Patch V3
Comment 9 Kenichi Ishibashi 2011-09-06 00:52:32 PDT
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.
Comment 10 Kent Tamura 2011-09-06 22:46:12 PDT
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.
Comment 11 Kent Tamura 2011-09-06 22:47:31 PDT
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.
Comment 12 Kenichi Ishibashi 2011-09-07 00:27:43 PDT
Created attachment 106551 [details]
Patch V4
Comment 13 Kenichi Ishibashi 2011-09-07 00:29:16 PDT
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.
Comment 14 Kenichi Ishibashi 2011-09-07 00:29:59 PDT
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.
Comment 15 Kenichi Ishibashi 2011-09-27 23:17:31 PDT
Could someone please review this? Or could you advice me to find the right person for review?
Comment 16 Hajime Morrita 2011-09-29 00:09:05 PDT
(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.
Comment 17 Kenneth Russell 2011-09-29 08:42:35 PDT
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.
Comment 18 Kenichi Ishibashi 2011-09-29 21:38:53 PDT
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.
Comment 19 Mike Reed 2011-09-30 05:47:54 PDT
Looks fine, though I'm not familiar with these new APIs yet.
Comment 20 Kenneth Russell 2011-09-30 12:12:28 PDT
(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
Comment 21 Kenichi Ishibashi 2011-10-02 22:25:11 PDT
(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.
Comment 22 Kenichi Ishibashi 2011-10-04 01:35:20 PDT
(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.
Comment 23 Kenichi Ishibashi 2011-10-04 01:39:14 PDT
Created attachment 109592 [details]
Patch for land
Comment 24 Kenichi Ishibashi 2011-10-04 01:49:50 PDT
Created attachment 109595 [details]
Patch for landing
Comment 25 WebKit Review Bot 2011-10-04 02:53:34 PDT
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
Comment 26 Kenichi Ishibashi 2011-10-04 03:00:23 PDT
Created attachment 109600 [details]
Patch for landing
Comment 27 WebKit Review Bot 2011-10-04 03:15:13 PDT
Comment on attachment 109600 [details]
Patch for landing

Clearing flags on attachment: 109600

Committed r96582: <http://trac.webkit.org/changeset/96582>
Comment 28 WebKit Review Bot 2011-10-04 03:15:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Adam Barth 2011-10-04 09:53:04 PDT
This patch cause this test to change behavior:

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

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