Check for Unicode in Font before ICU call

Check for Unicode in Font before ICU call

Checking for Unicode is current font before checking for normalization
in ICU.
With above patch, following test case output is matching with Firefox now.
1. fast/text/atsui-multiple-renderers.html
2. fast/text/capitalize-boundaries.html
3. fast/text/should-use-atsui.html

Need to create Test case specific to Vietnamese similar to one 
reported in issue. Trying to make font which emulates the issue.

BUG=449479

[h.joshi, 2015-01-27 13:08:40]

h.joshi@samsung.com changed reviewers:
+ behdad@chromium.org, eae@chromium.org

[h.joshi, 2015-01-27 15:00:12]

Emil/Behdad: This is not final patch as Layout test specific to Vietnamese is
not ready yet.
If possible, pls check changes and suggest if its on the 
right path or not.

Issue is, Font is not having all Unicode's and ICU call
is not checking for it. Due to which default glyph is rendered.

[behdad_google, 2015-01-27 19:27:21]

On 2015/01/27 15:00:12, h.joshi wrote:
> Emil/Behdad: This is not final patch as Layout test specific to Vietnamese is
> not ready yet.
> If possible, pls check changes and suggest if its on the 
> right path or not.
> 
> Issue is, Font is not having all Unicode's and ICU call
> is not checking for it. Due to which default glyph is rendered.

Long-term we want to remove normalization during optimization and rely on
harfbuzz itself.  But we are rather far from that.

[h.joshi, 2015-01-29 11:01:57]

@behdad: So all normalization etc for which ICU is used 
will be done by Harfbuzz?

Do we have some document etc which I can read about the same.
Pls also suggest for this patch, external font used in the Vietnamese
site does not seems to be open source so cannot use the same to make test case.
Trying to look for some matching open source font.

One more information about this patch is, this issue was not reproducible in old
Chrome 
(Version 31/34). Issue came between Chromium CL 284099 and 284275.
 between these two CL, major update seems to be ICU which is upgraded from 4.6
to 5.2, other
Blink CL does not seems to be related to this as most font/text related patches
are platform dependent.

[h.joshi, 2015-02-09 04:57:03]

It seems that font is not having correct OTF rules for diacritical marks or in
some fonts
one glyph is missing due to which Harfbuzz is not able to convert to final
output which is correct. 
Its interesting that final form is present in the font.

 Checked behavior in Firefox and Safari, they are able to render things and are
not showing
default glyph what is shown in Blink/Chrome. Debugged more and found if we use
following ICU 
normalization API:
icu::Normalizer::normalize
 which converts text which has diacritical marks to its final form and after
this call we are getting correct output.
Working on test case for this patch, will be helpful if you could tell me if
this new approach is correct
or not.

Or if we want to replace "normalization" of ICU completely what can we do with
this issue.
Pls suggest.

[eae, 2015-02-10 15:40:12]

I like the idea behind this change but as Behdad just said we are trying to
remove the normalization path all together. Adding an extra loop over the string
seems really expensive. Can we get away with not doing the extra pass?

https://codereview.chromium.org/870523003/diff/20001/Source/platform/fonts/Si...
File Source/platform/fonts/SimpleFontData.cpp (right):

https://codereview.chromium.org/870523003/diff/20001/Source/platform/fonts/Si...
Source/platform/fonts/SimpleFontData.cpp:465: unsigned pageNumber = (character /
GlyphPage::size);
Remove unnecessary parentheses.

https://codereview.chromium.org/870523003/diff/20001/Source/platform/fonts/sh...
File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/870523003/diff/20001/Source/platform/fonts/sh...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:366: for (unsigned i = 0; i <
(unsigned)run.length(); ++i) {
Here and elsewhere, use static_cast instead of C-style casts.

https://codereview.chromium.org/870523003/diff/20001/Source/platform/fonts/sh...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:367: UChar ch = source[i];
Only used once, no need for local variable.

[h.joshi, 2015-02-17 03:44:48]

@Emil: Made suggested changes, Pls review.

https://codereview.chromium.org/870523003/diff/20001/Source/platform/fonts/Si...
File Source/platform/fonts/SimpleFontData.cpp (right):

https://codereview.chromium.org/870523003/diff/20001/Source/platform/fonts/Si...
Source/platform/fonts/SimpleFontData.cpp:465: unsigned pageNumber = (character /
GlyphPage::size);
On 2015/02/10 15:40:12, eae wrote:
> Remove unnecessary parentheses.

Done.

https://codereview.chromium.org/870523003/diff/20001/Source/platform/fonts/sh...
File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/870523003/diff/20001/Source/platform/fonts/sh...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:366: for (unsigned i = 0; i <
(unsigned)run.length(); ++i) {
On 2015/02/10 15:40:12, eae wrote:
> Here and elsewhere, use static_cast instead of C-style casts.

Done.

https://codereview.chromium.org/870523003/diff/20001/Source/platform/fonts/sh...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:367: UChar ch = source[i];
On 2015/02/10 15:40:12, eae wrote:
> Only used once, no need for local variable.

Done.

[eae, 2015-02-17 16:27:13]

On Mon, Feb 16, 2015 at 7:44 PM,  <h.joshi@samsung.com> wrote:
> @Emil: Made suggested changes, Pls review.

We're trying to remove the entire concept of normalization so I'd
rather we not add any more logic there. Can you think of any way to
make this check work without the normalization path?

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[h.joshi, 2015-02-24 09:41:25]

@Emil: Need your guidance and also help to clear doubt.
In current Blink code, ICU normalization is used and called in following code
flow:
HarfBuzzShaper::createHarfBuzzRuns() -> collectCandidateRuns ->
handleMultipleUChar -> canRenderCombiningCharacterSequence -> unorm_normalize

 As per changes in this patch, I am doing normalization at upper stage.
If we do normalization as per this patch and remove current normalization patch
as discribed above, will that be okey?

Patch 1 (https://codereview.chromium.org/870523003/#ps1) was without
normalization, it was working fine on Linux but on Android
overlapping was observed.

To do without ICU normalization is to either implement state or array based
approach for diacritical marks.
Pls Suggest.

[eae, 2015-02-24 16:33:16]

Let's discuss this once Behdad is back, he understands this better than anyone
else.

[h.joshi, 2015-03-17 12:09:27]

@Behdad: Need your suggestion on this.
Pls see discussion with Emil above mainly
(https://codereview.chromium.org/870523003/#msg9) and suggest.

[h.joshi, 2015-04-03 14:10:51]

@Behdad: Pls review and suggest.

[h.joshi, 2015-04-30 11:18:14]

@Behdad: Pls review and suggest.

[behdad_google, 2015-05-18 21:59:16]

behdad@google.com changed reviewers:
+ behdad@google.com

[behdad_google, 2015-05-18 21:59:17]

I don't want to add the normalizer call.  We have other, longer-term, plans to
fix that issue.

The refactoring looks good though.  Please submit that as a separate change.

[behdad_google, 2015-05-18 22:01:23]

Also, as noted on the bug, this should be fixed already:

  https://code.google.com/p/chromium/issues/detail?id=464148

[h.joshi, 2015-05-19 12:08:48]

@behdad_google:
1. Checked in M44 (Android and Linux) and issue is not fixed yet.
2. for the refactoring: Okey, will make new CL and submit the same.

[h.joshi, 2015-05-20 11:42:37]

Patchset #5 (id:80001) has been deleted

[rwlbuis, 2016-11-11 21:58:57]

Will close, bug is marked as fixed and I can't reproduce the misrendering
either.