Merge FontPlatformDataHarfBuzz and FontPlatformData headers

Merge FontPlatformDataHarfBuzz and FontPlatformData headers

Those two files were split apart and redundantly copied
for non-Mac and Mac platforms. This is an attempt an unifying
them, as a preparation for removing the CoreText shaper.
We can merge the implementations in FontPlatformDataHarfBuzz.cpp
and FontPlatformData.cpp as a next step.

This is a reland after the original change 72f392cc147764ec75e4f
got reverted in bcfdb1312a26b0e611. Fix is to
change to the previous operator== implementation and add
m_isHashTableDeletedValue to the hash computation. 

I believe this addresses the observed crash issue as well.

BUG=334269, 411287
R=eae,arv

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

[Dominik Röttsches, 2014-09-05 10:01:53]

Emil - I know where the ASSERT happens, but I am not 100% sure about the root
cause. I am suspecting an issue with comparing the WTF::HashTableDeletedIssue
and the empty FontPlatformData, but I don't have OSX 10.7 system to debug this.

[eae, 2014-09-05 14:20:30]

On 2014/09/05 10:01:53, Dominik Röttsches wrote:
> Emil - I know where the ASSERT happens, but I am not 100% sure about the root
> cause. I am suspecting an issue with comparing the WTF::HashTableDeletedIssue
> and the empty FontPlatformData, but I don't have OSX 10.7 system to debug
this.

Seems likely, we've seen similar failures in the past. I'll see if I can find a
10.7 system to test it on.

[eae, 2014-09-05 14:20:31]

On 2014/09/05 10:01:53, Dominik Röttsches wrote:
> Emil - I know where the ASSERT happens, but I am not 100% sure about the root
> cause. I am suspecting an issue with comparing the WTF::HashTableDeletedIssue
> and the empty FontPlatformData, but I don't have OSX 10.7 system to debug
this.

Seems likely, we've seen similar failures in the past. I'll see if I can find a
10.7 system to test it on.

[Dominik Röttsches, 2014-09-05 15:00:43]

> Seems likely, we've seen similar failures in the past. I'll see if I can find
a
> 10.7 system to test it on.

Thansk! If you can find one, that'd be great - otherwise I can try to set up a
Lion VM on my mac at home.

[arv (Not doing code reviews), 2014-09-05 16:52:07]

On 2014/09/05 15:00:43, Dominik Röttsches wrote:
> > Seems likely, we've seen similar failures in the past. I'll see if I can
find
> a
> > 10.7 system to test it on.
> 
> Thansk! If you can find one, that'd be great - otherwise I can try to set up a
> Lion VM on my mac at home.

I'm also fine if you just want to land this. We can always revert if it happens
again.

[eae, 2014-09-05 18:33:22]

On 2014/09/05 16:52:07, arv wrote:
> On 2014/09/05 15:00:43, Dominik Röttsches wrote:
> > > Seems likely, we've seen similar failures in the past. I'll see if I can
> find
> > a
> > > 10.7 system to test it on.
> > 
> > Thansk! If you can find one, that'd be great - otherwise I can try to set up
a
> > Lion VM on my mac at home.
> 
> I'm also fine if you just want to land this. We can always revert if it
happens
> again.

No luck finding a 10.7 machine. I'm with arv on this one, lets land and see what
happens.

[eae, 2014-09-05 18:33:31]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-09-05 18:33:39]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-09-05 18:33:40]

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

[commit-bot: I haz the power, 2014-09-05 18:33:41]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[eae, 2014-09-05 18:37:07]

LGTM

[eae, 2014-09-05 18:37:11]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-09-05 18:37:56]

CQ is trying da patch. Follow status at

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

[eae, 2014-09-05 18:45:43]

The CQ bit was unchecked by eae@chromium.org

[eae, 2014-09-05 18:46:03]

Unchecking CQ due to https://code.google.com/p/chromium/issues/detail?id=411287

[eae, 2014-09-05 18:46:04]

Unchecking CQ due to https://code.google.com/p/chromium/issues/detail?id=411287

[d.roettsches, 2014-09-06 11:01:26]

> Unchecking CQ due to
https://code.google.com/p/chromium/issues/detail?id=411287

I have a reproduction on a 10.7 VM now, looks like it is an issue with
0-text-sized FontPlatformData which has m_font and cgFont vs. a 0-sized, null
m_font comparison. So far tested with the initial patch, will test with the
speculative fix next.

[d.roettsches, 2014-09-06 19:10:17]

Tested on 10.7 and confirming that this version of the patch does not show the
assertion failure and I am fairly certain this addresses the crash issue as
well. What I suspect that happened was that perhaps we tried to convert between
NSFont and CG/CTFont with null pointers. The reason was that after I had changed
the comparision, a zero-text-sized font with initialized m_font or m_cgFont was
not distinguished from a zero-textsized font with those objects being null. I'll
land it on Monday when there's sheriffs around.

[Dominik Röttsches, 2014-09-08 07:07:50]

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

[commit-bot: I haz the power, 2014-09-08 07:08:45]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-09-08 07:09:14]

Committed patchset #2 (id:20001) as 181541

[Dominik Röttsches, 2014-09-08 10:21:28]

Looks like it went through without issues now:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.7/builds/...

[eae, 2014-09-08 14:35:47]

On 2014/09/08 10:21:28, Dominik Röttsches wrote:
> Looks like it went through without issues now:
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.7/builds/...

Excellent, thanks Dominik!

[arv (Not doing code reviews), 2014-09-08 14:56:30]

Great!

On Mon, Sep 8, 2014 at 10:35 AM, <eae@chromium.org> wrote:

> On 2014/09/08 10:21:28, Dominik Röttsches wrote:
>
>> Looks like it went through without issues now:
>>
>
> http://build.chromium.org/p/chromium.webkit/builders/
> WebKit%20Mac10.7/builds/31333
>
> Excellent, thanks Dominik!
>
> https://codereview.chromium.org/542653002/
>



-- 
erik

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[d.roettsches, 2014-09-08 20:13:19]

Unfortunately, this time it broke the build on the Windows bot. Next update
tomorrow. In the meantime, I am making progress with merging the implementation
files, too.