Remove usePrinterFont() property of FontDescription

Remove usePrinterFont() property of FontDescription

Since we have subpixel positioning, we can permanently switch to
using so called "printer fonts" on Mac and remove the platform
specific property from FontDescription. This permanently disables
hinting, as discussed with eseidel and behdad.
For LayoutTests, as on other platforms, we keep hinted fonts, 
only disable hinting for a group of subpixel positioning tests,
as previously done on Windows for DirectWrite testing.

BUG=373240, 377696
R=eae,dglazkov,leviw

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

[eae, 2014-05-14 11:15:22]

This is great! LGTM

[eae, 2014-05-14 11:15:26]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-05-14 11:15:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/275913005/1

[Dominik Röttsches, 2014-05-14 15:05:22]

On 2014/05/14 11:15:22, eae wrote:
> This is great! LGTM

Thx! - Still some issues, probably some constructor argument list is now
mismatched on Win/Linux. Will take a look tomorrow.

[Dominik Röttsches, 2014-05-16 12:46:58]

Elliot, could you perhaps help. Looks like I am hitting an ancient FIXME in
Document.cpp when removing the 
fontDescription.setUsePrinterFont(m_document->printing()); line in
FontBuilder.cpp in createFontForDocument().

This change will lead to the two tests
printing/width-overflow.html
printing/setPrinting.html
failing on Linux and Windows. (Mac failures are a expected.)

These are exactly the same tests as mentioned in:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

When I do something else in createFontForDocument to touch the font, or if I
always return "ReAttach" from RenderStyle::stylePropagationDiff, the test
passes.
(like in http://pastebin.com/5caPTWqQ )

The test failure is difference in viewport size. So something in the viewport
size calulcation seems to depend on a change to the document font. 

Could this printing dependent font change perhaps be the unwanted side effect
mentioned in Document.cpp? Perhaps you can help finding the connection between
removing the font change here and the lacking of viewport size update?

[esprehn, 2014-05-19 21:31:22]

On 2014/05/16 12:46:58, Dominik Röttsches wrote:
> Elliot, could you perhaps help. Looks like I am hitting an ancient FIXME in
> Document.cpp when removing the 
> fontDescription.setUsePrinterFont(m_document->printing()); line in
> FontBuilder.cpp in createFontForDocument().
> 
> This change will lead to the two tests
> printing/width-overflow.html
> printing/setPrinting.html
> failing on Linux and Windows. (Mac failures are a expected.)
> 
> These are exactly the same tests as mentioned in:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> When I do something else in createFontForDocument to touch the font, or if I
> always return "ReAttach" from RenderStyle::stylePropagationDiff, the test
> passes.
> (like in http://pastebin.com/5caPTWqQ )
> 
> The test failure is difference in viewport size. So something in the viewport
> size calulcation seems to depend on a change to the document font. 
> 
> Could this printing dependent font change perhaps be the unwanted side effect
> mentioned in Document.cpp? Perhaps you can help finding the connection between
> removing the font change here and the lacking of viewport size update?

I suspect it is, you should probably try to fix that first.

[Dominik Röttsches, 2014-05-23 15:45:54]

On 2014/05/19 21:31:22, esprehn wrote:

> > Could this printing dependent font change perhaps be the unwanted side
effect
> > mentioned in Document.cpp? Perhaps you can help finding the connection
between
> > removing the font change here and the lacking of viewport size update?
> 
> I suspect it is, you should probably try to fix that first.

After long debugging (and very limited knowledge about the layout code), I
believe I know what causes the difference in the layout test result and I think
we can rebaseline, even though that might not be the full solution for avoiding
a similar case in the future.

When running content shell as testrunner, layout starts with viewWidth of 800
and completes with determining a viewWidth of 785 pixels. Then, the test
activates printing mode, and externalRepresentation in RenderTreeAsText starts
the dump, starting by dumping layers, then down to 
String externalRepresentation(LocalFrame* frame, RenderAsTextBehavior behavior)
where 
printContext.begin(toRenderBox(renderer)->width().toFloat());
sets the available page width to the previously determined size of 785.

However, viewWidth() returns 0 during printing. And pageLogicalWidth() returns
981 during printing mode for some reason. 

PrintContext.begin() triggers updateStyle() and createFontForDocument() - which
now sees printing mode, updates the font to setUsePrinterFont() which does
trigger some relayouting and while doing line breaking, the available size is
not 785 or 800, but instead something larger (probably 981), which leads to the
word "there" still fitting in the line. This only partially valid representation
is then dumped.

So, I think, removing the setUsePrinterFont() is not causing a bug, it makes the
representation more valid, since during testing printing, no relayout is
intended.

However, if we really want to fix the dumprendertree representation and avoid
partially invalid representations based on partial relayouts, we probably want
to make sure that the available viewWidth() during printing is something
reasonable, not just 0. 

Elliot, Emil - what do you think? Can I land this with rebaselining the two
printing cases and file a separate bug?

[eae, 2014-05-23 17:21:06]

On 2014/05/23 15:45:54, Dominik Röttsches wrote:
> Elliot, Emil - what do you think? Can I land this with rebaselining the two
> printing cases and file a separate bug?

Go for it, please make sure you do file a separate bug before doing so though
and add a FIXME to the place where we return 0.

[Dominik Röttsches, 2014-05-27 19:14:40]

Emil, could you take a quick look again, there's been still a few changes after
looking closer at the LayoutTest failures on Mac.

I did not add a FIXME since I continued the analysis and came to slightly
different conclusion on what's going on during printing. I put my findings in
https://code.google.com/p/chromium/issues/detail?id=377696

Then, I had to change ComplexTextController and FontCacheMac to keep hinting
enabled during non-subpixel LayoutTests, similar to what you did on Windows for
DirectWrite, and disable it for the virtual test suite.

Also, I am rebaselining a set of tests that contain small caps. They previously
did not do hinting, now they do. I am suspecting there was an inconsistency in
the hinting setting when generating the small caps font. I think this is okay
for now. For landing the CoreText shaper removal we have to revisit the small
caps case anyway.

If it looks okay to you and the bots turn green (they should now), feel free to
commit. Thanks.

[eae, 2014-05-27 19:17:40]

LGTM

[eae, 2014-05-27 19:18:06]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-05-27 19:18:44]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/275913005/...

[commit-bot: I haz the power, 2014-05-27 21:45:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-27 21:46:12]

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

[commit-bot: I haz the power, 2014-05-27 21:46:13]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Dominik Röttsches, 2014-05-28 04:39:14]

The CQ bit was checked by dominik.rottsches@intel.com

[commit-bot: I haz the power, 2014-05-28 04:39:29]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/275913005/...

[commit-bot: I haz the power, 2014-05-28 04:40:04]

Change committed as 174930