Add unittest for OpenTypeSanitizer

Add unittest for OpenTypeSanitizer

Since TestingPlatformSupport::unitTestSupport() doesn't implement required methods(webKitRootDir and readFromFile), I needed to put this test in platform_web_unittest_files.

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

[bashi, 2014-06-06 08:09:15]

Hi darin@,

Could you take a look?

[jamesr, 2014-06-06 18:56:04]

not lgtm

https://codereview.chromium.org/316343004/diff/20001/Source/platform/TestingP...
File Source/platform/TestingPlatformSupport.cpp (right):

https://codereview.chromium.org/316343004/diff/20001/Source/platform/TestingP...
Source/platform/TestingPlatformSupport.cpp:35: #include <base/file_util.h>
<> style includes should only be used for standard library.

I suspect the reason you've used them here is because the checkdeps script
(correctly) flags these as a bad dependency.  Any code depending on base::
should be in the chromium repository and routed to blink through the blink APIs.
 We have several test support APIs in the public/ directories already

[bashi, 2014-06-13 06:33:26]

On 2014/06/06 18:56:04, jamesr wrote:
> not lgtm
> 
>
https://codereview.chromium.org/316343004/diff/20001/Source/platform/TestingP...
> File Source/platform/TestingPlatformSupport.cpp (right):
> 
>
https://codereview.chromium.org/316343004/diff/20001/Source/platform/TestingP...
> Source/platform/TestingPlatformSupport.cpp:35: #include <base/file_util.h>
> <> style includes should only be used for standard library.
> 
> I suspect the reason you've used them here is because the checkdeps script
> (correctly) flags these as a bad dependency.  Any code depending on base::
> should be in the chromium repository and routed to blink through the blink
APIs.

No. As I wrote in the description, Source/platform/testing/RunAllTests.cpp uses
<> style includes for base/ so I just thought it's allowed (but isn't?).

>  We have several test support APIs in the public/ directories already

Yes, but I couldn't figure out how to use it from Source/platform/ unittests.
Could you give me suggestions?

[bashi, 2014-06-13 07:44:11]

> >  We have several test support APIs in the public/ directories already
> 
> Yes, but I couldn't figure out how to use it from Source/platform/ unittests.
> Could you give me suggestions?

Moved to Source/web/tests. Could you take another look?

[jamesr, 2014-06-13 20:14:42]

I'm confused. How does moving this to Source/web/ help?

The functionality you want is exposed by WebUnitTests, as you've discovered. 
You can use that from a test in Source/platform/

[bashi, 2014-06-17 01:04:49]

On 2014/06/13 20:14:42, jamesr wrote:
> I'm confused. How does moving this to Source/web/ help?

For Source/platform/*Test, blink::Platform is
Source/platform/TestingPlatformSupport and unitTestSupport() is null. For
Source/web/tests/*Test, blink::Platform is public/platform/Platform and
unitTestSupport() is public/platform/WebUnitTestSupport.

> 
> The functionality you want is exposed by WebUnitTests, as you've discovered. 
> You can use that from a test in Source/platform/

Could you explain how I can use it from Source/platform?

[jamesr, 2014-06-18 05:42:53]

You probably want to be in Source/platform/blink_platform.gypi's
platform_web_unittest_files list to have a unit test in Source/platform/ that
depends on /web/ setup (like setting the platform).  There's a comment
discouraging the practice, as it's a very odd place to be dependency-wise, but
until we merge repositories i don't see a better option for unit tests for code
that lives in /platform/ and needs functionality from //base/test

[bashi, 2014-06-18 08:35:01]

On 2014/06/18 05:42:53, jamesr wrote:
> You probably want to be in Source/platform/blink_platform.gypi's
> platform_web_unittest_files list to have a unit test in Source/platform/ that
> depends on /web/ setup (like setting the platform).  There's a comment
> discouraging the practice, as it's a very odd place to be dependency-wise, but
> until we merge repositories i don't see a better option for unit tests for
code
> that lives in /platform/ and needs functionality from //base/test

Thanks. Added the test in platform_web_unittest_files. Could you take another
look?

[jamesr, 2014-06-18 16:34:11]

thanks, lgtm

This situation (and many others) will get better once we merge repos.

[bashi, 2014-06-19 22:47:04]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-06-19 22:48:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bashi@chromium.org/316343004/60001

[commit-bot: I haz the power, 2014-06-20 02:15:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/12106)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/12768)

[commit-bot: I haz the power, 2014-06-20 02:33:23]

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

[commit-bot: I haz the power, 2014-06-20 02:33:24]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[bashi, 2014-06-20 05:02:49]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-06-20 05:04:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bashi@chromium.org/316343004/80001

[commit-bot: I haz the power, 2014-06-20 05:38:29]

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

[commit-bot: I haz the power, 2014-06-20 06:22:31]

Change committed as 176593

[jamesr, 2014-06-20 19:30:21]

Unfortunately this test crashes on the android nexus 4 bot:

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28Ne...

[ RUN      ] OpenTypeSanitizerTest.TestWOFF
../../third_party/WebKit/Source/platform/fonts/opentype/OpenTypeSanitizerTest.cpp:54:
Failure
Expected: (buffer->size()) >= (4u), actual: 0 vs 4
[ CRASHED    

and due to the way that harness is configured this means none of the rest of the
unit tests in the suite run at all.

[jamesr, 2014-06-20 19:31:40]

https://codereview.chromium.org/316343004/diff/80001/Source/platform/fonts/op...
File Source/platform/fonts/opentype/OpenTypeSanitizerTest.cpp (right):

https://codereview.chromium.org/316343004/diff/80001/Source/platform/fonts/op...
Source/platform/fonts/opentype/OpenTypeSanitizerTest.cpp:61:
EXPECT_GE(sanitized->size(), 4u);
this should probably be an ASSERT_GT() so that if the assertion fails the test
ends right here instead of crashing on line 64

same for the other size checks - if the test can't proceed past a failing
invariant check without crashing then use ASSERT_FOO() to terminate the test
then and there

[jamesr, 2014-06-20 19:32:31]

A revert of this CL has been created in
https://codereview.chromium.org/347173002/ by jamesr@chromium.org.

The reason for reverting is: Crashes on WebKit Android (Nexus4) bot:

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28Ne...

[----------] 1 test from OpenTypeSanitizerTest
[ RUN      ] OpenTypeSanitizerTest.TestWOFF
../../third_party/WebKit/Source/platform/fonts/opentype/OpenTypeSanitizerTest.cpp:54:
Failure
Expected: (buffer->size()) >= (4u), actual: 0 vs 4
[ CRASHED      ].