Bug 65904

Created attachment 103754 [details] Patch

(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.

Created attachment 106109 [details] Patch V1

(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...

Created attachment 106375 [details] Patch V2

(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 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.

Created attachment 106396 [details] Patch V3

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 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 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.

Created attachment 106551 [details] Patch V4

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 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.

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

(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 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.

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.

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

(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

(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.

(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.

Created attachment 109592 [details] Patch for land

Created attachment 109595 [details] Patch for landing

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

Created attachment 109600 [details] Patch for landing

Comment on attachment 109600 [details] Patch for landing Clearing flags on attachment: 109600 Committed r96582: <http://trac.webkit.org/changeset/96582>

All reviewed patches have been landed. Closing bug.

This patch cause this test to change behavior: http://trac.webkit.org/changeset/96602 If that change is a problem, please let me know.