Correcting text baseline for tiny fonts

Correcting text baseline for tiny fonts

When a tiny font size is used to draw text on canvas and the canvas
is scaled up afterward, the rounding operations in
CanvasRenderingContext2D::getFontBaseline and
SimpleFontData::platformInit() causes different baselines to converge.
This CL resolves this issue by using floating point ascent and descent
properties. Visual results and comparison to previous state can be
found in bug webpage: crbug.com/338908.

BUG=338908
Committed: https://crrev.com/c5395a1764dd6ef1439507d88e801eaf06820476
Cr-Commit-Position: refs/heads/master@{#425618}

[zakerinasab, 2016-10-04 20:05:09]

Description was changed from

==========
Using float calculations for text baseline.

BUG=338908
==========

to

==========
Correcting text baseline for tiny fonts

When the font size is very small (for example 1px) the rounding
operations in CanvasRenderingContext2D::getFontBaseline and
SimpleFontData::platformInit() causes different baselines to converge.
This CL resolves this issue by using floating point ascent and descent
properties. Visual results and comparison to previous state can be
found in bug webpage: crbug.com/338908.

BUG=338908
==========

[zakerinasab, 2016-10-04 20:05:09]

zakerinasab@chromium.org changed reviewers:
+ eae@chromium.org, junov@chromium.org

[zakerinasab, 2016-10-04 20:14:53]

Description was changed from

==========
Correcting text baseline for tiny fonts

When the font size is very small (for example 1px) the rounding
operations in CanvasRenderingContext2D::getFontBaseline and
SimpleFontData::platformInit() causes different baselines to converge.
This CL resolves this issue by using floating point ascent and descent
properties. Visual results and comparison to previous state can be
found in bug webpage: crbug.com/338908.

BUG=338908
==========

to

==========
Correcting text baseline for tiny fonts

When the tiny font size is used to draw text on canvas and the canvas
is scaled up afterward, the rounding operations in 
CanvasRenderingContext2D::getFontBaseline and 
SimpleFontData::platformInit() causes different baselines to converge.
This CL resolves this issue by using floating point ascent and descent
properties. Visual results and comparison to previous state can be
found in bug webpage: crbug.com/338908.

BUG=338908
==========

[zakerinasab, 2016-10-04 20:15:15]

Description was changed from

==========
Correcting text baseline for tiny fonts

When the tiny font size is used to draw text on canvas and the canvas
is scaled up afterward, the rounding operations in 
CanvasRenderingContext2D::getFontBaseline and 
SimpleFontData::platformInit() causes different baselines to converge.
This CL resolves this issue by using floating point ascent and descent
properties. Visual results and comparison to previous state can be
found in bug webpage: crbug.com/338908.

BUG=338908
==========

to

==========
Correcting text baseline for tiny fonts

When a tiny font size is used to draw text on canvas and the canvas
is scaled up afterward, the rounding operations in 
CanvasRenderingContext2D::getFontBaseline and 
SimpleFontData::platformInit() causes different baselines to converge.
This CL resolves this issue by using floating point ascent and descent
properties. Visual results and comparison to previous state can be
found in bug webpage: crbug.com/338908.

BUG=338908
==========

[zakerinasab, 2016-10-04 20:15:41]

CL uploaded.

[eae, 2016-10-04 20:43:12]

We tried this before and it broke text line alignment for a bunch of tests and a
number of websites. It is possible that it'll work better now but look very
carefully at layout tests diffs and be prepared to revert it.

https://codereview.chromium.org/2392033002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2392033002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:908:
return fontMetrics.floatAscent();
In CanvasRenderingContext2D::measureText you changed it from floatAscent to
ascent and here the opposite despite that fact that both (presumably) return the
float value now. We should be consistent and use the same everywhere and remove
the float version if the regular one has float precision.

[zakerinasab, 2016-10-06 17:54:11]

Patchset #11 (id:200001) has been deleted

[zakerinasab, 2016-10-06 17:56:46]

Patchset #11 (id:220001) has been deleted

[zakerinasab1, 2016-10-06 20:02:29]

On 2016/10/04 20:43:12, eae wrote:
> We tried this before and it broke text line alignment for a bunch of tests and
a
> number of websites. It is possible that it'll work better now but look very
> carefully at layout tests diffs and be prepared to revert it.
> 
>
https://codereview.chromium.org/2392033002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
> (right):
> 
>
https://codereview.chromium.org/2392033002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:908:
> return fontMetrics.floatAscent();
> In CanvasRenderingContext2D::measureText you changed it from floatAscent to
> ascent and here the opposite despite that fact that both (presumably) return
the
> float value now. We should be consistent and use the same everywhere and
remove
> the float version if the regular one has float precision.

I "think" I addressed all the failing layout tests, so hopefully we should be
good to go. Please take a look at the latest patch set.

[zakerinasab1, 2016-10-06 20:02:51]

zakerinasab@google.com changed reviewers:
+ zakerinasab@google.com

[zakerinasab1, 2016-10-06 20:02:52]

https://codereview.chromium.org/2392033002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2392033002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:908:
return fontMetrics.floatAscent();
On 2016/10/04 20:43:11, eae wrote:
> In CanvasRenderingContext2D::measureText you changed it from floatAscent to
> ascent and here the opposite despite that fact that both (presumably) return
the
> float value now. We should be consistent and use the same everywhere and
remove
> the float version if the regular one has float precision.

Fixed.

[eae, 2016-10-06 20:18:50]

Much better, thank you!

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

https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1155: crbug.com/338908
fast/canvas/canvas-text-baseline-tiny-fonts.html [ NeedsRebaseline ]
Don't add new NeedsRebaseline lines, instead use "webkit-patch rebaseline-cl" to
apply the new baselines directly to your CL from the bots.

https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/win/paint/invalidation/background-resize-height-expected.txt
(right):

https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/win/paint/invalidation/background-resize-height-expected.txt:15:
"contentsOpaque": true,
Why should it be Opaque now?

https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right):

https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:144: if
(-metrics.fAscent < 3 || -metrics.fAscent + metrics.fDescent < 2) {
This applies to all text, SVG, DOM and canvas. Please either update the CL
description and tests or limit it to only apply to canvas.

[zakerinasab, 2016-10-07 14:31:10]

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

https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1155: crbug.com/338908
fast/canvas/canvas-text-baseline-tiny-fonts.html [ NeedsRebaseline ]
On 2016/10/06 20:18:49, eae wrote:
> Don't add new NeedsRebaseline lines, instead use "webkit-patch rebaseline-cl"
to
> apply the new baselines directly to your CL from the bots.

Acknowledged.

https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/win/paint/invalidation/background-resize-height-expected.txt
(right):

https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/win/paint/invalidation/background-resize-height-expected.txt:15:
"contentsOpaque": true,
On 2016/10/06 20:18:49, eae wrote:
> Why should it be Opaque now?

No idea. I'm not sure why it should have anything to do with this CL. But I will
look into it.

https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right):

https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:144: if
(-metrics.fAscent < 3 || -metrics.fAscent + metrics.fDescent < 2) {
On 2016/10/06 20:18:49, eae wrote:
> This applies to all text, SVG, DOM and canvas. Please either update the CL
> description and tests or limit it to only apply to canvas.

I'll limit this one to canvas and file new bugs for text and SVG corrections.

[zakerinasab, 2016-10-11 21:10:57]

Patchset #14 (id:300001) has been deleted

[zakerinasab, 2016-10-12 18:45:48]

Patchset #16 (id:360001) has been deleted

[zakerinasab, 2016-10-12 18:45:58]

Patchset #10 (id:180001) has been deleted

[zakerinasab, 2016-10-12 18:46:05]

Patchset #6 (id:100001) has been deleted

[zakerinasab, 2016-10-12 19:03:32]

Patchset #13 (id:340001) has been deleted

[zakerinasab1, 2016-10-12 19:22:49]

Patchset #13 (id:380001) has been deleted

[zakerinasab, 2016-10-12 20:14:38]

Patchset #13 (id:400001) has been deleted

[qyearsley, 2016-10-12 20:57:51]

On 2016/10/07 at 14:31:10, zakerinasab wrote:
>
https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/TestExpectations:1155: crbug.com/338908
fast/canvas/canvas-text-baseline-tiny-fonts.html [ NeedsRebaseline ]
> On 2016/10/06 20:18:49, eae wrote:
> > Don't add new NeedsRebaseline lines, instead use "webkit-patch
rebaseline-cl" to
> > apply the new baselines directly to your CL from the bots.
> 
> Acknowledged.

Also, if this isn't working very well right now, I'd be interesting in knowing
about and fixing any bugs :-)

One known issue right now is that by default `webkit-patch rebaseline-cl`
currently still downloads baselines for flaky tests that fail both with and
without the patch (http://crbug.com/642980). This would be why it downloaded
fast/text/international/thai-line-breaks-expected.png. The workaround for this
currently is to run `webkit-patch rebaseline-cl --only-changed-tests`.

Anyway, besides that, let me know if you see anything about `webkit-patch
rebaseline-cl` that's broken :-)

[Justin Novosad, 2016-10-12 21:20:14]

lgtm with nits for modules/canvas2d

https://codereview.chromium.org/2392033002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2392033002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:920: //
If you changed the heuristic here, for consistency please also change it
Nit: changed -> change

https://codereview.chromium.org/2392033002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:922:
bool useFloatMetrics = (fontMetrics.ascent() < 3 || fontMetrics.height() < 2);
I think it would be more accurate to call this variable useFloatAscentDescent

[eae, 2016-10-12 21:24:06]

https://codereview.chromium.org/2392033002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontDescription.cpp (right):

https://codereview.chromium.org/2392033002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontDescription.cpp:46:
FontDescription::SubpixelAscentDescent subpixelAscentDescent;
There is enough size left in the bitfields, you should be able to add it there
without increasing the size of the class.

https://codereview.chromium.org/2392033002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontDescription.h (right):

https://codereview.chromium.org/2392033002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontDescription.h:96:
m_subpixelAscentDescent(None) {
This should be a bitfield instead.

[zakerinasab, 2016-10-13 14:46:38]

Comments addressed. New patch submitted.

https://codereview.chromium.org/2392033002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2392033002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:920: //
If you changed the heuristic here, for consistency please also change it
On 2016/10/12 21:20:14, Justin Novosad wrote:
> Nit: changed -> change

Done.

https://codereview.chromium.org/2392033002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:922:
bool useFloatMetrics = (fontMetrics.ascent() < 3 || fontMetrics.height() < 2);
On 2016/10/12 21:20:13, Justin Novosad wrote:
> I think it would be more accurate to call this variable useFloatAscentDescent

Done.

https://codereview.chromium.org/2392033002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontDescription.cpp (right):

https://codereview.chromium.org/2392033002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontDescription.cpp:46:
FontDescription::SubpixelAscentDescent subpixelAscentDescent;
On 2016/10/12 21:24:05, eae wrote:
> There is enough size left in the bitfields, you should be able to add it there
> without increasing the size of the class.

Done.

https://codereview.chromium.org/2392033002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontDescription.h (right):

https://codereview.chromium.org/2392033002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontDescription.h:96:
m_subpixelAscentDescent(None) {
On 2016/10/12 21:24:05, eae wrote:
> This should be a bitfield instead.

Done.

[zakerinasab, 2016-10-13 17:23:41]

New patch set (16) uploaded with results from webkit-patch rebaseline-cl. For
some reason results are different on different platforms, for example on linux
and linux-precise. Also even though the code works completely fine on my local
Win 10 machine, results are not as expected on Windows try bots. I'm going to
run the -rel trybots again to see if the result changes this time.

[eae, 2016-10-13 17:29:45]

LGTM

[zakerinasab, 2016-10-13 21:12:08]

Patchset #9 (id:240001) has been deleted

[zakerinasab, 2016-10-13 21:12:18]

Patchset #13 (id:440001) has been deleted

[zakerinasab, 2016-10-13 21:12:41]

Patchset #15 (id:500001) has been deleted

[zakerinasab, 2016-10-14 14:27:03]

Patchset #15 (id:520001) has been deleted

[zakerinasab, 2016-10-14 16:01:08]

On 2016/10/12 20:57:51, qyearsley wrote:
> On 2016/10/07 at 14:31:10, zakerinasab wrote:
> >
>
https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Lay...
> > File third_party/WebKit/LayoutTests/TestExpectations (right):
> > 
> >
>
https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Lay...
> > third_party/WebKit/LayoutTests/TestExpectations:1155: crbug.com/338908
> fast/canvas/canvas-text-baseline-tiny-fonts.html [ NeedsRebaseline ]
> > On 2016/10/06 20:18:49, eae wrote:
> > > Don't add new NeedsRebaseline lines, instead use "webkit-patch
> rebaseline-cl" to
> > > apply the new baselines directly to your CL from the bots.
> > 
> > Acknowledged.
> 
> Also, if this isn't working very well right now, I'd be interesting in knowing
> about and fixing any bugs :-)
> 
> One known issue right now is that by default `webkit-patch rebaseline-cl`
> currently still downloads baselines for flaky tests that fail both with and
> without the patch (http://crbug.com/642980). This would be why it downloaded
> fast/text/international/thai-line-breaks-expected.png. The workaround for this
> currently is to run `webkit-patch rebaseline-cl --only-changed-tests`.
> 
> Anyway, besides that, let me know if you see anything about `webkit-patch
> rebaseline-cl` that's broken :-)

Still I cannot get the proper expected.* files from rebaseline-cl. On several
tries, it gave me some files and when I uploaded them the tests were still
failing, I though might be the flaky test, so I modified the layout test. Now it
returns nothing and says "Failed to request layout test results from
"https://storage.google...". I started the tests less than 2 hrs ago.
Any ideas on how to resolve this? Trying to re-run rebaseline-cl is not
successful. The patch is fine if NeedsRebaseline is added to TestExpectations.

[qyearsley, 2016-10-14 17:46:44]

https://codereview.chromium.org/2392033002/diff/560001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-text-baseline-tiny-fonts.html
(right):

https://codereview.chromium.org/2392033002/diff/560001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-text-baseline-tiny-fonts.html:14:
for(i = 2; i <= 4; i++) {
Nit: Normally there's a space after `for` and before the opening parenthesis.

https://codereview.chromium.org/2392033002/diff/560001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-text-baseline-tiny-fonts.html:31:
ctx.textBaseline = "bottom"
Nit: could add semicolons at the end of each statement above.

https://codereview.chromium.org/2392033002/diff/560001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-text-baseline-tiny-fonts.html:38:

Nit: extra newline

[qyearsley, 2016-10-14 17:53:08]

On 2016/10/14 at 16:01:08, zakerinasab wrote:
> On 2016/10/12 20:57:51, qyearsley wrote:
> > On 2016/10/07 at 14:31:10, zakerinasab wrote:
> > >
> >
https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Lay...
> > > File third_party/WebKit/LayoutTests/TestExpectations (right):
> > > 
> > >
> >
https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Lay...
> > > third_party/WebKit/LayoutTests/TestExpectations:1155: crbug.com/338908
> > fast/canvas/canvas-text-baseline-tiny-fonts.html [ NeedsRebaseline ]
> > > On 2016/10/06 20:18:49, eae wrote:
> > > > Don't add new NeedsRebaseline lines, instead use "webkit-patch
> > rebaseline-cl" to
> > > > apply the new baselines directly to your CL from the bots.
> > > 
> > > Acknowledged.
> > 
> > Also, if this isn't working very well right now, I'd be interesting in
knowing
> > about and fixing any bugs :-)
> > 
> > One known issue right now is that by default `webkit-patch rebaseline-cl`
> > currently still downloads baselines for flaky tests that fail both with and
> > without the patch (http://crbug.com/642980). This would be why it downloaded
> > fast/text/international/thai-line-breaks-expected.png. The workaround for
this
> > currently is to run `webkit-patch rebaseline-cl --only-changed-tests`.
> > 
> > Anyway, besides that, let me know if you see anything about `webkit-patch
> > rebaseline-cl` that's broken :-)
> 
> Still I cannot get the proper expected.* files from rebaseline-cl. On several
tries, it gave me some files and when I uploaded them the tests were still
failing, I though might be the flaky test, so I modified the layout test. Now it
returns nothing and says "Failed to request layout test results from
"https://storage.google...". I started the tests less than 2 hrs ago.
> Any ideas on how to resolve this? Trying to re-run rebaseline-cl is not
successful. The patch is fine if NeedsRebaseline is added to TestExpectations.

Sorry for the trouble - if you can't get rebaseline-cl to work on the latest
patchset,
then to unblock this CL, I'd be fine with trying to add NeedsRebaseline lines in
TestExpectations.

Although, there might be a problem with rebaseline-o-matic with virtual tests in
virtual/display_list_2d_canvas; see http://crbug.com/627568. If you could upload
the baselines and edit/move them until they pass on the try bots, then you would
have more assurance that they're the correct baselines.

The other advantage of rebaselining from try jobs is that you can inspect them
before committing. Looking at the uploaded baselines and the layout test results
from that patch, (e.g.
https://storage.googleapis.com/chromium-layout-test-archives/linux_precise_bl...)
do you think you can see what's wrong the uploaded baselines?


So, I see a few couple issues with the rebaseline-cl tool uncovered here:

1. Failed to request layout test results from
  
"https://storage.googleapis.com/chromium-layout-test-archives/mac10_9_blink_rel/1074/layout-test-results"
and
  
"https://storage.googleapis.com/chromium-layout-test-archives/linux_precise_blink_rel/3864/layout-test-results".
   Filed bug http://crbug.com/656063.

2. Looking at the layout test results.html from patchset 14 (e.g.
  
https://storage.googleapis.com/chromium-layout-test-archives/win7_blink_rel/1...)
   it looks like the uploaded baseline was an empty file, and the actual result
was a file with 3 space characters,
   and the pixel result didn't match either. Note that only the virtual tests
are failing.
   Filed bug http://crbug.com/656065.

[zakerinasab, 2016-10-14 17:55:26]

On 2016/10/14 17:53:08, qyearsley wrote:
> On 2016/10/14 at 16:01:08, zakerinasab wrote:
> > On 2016/10/12 20:57:51, qyearsley wrote:
> > > On 2016/10/07 at 14:31:10, zakerinasab wrote:
> > > >
> > >
>
https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Lay...
> > > > File third_party/WebKit/LayoutTests/TestExpectations (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2392033002/diff/240001/third_party/WebKit/Lay...
> > > > third_party/WebKit/LayoutTests/TestExpectations:1155: crbug.com/338908
> > > fast/canvas/canvas-text-baseline-tiny-fonts.html [ NeedsRebaseline ]
> > > > On 2016/10/06 20:18:49, eae wrote:
> > > > > Don't add new NeedsRebaseline lines, instead use "webkit-patch
> > > rebaseline-cl" to
> > > > > apply the new baselines directly to your CL from the bots.
> > > > 
> > > > Acknowledged.
> > > 
> > > Also, if this isn't working very well right now, I'd be interesting in
> knowing
> > > about and fixing any bugs :-)
> > > 
> > > One known issue right now is that by default `webkit-patch rebaseline-cl`
> > > currently still downloads baselines for flaky tests that fail both with
and
> > > without the patch (http://crbug.com/642980). This would be why it
downloaded
> > > fast/text/international/thai-line-breaks-expected.png. The workaround for
> this
> > > currently is to run `webkit-patch rebaseline-cl --only-changed-tests`.
> > > 
> > > Anyway, besides that, let me know if you see anything about `webkit-patch
> > > rebaseline-cl` that's broken :-)
> > 
> > Still I cannot get the proper expected.* files from rebaseline-cl. On
several
> tries, it gave me some files and when I uploaded them the tests were still
> failing, I though might be the flaky test, so I modified the layout test. Now
it
> returns nothing and says "Failed to request layout test results from
> "https://storage.google...". I started the tests less than 2 hrs ago.
> > Any ideas on how to resolve this? Trying to re-run rebaseline-cl is not
> successful. The patch is fine if NeedsRebaseline is added to TestExpectations.
> 
> Sorry for the trouble - if you can't get rebaseline-cl to work on the latest
> patchset,
> then to unblock this CL, I'd be fine with trying to add NeedsRebaseline lines
in
> TestExpectations.
> 
> Although, there might be a problem with rebaseline-o-matic with virtual tests
in
> virtual/display_list_2d_canvas; see http://crbug.com/627568. If you could
upload
> the baselines and edit/move them until they pass on the try bots, then you
would
> have more assurance that they're the correct baselines.
> 
> The other advantage of rebaselining from try jobs is that you can inspect them
> before committing. Looking at the uploaded baselines and the layout test
results
> from that patch, (e.g.
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_precise_bl...)
> do you think you can see what's wrong the uploaded baselines?
> 
> 
> So, I see a few couple issues with the rebaseline-cl tool uncovered here:
> 
> 1. Failed to request layout test results from
>   
>
"https://storage.googleapis.com/chromium-layout-test-archives/mac10_9_blink_rel/1074/layout-test-results"
> and
>   
>
"https://storage.googleapis.com/chromium-layout-test-archives/linux_precise_blink_rel/3864/layout-test-results".
>    Filed bug http://crbug.com/656063.
> 
> 2. Looking at the layout test results.html from patchset 14 (e.g.
>   
>
https://storage.googleapis.com/chromium-layout-test-archives/win7_blink_rel/1...)
>    it looks like the uploaded baseline was an empty file, and the actual
result
> was a file with 3 space characters,
>    and the pixel result didn't match either. Note that only the virtual tests
> are failing.
>    Filed bug http://crbug.com/656065.

I'll give it another try. If failed again, I'll go with adding NeedsBaseline to
TestExpectations.

[zakerinasab, 2016-10-14 20:25:52]

Patchset #18 (id:600001) has been deleted

[zakerinasab, 2016-10-17 03:38:10]

The CQ bit was checked by zakerinasab@chromium.org

[zakerinasab, 2016-10-17 03:38:11]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2392033002/#ps620001
(title: "Rebaseline.")

[commit-bot: I haz the power, 2016-10-17 03:38:25]

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-10-17 05:03:23]

Description was changed from

==========
Correcting text baseline for tiny fonts

When a tiny font size is used to draw text on canvas and the canvas
is scaled up afterward, the rounding operations in 
CanvasRenderingContext2D::getFontBaseline and 
SimpleFontData::platformInit() causes different baselines to converge.
This CL resolves this issue by using floating point ascent and descent
properties. Visual results and comparison to previous state can be
found in bug webpage: crbug.com/338908.

BUG=338908
==========

to

==========
Correcting text baseline for tiny fonts

When a tiny font size is used to draw text on canvas and the canvas
is scaled up afterward, the rounding operations in 
CanvasRenderingContext2D::getFontBaseline and 
SimpleFontData::platformInit() causes different baselines to converge.
This CL resolves this issue by using floating point ascent and descent
properties. Visual results and comparison to previous state can be
found in bug webpage: crbug.com/338908.

BUG=338908
==========

[commit-bot: I haz the power, 2016-10-17 05:03:24]

Committed patchset #18 (id:620001)

[commit-bot: I haz the power, 2016-10-17 05:05:28]

Description was changed from

==========
Correcting text baseline for tiny fonts

When a tiny font size is used to draw text on canvas and the canvas
is scaled up afterward, the rounding operations in 
CanvasRenderingContext2D::getFontBaseline and 
SimpleFontData::platformInit() causes different baselines to converge.
This CL resolves this issue by using floating point ascent and descent
properties. Visual results and comparison to previous state can be
found in bug webpage: crbug.com/338908.

BUG=338908
==========

to

==========
Correcting text baseline for tiny fonts

When a tiny font size is used to draw text on canvas and the canvas
is scaled up afterward, the rounding operations in
CanvasRenderingContext2D::getFontBaseline and
SimpleFontData::platformInit() causes different baselines to converge.
This CL resolves this issue by using floating point ascent and descent
properties. Visual results and comparison to previous state can be
found in bug webpage: crbug.com/338908.

BUG=338908

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

[commit-bot: I haz the power, 2016-10-17 05:05:30]

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

[qyearsley, 2016-10-17 16:36:16]

On 2016/10/17 at 05:05:30, commit-bot wrote:
> Patchset 18 (id:??) landed as
https://crrev.com/c5395a1764dd6ef1439507d88e801eaf06820476
> Cr-Commit-Position: refs/heads/master@{#425618}

Rebaseline-o-matic appears to have worked correctly:

Rebaseline CL: http://crrev.com/2422123002
Blink waterfall is still green :-)

Now I have to figure out why `webkit-patch rebaseline-cl` didn't work, and fix
it...

[drott, 2016-10-18 10:42:16]

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

[drott, 2016-10-18 10:42:18]

Excuse my post-land, drive-by review - some comments here.

https://codereview.chromium.org/2392033002/diff/620001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right):

https://codereview.chromium.org/2392033002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:145: if
(subpixelAscentDescent &&
zakerinasab, did you say the same problem applies to regular text and SVG?

eae@, what do you think, would it perhaps make sense to make this the default
here using a decision function in FontCache.h for example, something like
"shouldUseSubpixelMetricsForAscentDescent(float, float) - and synchronize it
that way with the code in Canvas, instead of comments and piping through the
bool flag?

I'm also slightly worried that the bool flag is not part of
FontDescription::cacheKey. In an unlucky situation we might mix up a
SimpleFontData with subpixel baseline and without.m

[zakerinasab, 2016-10-18 13:48:07]

On 2016/10/18 10:42:18, drott wrote:
> Excuse my post-land, drive-by review - some comments here.
> 
>
https://codereview.chromium.org/2392033002/diff/620001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right):
> 
>
https://codereview.chromium.org/2392033002/diff/620001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:145: if
> (subpixelAscentDescent &&
> zakerinasab, did you say the same problem applies to regular text and SVG?
> 
> eae@, what do you think, would it perhaps make sense to make this the default
> here using a decision function in FontCache.h for example, something like
> "shouldUseSubpixelMetricsForAscentDescent(float, float) - and synchronize it
> that way with the code in Canvas, instead of comments and piping through the
> bool flag?
> 
> I'm also slightly worried that the bool flag is not part of
> FontDescription::cacheKey. In an unlucky situation we might mix up a
> SimpleFontData with subpixel baseline and without.m

This problem applies to all the text drawing in chromium, including regular text
and SVG. The idea was to land three different patches for canvas, SVG and
regular text so that if one of them breaks the user websites we revert only that
one. I'm now trying to do the piping for SVG, but I vote for a universal
decision function too.

[drott, 2016-11-23 13:58:24]

> This problem applies to all the text drawing in chromium, including regular
text and SVG. The idea was to land three different patches for canvas, SVG and
regular text so that if one of them breaks the user websites we revert only that
one. I'm now trying to do the piping for SVG, but I vote for a universal
decision function too.

How is this going?

[zakerinasab, 2016-11-23 16:48:53]

On 2016/11/23 13:58:24, drott wrote:
> > This problem applies to all the text drawing in chromium, including regular
> text and SVG. The idea was to land three different patches for canvas, SVG and
> regular text so that if one of them breaks the user websites we revert only
that
> one. I'm now trying to do the piping for SVG, but I vote for a universal
> decision function too.
> 
> How is this going?

As I understand the decision was to go with separate patches. I worked on the
baseline
problem in SVG but there is a problem with release try bots. I'm trying to
resolve this
and land the patch:
https://codereview.chromium.org/2427773002/

[drott, 2016-11-23 17:20:10]

On 2016/11/23 at 16:48:53, zakerinasab wrote:
> On 2016/11/23 13:58:24, drott wrote:
> > > This problem applies to all the text drawing in chromium, including
regular
> > text and SVG. The idea was to land three different patches for canvas, SVG
and
> > regular text so that if one of them breaks the user websites we revert only
that
> > one. I'm now trying to do the piping for SVG, but I vote for a universal
> > decision function too.
> > 
> > How is this going?
> 
> As I understand the decision was to go with separate patches. I worked on the
baseline
> problem in SVG but there is a problem with release try bots. I'm trying to
resolve this
> and land the patch:
> https://codereview.chromium.org/2427773002/

Great, yes - agree that the work is split into separate CL, just by "this" I
just meant this effort in general. Thanks for following up on this.