Closed Bug 897007 Opened 6 years ago Closed 6 years ago

Implement ScaledFontMac::GetFontFileData

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch font-data (obsolete) — Splinter Review
This implements GetFontFileData by reconstructing a truetype font from the table data.
Attachment #779730 - Flags: review?(bas)
A drive-by note: the patch here looks like it'll fail for CFF fonts - the version field in the header of the resulting data will be incorrect.

(Just out of curiosity, why do we need to do this?)
(In reply to Jonathan Kew (:jfkthame) from comment #1)
> A drive-by note: the patch here looks like it'll fail for CFF fonts - the
> version field in the header of the resulting data will be incorrect.

Is there a way to figure out if I have a CFF font from a CGFont

> (Just out of curiosity, why do we need to do this?)

This is needed for the drawtarget recording stuff.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> (In reply to Jonathan Kew (:jfkthame) from comment #1)
> > A drive-by note: the patch here looks like it'll fail for CFF fonts - the
> > version field in the header of the resulting data will be incorrect.
> 
> Is there a way to figure out if I have a CFF font from a CGFont

There'll be a 'CFF ' table present, and no 'loca' or 'glyf'.

A couple other things while we're here: the table directory should be sorted by tag (it seems likely CGFontCopyTableTags will give you the tags in already-sorted order, but AFAIK that's not actually documented/guaranteed behavior); and there are a couple of checksum-related issues - the 'head' table checksum will be incorrect (its calculation needs to ignore the checkSumAdjustment field within the table), and the checkSumAdjustment field itself will also be incorrect, unless you got lucky and reconstructed a font where it's identical to the original.
Comment on attachment 779730 [details] [diff] [review]
font-data

Review of attachment 779730 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/ScaledFontMac.cpp
@@ +96,5 @@
>        TemporaryRef<Path> ret = new PathCG(path, FILL_WINDING);
>        CGPathRelease(path);
>        return ret;
>    } else {
>        return ScaledFontBase::GetPathForGlyphs(aBuffer, aTarget);

nit: Seems like there's a bunch of whitespace indentation inconsistencies here. Let's make sure the new parts are 2-space indent.

@@ +128,5 @@
> +    }
> +    return shift;
> +}
> +
> +struct writeBuf

nit: Some consistency in the camel casing would be nice :)

@@ +142,5 @@
> +
> +    template <class T>
> +    void writeElement(T a)
> +    {
> +	*reinterpret_cast<T*>(&this->data[this->offset]) = a;

I'm a little bit scared here as for pointers this will mess up 64 vs 32-bit. But since you're not using it that way and the structure is internal to this file I guess that's ok.

@@ +202,5 @@
> +	CFRelease(records[i].data);
> +    }
> +
> +    // we always use an index of 0
> +    aDataCallback(buf.data, buf.offset, 0, mSize, aBaton);

delete [] records; ?
Attachment #779730 - Flags: review?(bas) → review+
Attached patch font-data v2Splinter Review
This version should get the checksums correct.
Attachment #779730 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/93dd8e2c8d30
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.