Adjust visual overflow rect for rounded/shifted ascent/descent

Adjust visual overflow rect for rounded/shifted ascent/descent

For now we round and/or shift ascent and descent to integers in some cases,
and we may layout using ascent/descent smaller than the original, causing that
painted text at the laid-out position may overflow the visual overflow
calculated based on layout geometries.

Now record the loss in FontMetrics and adjust visual overflow rect to cover
the overflows.

BUG=706298
Review-Url: https://codereview.chromium.org/2803483002
Cr-Commit-Position: refs/heads/master@{#464689}
Committed: https://chromium.googlesource.com/chromium/src/+/bee058616b53303711940b8984f7168d776df959

[Xianzhu, 2017-04-04 22:37:38]

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

[commit-bot: I haz the power, 2017-04-04 22:38:31]

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

[Xianzhu, 2017-04-04 22:41:00]

wangxianzhu@chromium.org changed reviewers:
+ drott@chromium.org, eae@chromium.org

[Xianzhu, 2017-04-04 22:41:00]

I wonder if this CL is a good solution, but I need to fix the "regression"
exposed by my caret paint invalidation change before m58 stable. I leave bug
707807 for better solutions.

[commit-bot: I haz the power, 2017-04-05 00:17:02]

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

[commit-bot: I haz the power, 2017-04-05 00:17:02]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[drott, 2017-04-05 08:57:46]

On 2017/04/04 at 22:41:00, wangxianzhu wrote:
> I wonder if this CL is a good solution, but I need to fix the "regression"
exposed by my caret paint invalidation change before m58 stable. I leave bug
707807 for better solutions.

I am very skeptical of adding pixels here and there. This usually causes more
trouble down the line and degrades rendering and repainting accuracy.

[Xianzhu, 2017-04-05 15:43:58]

On 2017/04/05 08:57:46, drott wrote:
> On 2017/04/04 at 22:41:00, wangxianzhu wrote:
> > I wonder if this CL is a good solution, but I need to fix the "regression"
> exposed by my caret paint invalidation change before m58 stable. I leave bug
> 707807 for better solutions.
> 
> I am very skeptical of adding pixels here and there. This usually causes more
> trouble down the line and degrades rendering and repainting accuracy.

Any idea about the problem? There are actual physical pixels (sub-css-pixels)
exceeding the calculated visual overflow based on glyph overflows reported by
the font.

[Xianzhu, 2017-04-05 18:03:45]

On 2017/04/05 15:43:58, Xianzhu wrote:
> On 2017/04/05 08:57:46, drott wrote:
> > On 2017/04/04 at 22:41:00, wangxianzhu wrote:
> > > I wonder if this CL is a good solution, but I need to fix the "regression"
> > exposed by my caret paint invalidation change before m58 stable. I leave bug
> > 707807 for better solutions.
> > 
> > I am very skeptical of adding pixels here and there. This usually causes
more
> > trouble down the line and degrades rendering and repainting accuracy.
> 
> Any idea about the problem? There are actual physical pixels (sub-css-pixels)
> exceeding the calculated visual overflow based on glyph overflows reported by
> the font.

drott@ and eae@ I do need a fix before M-58 stable because we don't want the
"regression" to enter stable. Though the issue is not new, it has been exposed
more frequently since my caret paint invalidation change perhaps because of less
over-invalidations during editing, but I think it's really not my CL's fault :(.

I don't know why the glyph overflow is not adequate. For the bug case, some
indic glyphs report 1 pixel overflow, but when the glyphs are rasterized, some
physical pixels outside of the overflow are also affected. Inflating inaccurate
glyph overflow seems a quick but dirty approach. Will appreciate any ideas from
you :)

[Xianzhu, 2017-04-06 04:07:29]

crbug.com/708784 seems the reason of the under-invalidation. However, I'm afraid
it's risky to switch subpixel ascent/descent everywhere, especially in M58.

How about the following plan:

1. Update comments in this CL and add a TODO:
  // TODO(crbug.com/708784): for now we round ascent/descent to integer values
in SimpleFontData, which might cause glyphs to render some subpixels overflowing
the visual overflow if the ascent or descent is rounded down. Inflate the visual
rect by 1 pixel to ensure the subpixel overflow is covered. Should remove the
inflation when we enable subpixel ascent/descent.

2. Land this CL in M59 and merge into M58 to fix the RleaseBlockStable
crbug.com/706298.

3. Revert this CL and experiment https://codereview.chromium.org/2797293003/ in
M59.

[Xianzhu, 2017-04-06 17:27:54]

Description was changed from

==========
Inflate glyph overflow by 1 pixel

Sometimes the glyph overflow doesn't cover all subpixel rendering
effects of glyphs. Inflate by 1 pixel to ensure coverage.

BUG=706298
==========

to

==========
Adjust visual overflow rect for ascent/descent fraction losses

For now we round ascent and descent to integers in some cases.
When the rounded value is smaller than the original value. Previously
visual overflow is also calculated based on layout geometries which are
based on the rounded values. When we painted text at the laid-out
position, the painted glyphs may exceed the visual overflow rect when
rasterized.

Now record the loss of fractions in FontMetrics and adjust visual
overflow rect to cover the overflows.

BUG=706298
==========

[Xianzhu, 2017-04-06 17:28:13]

Description was changed from

==========
Adjust visual overflow rect for ascent/descent fraction losses

For now we round ascent and descent to integers in some cases.
When the rounded value is smaller than the original value. Previously
visual overflow is also calculated based on layout geometries which are
based on the rounded values. When we painted text at the laid-out
position, the painted glyphs may exceed the visual overflow rect when
rasterized.

Now record the loss of fractions in FontMetrics and adjust visual
overflow rect to cover the overflows.

BUG=706298
==========

to

==========
Adjust visual overflow rect for ascent/descent fraction losses

For now we round ascent and descent to integers in some cases.
When the rounded value is smaller than the original value. Previously
visual overflow is also calculated based on layout geometries which are
based on the rounded values. When we painted text at the laid-out
position, the painted glyphs may exceed the visual overflow rect when
rasterized.

Now record the loss of fractions in FontMetrics and adjust visual
overflow rect to cover the overflows.

BUG=706298
==========

[Xianzhu, 2017-04-06 17:30:16]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org

[Xianzhu, 2017-04-06 17:30:16]

I think the lastest patch is a better solution which adjusts visual overflow
rect for ascent/descent fraction losses. Please see the updated CL description
for details.

PTAL.

+chrishtr from paint team as this is basically a painting issue.

[Xianzhu, 2017-04-06 18:24:32]

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

[commit-bot: I haz the power, 2017-04-06 18:25:40]

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, 2017-04-06 20:01:50]

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

[chrishtr, 2017-04-06 21:07:00]

The general approach seeks ok from a non-expert POV.

https://codereview.chromium.org/2803483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right):

https://codereview.chromium.org/2803483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:154: #if OS(LINUX)
|| OS(ANDROID)
This Android/Linux part used to be conditioned on the conditionals at lines
136-138 being *true). Now it's conditioned on that conditional being *false*.
Is that ok?

[Xianzhu, 2017-04-06 21:20:47]

https://codereview.chromium.org/2803483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right):

https://codereview.chromium.org/2803483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:154: #if OS(LINUX)
|| OS(ANDROID)
On 2017/04/06 21:07:00, chrishtr wrote:
> This Android/Linux part used to be conditioned on the conditionals at lines
> 136-138 being *true). Now it's conditioned on that conditional being *false*.
> Is that ok?

Yes. The condition 'descent < SkScalarToFloat(metrics.fDescent)' can be true
only if we have rounded the values.

[Xianzhu, 2017-04-07 00:56:19]

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

[commit-bot: I haz the power, 2017-04-07 00:57:03]

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, 2017-04-07 03:06:42]

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

[commit-bot: I haz the power, 2017-04-07 03:06:43]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2017-04-07 15:57:19]

eae@ and drott@ could you review the patch? Thanks!

[Xianzhu, 2017-04-07 16:04:37]

On 2017/04/07 15:57:19, Xianzhu wrote:
> eae@ and drott@ could you review the patch? Thanks!

Note that the latest patch no longer inflate visual overflow rect blindly. I
have figured out that the root cause of the under-invalidation is adjustment of
ascent and descent causing layout geometry not fully containing the painted
glyphs. Now I adjust visual overflow according to how much we adjusted
ascent/descent to ensure visual overflow to fully cover the painted glyphs.

[eae, 2017-04-07 16:34:56]

https://codereview.chromium.org/2803483002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2803483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1974: const auto&
firstStyle = styleRef(firstTextBox()->isFirstLineStyle());
If I understand the logic right you are inflating the visual overflow rect by
one pixel for both the ascent and decent if they were rounded down.

That certainly makes sense but the computations and the API for it seems rather
convoluted.

Instead of adding overflowAdjustmentForAscent etc and computing the adjustment
values in FontMetrics and SimpleFontData why not compare the ascent/decent and
floatAscent/floatDecent values here?


Something like:

  FontMetrics* metrics = ...;
  ascentAdjustment = metrics.floatAscent() > metrics.ascent() ? 1 : 0;
  decentAdjustment = metrics.floatDecent() > metrics.decent() ? 1 : 0;
  logicalTop -= ascentAdjustment;
  logicalHeight += ascentAdjustment + descentAdjustment;

https://codereview.chromium.org/2803483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1978: const auto& lastStyle
= styleRef(lastTextBox()->isFirstLineStyle());
The names here are bad. It's not really an ascentAjdutsment and a
descentAdjustment, it is a placement and height adjustment based on the rounding
of the ascent/decent values.

This could really use a couple of comments. I know this code pretty well but it
still took me a couple of minutes to understand what you're trying to do here.

[Xianzhu, 2017-04-07 17:26:21]

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

[commit-bot: I haz the power, 2017-04-07 17:26:57]

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

[Xianzhu, 2017-04-07 17:27:12]

https://codereview.chromium.org/2803483002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2803483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1974: const auto&
firstStyle = styleRef(firstTextBox()->isFirstLineStyle());
On 2017/04/07 16:34:56, eae wrote:
> If I understand the logic right you are inflating the visual overflow rect by
> one pixel for both the ascent and decent if they were rounded down.
> 
> That certainly makes sense but the computations and the API for it seems
rather
> convoluted.
> 
> Instead of adding overflowAdjustmentForAscent etc and computing the adjustment
> values in FontMetrics and SimpleFontData why not compare the ascent/decent and
> floatAscent/floatDecent values here?
> 
> 
> Something like:
> 
>   FontMetrics* metrics = ...;
>   ascentAdjustment = metrics.floatAscent() > metrics.ascent() ? 1 : 0;
>   decentAdjustment = metrics.floatDecent() > metrics.decent() ? 1 : 0;
>   logicalTop -= ascentAdjustment;
>   logicalHeight += ascentAdjustment + descentAdjustment;

This won't work because
1. in many cases floatAscent() and floatDescent() are also rounded before they
are set:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/fonts...
 
2. rounding is not the only cause, but also shifting:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/fonts...

Perhaps we should remove both of the above code to let floatAscent/floatDescent
always equal to the orignal ascent/descent, but I would like that to be a
follow-up because we need to merge this patch into M58 and it should be on the
safe side.

https://codereview.chromium.org/2803483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1978: const auto& lastStyle
= styleRef(lastTextBox()->isFirstLineStyle());
On 2017/04/07 16:34:56, eae wrote:
> The names here are bad. It's not really an ascentAjdutsment and a
> descentAdjustment, it is a placement and height adjustment based on the
rounding
> of the ascent/decent values.
> 
> This could really use a couple of comments. I know this code pretty well but
it
> still took me a couple of minutes to understand what you're trying to do here.


Renamed visualOverflowAdjustmentXxx to visualOverflowInflationXxx, and
xxxAdjustment to xXXAdjustment to inflationForXxx. Also simplified code here.

[eae, 2017-04-07 17:41:57]

https://codereview.chromium.org/2803483002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2803483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1960: // Inflate visual
overflow if we have adjusted ascent/descent causing the
This is better, thank you. I'm still not convinced it is correct however.

https://codereview.chromium.org/2803483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1965: .primaryFont()
primaryFont may be null.

https://codereview.chromium.org/2803483002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontMetrics.h (right):

https://codereview.chromium.org/2803483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontMetrics.h:66:
m_visualOverflowInflationForAscent++;
FontMetrics already stores the rounded and unrounded values, why do we also need
to store the adjustment? Also, why increment here? If setAscent is called twice
why would we want to increment the adjustment, that seems wrong.

https://codereview.chromium.org/2803483002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right):

https://codereview.chromium.org/2803483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:161:
overflowInflationForDescent = 0;
Why? The decent has already been inflated to account for the rounding error.

[Xianzhu, 2017-04-07 18:02:53]

https://codereview.chromium.org/2803483002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2803483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1960: // Inflate visual
overflow if we have adjusted ascent/descent causing the
On 2017/04/07 17:41:57, eae wrote:
> This is better, thank you. I'm still not convinced it is correct however.

What do you think about my previous comment:
https://codereview.chromium.org/2803483002/diff/40001/third_party/WebKit/Sour...
?

https://codereview.chromium.org/2803483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1965: .primaryFont()
On 2017/04/07 17:41:57, eae wrote:
> primaryFont may be null.

Done.

Should we have DCHECK(primaryFont) here? I saw many existing such DCHECKs.

https://codereview.chromium.org/2803483002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontMetrics.h (right):

https://codereview.chromium.org/2803483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontMetrics.h:66:
m_visualOverflowInflationForAscent++;
On 2017/04/07 17:41:57, eae wrote:
> FontMetrics already stores the rounded and unrounded values, why do we also
need
> to store the adjustment? Also, why increment here? If setAscent is called
twice
> why would we want to increment the adjustment, that seems wrong.

FontMetrics doesn't store the unrounded values in many cases. See my previous
comment:
https://codereview.chromium.org/2803483002/diff/40001/third_party/WebKit/Sour...

https://codereview.chromium.org/2803483002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right):

https://codereview.chromium.org/2803483002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:161:
overflowInflationForDescent = 0;
On 2017/04/07 17:41:57, eae wrote:
> Why? The decent has already been inflated to account for the rounding error.

The descent is incremented above (line 158) so it's now bigger than the original
descent. Ascent is decremented so we should inflate overflow for ascent instead.

[Xianzhu, 2017-04-07 18:11:09]

P.S. I think your idea is great, but I would like to make follow-up CL for it
because it requires removing the rounding/shifting logic in SimpleFontData.cpp.
I would not like to include it in this patch because this patch is for a
ReleaseBlockStable but for m58.

Wdyt?

[Xianzhu, 2017-04-07 18:36:42]

This is the following CL (always use original ascent/descent for
FontMetrics::floatAscent|floatDescent):
https://codereview.chromium.org/2797423005.

[Xianzhu, 2017-04-07 18:51:02]

Description was changed from

==========
Adjust visual overflow rect for ascent/descent fraction losses

For now we round ascent and descent to integers in some cases.
When the rounded value is smaller than the original value. Previously
visual overflow is also calculated based on layout geometries which are
based on the rounded values. When we painted text at the laid-out
position, the painted glyphs may exceed the visual overflow rect when
rasterized.

Now record the loss of fractions in FontMetrics and adjust visual
overflow rect to cover the overflows.

BUG=706298
==========

to

==========
Adjust visual overflow rect for rounded/shifted ascent/descent

For now we round and/or shift ascent and descent to integers in some cases,
and we may layout using ascent/descent smaller than the original, causing that
painted text at the laid-out position may overflow the visual overflow
calculated based on layout geometries.

Now record the loss in FontMetrics and adjust visual overflow rect to cover
the overflows.

BUG=706298
==========

[eae, 2017-04-08 06:45:00]

On 2017/04/07 18:11:09, Xianzhu OOO 4.8 - 4.16 wrote:
> P.S. I think your idea is great, but I would like to make follow-up CL for it
> because it requires removing the rounding/shifting logic in
SimpleFontData.cpp.
> I would not like to include it in this patch because this patch is for a
> ReleaseBlockStable but for m58.
> 
> Wdyt?

I'm sorry but I don't like this change at all, I would also be very
uncomfortable down integrating it into beta.


> Should we have DCHECK(primaryFont) here? I saw many existing such DCHECKs.

No need in this case, in debug builds it will assert way before getting here if
primaryFont is null.

> FontMetrics doesn't store the unrounded values in many cases. See my previous
> comment:
>
https://codereview.chromium.org/2803483002/diff/40001/third_party/WebKit/Sour...

That happens *before* the setAscent and setDecent methods are called so by the
time it is called both the rounded and the unrounded values have been updated
making the branch in the setAscent/setDecent methods a no-op.

I might be missing something here but I do not understand why you need to change
FontMetrics. m_ascent and m_descent are only modified in the setAscent and
setDescent methods repetitively and neither FontMetrics nor SimpleFontData
modifies either value once set. I really don't understand the need for
m_visualOverflowInflationForAscent and m_visualOverflowInflationForDescent. 

If you want to compensate for rounding in SimpleFontData then please have that
logic contained there rather than splitting it across multiple unrelated files.

[eae, 2017-04-08 07:44:54]

All the computation takes place in SimpleFontData::platformInit, it would be
great if we could contain this logic there too.

As for a minimal safe change to down integrate inflating the rect by a fixed
amount in SimpleFontData is probably the way to go.

Thanks for working on this!

[Xianzhu, 2017-04-11 01:05:38]

On 2017/04/08 06:45:00, eae wrote:
> On 2017/04/07 18:11:09, Xianzhu OOO 4.8 - 4.16 wrote:
> > P.S. I think your idea is great, but I would like to make follow-up CL for
it
> > because it requires removing the rounding/shifting logic in
> SimpleFontData.cpp.
> > I would not like to include it in this patch because this patch is for a
> > ReleaseBlockStable but for m58.
> > 
> > Wdyt?
> 
> I'm sorry but I don't like this change at all, I would also be very
> uncomfortable down integrating it into beta.
> 
> 
> > Should we have DCHECK(primaryFont) here? I saw many existing such DCHECKs.
> 
> No need in this case, in debug builds it will assert way before getting here
if
> primaryFont is null.
> 
> > FontMetrics doesn't store the unrounded values in many cases. See my
previous
> > comment:
> >
>
https://codereview.chromium.org/2803483002/diff/40001/third_party/WebKit/Sour...
> 
> That happens *before* the setAscent and setDecent methods are called so by the
> time it is called both the rounded and the unrounded values have been updated
> making the branch in the setAscent/setDecent methods a no-op.
> 

Perhaps the branch in setAscent/setDescent is unnecessary. I added them because
I don't know how the integer ascent/descent are used and I supposed they can
also cause overflow rect inaccuracies. They are no-op if setAscent/setDescent is
called with rounded value, but are not no-op if setAscent/setDescent is called
with the original value (e.g. when vdmx is used). 

> I might be missing something here but I do not understand why you need to
change
> FontMetrics. m_ascent and m_descent are only modified in the setAscent and
> setDescent methods repetitively and neither FontMetrics nor SimpleFontData
> modifies either value once set. I really don't understand the need for
> m_visualOverflowInflationForAscent and m_visualOverflowInflationForDescent. 
> 
> If you want to compensate for rounding in SimpleFontData then please have that
> logic contained there rather than splitting it across multiple unrelated
files.

I'm not sure how to do it. Ascent/descent adjustment and overflow rect
calculation happen at different times. During overflow rect calculation, we have
already lost the fraction parts of ascent and descent (when floatAscent and
floatDescent store rounded values). m_visualOverflowInflationForXXX is to
recover them. Another method is to add originalAscent/originalDescent to store
the original values. I don't want to change what flaotAscent and floatDescent
store because that will change our rendering behavior and is too risky for m58.

[Xianzhu, 2017-04-11 03:02:00]

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

[commit-bot: I haz the power, 2017-04-11 03:03: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, 2017-04-11 04:48:00]

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

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

Dry run: This issue passed the CQ dry run.

[eae, 2017-04-11 05:12:37]

At this point, given the desire to DI this into beta, I';d argue we should
inflate by a fixed number (like 1px). Trying to do something clever with the
ascent requires a lot of plumbing and the logic needed is rather complex given
the current implementation sadly.

[Xianzhu, 2017-04-12 02:13:35]

On 2017/04/11 05:12:37, eae wrote:
> At this point, given the desire to DI this into beta, I';d argue we should
> inflate by a fixed number (like 1px). Trying to do something clever with the
> ascent requires a lot of plumbing and the logic needed is rather complex given
> the current implementation sadly.

1px might be not enough for ascent in some cases because we may lose 1px for
rounding and another 1px borrowed by descent. In the case
SimpleFontData::visual_rect_inflation_for_ascent is 2 and
visual_rect_inflation_for_descent is 0.

Another concern is that it's not a common case needing inflation, but
unconditionally inflating will affect the common cases. We would also need to
rebaseline hundreds of layout tests.

Wdyt about Patch Set 7 which only inflate the rect if SimpleFontData adjusts
ascent/descent before setAscent/setDescent? It seems that the difference between
floatAscent/floatDescent and intAscent/intDescent doesn't matter because after I
removed the logic in FontMetrics::setAscent/setDescent, all layout tests still
pass.

[eae, 2017-04-12 07:31:10]

I still don't think it is correct but at least it is well contained, we can give
it a try.

Please try to pack the values better though, using 64 bits to represent four
bits of information isn't great and we have a lot of SimpleFontData objects.

There is enough space to back both values with the bools which would all us to
to add the adjustment fields without increasing the size of each object.

[eae, 2017-04-12 09:08:20]

Giving LGTM to unblock but please try to make it a bit more compact as
suggested.

Thanks for your patience!

[Xianzhu, 2017-04-14 02:25:50]

On 2017/04/12 07:31:10, eae wrote:
> I still don't think it is correct but at least it is well contained, we can
give
> it a try.
> 
> Please try to pack the values better though, using 64 bits to represent four
> bits of information isn't great and we have a lot of SimpleFontData objects.
> 
> There is enough space to back both values with the bools which would all us to
> to add the adjustment fields without increasing the size of each object.

I also don't think the *patched code* is correct. However, the incorrectness
doesn't come from this patch, but is at the rounding and shifting of
ascent/descent of the current code. As we can't fix the latter for M58, we have
to workaround it to make it not to cause paint invalidation problems.

[Xianzhu, 2017-04-14 02:52:41]

On 2017/04/12 07:31:10, eae wrote:
> Please try to pack the values better though, using 64 bits to represent four
> bits of information isn't great and we have a lot of SimpleFontData objects.
> 
> There is enough space to back both values with the bools which would all us to
> to add the adjustment fields without increasing the size of each object.

Done.

[Xianzhu, 2017-04-14 02:52:49]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-04-14 02:52:50]

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/2803483002/#ps140001
(title: "pack")

[commit-bot: I haz the power, 2017-04-14 02:53:02]

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-04-14 06:16:23]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492138369369160,
"parent_rev": "852fdb990ef3e2999b14608e3f7c0038762f3199", "commit_rev":
"bee058616b53303711940b8984f7168d776df959"}

[commit-bot: I haz the power, 2017-04-14 06:17:51]

Description was changed from

==========
Adjust visual overflow rect for rounded/shifted ascent/descent

For now we round and/or shift ascent and descent to integers in some cases,
and we may layout using ascent/descent smaller than the original, causing that
painted text at the laid-out position may overflow the visual overflow
calculated based on layout geometries.

Now record the loss in FontMetrics and adjust visual overflow rect to cover
the overflows.

BUG=706298
==========

to

==========
Adjust visual overflow rect for rounded/shifted ascent/descent

For now we round and/or shift ascent and descent to integers in some cases,
and we may layout using ascent/descent smaller than the original, causing that
painted text at the laid-out position may overflow the visual overflow
calculated based on layout geometries.

Now record the loss in FontMetrics and adjust visual overflow rect to cover
the overflows.

BUG=706298

Review-Url: https://codereview.chromium.org/2803483002
Cr-Commit-Position: refs/heads/master@{#464689}
Committed:
https://chromium.googlesource.com/chromium/src/+/bee058616b53303711940b8984f7...
==========

[commit-bot: I haz the power, 2017-04-14 06:17:53]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/bee058616b53303711940b8984f7...