Add unit tests for FontPlatformData::hasSpaceInLigaturesOrKerning

Add unit tests for FontPlatformData::hasSpaceInLigaturesOrKerning

Add a set of unit tests for the hasSpaceInLigaturesOrKerning method that
verifies that the correct value is returned for a font that has space in
either the GPOS or GSUB tables vs for one that does not. Also tests that
the check is bypassed and false is returned if kerning and ligatures are
both disabled. Finally, it fixes the failure mode in that the method now
returns false in cases where the font tables cannot be accessed.

R=drott@chromium.org

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

[eae, 2015-07-15 23:16:35]

Patchset #1 (id:1) has been deleted

[eae, 2015-07-15 23:24:02]

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

[eae, 2015-07-15 23:24:18]

eae@chromium.org changed reviewers:
+ leviw@chromium.org

[leviw_travelin_and_unemployed, 2015-07-16 00:01:39]

lgtm

https://codereview.chromium.org/1238753005/diff/20001/Source/platform/blink_p...
File Source/platform/blink_platform.gypi (right):

https://codereview.chromium.org/1238753005/diff/20001/Source/platform/blink_p...
Source/platform/blink_platform.gypi:1005: # NOTE: These are legacy unit tests
and tests that require a Platform
This comment is a little confusing to me.

https://codereview.chromium.org/1238753005/diff/20001/Source/platform/fonts/F...
File Source/platform/fonts/FontPlatformData.cpp (right):

https://codereview.chromium.org/1238753005/diff/20001/Source/platform/fonts/F...
Source/platform/fonts/FontPlatformData.cpp:292: return false;
This makes way more sense.

https://codereview.chromium.org/1238753005/diff/20001/Source/platform/fonts/T...
File Source/platform/fonts/TestFontSelector.h (right):

https://codereview.chromium.org/1238753005/diff/20001/Source/platform/fonts/T...
Source/platform/fonts/TestFontSelector.h:57: ~TestFontSelector() override { }
You don't need to call the base destructor?

[eae, 2015-07-16 00:08:58]

On 2015/07/16 00:01:39, leviw wrote:
> lgtm
> 
>
https://codereview.chromium.org/1238753005/diff/20001/Source/platform/blink_p...
> File Source/platform/blink_platform.gypi (right):
> 
>
https://codereview.chromium.org/1238753005/diff/20001/Source/platform/blink_p...
> Source/platform/blink_platform.gypi:1005: # NOTE: These are legacy unit tests
> and tests that require a Platform
> This comment is a little confusing to me.

What is confusing about it? The conflicting advice or the fact that the tests
end up in a different binary?

> 
>
https://codereview.chromium.org/1238753005/diff/20001/Source/platform/fonts/F...
> File Source/platform/fonts/FontPlatformData.cpp (right):
> 
>
https://codereview.chromium.org/1238753005/diff/20001/Source/platform/fonts/F...
> Source/platform/fonts/FontPlatformData.cpp:292: return false;
> This makes way more sense.

:)

> 
>
https://codereview.chromium.org/1238753005/diff/20001/Source/platform/fonts/T...
> File Source/platform/fonts/TestFontSelector.h (right):
> 
>
https://codereview.chromium.org/1238753005/diff/20001/Source/platform/fonts/T...
> Source/platform/fonts/TestFontSelector.h:57: ~TestFontSelector() override { }
> You don't need to call the base destructor?

No, destructors are called automatically in the reverse order of construction

[leviw_travelin_and_unemployed, 2015-07-16 00:16:09]

On 2015/07/16 at 00:08:58, eae wrote:
> On 2015/07/16 00:01:39, leviw wrote:
> > lgtm
> > 
> >
https://codereview.chromium.org/1238753005/diff/20001/Source/platform/blink_p...
> > File Source/platform/blink_platform.gypi (right):
> > 
> >
https://codereview.chromium.org/1238753005/diff/20001/Source/platform/blink_p...
> > Source/platform/blink_platform.gypi:1005: # NOTE: These are legacy unit
tests
> > and tests that require a Platform
> > This comment is a little confusing to me.
> 
> What is confusing about it? The conflicting advice or the fact that the tests
end up in a different binary?

The run-on sentence makes it unclear what's being referred to at the end.

> 
> > 
> >
https://codereview.chromium.org/1238753005/diff/20001/Source/platform/fonts/F...
> > File Source/platform/fonts/FontPlatformData.cpp (right):
> > 
> >
https://codereview.chromium.org/1238753005/diff/20001/Source/platform/fonts/F...
> > Source/platform/fonts/FontPlatformData.cpp:292: return false;
> > This makes way more sense.
> 
> :)
> 
> > 
> >
https://codereview.chromium.org/1238753005/diff/20001/Source/platform/fonts/T...
> > File Source/platform/fonts/TestFontSelector.h (right):
> > 
> >
https://codereview.chromium.org/1238753005/diff/20001/Source/platform/fonts/T...
> > Source/platform/fonts/TestFontSelector.h:57: ~TestFontSelector() override {
}
> > You don't need to call the base destructor?
> 
> No, destructors are called automatically in the reverse order of construction

[eae, 2015-07-16 00:21:55]

https://codereview.chromium.org/1238753005/diff/20001/Source/platform/blink_p...
File Source/platform/blink_platform.gypi (right):

https://codereview.chromium.org/1238753005/diff/20001/Source/platform/blink_p...
Source/platform/blink_platform.gypi:1005: # NOTE: These are legacy unit tests
and tests that require a Platform
On 2015/07/16 00:01:39, leviw wrote:
> This comment is a little confusing to me.

will reword, thanks.

[eae, 2015-07-16 17:04:12]

The CQ bit was checked by eae@chromium.org

[eae, 2015-07-16 17:04:13]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1238753005/#ps80001
(title: "Address review comments")

[commit-bot: I haz the power, 2015-07-16 17:04:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1238753005/80001

[eae, 2015-07-16 17:53:45]

The CQ bit was unchecked by eae@chromium.org

[eae, 2015-07-16 17:56:29]

The CQ bit was checked by eae@chromium.org

[eae, 2015-07-16 17:56:29]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1238753005/#ps100001
(title: "w/TestExpectations")

[commit-bot: I haz the power, 2015-07-16 17:56:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1238753005/100001

[commit-bot: I haz the power, 2015-07-16 17:59:34]

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

[commit-bot: I haz the power, 2015-07-16 17:59:35]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/63147)

[eae, 2015-07-16 18:35:44]

The CQ bit was checked by eae@chromium.org

[eae, 2015-07-16 18:35:45]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1238753005/#ps120001
(title: "Rebase w/HEAD")

[commit-bot: I haz the power, 2015-07-16 18:36:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1238753005/120001

[commit-bot: I haz the power, 2015-07-16 21:00:41]

Committed patchset #6 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199059

[Julien - ping for review, 2015-07-17 01:36:28]

A revert of this CL (patchset #6 id:120001) has been created in
https://codereview.chromium.org/1240963002/ by jchaffraix@chromium.org.

The reason for reverting is: The new tests are hitting some ASSERT in debug:

********************************************************************************
C   87.837s Main  [CRASH]
FontPlatformDataTest.AhemHasNoSpaceInLigaturesOrKerning:
C   87.837s Main  ASSERTION FAILED: m_ptr
C   87.837s Main  ../../third_party/WebKit/Source/wtf/OwnPtr.h(66) :
WTF::OwnPtr<T>::ValueType* WTF::OwnPtr<T>::operator->() const [with T =
blink::FontCustomPlatformData; WTF::OwnPtr<T>::PtrType =
blink::FontCustomPlatformData*; WTF::OwnPtr<T>::ValueType =
blink::FontCustomPlatformData]
C   87.837s Main  
C   87.837s Main  [CRASH]
FontPlatformDataTest.AhemSpaceLigatureHasNoSpaceWithoutFontFeatures:
C   87.837s Main  ASSERTION FAILED: m_ptr
C   87.837s Main  ../../third_party/WebKit/Source/wtf/OwnPtr.h(66) :
WTF::OwnPtr<T>::ValueType* WTF::OwnPtr<T>::operator->() const [with T =
blink::FontCustomPlatformData; WTF::OwnPtr<T>::PtrType =
blink::FontCustomPlatformData*; WTF::OwnPtr<T>::ValueType =
blink::FontCustomPlatformData]
C   87.837s Main  
C   87.837s Main  [CRASH]
FontPlatformDataTest.AhemSpaceLigatureHasSpaceInLigaturesOrKerning:
C   87.837s Main  ASSERTION FAILED: m_ptr
C   87.838s Main  ../../third_party/WebKit/Source/wtf/OwnPtr.h(66) :
WTF::OwnPtr<T>::ValueType* WTF::OwnPtr<T>::operator->() const [with T =
blink::FontCustomPlatformData; WTF::OwnPtr<T>::PtrType =
blink::FontCustomPlatformData*; WTF::OwnPtr<T>::ValueType =
blink::FontCustomPlatformData]
C   87.838s Main 
********************************************************************************

See the full output in
https://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28db....

[Julien - ping for review, 2015-07-17 01:37:03]

On 2015/07/17 at 01:36:28, Julien Chaffraix - PST wrote:
> A revert of this CL (patchset #6 id:120001) has been created in
https://codereview.chromium.org/1240963002/ by jchaffraix@chromium.org.
> 
> The reason for reverting is: The new tests are hitting some ASSERT in debug:
> 
>
********************************************************************************
> C   87.837s Main  [CRASH]
FontPlatformDataTest.AhemHasNoSpaceInLigaturesOrKerning:
> C   87.837s Main  ASSERTION FAILED: m_ptr
> C   87.837s Main  ../../third_party/WebKit/Source/wtf/OwnPtr.h(66) :
WTF::OwnPtr<T>::ValueType* WTF::OwnPtr<T>::operator->() const [with T =
blink::FontCustomPlatformData; WTF::OwnPtr<T>::PtrType =
blink::FontCustomPlatformData*; WTF::OwnPtr<T>::ValueType =
blink::FontCustomPlatformData]
> C   87.837s Main  
> C   87.837s Main  [CRASH]
FontPlatformDataTest.AhemSpaceLigatureHasNoSpaceWithoutFontFeatures:
> C   87.837s Main  ASSERTION FAILED: m_ptr
> C   87.837s Main  ../../third_party/WebKit/Source/wtf/OwnPtr.h(66) :
WTF::OwnPtr<T>::ValueType* WTF::OwnPtr<T>::operator->() const [with T =
blink::FontCustomPlatformData; WTF::OwnPtr<T>::PtrType =
blink::FontCustomPlatformData*; WTF::OwnPtr<T>::ValueType =
blink::FontCustomPlatformData]
> C   87.837s Main  
> C   87.837s Main  [CRASH]
FontPlatformDataTest.AhemSpaceLigatureHasSpaceInLigaturesOrKerning:
> C   87.837s Main  ASSERTION FAILED: m_ptr
> C   87.838s Main  ../../third_party/WebKit/Source/wtf/OwnPtr.h(66) :
WTF::OwnPtr<T>::ValueType* WTF::OwnPtr<T>::operator->() const [with T =
blink::FontCustomPlatformData; WTF::OwnPtr<T>::PtrType =
blink::FontCustomPlatformData*; WTF::OwnPtr<T>::ValueType =
blink::FontCustomPlatformData]
> C   87.838s Main 
********************************************************************************
> 
> See the full output in
>
https://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28db....

I should add that this is only on Android, it was not seen on another platform.