Support includePartialGlyphs=false in Font::offsetForPositionForComplexText

Support includePartialGlyphs=false in Font::offsetForPositionForComplexText

This patch supports includePartialGlyphs=false in
Font::offsetForPositionForComplexText(). This flag was supported in
offsetForPositionForSimpleText(), but was assumed to be always true in
offsetForPositionForComplexText().

Over 70 existing tests fail by the DCHECK added in this CL without this
fix when the hack in rewindToMidWordBreak() was removed.

BUG=610791
Committed: https://crrev.com/ad66862296c227c1b74a15f96632a67d941b6643
Cr-Commit-Position: refs/heads/master@{#393300}

[kojii, 2016-05-10 19:34:44]

Description was changed from

==========
WIP: offsetForPosition

BUG=
==========

to

==========
Support includePartialGlyphs=false in Font::offsetForPositionForComplexText

This patch supports includePartialGlyphs=false in
Font::offsetForPositionForComplexText(). This flag was ignored before
this patch and is always assumed to be true.

BUG=610791
==========

[kojii, 2016-05-10 19:37:15]

Description was changed from

==========
Support includePartialGlyphs=false in Font::offsetForPositionForComplexText

This patch supports includePartialGlyphs=false in
Font::offsetForPositionForComplexText(). This flag was ignored before
this patch and is always assumed to be true.

BUG=610791
==========

to

==========
Support includePartialGlyphs=false in Font::offsetForPositionForComplexText

This patch supports includePartialGlyphs=false in
Font::offsetForPositionForComplexText(). This flag was ignored before
this patch and is always assumed to be true.

Over 70 existing tests fail by the DCHECK added in this CL without this
fix.

BUG=610791
==========

[kojii, 2016-05-11 04:32:55]

Description was changed from

==========
Support includePartialGlyphs=false in Font::offsetForPositionForComplexText

This patch supports includePartialGlyphs=false in
Font::offsetForPositionForComplexText(). This flag was ignored before
this patch and is always assumed to be true.

Over 70 existing tests fail by the DCHECK added in this CL without this
fix.

BUG=610791
==========

to

==========
Support includePartialGlyphs=false in Font::offsetForPositionForComplexText

This patch supports includePartialGlyphs=false in
Font::offsetForPositionForComplexText(). This flag was ignored before
this patch and was always assumed to be true.

Over 70 existing tests fail by the DCHECK added in this CL without this
fix.

BUG=610791
==========

[kojii, 2016-05-11 04:35:19]

Description was changed from

==========
Support includePartialGlyphs=false in Font::offsetForPositionForComplexText

This patch supports includePartialGlyphs=false in
Font::offsetForPositionForComplexText(). This flag was ignored before
this patch and was always assumed to be true.

Over 70 existing tests fail by the DCHECK added in this CL without this
fix.

BUG=610791
==========

to

==========
Support includePartialGlyphs=false in Font::offsetForPositionForComplexText

This patch supports includePartialGlyphs=false in
Font::offsetForPositionForComplexText(). This flag was supported in
offsetForPositionForSimpleText(), but was assumed to be always true in
offsetForPositionForComplexText().

Over 70 existing tests fail by the DCHECK added in this CL without this
fix.

BUG=610791
==========

[kojii, 2016-05-11 20:58:56]

Description was changed from

==========
Support includePartialGlyphs=false in Font::offsetForPositionForComplexText

This patch supports includePartialGlyphs=false in
Font::offsetForPositionForComplexText(). This flag was supported in
offsetForPositionForSimpleText(), but was assumed to be always true in
offsetForPositionForComplexText().

Over 70 existing tests fail by the DCHECK added in this CL without this
fix.

BUG=610791
==========

to

==========
Support includePartialGlyphs=false in Font::offsetForPositionForComplexText

This patch supports includePartialGlyphs=false in
Font::offsetForPositionForComplexText(). This flag was supported in
offsetForPositionForSimpleText(), but was assumed to be always true in
offsetForPositionForComplexText().

Over 70 existing tests fail by the DCHECK added in this CL without this
fix when the hack in rewindToMidWordBreak() was removed.

BUG=610791
==========

[kojii, 2016-05-12 16:07:03]

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

[kojii, 2016-05-12 16:07:04]

PTAL.

[eae, 2016-05-12 16:12:46]

This is great! LGTM

https://codereview.chromium.org/1966703002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.cpp (right):

https://codereview.chromium.org/1966703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.cpp:108: if
(includePartialGlyphs) {
Could you add a comment here explaining the logic please? Especially the / 2.0
part.

[kojii, 2016-05-12 16:35:57]

The CQ bit was checked by kojii@chromium.org

[kojii, 2016-05-12 16:35:58]

https://codereview.chromium.org/1966703002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.cpp (right):

https://codereview.chromium.org/1966703002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.cpp:108: if
(includePartialGlyphs) {
On 2016/05/12 at 16:12:45, eae wrote:
> Could you add a comment here explaining the logic please? Especially the / 2.0
part.

Done, thank you!

[kojii, 2016-05-12 16:35:58]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1966703002/#ps180001
(title: "Add comment as per eae review")

[commit-bot: I haz the power, 2016-05-12 16:36:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1966703002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1966703002/180001

[commit-bot: I haz the power, 2016-05-12 18:06:49]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-05-12 18:07:56]

Description was changed from

==========
Support includePartialGlyphs=false in Font::offsetForPositionForComplexText

This patch supports includePartialGlyphs=false in
Font::offsetForPositionForComplexText(). This flag was supported in
offsetForPositionForSimpleText(), but was assumed to be always true in
offsetForPositionForComplexText().

Over 70 existing tests fail by the DCHECK added in this CL without this
fix when the hack in rewindToMidWordBreak() was removed.

BUG=610791
==========

to

==========
Support includePartialGlyphs=false in Font::offsetForPositionForComplexText

This patch supports includePartialGlyphs=false in
Font::offsetForPositionForComplexText(). This flag was supported in
offsetForPositionForSimpleText(), but was assumed to be always true in
offsetForPositionForComplexText().

Over 70 existing tests fail by the DCHECK added in this CL without this
fix when the hack in rewindToMidWordBreak() was removed.

BUG=610791

Committed: https://crrev.com/ad66862296c227c1b74a15f96632a67d941b6643
Cr-Commit-Position: refs/heads/master@{#393300}
==========

[commit-bot: I haz the power, 2016-05-12 18:07:57]

Patchset 10 (id:??) landed as
https://crrev.com/ad66862296c227c1b74a15f96632a67d941b6643
Cr-Commit-Position: refs/heads/master@{#393300}

[tapted, 2016-05-13 00:26:13]

tapted@chromium.org changed reviewers:
+ tapted@chromium.org

[tapted, 2016-05-13 00:26:14]

Should this be reverted? - see below

https://codereview.chromium.org/1966703002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1966703002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1297: crbug.com/610791
virtual/spv2/fast/overflow/line-clamp.html [ NeedsRebaseline ]
Hi there! I think this has caused a persistent failure.

webkit_tests reports unexpected_failures:
virtual/spv2/fast/overflow/line-clamp.html

since

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/42280

which includes the "Auto-rebaseline for r393300"

[kojii, 2016-05-13 00:29:49]

On 2016/05/13 at 00:26:14, tapted wrote:
> Should this be reverted? - see below
> 
>
https://codereview.chromium.org/1966703002/diff/180001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/1966703002/diff/180001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/TestExpectations:1297: crbug.com/610791
virtual/spv2/fast/overflow/line-clamp.html [ NeedsRebaseline ]
> Hi there! I think this has caused a persistent failure.
> 
> webkit_tests reports unexpected_failures:
> virtual/spv2/fast/overflow/line-clamp.html
> 
> since
> 
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/42280
> 
> which includes the "Auto-rebaseline for r393300"

Can you mark it as failure? I'm out of office, but will take a look later today.

[drott, 2016-05-23 07:30:44]

Thanks for fixing this!