Bug 130572 - Operator stretching: expose a math data API
Summary: Operator stretching: expose a math data API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 130682 130872
Blocks: 130324
  Show dependency treegraph
 
Reported: 2014-03-21 01:50 PDT by Frédéric Wang (:fredw)
Modified: 2014-04-02 00:05 PDT (History)
7 users (show)

See Also:


Attachments
Patch (22.91 KB, patch)
2014-03-21 03:13 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (22.83 KB, patch)
2014-03-24 02:07 PDT, Frédéric Wang (:fredw)
cfleizach: review+
Details | Formatted Diff | Diff
Patch (23.13 KB, patch)
2014-03-25 03:06 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Difference from the previous commit (3.01 KB, patch)
2014-03-25 03:10 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (23.23 KB, patch)
2014-03-27 00:31 PDT, Frédéric Wang (:fredw)
cfleizach: review+
Details | Formatted Diff | Diff
Patch (18.03 KB, patch)
2014-03-28 11:57 PDT, Frédéric Wang (:fredw)
cfleizach: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2014-03-21 01:50:49 PDT
This is essentially the work of bug 130321, without the hardcoded data.

I think we can forget the hardcoded data, since the CGFontCopyTableForTag seems to be what we want to load a table on Mac. 
I'm not sure what are the other platforms that are missing the feature to load OpenType, but I think we can ignore them for now. The parsing of the MATH table and everything that requires adding a compile flag will be handled in bug 130324.
Comment 1 Frédéric Wang (:fredw) 2014-03-21 03:13:45 PDT
Created attachment 227408 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2014-03-21 10:15:45 PDT
> > Source/WebCore/platform/graphics/FontPlatformData.cpp:30
> > +#if OS(DARWIN)
>
> OS(DARWIN) might be correct, but my first guess would have been to use
> PLATFORM(COCOA)

If I read Source/WTF/wtf/Platform.h correctly, OS(DARWIN) implies PLATFORM(COCOA). OS(DARWIN) is what is used in FontPlaformData.h.
Comment 3 chris fleizach 2014-03-21 12:30:36 PDT
(In reply to comment #2)
> > > Source/WebCore/platform/graphics/FontPlatformData.cpp:30
> > > +#if OS(DARWIN)
> >
> > OS(DARWIN) might be correct, but my first guess would have been to use
> > PLATFORM(COCOA)
> 
> If I read Source/WTF/wtf/Platform.h correctly, OS(DARWIN) implies PLATFORM(COCOA). OS(DARWIN) is what is used in FontPlaformData.h.

Ok
Comment 4 Frédéric Wang (:fredw) 2014-03-24 02:07:20 PDT
Created attachment 227629 [details]
Patch

This patch addresses bug 130321 comment 13 (except the #if OS(DARWIN)).
Comment 5 chris fleizach 2014-03-24 09:14:11 PDT
Comment on attachment 227629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227629&action=review

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.h:44
> +    bool hasMathData() const { return !!m_mathBuffer; }

should this just be return m_mathBuffer.get(), instead of !!
Comment 6 Frédéric Wang (:fredw) 2014-03-24 09:20:45 PDT
Committed r166169: <http://trac.webkit.org/changeset/166169>
Comment 7 WebKit Commit Bot 2014-03-24 10:29:31 PDT
Re-opened since this is blocked by bug 130682
Comment 8 Frédéric Wang (:fredw) 2014-03-24 10:44:41 PDT
So it seems that some platform/font can not generate a hash(). I'll need to add some special handling to ignore these cases.
Comment 9 Frédéric Wang (:fredw) 2014-03-25 03:06:32 PDT
Created attachment 227733 [details]
Patch
Comment 10 Frédéric Wang (:fredw) 2014-03-25 03:10:37 PDT
Created attachment 227734 [details]
Difference from the previous commit

So I think openTypeTable should only be defined for "OS(DARWIN) && USE(CG)", as cgFont() is not available otherwise.

I've modified the code to leave m_mathData uninitialized when m_font is undefined on Mac/Win, since otherwise the hash() asserts or return 0.
Comment 11 chris fleizach 2014-03-26 14:15:36 PDT
Comment on attachment 227734 [details]
Difference from the previous commit

View in context: https://bugs.webkit.org/attachment.cgi?id=227734&action=review

> Source/WebCore/platform/graphics/SimpleFontData.cpp:66
> +    bool tryMathData = true;

should this be initialized with false so that it's explicit who is testing for math?
Comment 12 Frédéric Wang (:fredw) 2014-03-27 00:31:51 PDT
Created attachment 227931 [details]
Patch

I've refined tryMathData so that only platforms that will have ENABLE(OPENTYPE_MATH) may set it to true. Also for freetype, there is an ASSERT(hash()) when creating the verticalData. I'm not sure when the hash() can be zero on freetype, but I leave tryMathData to false in that case too.
Comment 13 Frédéric Wang (:fredw) 2014-03-27 11:06:54 PDT
Committed r166364: <http://trac.webkit.org/changeset/166364>
Comment 15 WebKit Commit Bot 2014-03-27 16:20:05 PDT
Re-opened since this is blocked by bug 130872
Comment 16 Frédéric Wang (:fredw) 2014-03-28 11:57:11 PDT
Created attachment 228073 [details]
Patch

So the new assertion was

ASSERT(!lookupForWriting(Extractor::extract(entry)).second)

which is, IIUC, that the entry already exists in the hash table.

I now realize that what FontCache does for vertical data is very weird: it uses the hash() of the FontPlatformData as a key for the hash table (and so of course this key is itself hashed again!). So two fonts with different Open Type tables are likely to get the same key...

Open Type tables are encoded in the font file, so they should really be per FontPlatformData and I'm even assuming per SimpleFontData (unless two SimpleFontData's can share the same FontPlatformData). So I'm not really sure it is relevant to use a cache for the Open Type tables.

Also, there are not many fonts with Open Type MATH tables and the math data are likely to only be used in the MathML code. So trying to initialize the data only the first time the mathData() is called is probably a better strategy.

In this patch, I've removed the cache and moved the initialization to mathData().
Comment 17 chris fleizach 2014-04-01 17:32:51 PDT
Comment on attachment 228073 [details]
Patch

Please watch the bots after landing
Comment 18 Frédéric Wang (:fredw) 2014-04-02 00:05:47 PDT
Committed r166633: <http://trac.webkit.org/changeset/166633>