Calculate Underline thickness from Font

To draw underline currently Chrome is depending on Font Size taking 10%
Font Size as underline thickness which is not correct. Font properties
should be taken into account to calculate underline thickness. Following change does the same, some Skia files are also updated for this change.

Blink and Skia git repo are different so added two more patches related to Skia changes only.
https://codereview.chromium.org/148453005/
https://codereview.chromium.org/137543008/
 and made this patch Skia independent.

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

[h.joshi, 2014-01-28 09:11:51]

To draw underline currently Chrome is depending on Font Size taking 10% Font
Size as underline thickness which is not correct. Font properties should be
taken into account to calculate underline thickness.

Blink and Skia git repo are different so added two more patches related to Skia
changes only. 
https://codereview.chromium.org/148453005/ 
https://codereview.chromium.org/137543008/ 
and made this Blink patch Skia independent.

[eae, 2014-01-29 16:36:04]

On 2014/01/28 09:11:51, h.joshi wrote:
> To draw underline currently Chrome is depending on Font Size taking 10% Font
> Size as underline thickness which is not correct. Font properties should be
> taken into account to calculate underline thickness.
> 
> Blink and Skia git repo are different so added two more patches related to
Skia
> changes only. 
> https://codereview.chromium.org/148453005/ 
> https://codereview.chromium.org/137543008/ 
> and made this Blink patch Skia independent.

Why is this not correct, is it speced somewhere? Does the new implementation
match other browsers/applications?

Also, this really needs a test.

[h.joshi, 2014-01-31 09:19:18]

On 2014/01/29 16:36:04, eae wrote:
> On 2014/01/28 09:11:51, h.joshi wrote:
> > To draw underline currently Chrome is depending on Font Size taking 10% Font
> > Size as underline thickness which is not correct. Font properties should be
> > taken into account to calculate underline thickness.
> > 
> > Blink and Skia git repo are different so added two more patches related to
> Skia
> > changes only. 
> > https://codereview.chromium.org/148453005/ 
> > https://codereview.chromium.org/137543008/ 
> > and made this Blink patch Skia independent.
> 
> Why is this not correct, is it speced somewhere? Does the new implementation
> match other browsers/applications?
> 
> Also, this really needs a test.

Current Chrome calculation is correct if Font has bad metrics data, that check I
have added in code. Similar to Ascent/Descent Underline thickness is also
present in font which we are using with this patch.

Compared FireFox and Chrome output and after patch we are matching FireFox,
before patch on font size greater than 20-25 Chrome was showing very thick line
compared to FireFox.
 Firefox is also using Font data in this case, following is the calculation used
by FireFox:

aMetrics->underlineSize = std::max(1.0, aMetrics->underlineSize);

const PostTable *post = reinterpret_cast<const
PostTable*>(hb_blob_get_data(postTable, &len));
SET_UNSIGNED(underlineSize, post->underlineThickness);

For Windows:
//otmsUnderscoreSize is Underline Thickness obtained from Font
mMetrics->underlineSize = (double)oMetrics.otmsUnderscoreSize;

For Mac:
//CTFontGetUnderlineThickness returns Underline Thickness from Font
mMetrics.underlineSize = ::CTFontGetUnderlineThickness(ctFont);

[eae, 2014-01-31 18:15:53]

Ok, that makes sense, thanks for the explanation.
I'd still like to see a test for this though.

[hj, 2014-01-31 18:18:31]

On 2014/01/31 18:15:53, eae wrote:
> Ok, that makes sense, thanks for the explanation.
> I'd still like to see a test for this though.

Sure, will work to make test pages for this in Blink Layout Test cases.

[h.joshi, 2014-02-02 15:07:47]

On 2014/01/31 18:18:31, hj wrote:
> On 2014/01/31 18:15:53, eae wrote:
> > Ok, that makes sense, thanks for the explanation.
> > I'd still like to see a test for this though.
> 
> Sure, will work to make test pages for this in Blink Layout Test cases.

Hi, pls help me what type of test i need to make for this. Was checking Layout
test and was not sure what kind of test will be good for this. Pls help me by
suggesting some points.

[eae, 2014-02-02 21:47:00]

On 2014/02/02 15:07:47, h.joshi wrote:
> On 2014/01/31 18:18:31, hj wrote:
> > On 2014/01/31 18:15:53, eae wrote:
> > > Ok, that makes sense, thanks for the explanation.
> > > I'd still like to see a test for this though.
> > 
> > Sure, will work to make test pages for this in Blink Layout Test cases.
> 
> Hi, pls help me what type of test i need to make for this. Was checking Layout
> test and was not sure what kind of test will be good for this. Pls help me by
> suggesting some points.

For things like this you'll probably need a pixel test.

[hj, 2014-02-07 01:49:27]

On 2014/02/02 21:47:00, eae wrote:
> On 2014/02/02 15:07:47, h.joshi wrote:
> > On 2014/01/31 18:18:31, hj wrote:
> > > On 2014/01/31 18:15:53, eae wrote:
> > > > Ok, that makes sense, thanks for the explanation.
> > > > I'd still like to see a test for this though.
> > > 
> > > Sure, will work to make test pages for this in Blink Layout Test cases.
> > 
> > Hi, pls help me what type of test i need to make for this. Was checking
Layout
> > test and was not sure what kind of test will be good for this. Pls help me
by
> > suggesting some points.
> 
> For things like this you'll probably need a pixel test.

I was looking into Pixel test and found we already have one
"line-thickness-underline-strikethrough-overline.html" in "LayoutTest/fast/css".
Pls suggest do I need to make one more test or updating images in "mac" folder
present in "LayoutTest/platform/mac" will do.

[eae, 2014-02-07 01:55:41]

On 2014/02/07 01:49:27, hj wrote:
> On 2014/02/02 21:47:00, eae wrote:
> > On 2014/02/02 15:07:47, h.joshi wrote:
> > > On 2014/01/31 18:18:31, hj wrote:
> > > > On 2014/01/31 18:15:53, eae wrote:
> > > > > Ok, that makes sense, thanks for the explanation.
> > > > > I'd still like to see a test for this though.
> > > > 
> > > > Sure, will work to make test pages for this in Blink Layout Test cases.
> > > 
> > > Hi, pls help me what type of test i need to make for this. Was checking
> Layout
> > > test and was not sure what kind of test will be good for this. Pls help me
> by
> > > suggesting some points.
> > 
> > For things like this you'll probably need a pixel test.
> 
> I was looking into Pixel test and found we already have one
> "line-thickness-underline-strikethrough-overline.html" in
"LayoutTest/fast/css".
> Pls suggest do I need to make one more test or updating images in "mac" folder
> present in "LayoutTest/platform/mac" will do.

Updating the existing one will do.

[h.joshi, 2014-02-08 03:54:26]

On 2014/02/07 01:55:41, eae wrote:
> On 2014/02/07 01:49:27, hj wrote:
> > On 2014/02/02 21:47:00, eae wrote:
> > > On 2014/02/02 15:07:47, h.joshi wrote:
> > > > On 2014/01/31 18:18:31, hj wrote:
> > > > > On 2014/01/31 18:15:53, eae wrote:
> > > > > > Ok, that makes sense, thanks for the explanation.
> > > > > > I'd still like to see a test for this though.
> > > > > 
> > > > > Sure, will work to make test pages for this in Blink Layout Test
cases.
> > > > 
> > > > Hi, pls help me what type of test i need to make for this. Was checking
> > Layout
> > > > test and was not sure what kind of test will be good for this. Pls help
me
> > by
> > > > suggesting some points.
> > > 
> > > For things like this you'll probably need a pixel test.
> > 
> > I was looking into Pixel test and found we already have one
> > "line-thickness-underline-strikethrough-overline.html" in
> "LayoutTest/fast/css".
> > Pls suggest do I need to make one more test or updating images in "mac"
folder
> > present in "LayoutTest/platform/mac" will do.
> 
> Updating the existing one will do.

Hi,
 Uploading pixel test image(png).

[eae, 2014-02-10 00:09:29]

LGTM

[h.joshi, 2014-02-10 04:47:57]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-02-10 04:48:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/h.joshi@samsung.com/147703002/220001

[h.joshi, 2014-02-10 05:44:09]

On 2014/02/10 00:09:29, eae wrote:
> LGTM

Thank you,
 I have some questions mainly regarding behavior in other platforms like Android
and Windows.
For Android/Skia I have submitted following patches:

Skia Git present in Chrome source:
https://codereview.chromium.org/148453005 - Skia source file
https://codereview.chromium.org/137543008 - main Header

In Main Skia Git:
https://codereview.chromium.org/152073003/

 Above Skia changes are not reviewed, still waiting for comments.

  I feel we do need changes Windows as well, I do not have Windows setup, one
option which I came to know by googling is to make changes to
"SimpleFontDataWin.cpp" and also make changes to "TextExpectation" and specify
like following:
crbug.com/338148 [ Win ]
fast/css/line-thickness-underline-strikethrough-overline.html [ NeedsRebaseline
]

Pls Suggest, do we need changes for Windows as well.

[commit-bot: I haz the power, 2014-02-10 06:45:57]

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

[commit-bot: I haz the power, 2014-02-10 06:45:58]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[h.joshi, 2014-02-10 16:12:33]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-02-10 16:12:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/h.joshi@samsung.com/147703002/420001

[commit-bot: I haz the power, 2014-02-10 16:12:49]

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

[commit-bot: I haz the power, 2014-02-10 16:12:50]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 859.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
aff26fe86cb49446dd8c2c487a327a38be02f604..59d647af06809f439662a9fb0c38283aa2c56e89
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -859,3 +859,57 @@ crbug.com/336222 svg/filters/feDropShadow.svg [
NeedsRebaseline ]
 
 # FreeType used in layout tests changed. Will be changed to NeedsRebaseline
after landing https://codereview.chromium.org/138123004/ .
 Bug(bungeman) [ Linux ] fast/text/chromium-linux-fontconfig-renderstyle.html [
NeedsManualRebaseline ]
+
+# Way to Underline thickness code changed, need to rebase files
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/pasteboard/paste-line-endings-002.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/text/international/rtl-white-space-pre-wrap.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/pasteboard/paste-4038267-fix.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/pasteboard/paste-line-endings-008.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/repaint/shadow-multiple-strict-vertical.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/block/float/centered-float-avoidance-complexity.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
http/tests/misc/acid2.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
ietestcenter/css3/text/textshadow-002.htm [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/text/line-breaks.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/pasteboard/paste-text-019.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/pasteboard/paste-line-endings-001.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/block/float/float-in-float-painting.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/pasteboard/paste-line-endings-003.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/pasteboard/paste-line-endings-007.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/style/block-styles-007.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
tables/mozilla/bugs/bug128229.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/block/positioning/auto/vertical-rl/007.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/repaint/shadow-multiple-strict-horizontal.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/pasteboard/paste-line-endings-004.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/pasteboard/paste-line-endings-006.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/inserting/return-key-with-selection-002.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/block/positioning/auto/007.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
svg/custom/svg-fonts-without-missing-glyph.xhtml [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/block/positioning/auto/vertical-lr/007.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/text/stroking-decorations.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/inserting/insert-div-023.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/style/style-3998892-fix.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/inserting/return-key-with-selection-003.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/repaint/shadow-multiple-horizontal.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/pasteboard/paste-match-style-002.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/css/acid2.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/block/float/float-in-float-hit-testing.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-style.html [
NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/css/clip-zooming.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/pasteboard/paste-line-endings-010.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/text/trailing-white-space.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/inserting/insert-div-022.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/pasteboard/paste-line-endings-005.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/pasteboard/paste-line-endings-009.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/css3-text/css3-text-decoration/text-decoration-style.html [ NeedsRebaseline
]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/style/smoosh-styles-003.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/text/decorations-with-text-combine.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/css/line-thickness-underline-strikethrough-overline.html [ NeedsRebaseline
]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/dom/clone-node-dynamic-style.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/repaint/shadow-multiple-vertical.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/text/trailing-white-space-2.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/css/first-line-text-decoration-inherited-from-parent.html [ NeedsRebaseline
]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/css/first-line-text-decoration.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/inserting/return-key-with-selection-001.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/inserting/insert-div-024.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/inserting/insert-div-026.html [ NeedsRebaseline ]
+crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/deleting/delete-4083333-fix.html [ NeedsRebaseline ]

[h.joshi, 2014-02-11 02:14:18]

The CQ bit was checked by h.joshi@samsung.com

[commit-bot: I haz the power, 2014-02-11 02:14:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/h.joshi@samsung.com/147703002/490001

[commit-bot: I haz the power, 2014-02-11 03:54:59]

Change committed as 166890