Create an antialiased virtual layout test group for font testing on Mac

Create an antialiased virtual layout test group for font testing on Mac.

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

[Dominik Röttsches, 2014-04-23 13:39:55]

Thanks for the pointers, Emil. In this CL, I try doing this similar to your
commit 363817d21. While this mostly works, there's an issue with reftests. The
expected results in RefTests are generated without antialiasing. It looks like
isRunningLayoutTests() (form LayoutTestSupport.h) is false while the expected
result is generated. 
Did you have an issue with this on Windows?

[Dominik Röttsches, 2014-04-23 14:40:32]

On 2014/04/23 13:39:55, Dominik Röttsches wrote:
> Did you have an issue with this on Windows?

Okay, so I see that you disabled the RefTests in TestExpectations.

When changing
   args=self._port.lookup_virtual_test_args(reference_test_name)
to
   args=self._port.lookup_virtual_test_args(self._test_name)
in
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

RefTests for antialiased text rendering start working. But I have at least some
doubts that this is the right approach. Are the virtual testsuites supposed to
compare their output to the original non command-line switched rendering? Dirk?

Should we make this an option when declaring a VirtualTestSuite? I.e. a virtual
test suite, where RefTests are compared against the command-line switched
version as well?

I'd generally prefer RefTests to be working inside the virtual suite - even
though some of them will need some CSS for disabling antialiasing for Ahem font
geometric blocks or similar.

[eae, 2014-04-23 16:25:03]

On 2014/04/23 13:39:55, Dominik Röttsches wrote:
> Thanks for the pointers, Emil. In this CL, I try doing this similar to your
> commit 363817d21. While this mostly works, there's an issue with reftests. The
> expected results in RefTests are generated without antialiasing. It looks like
> isRunningLayoutTests() (form LayoutTestSupport.h) is false while the expected
> result is generated. 
> Did you have an issue with this on Windows?

Yeah, ref tests run with the default configuration for the reference run, I've
been meaning to add an option to change that but haven't gotten around to it
yet. I'll check with Dirk and see what he thinks of that plan.

[Dirk Pranke, 2014-04-23 19:25:47]

(comments on the patch itself, not answering the other email comments yet ...)

The patch basically looks right. A few thoughts below.

https://codereview.chromium.org/249343002/diff/1/LayoutTests/NeverFixTests
File LayoutTests/NeverFixTests (right):

https://codereview.chromium.org/249343002/diff/1/LayoutTests/NeverFixTests#ne...
LayoutTests/NeverFixTests:22: [ Linux Win Android XP ] virtual/antialiasedtext [
WontFix ]
If this suite was only ever going to run on the mac, I'd probably call it
mac-antialiasestext or something like that.

https://codereview.chromium.org/249343002/diff/1/Source/platform/fonts/mac/Fo...
File Source/platform/fonts/mac/FontMac.cpp (right):

https://codereview.chromium.org/249343002/diff/1/Source/platform/fonts/mac/Fo...
Source/platform/fonts/mac/FontMac.cpp:108: if (!isFontSmoothingEnabledForTest())
I guess we're just re-using existing function names here, but this is confusing.
Can you say something like

shouldAntialias = shouldAntialias && isAntiAliasingEnabledForTest() 

and change the names? If you can't change the names, perhaps at least add a
comment about why isFontSmoothingEnabled() affects anti-aliasing but not font
smoothing :).

https://codereview.chromium.org/249343002/diff/1/Tools/Scripts/webkitpy/layou...
File Tools/Scripts/webkitpy/layout_tests/port/base.py (right):

https://codereview.chromium.org/249343002/diff/1/Tools/Scripts/webkitpy/layou...
Tools/Scripts/webkitpy/layout_tests/port/base.py:1603:
['--enable-font-smoothing']),
Same sort of comment here (i.e.,. add a comment about the names if we can't fix
the names).

[Dirk Pranke, 2014-04-23 19:50:33]

On 2014/04/23 16:25:03, eae wrote:
> Yeah, ref tests run with the default configuration for the reference run, I've
> been meaning to add an option to change that but haven't gotten around to it
> yet. I'll check with Dirk and see what he thinks of that plan.

I don't think I've ever given the issue of virtual test suites and reference
tests much thought before.

Obviously, we should try to make the ref tests work as well, and having an
antialiased test try to match an un-antialiased reference is not going to work.

I don't think you want to change the lookup to use test name (rather than the
reference name) unconditionally. I can imagine cases where you want to ensure
that the virtual suite is actually matching the non-virtual reference, and
applying the flags to the reference might change the output of the reference in
ways you don't want.

I think you could solve this by checking in an -expected.html in the virtual
test directory, just as you would check in an -expected.png. 

The downside to that is duplication of the references, of course, though maybe
you could get around it to some degree by having the virtual -expected.html just
redirect to (or iframe) the original reference

The W3C-style <link rel='match'> method of specifying references won't work,
since you can't use that to specify a virtual override. 

There is a third approach to specifying references that we don't really use but
probably should use more often, which is to specify a manifest file that lists
the references. I'm not sure how that interacts w/ the -expected.html mechanism,
so this might be something that has to be tackled as a separate effort.

You could also probably make it a per-suite option as Emil suggests. I'm a
little reluctant to add yet another branch in this whole chain of logic, and I
also wonder if you might need the ability to flag something per-test rather than
per-suite, so I'm a bit lukewarm on this idea.

WDYT of the "check in an -expected.html" approach for this?

Lastly, *for this specific test suite* I'm a little reluctant to say that even
using ref tests is a good idea. It kind of seems like if you want to test that
the antialiasing is being done correctly, the only way to do that is through
pixel tests. Are there examples where you really do also want to check that
something is matching another reference? Maybe skipping the ref tests is the
right thing to do?

[Dominik Röttsches, 2014-04-25 12:16:35]

Hi Dirk - always grateful for your support - thanks!

On 2014/04/23 19:50:33, Dirk Pranke wrote:

> I think you could solve this by checking in an -expected.html in the virtual
> test directory, just as you would check in an -expected.png. 
> 
> The downside to that is duplication of the references, of course, though maybe
> you could get around it to some degree by having the virtual -expected.html
just
> redirect to (or iframe) the original reference
> […]
> WDYT of the "check in an -expected.html" approach for this?

Alright, after reading your analysis of several possible approaches, I chose
this one and automatically referenced the -expected.html files from the original
places using a bit of JS magic in order to avoid expected results content
duplication. (See link-reftest.js) This works quite reliably with a few
exceptions for selection color and scrollbars.

Notes on your review:

> https://codereview.chromium.org/249343002/diff/1/LayoutTests/NeverFixTests
> File LayoutTests/NeverFixTests (right):
> 
>
https://codereview.chromium.org/249343002/diff/1/LayoutTests/NeverFixTests#ne...
> LayoutTests/NeverFixTests:22: [ Linux Win Android XP ] virtual/antialiasedtext
[
> WontFix ]
> If this suite was only ever going to run on the mac, I'd probably call it
> mac-antialiasestext or something like that.

Renamed.

>
https://codereview.chromium.org/249343002/diff/1/Source/platform/fonts/mac/Fo...
> File Source/platform/fonts/mac/FontMac.cpp (right):
> 
>
https://codereview.chromium.org/249343002/diff/1/Source/platform/fonts/mac/Fo...
> Source/platform/fonts/mac/FontMac.cpp:108: if
(!isFontSmoothingEnabledForTest())
> I guess we're just re-using existing function names here, but this is
confusing.
> Can you say something like
> 
> shouldAntialias = shouldAntialias && isAntiAliasingEnabledForTest() 
> 
> and change the names? If you can't change the names, perhaps at least add a
> comment about why isFontSmoothingEnabled() affects anti-aliasing but not font
> smoothing :).

Inlined the condition and added a FIXME comment. I would like to handle the
command line switch rename separately, filed crbug.com/367082.

>
https://codereview.chromium.org/249343002/diff/1/Tools/Scripts/webkitpy/layou...
> File Tools/Scripts/webkitpy/layout_tests/port/base.py (right):
> 
>
https://codereview.chromium.org/249343002/diff/1/Tools/Scripts/webkitpy/layou...
> Tools/Scripts/webkitpy/layout_tests/port/base.py:1603:
> ['--enable-font-smoothing']),
> Same sort of comment here (i.e.,. add a comment about the names if we can't
fix
> the names).

Same FIXME comment added here.

Again, thanks for your help and the review!

[Dominik Röttsches, 2014-04-25 15:19:02]

I am running out of ideas to get that through the trybots... :-(
The commit is here as well:
https://github.com/drott/blink-crosswalk/commit/a7dc17089568532518e0168b63e92...

[Dirk Pranke, 2014-04-25 15:43:37]

I expect you're probably hitting problems applying the patch because it is so
large. 

You might need to omit the PNGs and mark the tests as NeedsRebaseline and let
the auto-rebaseline-bot fix them up for you (or use NeedsManualRebaseline and do
some manually in subsequent patches).

I'm fine w/ renaming the command line flag in a follow-on patch, but please use
bug numbers when you add # FIXME comments so that we have a better chance of
figuring out if the FIXMEs are still relevant in the future.

I'm surprised that you have so many ref tests that you want to run; are these
really testing interesting cases in the code that are different in the
antialiasing case (i.e., is running those tests really adding value and likely
to detect bugs)?

https://codereview.chromium.org/249343002/diff/70001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/249343002/diff/70001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:418: crbug.com/334269 [ Mac ]
fast/text/international/bdo-bidi-width.html [ ImageOnlyFailure ]
Are these failures actual failures, or are they due to just not having the right
baselines?

https://codereview.chromium.org/249343002/diff/70001/Source/platform/fonts/ma...
File Source/platform/fonts/mac/FontMac.cpp (right):

https://codereview.chromium.org/249343002/diff/70001/Source/platform/fonts/ma...
Source/platform/fonts/mac/FontMac.cpp:108: // FIXME:
isFontSmoothingEnabledForTest() really means isFontAntialiasingEnabledForTest()
Nit: please include the bug number in the comment.

https://codereview.chromium.org/249343002/diff/70001/Tools/Scripts/webkitpy/l...
File Tools/Scripts/webkitpy/layout_tests/port/base.py (right):

https://codereview.chromium.org/249343002/diff/70001/Tools/Scripts/webkitpy/l...
Tools/Scripts/webkitpy/layout_tests/port/base.py:1598: # FIXME:
--enable-font-smoothing really means font antialiasing.
Nit: please include the bug number in the comment.

[Dominik Röttsches, 2014-04-25 16:48:23]

Thanks for the quick review.

On 2014/04/25 15:43:37, Dirk Pranke wrote:
> I expect you're probably hitting problems applying the patch because it is so
> large. 
> 
> You might need to omit the PNGs and mark the tests as NeedsRebaseline and let
> the auto-rebaseline-bot fix them up for you (or use NeedsManualRebaseline and
do
> some manually in subsequent patches).

Moved all the rebaselines (difference between non-png and png was only four
tests or so) to the bot.

> I'm fine w/ renaming the command line flag in a follow-on patch, but please
use
> bug numbers when you add # FIXME comments so that we have a better chance of
> figuring out if the FIXMEs are still relevant in the future.

Sure, added the bug reference.

> I'm surprised that you have so many ref tests that you want to run; are these
> really testing interesting cases in the code that are different in the
> antialiasing case (i.e., is running those tests really adding value and likely
> to detect bugs)?

Without having analysed all of the reftests in detail: Since we currently still
have two font backends, one for complex and one for simple text, we sometimes
hit cases in reftests where the source test renders through the one path, the
reference result through the other. Running this in antialiased as well allows
us to catch situations where the backend would incorrectly manipulate
FontDescription, other font properties or setup the Skia surface differently,
changing the rendering. (While at the same time, the long term goal is to unify
these two codepaths). 

Generally, coverage for international text, anything that goes through the
complex code path, is quite low. So I prefer to run as many as them as possible,
even though that in the antialiasing vs. non-antialiasing case this might be a
bit of a shotgun approach.

But I'll keep this in mind during development, and reconsider the cost/benefit
tradeoff of running these.

>
https://codereview.chromium.org/249343002/diff/70001/LayoutTests/TestExpectat...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/249343002/diff/70001/LayoutTests/TestExpectat...
> LayoutTests/TestExpectations:418: crbug.com/334269 [ Mac ]
> fast/text/international/bdo-bidi-width.html [ ImageOnlyFailure ]
> Are these failures actual failures, or are they due to just not having the
right
> baselines?

This one and the zerowidthjoiner case are actual Bidi failures.

[Dirk Pranke, 2014-04-26 01:43:48]

Another committer hit this issue today, and after further discussion, we think
that re-using the virtual test suite args by default for running references
seems like it should be the default (and I'm just being too paranoid /
conservative).

(See comments in https://code.google.com/p/chromium/issues/detail?id=367315 ).

danakj has posted a patch to change the default here:

https://codereview.chromium.org/257843006/

so perhaps you can depend on that and simplify your patch substantially?

[Dominik Röttsches, 2014-04-28 08:36:32]

On 2014/04/26 01:43:48, Dirk Pranke wrote:
> [...] we think
> that re-using the virtual test suite args by default for running references
> seems like it should be the default (and I'm just being too paranoid /
> conservative).
> 
> So perhaps you can depend on that and simplify your patch substantially?

Yes, thanks - updated. Now depending on landing
https://codereview.chromium.org/257843006

[Dirk Pranke, 2014-05-06 20:42:13]

lgtm.

[Dominik Röttsches, 2014-05-07 06:20:31]

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

[commit-bot: I haz the power, 2014-05-07 06:21:33]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-05-07 07:31:59]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  blink_presubmit on tryserver.blink

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

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  blink_presubmit on tryserver.blink

[commit-bot: I haz the power, 2014-05-07 08:00:25]

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

[commit-bot: I haz the power, 2014-05-07 08:00:26]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink

[Dominik Röttsches, 2014-05-07 10:45:39]

I would need an owner lgtm for FontMac.cpp. Thanks in advance.

[eae, 2014-05-07 16:34:53]

LGTM for Source/platform

[eae, 2014-05-07 16:34:58]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-05-07 16:35:29]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-05-07 16:48:01]

Change committed as 173545

[Dominik Röttsches, 2014-05-07 18:23:02]

Thanks, Emil!