Do not skip ink for ideographic scripts

Do not skip ink for ideographic scripts

This patch changes not to skip ink for ideographic scripts, where
skipping ink is inappropriate.

In order to know when not to apply ink skipping, this patch adds an
optional vector to GlyphBuffer containing the glyph indexes that are
exempt from ink skipping.

BUG=677093
Committed: https://crrev.com/8584def66acd7836e089a59a598f9f44e50ace7b
Cr-Commit-Position: refs/heads/master@{#441593}

[kojii, 2016-12-27 14:27:14]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-27 14:27:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-27 15:58:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-27 15:58:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kojii, 2016-12-28 03:49:11]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-28 03:49:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-12-28 04:01:03]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[kojii, 2016-12-28 04:01:13]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-12-28 04:01:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-12-28 04:06:43]

Description was changed from

==========
WIP

BUG=677093
==========

to

==========
Do not skip ink for ideographic scripts

This patch changes not to skip ink for ideographic scripts, where
skipping ink is inappropriate.

To know the base code point of glyphs, this patch adds an optional
vector to GlyphBuffer.
==========

[kojii, 2016-12-28 04:09:41]

kojii@chromium.org changed reviewers:
+ drott@chromium.org

[kojii, 2016-12-28 04:09:42]

PTAL.

Tried another approach by splitting runs http://crrev.com/2601753002 but this
approach looks more reasonable to me.

The optional vector for GlyphBuffer is the same idea as http://wkb.ug/128145 , I
just preferred different name.

WDYT?

[kojii, 2016-12-28 04:51:51]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-28 04:52:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-12-28 04:57:53]

Description was changed from

==========
Do not skip ink for ideographic scripts

This patch changes not to skip ink for ideographic scripts, where
skipping ink is inappropriate.

To know the base code point of glyphs, this patch adds an optional
vector to GlyphBuffer.
==========

to

==========
Do not skip ink for ideographic scripts

This patch changes not to skip ink for ideographic scripts, where
skipping ink is inappropriate.

To know the base code point of glyphs, this patch adds an optional
vector to GlyphBuffer.

BUG=677093
==========

[commit-bot: I haz the power, 2016-12-28 06:44:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-28 06:44:00]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kojii, 2016-12-28 07:16:20]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-28 07:16:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-28 08:46:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-28 08:46:14]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-12-28 14:04:39]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-28 14:04:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-28 15:26:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-28 15:26:25]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-12-30 06:51:10]

text-underline-position: under has a perf concern, and that hyatt commented that
he made it for vertical flow but don't want to do so for CJK horizontal. Our
code is slightly different but has the same n^2 problem. We probably need to
move text decorations to the line-level and add a position cache.

As Myles requested to add text-underline-offset, computing default position only
mitigates but does not solve CJK skip-ink problem. I hope you understand that
and feel ok to take this?

[drott, 2016-12-30 09:02:53]

Again, Koji, thanks for pushing this and trying to resolve the CJK ink skipping.


I've been discussing with eae@ and fmalita@ that we ultimately would like to
remove GlyphBuffer and connect ShapeResultBuffer to the Skia text blobs
directly. This is more of a long term goal but reason for me to be hesitant to
load the GlyphBuffer with much additional data or logic. The second reason I'm
suggesting a slightly different approach is that the fillGlyphBuffer functions
are now always passing a characterIndex but then it's optionally discarded in
GlyphBuffer, depending on whether it has the vector allocated - I find that
surprising to read. Also, we can avoid data redundancy between RunInfo and
GlyphBuffer (character indices) by only passing information on which glyphs are
to be excluded from ink skipping.

How about we add something like 
enum {
ProcessCJKInkSkippingExceptions;
NoCJKInkSkipProcessing;
}
as an argument to buildGlyphBufer in Font, then pass this down to
ShapeResultBuffer. In GlyphBuffer, we'd put a Vector<bool,0> m_skipExceptions
member and only assign to this from ShapeResultBuffer if the mode is
ProcessCJKInkSkippingExceptions. If m_skipExceptions.size() > 0, then we can use
it in the Bloberizer to break up runs there and look up the bools alongside the
glyphs.

On 2016/12/30 at 06:51:10, kojii wrote:
> text-underline-position: under has a perf concern, and that hyatt commented
that he made it for vertical flow but don't want to do so for CJK horizontal.
Our code is slightly different but has the same n^2 problem. We probably need to
move text decorations to the line-level and add a position cache.

I missed this comment, would you have a link for me? Can you elaborate on the
the n^2 problem? 

> As Myles requested to add text-underline-offset, computing default position
only mitigates but does not solve CJK skip-ink problem. I hope you understand
that and feel ok to take this?

Unfortunately I don't fully understand this point, could you explain a little
bit more? What do you mean by computing default position - you mean computing a
line-scope position for the underline based on the Script mix, e.g. computing a
better line position if there is mixed Latin and CJK? Why does it only mitigate,
but not solve?

Is it true that we're likely to fix line/run-wide underline placement only with
LayoutNG? In that case, I think we can take a per-character based approach, with
the suggestions as mentioned above. Let me know what you think - if you find the
suggestions too much hassle, I can address and implement those, but might to get
to it before the 57 branch point.

[kojii, 2017-01-02 02:31:27]

Hey, happy new year. I had bad cold and could not respond early, but thank you
having a look anyway.

On 2016/12/30 at 09:02:53, drott wrote:
> Again, Koji, thanks for pushing this and trying to resolve the CJK ink
skipping. 
> 
> I've been discussing with eae@ and fmalita@ that we ultimately would like to
remove GlyphBuffer and connect ShapeResultBuffer to the Skia text blobs
directly. This is more of a long term goal but reason for me to be hesitant to
load the GlyphBuffer with much additional data or logic. The second reason I'm
suggesting a slightly different approach is that the fillGlyphBuffer functions
are now always passing a characterIndex but then it's optionally discarded in
GlyphBuffer, depending on whether it has the vector allocated - I find that
surprising to read. Also, we can avoid data redundancy between RunInfo and
GlyphBuffer (character indices) by only passing information on which glyphs are
to be excluded from ink skipping.

The plan sounds great to me, thank you for sharing that with me.

> How about we add something like 
> enum {
> ProcessCJKInkSkippingExceptions;
> NoCJKInkSkipProcessing;
> }
> as an argument to buildGlyphBufer in Font, then pass this down to
ShapeResultBuffer. In GlyphBuffer, we'd put a Vector<bool,0> m_skipExceptions
member and only assign to this from ShapeResultBuffer if the mode is
ProcessCJKInkSkippingExceptions. If m_skipExceptions.size() > 0, then we can use
it in the Bloberizer to break up runs there and look up the bools alongside the
glyphs.

Thank you, so you recommend:
* Have m_skipExceptions instead of m_characterIndexes in GlyphBuffer
* Use an argument instead of a state in GlyphBuffer
is that correct? The latter is a little less efficient than using a state in
GlyphBuffer, are you sure you prefer this?

> On 2016/12/30 at 06:51:10, kojii wrote:
> > text-underline-position: under has a perf concern, and that hyatt commented
that he made it for vertical flow but don't want to do so for CJK horizontal.
Our code is slightly different but has the same n^2 problem. We probably need to
move text decorations to the line-level and add a position cache.
> 
> I missed this comment, would you have a link for me?

The comment I referred to was in WebKit trunk:
https://trac.webkit.org/browser/trunk/Source/WebCore/style/InlineTextBoxStyle...

> Can you elaborate on the the n^2 problem?

We need to find the lowest of the text-bottom (the bottom of the em-box) across
the decorated elements. Each InlineTextBoxPainter determines the position, we
scan all siblings (|inlineTextBox->root().maxLogicalTop()| scans all siblings)
for all elements, resulting O(n^2).

> > As Myles requested to add text-underline-offset, computing default position
only mitigates but does not solve CJK skip-ink problem. I hope you understand
that and feel ok to take this?
> 
> Unfortunately I don't fully understand this point, could you explain a little
bit more? What do you mean by computing default position - you mean computing a
line-scope position for the underline based on the Script mix, e.g. computing a
better line position if there is mixed Latin and CJK? Why does it only mitigate,
but not solve?

Sorry I was too terse. Before, we were thinking, if we can always force "under"
for any CJK characters, we don't have to "not to skip CJK".

But with "text-underline-offset", author can set underlines to any positions, so
even if our "underline-position: auto" uses "under" when there's at least one
CJK character, we still need to "not to skip CJK" too because author can set
"underline-position-offset: -0.1em" to intercept with glyphs.

So I wanted to say, now the positioning and "not-to-skip CJK" are two
independent issues. Is this clear? Sorry but please keep me asking if this isn't
clear enough.

> Is it true that we're likely to fix line/run-wide underline placement only
with LayoutNG?

dunno...it's probably whether you, me, or Stefan are willing to take or not.
This is, AFAIU, unfortunately less important than before, since the most
important use case was <u>H<sup>2</sup>O</u>, but people use synthesized sup/sub
much less today.

With "text-underline-position" and "text-underline-position-offset", the
consistent underline position is becoming more important again.

We probably need to move decoration positioning from InlineBox/InlineTextBox to
InlineFlowBox/RootInlineBox, and possibly keep some computed values in cache.
But I'm less familiar with decoration code than you are, I hope you have better
idea how it should be.

> In that case, I think we can take a per-character based approach, with the
suggestions as mentioned above. Let me know what you think - if you find the
suggestions too much hassle, I can address and implement those, but might to get
to it before the 57 branch point.

As above, I think these two issues are both needed, but in the short term, we
can live with one of them.

My goal is to turn on ink skipping by default, and this is the only blocker
AFAIU, so I hope us to take the fix before the branch and to allow us to enable
by default!

[kojii, 2017-01-02 05:11:57]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-02 05:12:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2017-01-02 05:22:47]

PTAL. PS6 changed to follow your advises, except that it uses a state in
GlyphBuffer than an additional mode argument. I think it's more efficient to
pass rarely used mode arguments down to several call stacks, but if you prefer
arguments over state, I'm ok to use that, please let me know.

[commit-bot: I haz the power, 2017-01-02 06:24:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-02 06:24:25]

Dry run: This issue passed the CQ dry run.

[drott, 2017-01-02 15:07:46]

Hi Koji,

first of all sorry that some of my comments could have been in the initial
review - I try to avoid that as much as possible but some things I only noticed
in the second round. I am aware this increses round trip time - my apologies.

On 2017/01/02 at 02:31:27, kojii wrote:
> Hey, happy new year. I had bad cold and could not respond early, but thank you
having a look anyway.

明けましておめでとうございます！
 
> > How about we add something like 
> > enum {
> > ProcessCJKInkSkippingExceptions;
> > NoCJKInkSkipProcessing;
> > }
> > as an argument to buildGlyphBufer in Font, then pass this down to
ShapeResultBuffer. In GlyphBuffer, we'd put a Vector<bool,0> m_skipExceptions
member and only assign to this from ShapeResultBuffer if the mode is
ProcessCJKInkSkippingExceptions. If m_skipExceptions.size() > 0, then we can use
it in the Bloberizer to break up runs there and look up the bools alongside the
glyphs.
> 
> Thank you, so you recommend:
> * Have m_skipExceptions instead of m_characterIndexes in GlyphBuffer

Yes.

> * Use an argument instead of a state in GlyphBuffer
> is that correct? The latter is a little less efficient than using a state in
GlyphBuffer, are you sure you prefer this?

Both works, state is fine, thanks. (One minor suggestion to avoid the parallel
boolean).

> > I missed this comment, would you have a link for me?
> 
> The comment I referred to was in WebKit trunk:
>
https://trac.webkit.org/browser/trunk/Source/WebCore/style/InlineTextBoxStyle...

Thx.

> > > As Myles requested to add text-underline-offset, computing default
position only mitigates but does not solve CJK skip-ink problem. I hope you
understand that and feel ok to take this?
> > 
> > Unfortunately I don't fully understand this point, could you explain a
little bit more? What do you mean by computing default position - you mean
computing a line-scope position for the underline based on the Script mix, e.g.
computing a better line position if there is mixed Latin and CJK? Why does it
only mitigate, but not solve?
> 
> Sorry I was too terse. Before, we were thinking, if we can always force
"under" for any CJK characters, we don't have to "not to skip CJK".
> 
> But with "text-underline-offset", author can set underlines to any positions,
so even if our "underline-position: auto" uses "under" when there's at least one
CJK character, we still need to "not to skip CJK" too because author can set
"underline-position-offset: -0.1em" to intercept with glyphs.

Got it, thanks. So as is, without underline-offset, we would be okay if we had
good "under" placement. But once we'll have underline-offset and the value is
small, we'll run into the same problem again. Right?

> > Is it true that we're likely to fix line/run-wide underline placement only
with LayoutNG?
> 
> dunno...it's probably whether you, me, or Stefan are willing to take or not.
This is, AFAIU, unfortunately less important than before, since the most
important use case was <u>H<sup>2</sup>O</u>, but people use synthesized sup/sub
much less today.
> 
> With "text-underline-position" and "text-underline-position-offset", the
consistent underline position is becoming more important again.
> 
> We probably need to move decoration positioning from InlineBox/InlineTextBox
to InlineFlowBox/RootInlineBox, and possibly keep some computed values in cache.
But I'm less familiar with decoration code than you are, I hope you have better
idea how it should be.

I am only familiar with the painting part and positioning once the basic offsets
on the line/run level have been determined by layout. I don't know the parts
above this very well.

> > In that case, I think we can take a per-character based approach, with the
suggestions as mentioned above. Let me know what you think - if you find the
suggestions too much hassle, I can address and implement those, but might to get
to it before the 57 branch point.
> 
> As above, I think these two issues are both needed, but in the short term, we
can live with one of them.

Okay, let's start with this one. 

> My goal is to turn on ink skipping by default, and this is the only blocker
AFAIU, so I hope us to take the fix before the branch and to allow us to enable
by default!

Since text-decoration-skip has not been exposed to stable, I would suggest to
let 57 go to stable and see if there are issues coming up with this feature
before we switch to auto, wdyt?

https://codereview.chromium.org/2598393002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/2598393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/Font.cpp:418: UChar32 baseCharacter =
run.codepointAt(characterIndex);
Can we reuse Character::isCJKIdeographOrSymbol for this instead?

https://codereview.chromium.org/2598393002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip-ink-ideograph.html
(right):

https://codereview.chromium.org/2598393002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip-ink-ideograph.html:9:
<p>&#x696D; aaa
Can you add test lines without spaces, so that we can cover the cases when it's
one word in the word cache? And an additional second interleaved CJK sub-run to
ensure that subsequent offsets work as expected? Thanks.

https://codereview.chromium.org/2598393002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/2598393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/Font.cpp:303: void setSkipGlyphs(const
Vector<bool>* skipGlyphs) {
As mentioned below, I'd prefer if we use the glyphBuffer's
shouldSaveSkipInkExceptions() check, remove this function and do the backwards
running skip checks in the constructor.

https://codereview.chromium.org/2598393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/Font.cpp:365: const Vector<bool>*
m_skipGlyphs;
I believe we can use this from m_buffer directly?

https://codereview.chromium.org/2598393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/Font.cpp:412: const Vector<bool>*
skipGlyphs,
See below, I think we can remove this argument...

https://codereview.chromium.org/2598393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/Font.cpp:416:
bloberizer.setSkipGlyphs(skipGlyphs);
and this explicit transfer of the skipExceptions vector.

https://codereview.chromium.org/2598393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/Font.cpp:469: skipGlyphs =
&glyphBuffer.skipInkExceptions();
Using state in the GlyphBuffer for shouldSaveSkipInkExceptions() is okay for me,
but then can we use it here as well? AFAICT, we don't use this exact same glyph
buffer for anything else except computing the intercepts, so in the bloberizer,
we can ask the GlyphBuffer for shouldSaveSkipInkExceptions() and handle it
accordingly there, instead of passing the skipGlyphs vector pointer around here.

https://codereview.chromium.org/2598393002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/GlyphBuffer.h (right):

https://codereview.chromium.org/2598393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/GlyphBuffer.h:151: void
addIsSkipInkException(bool value) { m_skipInkExceptions.append(value); }
Can we add a DCHECK_EQ(m_offsets.size(), m_skipInkExceptions.size()); here?

https://codereview.chromium.org/2598393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/GlyphBuffer.h:153: Vector<bool>&
skipInkExceptions() { return m_skipInkExceptions; }
Can we add the same debug assertion here, before returning?
DCHECK_EQ(m_offsets.size(), m_skipInkExceptions.size())

https://codereview.chromium.org/2598393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/GlyphBuffer.h:164: Vector<bool>
m_skipInkExceptions;
Could you add a comment like:
// Flag vector of identical size to m_offset, true when glyph is to be exempted
from ink skipping, false otherwise.

Also, minor, but how about making this a std::unique_ptr initially nullptr and
allocating it when saveSkipInkExceptions is called, then removing the boolean?
And checking for existence of the vector in shouldSaveSkipInkExceptions()?

https://codereview.chromium.org/2598393002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/shaping/ShapeResultBuffer.cpp
(right):

https://codereview.chromium.org/2598393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/shaping/ShapeResultBuffer.cpp:25:
UBlockCode blockCode = ublock_getCode(baseCharacter);
I'd very much prefer if we could reuse the isCJKIdeographOrSymbol function
instead of defining another class. Is that possible? If not, at least the
test/code block list should move to Character.h

[drott, 2017-01-02 15:16:57]

Please ignore the first comment, it does not apply to the latest patchset,
Rietveld stored this from earlier and previously I clicked reply instead of
sending my comments. Sorry for the confusion.

>
https://codereview.chromium.org/2598393002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/fonts/Font.cpp (right):
> 
>
https://codereview.chromium.org/2598393002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/fonts/Font.cpp:418: UChar32 baseCharacter =
run.codepointAt(characterIndex);
> Can we reuse Character::isCJKIdeographOrSymbol for this instead?

[kojii, 2017-01-04 11:42:51]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 11:43:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2017-01-04 13:30:57]

PTAL, all done, thank you for your detailed review as always.

[commit-bot: I haz the power, 2017-01-04 14:32:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 14:32:24]

Dry run: This issue passed the CQ dry run.

[drott, 2017-01-04 14:42:15]

LGTM, looks great now, thank you Koji for reworking this repeatedly. I am glad
we have this feature and can move forward with the push for automatic ink
skipping.

Nit re commit msg:
> To know the base code point of glyphs, this patch adds an optional
vector to GlyphBuffer.

=> In order to know when not to apply ink skipping, this patch adds an optional
vector to GlyphBuffer containing the glyph indexes that are exempt from ink
skipping.

[drott, 2017-01-04 14:42:27]

https://codereview.chromium.org/2598393002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip-ink-ideograph.html
(right):

https://codereview.chromium.org/2598393002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip-ink-ideograph.html:9:
<p>&#x696D;aaa
Nit: Can we keep the lines with the spaces as well?
Perhaps adding one where there is an additional CJK character in the middle? (I
am always worried about run offsets, but the offsets look fine thanks to all
being stored as offset to the beginning of the glyph buffer.)
&#x696D;aaa&#x696D;aaa&#x696D; as well as
&#x696D; aaa &#x696D; aaa &#x696D;

[kojii, 2017-01-05 03:41:35]

Description was changed from

==========
Do not skip ink for ideographic scripts

This patch changes not to skip ink for ideographic scripts, where
skipping ink is inappropriate.

To know the base code point of glyphs, this patch adds an optional
vector to GlyphBuffer.

BUG=677093
==========

to

==========
Do not skip ink for ideographic scripts

This patch changes not to skip ink for ideographic scripts, where
skipping ink is inappropriate.

In order to know when not to apply ink skipping, this patch adds an
optional vector to GlyphBuffer containing the glyph indexes that are
exempt from ink skipping.

BUG=677093
==========

[kojii, 2017-01-05 04:22:48]

thank you, all nits done.

[kojii, 2017-01-05 04:22:51]

The CQ bit was checked by kojii@chromium.org

[kojii, 2017-01-05 04:22:51]

The patchset sent to the CQ was uploaded after l-g-t-m from drott@chromium.org
Link to the patchset: https://codereview.chromium.org/2598393002/#ps160001
(title: "drott nits")

[commit-bot: I haz the power, 2017-01-05 04:23:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 06:00:20]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1483590171187410,
"parent_rev": "f7975c4fb413652999a10ca313ca66b1729c6e82", "commit_rev":
"cc8744429ebd3fc507c0ed9895bcf857b52d6f05"}

[commit-bot: I haz the power, 2017-01-05 06:00:51]

Description was changed from

==========
Do not skip ink for ideographic scripts

This patch changes not to skip ink for ideographic scripts, where
skipping ink is inappropriate.

In order to know when not to apply ink skipping, this patch adds an
optional vector to GlyphBuffer containing the glyph indexes that are
exempt from ink skipping.

BUG=677093
==========

to

==========
Do not skip ink for ideographic scripts

This patch changes not to skip ink for ideographic scripts, where
skipping ink is inappropriate.

In order to know when not to apply ink skipping, this patch adds an
optional vector to GlyphBuffer containing the glyph indexes that are
exempt from ink skipping.

BUG=677093

Review-Url: https://codereview.chromium.org/2598393002
==========

[commit-bot: I haz the power, 2017-01-05 06:00:52]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2017-01-05 06:04:38]

Description was changed from

==========
Do not skip ink for ideographic scripts

This patch changes not to skip ink for ideographic scripts, where
skipping ink is inappropriate.

In order to know when not to apply ink skipping, this patch adds an
optional vector to GlyphBuffer containing the glyph indexes that are
exempt from ink skipping.

BUG=677093

Review-Url: https://codereview.chromium.org/2598393002
==========

to

==========
Do not skip ink for ideographic scripts

This patch changes not to skip ink for ideographic scripts, where
skipping ink is inappropriate.

In order to know when not to apply ink skipping, this patch adds an
optional vector to GlyphBuffer containing the glyph indexes that are
exempt from ink skipping.

BUG=677093

Committed: https://crrev.com/8584def66acd7836e089a59a598f9f44e50ace7b
Cr-Commit-Position: refs/heads/master@{#441593}
==========

[commit-bot: I haz the power, 2017-01-05 06:04:40]

Patchset 8 (id:??) landed as
https://crrev.com/8584def66acd7836e089a59a598f9f44e50ace7b
Cr-Commit-Position: refs/heads/master@{#441593}