Handle missing glyphs when adjusting spacing in SimpleShaper

Handle missing glyphs when adjusting spacing in SimpleShaper

Handle the case where the space glyph is used instead of a missing zero-
width-space, soft-hypen or non-printable space glyph when adjusting the
spacing in SimpleShaper::advanceInternal.

TEST=fast/text/soft-hyphen-simple-text.html
BUG=422050
R=pdr@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183685

[eae, 2014-10-13 22:08:42]

eae@chromium.org changed reviewers:
+ pdr@chromium.org

[eae, 2014-10-13 23:02:23]

eae@chromium.org changed reviewers:
+ dglazkov@chromium.org

[pdr., 2014-10-13 23:53:30]

https://codereview.chromium.org/648293003/diff/50001/Source/platform/fonts/sh...
File Source/platform/fonts/shaping/SimpleShaper.cpp (right):

https://codereview.chromium.org/648293003/diff/50001/Source/platform/fonts/sh...
Source/platform/fonts/shaping/SimpleShaper.cpp:210: if (hasExtraSpacing &&
width)
(Maybe as a followup) Can you add a bool to better describe what's going on when
width=0? This seems to have been first added back in
https://chromium.googlesource.com/chromium/blink/+/4a2a3ba905642998075cf4b161...
and it'll be easy to make this mistake again.

https://codereview.chromium.org/648293003/diff/50001/Source/platform/fonts/sh...
Source/platform/fonts/shaping/SimpleShaper.cpp:211: width = adjustSpacing(width,
charData, *fontData, glyphBuffer);
Does it make sense to check for width in adjustSpacing after this patch? I don't
fully understand the comment in adjustSpacing in the context of this patch.

[eae, 2014-10-13 23:55:46]

On 2014/10/13 23:53:30, pdr wrote:
>
https://codereview.chromium.org/648293003/diff/50001/Source/platform/fonts/sh...
> File Source/platform/fonts/shaping/SimpleShaper.cpp (right):
> 
>
https://codereview.chromium.org/648293003/diff/50001/Source/platform/fonts/sh...
> Source/platform/fonts/shaping/SimpleShaper.cpp:210: if (hasExtraSpacing &&
> width)
> (Maybe as a followup) Can you add a bool to better describe what's going on
when
> width=0? This seems to have been first added back in
>
https://chromium.googlesource.com/chromium/blink/+/4a2a3ba905642998075cf4b161...
> and it'll be easy to make this mistake again.
> 
>
https://codereview.chromium.org/648293003/diff/50001/Source/platform/fonts/sh...
> Source/platform/fonts/shaping/SimpleShaper.cpp:211: width =
adjustSpacing(width,
> charData, *fontData, glyphBuffer);
> Does it make sense to check for width in adjustSpacing after this patch? I
don't
> fully understand the comment in adjustSpacing in the context of this patch.

The workaround for missing glyphs really should remain in 
adjustSpacing advanceInternal and not leak out into the adjustSpacing. I could
use an extra bool instead of checking the width as suggested above if you think
that makes it clearer.

[pdr., 2014-10-14 00:12:24]

On 2014/10/13 at 23:55:46, eae wrote:
> On 2014/10/13 23:53:30, pdr wrote:
> >
https://codereview.chromium.org/648293003/diff/50001/Source/platform/fonts/sh...
> > File Source/platform/fonts/shaping/SimpleShaper.cpp (right):
> > 
> >
https://codereview.chromium.org/648293003/diff/50001/Source/platform/fonts/sh...
> > Source/platform/fonts/shaping/SimpleShaper.cpp:210: if (hasExtraSpacing &&
> > width)
> > (Maybe as a followup) Can you add a bool to better describe what's going on
when
> > width=0? This seems to have been first added back in
> >
https://chromium.googlesource.com/chromium/blink/+/4a2a3ba905642998075cf4b161...
> > and it'll be easy to make this mistake again.
> > 
> >
https://codereview.chromium.org/648293003/diff/50001/Source/platform/fonts/sh...
> > Source/platform/fonts/shaping/SimpleShaper.cpp:211: width =
adjustSpacing(width,
> > charData, *fontData, glyphBuffer);
> > Does it make sense to check for width in adjustSpacing after this patch? I
don't
> > fully understand the comment in adjustSpacing in the context of this patch.
> 
> The workaround for missing glyphs really should remain in 
> adjustSpacing advanceInternal and not leak out into the adjustSpacing. I could
use an extra bool instead of checking the width as suggested above if you think
that makes it clearer.

Just to make sure I follow, your plan is to handle the case when width==0 in
advanceInternal instead of adjustSpacing, probably with an ASSERT(width) in
adjustSpacing? If so, SGTM :)

It would be nice to better explain what's going on when width==0 using a bool
but, after trying it myself, I'm not sure a bool will make it more
understandable after all :/

[eae, 2014-10-14 00:14:57]

On 2014/10/14 00:12:24, pdr wrote:
> On 2014/10/13 at 23:55:46, eae wrote:
> > On 2014/10/13 23:53:30, pdr wrote:
> > >
>
https://codereview.chromium.org/648293003/diff/50001/Source/platform/fonts/sh...
> > > File Source/platform/fonts/shaping/SimpleShaper.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/648293003/diff/50001/Source/platform/fonts/sh...
> > > Source/platform/fonts/shaping/SimpleShaper.cpp:210: if (hasExtraSpacing &&
> > > width)
> > > (Maybe as a followup) Can you add a bool to better describe what's going
on
> when
> > > width=0? This seems to have been first added back in
> > >
>
https://chromium.googlesource.com/chromium/blink/+/4a2a3ba905642998075cf4b161...
> > > and it'll be easy to make this mistake again.
> > > 
> > >
>
https://codereview.chromium.org/648293003/diff/50001/Source/platform/fonts/sh...
> > > Source/platform/fonts/shaping/SimpleShaper.cpp:211: width =
> adjustSpacing(width,
> > > charData, *fontData, glyphBuffer);
> > > Does it make sense to check for width in adjustSpacing after this patch? I
> don't
> > > fully understand the comment in adjustSpacing in the context of this
patch.
> > 
> > The workaround for missing glyphs really should remain in 
> > adjustSpacing advanceInternal and not leak out into the adjustSpacing. I
could
> use an extra bool instead of checking the width as suggested above if you
think
> that makes it clearer.
> 
> Just to make sure I follow, your plan is to handle the case when width==0 in
> advanceInternal instead of adjustSpacing, probably with an ASSERT(width) in
> adjustSpacing? If so, SGTM :)
> 
> It would be nice to better explain what's going on when width==0 using a bool
> but, after trying it myself, I'm not sure a bool will make it more
> understandable after all :/

Patch set 3 uses a bool, see if that makes it better. If not I can revert back
to checking width and adding a comment and assert. Up to you.

[pdr., 2014-10-14 00:28:36]

LGTM

https://codereview.chromium.org/648293003/diff/230001/Source/platform/fonts/s...
File Source/platform/fonts/shaping/SimpleShaper.cpp (right):

https://codereview.chromium.org/648293003/diff/230001/Source/platform/fonts/s...
Source/platform/fonts/shaping/SimpleShaper.cpp:200: width =
characterWidth(charData.character, glyphData);
Would it make sense to add
ASSERT(!fontData.isZeroWidthSpaceGlyph(fontData.glyphForCharacter(charData.character)))
here?

[eae, 2014-10-14 00:30:36]

Thanks pdr.

https://codereview.chromium.org/648293003/diff/230001/Source/platform/fonts/s...
File Source/platform/fonts/shaping/SimpleShaper.cpp (right):

https://codereview.chromium.org/648293003/diff/230001/Source/platform/fonts/s...
Source/platform/fonts/shaping/SimpleShaper.cpp:200: width =
characterWidth(charData.character, glyphData);
On 2014/10/14 00:28:36, pdr wrote:
> Would it make sense to add
>
ASSERT(!fontData.isZeroWidthSpaceGlyph(fontData.glyphForCharacter(charData.character)))
> here?

Not really, we only go down the fallback-path above if the character is both a
zero-width-space glyph _and_ the font is missing said glyph.

[eae, 2014-10-14 17:58:54]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-10-14 17:59:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/648293003/230001

[commit-bot: I haz the power, 2014-10-14 18:03:51]

Committed patchset #3 (id:230001) as 183685