Force glyph overflow calculation

Force glyph overflow calculation

Currently we ignore horizontal glyph overflow in most cases, causing
overhang part of glyphs not included into overflow rect of line boxes.
This causes incorrect visual overflow of layout objects, and incorrect
cull rect.

Now always calculate glyph overflow.

BUG=496317
TEST=fast/text/glyph-overflow.html

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

[Xianzhu, 2015-06-06 00:50:16]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, fmalita@chromium.org, jbroman@chromium.org,
wkorman@chromium.org

[Xianzhu, 2015-06-06 00:50:17]

This CL works on Linux for the 'f' issue
(https://code.google.com/p/chromium/issues/detail?id=496317#c5), but degrades
performance of line box layout because it skips the optimized text measurement
issue.

Performance measurement showed that the non-optimized path is 10~30x slower than
the optimized path.

Trying to cache GlyphOverflow along with WordMeasurements.

[Xianzhu, 2015-06-06 05:23:50]

Patchset #3 (id:40001) has been deleted

[Xianzhu, 2015-06-06 05:23:54]

Patchset #2 (id:20001) has been deleted

[eae, 2015-06-06 05:42:47]

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

[eae, 2015-06-06 05:42:47]

I think this is fine. I'm working on reducing the cost of the overflow
calculation and to do much better caching between subsequent calls which should
mitigate the potential performance impact of this.

[Xianzhu, 2015-06-08 18:00:35]

On 2015/06/06 05:42:47, eae wrote:
> I think this is fine. I'm working on reducing the cost of the overflow
> calculation and to do much better caching between subsequent calls which
should
> mitigate the potential performance impact of this.

Great!

Patch Set 7 is now ready for review. Ptal.

Please see the try bot results of Patch Set 6 for layout test expectation
changes.

[Xianzhu, 2015-06-08 18:01:14]

wangxianzhu@chromium.org changed reviewers:
- fmalita@chromium.org, jbroman@chromium.org, wkorman@chromium.org

[Xianzhu, 2015-06-08 18:07:21]

Some notes

https://codereview.chromium.org/1153173011/diff/160001/Source/platform/fonts/...
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/1153173011/diff/160001/Source/platform/fonts/...
Source/platform/fonts/Font.cpp:222: glyphOverflow->right =
ceilf(glyphBounds.right());
This change (along with the changes in floatWidthForCompexText and
floatWidthForSimpleText) is to avoid wrong 1px overflow because of the error
from calculation from ceiled bounds and rounded ascent/descent.

https://codereview.chromium.org/1153173011/diff/160001/Source/web/tests/data/...
File Source/web/tests/data/rgm_column_test.html (right):

https://codereview.chromium.org/1153173011/diff/160001/Source/web/tests/data/...
Source/web/tests/data/rgm_column_test.html:10: <div> A </div>
Changed to all 'A' because 'C' and 'G' have glyph overflow at the top on Mac.

[eae, 2015-06-08 18:58:55]

LGTM w/nit. Feel free to commit once addressed.

https://codereview.chromium.org/1153173011/diff/160001/Source/platform/fonts/...
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/1153173011/diff/160001/Source/platform/fonts/...
Source/platform/fonts/Font.cpp:222: glyphOverflow->right =
ceilf(glyphBounds.right());
On 2015/06/08 18:07:21, Xianzhu wrote:
> This change (along with the changes in floatWidthForCompexText and
> floatWidthForSimpleText) is to avoid wrong 1px overflow because of the error
> from calculation from ceiled bounds and rounded ascent/descent.

Could you add a comment to that effect, explaining how the bounds are rounded?

[Xianzhu, 2015-06-08 23:55:42]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-06-08 23:55:43]

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/1153173011/#ps200001
(title: "Update test expectations")

[Xianzhu, 2015-06-08 23:55:52]

https://codereview.chromium.org/1153173011/diff/160001/Source/platform/fonts/...
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/1153173011/diff/160001/Source/platform/fonts/...
Source/platform/fonts/Font.cpp:222: glyphOverflow->right =
ceilf(glyphBounds.right());
On 2015/06/08 18:58:55, eae wrote:
> On 2015/06/08 18:07:21, Xianzhu wrote:
> > This change (along with the changes in floatWidthForCompexText and
> > floatWidthForSimpleText) is to avoid wrong 1px overflow because of the error
> > from calculation from ceiled bounds and rounded ascent/descent.
> 
> Could you add a comment to that effect, explaining how the bounds are rounded?

Done. (added in Font.h in GlyphOverflow).

[commit-bot: I haz the power, 2015-06-08 23:56:05]

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

[Xianzhu, 2015-06-09 01:31:51]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-06-09 01:31:51]

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/1153173011/#ps220001
(title: "More rebaselines")

[commit-bot: I haz the power, 2015-06-09 01:32:13]

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

[commit-bot: I haz the power, 2015-06-09 03:03:35]

Committed patchset #10 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196730