Bob Owen (:bobowen) Reporter
	Description • 9 years ago

Bug 1156742 introduces changes to enable printing via the main process using Moz2D recording.
Much of this is cross-platform, the main thing that isn't is fonts.
These might change a bit depending on reviews.

Steps to get this working for OSX are at least:
* Change SFNTNameTable::GetU16FullName to be able to at least read FullNames for Mac fonts. This will entail changing the internal workings, so that it can decode at least Mac Roman, I think. I was thinking of holding some sort of mapping from platform/lang/encoding to decoding functions and removing the no-decoding required restriction. Update the comments in this class and SFNTData.

* Implement ScaledFontMac::GetCairoFontFace() using cairo_quartz_font_face_create_for_cgfont (see ScaledFontDWrite::GetCairoFontFace).

* Create a new NativeFontResourceMac similar to NativeFontResourceWin and NativeFontResourceDWrite, but using code similar to the OS specific parts of gfxMacPlatformFontList::MakePlatformFont.

* Add creating NativeFontResourceMac into Factory::CreateNativeFontResource for FontType::CAIRO.

* Might need to check that the gfxPlatformMac::CreateOffscreenSurface creates something that will work for the print recording.

* Set the print-via-parent pref.

* Be lucky.

	Haik Aftandilian [:haik] Assignee
	Comment 4 • 8 years ago
mozreview-review

Comment on attachment 8785188 [details]
Bug 1228022 - part 2 - Add support for reading Mac OS Roman encoded names from SFNTNameTables;

https://reviewboard.mozilla.org/r/69724/#review72886

::: gfx/2d/SFNTNameTable.cpp:367
(Diff revision 1)
> +  if (mStringDataLength < offset + length) {
> +    gfxWarning() << "Name data too short to contain name string.";
> +    return false;
> +  }
> +  if (length > INT_MAX) {
> +    gfxWarning() << "Name record too long to decode.";

TODO: return false here.

	Bob Owen (:bobowen) Reporter
	Comment 5 • 8 years ago
mozreview-review

Comment on attachment 8785187 [details]
Bug 1228022 - part 1 - Support replay of Mac print stream, adds NativeFontResourceMac, ScaledFontMac::GetCairoFontFace;

https://reviewboard.mozilla.org/r/69722/#review73092

::: gfx/2d/NativeFontResourceMac.h:14
(Diff revision 1)
> +
> +#include "2D.h"
> +#include "mozilla/AlreadyAddRefed.h"
> +
> +#include <stdlib.h>
> +#include <string.h>

Do we need this?

::: gfx/2d/NativeFontResourceMac.h:35
(Diff revision 1)
> +private:
> +  NativeFontResourceMac(uint8_t *aFontData, uint32_t aDataLength,
> +                        bool aNeedsCairo)
> +    : mFontData(aFontData), mDataLength(aDataLength), mNeedsCairo(aNeedsCairo)
> +  {
> +    mFontData = (uint8_t *)malloc(aDataLength);

I think we should probably be using UniquePtr<uint8_t[]> and new (or maybe MakeUnique) in the member initialiser.
Then std::copy (I think we can use that on Mac now).

::: gfx/2d/NativeFontResourceMac.cpp:39
(Diff revision 1)
> +}
> +
> +already_AddRefed<ScaledFont>
> +NativeFontResourceMac::CreateScaledFont(uint32_t aIndex, uint32_t aGlyphSize)
> +{
> +  CGDataProviderRef provider =

Someone with more OS X knowledge might know better, but it feels like we should create this data provider up front and hold it as a member.

Also, the way this is written now, aren't we going to free mFontData on the first call to this? On subsequent calls, bad things can happen.

::: gfx/2d/NativeFontResourceMac.cpp:43
(Diff revision 1)
> +{
> +  CGDataProviderRef provider =
> +    ::CGDataProviderCreateWithData(nullptr, mFontData, mDataLength,
> +                                   &ReleaseData);
> +
> +  CGFontRef fontRef = ::CGFontCreateWithDataProvider(provider);

Actually, looks like this is the thing we should hold as our member, then we don't need to copy the original font data at all or hold onto the data provider.

As this is ref counted I assume that it takes a copy of any necessary data (someone who really knows what they're talking about should confirm :-) ).

We can then just CGFontRelease this in our destructor, not sure it gets released at the moment.

::: gfx/2d/ScaledFontMac.h:26
(Diff revision 1)
>  namespace gfx {
>  
>  class ScaledFontMac : public ScaledFontBase
>  {
>  public:
> -  MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(ScaledFontMac)
> +  MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(ScaledFontMac, override)

I don't think we should add in these overrides as part of this patch. It makes it harder to work out what this change is really about.

	Bob Owen (:bobowen) Reporter
	Comment 6 • 8 years ago
mozreview-review

Comment on attachment 8785188 [details]
Bug 1228022 - part 2 - Add support for reading Mac OS Roman encoded names from SFNTNameTables;

https://reviewboard.mozilla.org/r/69724/#review73106

::: gfx/2d/SFNTNameTable.h:55
(Diff revision 1)
>    SFNTNameTable(const NameHeader *aNameHeader, const uint8_t *aNameData,
>                  uint32_t aDataLength);
>  
> -  bool ReadU16Name(const NameRecordMatchers& aMatchers, mozilla::u16string& aU16Name);
> +  bool ReadU16Name(const NameRecordMatchers& aMatchers,
> +                   mozilla::u16string& aU16Name);
> +  bool ReadMacRomanName(const NameRecordMatchers& aMatchers,

What I'd envisioned here was a change to ReadU16Name to be able to convert MacRoman names.

Probably by removing the IsUTF16Encoding part from the matchers and changing ReadU16NameFromRecord to lookup a conversion function using platformID, encodingID and languageID (not sure if we need language ID as part of key).
Some of the current code in ReadU16NameFromRecord would probably move into that conversion function for the existing Unicode fonts.
PLATFORM_ID_MAC and ENCODING_ID_MAC_ROMAN would return a Mac Roman converter.

If we fail to find a conversion function we return false.

ReadU16Name would need to change as well, to make sure we keep looking for a name that we can convert.

::: gfx/2d/SFNTNameTable.cpp:374
(Diff revision 1)
> +  int encodedLength = static_cast<int>(length);
> +
> +  const char *startOfName = (char *)(mStringData + offset);
> +
> +  // get a MacRoman to Unicode coverter
> +  nsCOMPtr<nsIUnicodeDecoder> decoder =

Unless things have changed (would need confirming by gfx people) Moz2D can only use MFBT not other gecko stuff.

	Mike Conley (:mconley) (:⚙️)
	Comment 7 • 8 years ago
mozreview-review

Comment on attachment 8785189 [details]
Bug 1228022 - part 3 - Flip the print_via_parent pref to true on OS X, enabling remote printing;

https://reviewboard.mozilla.org/r/73700/#review73322

	Mike Conley (:mconley) (:⚙️)
	Comment 8 • 8 years ago
mozreview-review

Comment on attachment 8785188 [details]
Bug 1228022 - part 2 - Add support for reading Mac OS Roman encoded names from SFNTNameTables;

https://reviewboard.mozilla.org/r/69724/#review73326

Redirected, since after looking at this, I think this is more in bobowen's wheelhouse.

	Mike Conley (:mconley) (:⚙️)
	Comment 9 • 8 years ago
mozreview-review

Comment on attachment 8785187 [details]
Bug 1228022 - part 1 - Support replay of Mac print stream, adds NativeFontResourceMac, ScaledFontMac::GetCairoFontFace;

https://reviewboard.mozilla.org/r/69722/#review73328

Redirected, since after looking at this, I think this is more in bobowen's wheelhouse.

	Haik Aftandilian [:haik] Assignee
	Comment 13 • 8 years ago
mozreview-review-reply

Comment on attachment 8785187 [details]
Bug 1228022 - part 1 - Support replay of Mac print stream, adds NativeFontResourceMac, ScaledFontMac::GetCairoFontFace;

https://reviewboard.mozilla.org/r/69722/#review73092

> I think we should probably be using UniquePtr<uint8_t[]> and new (or maybe MakeUnique) in the member initialiser.
> Then std::copy (I think we can use that on Mac now).

Cleaned this up to use UniquePtr and std::copy.

> Someone with more OS X knowledge might know better, but it feels like we should create this data provider up front and hold it as a member.
> 
> Also, the way this is written now, aren't we going to free mFontData on the first call to this? On subsequent calls, bad things can happen.

Moved the provider creation/release to the constructor. Will try to confirm this is the right thing to do for OS X.

> Actually, looks like this is the thing we should hold as our member, then we don't need to copy the original font data at all or hold onto the data provider.
> 
> As this is ref counted I assume that it takes a copy of any necessary data (someone who really knows what they're talking about should confirm :-) ).
> 
> We can then just CGFontRelease this in our destructor, not sure it gets released at the moment.

Made fontRef a member variable and the provider is created/released in the constructor now.

> I don't think we should add in these overrides as part of this patch. It makes it harder to work out what this change is really about.

Took them out. I didn't realize before how clang requires you to have the keyword on either all overrides or none of them.

	Haik Aftandilian [:haik] Assignee
	Comment 14 • 8 years ago
mozreview-review-reply

Comment on attachment 8785187 [details]
Bug 1228022 - part 1 - Support replay of Mac print stream, adds NativeFontResourceMac, ScaledFontMac::GetCairoFontFace;

https://reviewboard.mozilla.org/r/69722/#review73092

> Do we need this?

It was for the memcmp. Not needed anymore.

	Haik Aftandilian [:haik] Assignee
	Comment 15 • 8 years ago
mozreview-review-reply

Comment on attachment 8785188 [details]
Bug 1228022 - part 2 - Add support for reading Mac OS Roman encoded names from SFNTNameTables;

https://reviewboard.mozilla.org/r/69724/#review73106

> What I'd envisioned here was a change to ReadU16Name to be able to convert MacRoman names.
> 
> Probably by removing the IsUTF16Encoding part from the matchers and changing ReadU16NameFromRecord to lookup a conversion function using platformID, encodingID and languageID (not sure if we need language ID as part of key).
> Some of the current code in ReadU16NameFromRecord would probably move into that conversion function for the existing Unicode fonts.
> PLATFORM_ID_MAC and ENCODING_ID_MAC_ROMAN would return a Mac Roman converter.
> 
> If we fail to find a conversion function we return false.
> 
> ReadU16Name would need to change as well, to make sure we keep looking for a name that we can convert.

I refactored the code to make the matchers return the encoding instead of a boolean value. So the changes don't touch as my functions now. See description for more details. Doing a more complex lookup table didn't seem worth it for the few name types we support.

> Unless things have changed (would need confirming by gfx people) Moz2D can only use MFBT not other gecko stuff.

I still have a question on the best way to do this. I'm wondering if the code in gfxFontUtils::DecodeFontName() can be used here. It already supports more encoding types.

	Jonathan Kew [:jfkthame]
	Comment 16 • 8 years ago
mozreview-review

Comment on attachment 8785188 [details]
Bug 1228022 - part 2 - Add support for reading Mac OS Roman encoded names from SFNTNameTables;

https://reviewboard.mozilla.org/r/69724/#review75602

::: gfx/2d/SFNTNameTable.cpp:12
(Diff revision 2)
> +#include "mozilla/dom/EncodingUtils.h"
> +
> +#include "nsCOMPtr.h"
> +#include "nsIUUIDGenerator.h"
> +#include "nsIUnicodeDecoder.h"

I don't believe you can use these things in Moz2D; it's not supposed to have dependencies on the rest of Gecko. So I think you need an alternative approach to decoding the MacRoman names.

Fortunately, this shouldn't be too hard; if it's for code that is only built on Mac OS (i.e. within #ifdef's, or within a Mac-specific source file), you could simply use CFString functions to do the conversion. E.g. create a string using CFStringCreateWithBytes, passing MacRoman as the encoding, and then use CFStringGetCharacters to get Unicode characters from it.

::: gfx/2d/SFNTNameTable.cpp:67
(Diff revision 2)
>  
> +enum ENameDecoder : int
> +{
> +  eNameDecoderNone,
> +  eNameDecoderUTF16,
> +  eNameDecoderMacRoman

If this is going to be supported only on Mac, it should probably be #ifdef'd everywhere it occurs, so we don't have fragments of code that look like they support MacRoman names on other platforms, yet it's not really all there.

::: gfx/2d/SFNTNameTable.cpp:165
(Diff revision 2)
>    // First, look for the English name (this will normally succeed).
>    if (!matchers->append(
>      [=](const NameRecord *aNameRecord) {
> -        return aNameRecord->nameID == aNameID &&
> +        if (aNameRecord->nameID == aNameID &&
> -               aNameRecord->languageID == CANONICAL_LANG_ID &&
> +            aNameRecord->languageID == CANONICAL_LANG_ID &&
> -               aNameRecord->platformID == PLATFORM_ID &&
> +            aNameRecord->platformID == PLATFORM_ID &&

I think we should get rid of these confusing PLATFORM_ID and CANONICAL_LANG_ID constants, which mean different things depending on the platform. Instead, explicitly use the MS platform and language constants here, and don't put this block into #if !defined(XP_MACOSX); just let it read the MS/Unicode names (if present) in all cases.

Then you can have an extra XP_MACOSX-only block that handles the MacRoman names (using the Mac platform and language constants; again, no need for the non-specific PLATFORM_ID and CANONICAL_LANG_ID copies).

::: gfx/2d/SFNTNameTable.cpp:311
(Diff revision 2)
> +        case eNameDecoderMacRoman:
> +          return ReadU16NameFromMacRomanRecord(record, aU16Name);

Put the eNameDecoderMacRoman case into an #ifdef, if we're only supporting this for the Mac platform.

::: gfx/2d/SFNTNameTable.cpp:347
(Diff revision 2)
> +bool
> +SFNTNameTable::ReadU16NameFromMacRomanRecord(const NameRecord *aNameRecord,
> +                                             mozilla::u16string& aU16Name)

And this whole method can be #ifdef'd, and then it can use Mac-specific things (such as CFString) to implement the decoding, instead of depending on gecko.

	Haik Aftandilian [:haik] Assignee
	Comment 20 • 8 years ago
mozreview-review-reply

Comment on attachment 8785188 [details]
Bug 1228022 - part 2 - Add support for reading Mac OS Roman encoded names from SFNTNameTables;

https://reviewboard.mozilla.org/r/69724/#review73106

> I still have a question on the best way to do this. I'm wondering if the code in gfxFontUtils::DecodeFontName() can be used here. It already supports more encoding types.

Changed this to do the decoding with Mac Core Foundation libraries now, as Jonathan recommended.

	Haik Aftandilian [:haik] Assignee
	Comment 21 • 8 years ago
mozreview-review-reply

Comment on attachment 8785188 [details]
Bug 1228022 - part 2 - Add support for reading Mac OS Roman encoded names from SFNTNameTables;

https://reviewboard.mozilla.org/r/69724/#review75602

Thanks for the quick review. I've updated the patch to follow your recommendations. I looked at the Apple docs and other uses of the CFStringCreateWithBytes and CFStringGetCharacters in Gecko to get an idea of how to use them, but a close look at that is probably needed.

> I don't believe you can use these things in Moz2D; it's not supposed to have dependencies on the rest of Gecko. So I think you need an alternative approach to decoding the MacRoman names.
> 
> Fortunately, this shouldn't be too hard; if it's for code that is only built on Mac OS (i.e. within #ifdef's, or within a Mac-specific source file), you could simply use CFString functions to do the conversion. E.g. create a string using CFStringCreateWithBytes, passing MacRoman as the encoding, and then use CFStringGetCharacters to get Unicode characters from it.

Done.

> If this is going to be supported only on Mac, it should probably be #ifdef'd everywhere it occurs, so we don't have fragments of code that look like they support MacRoman names on other platforms, yet it's not really all there.

I only plan to support Mac Roman name table entries on Mac. What we have is already working for Windows.

	Jonathan Kew [:jfkthame]
	Comment 22 • 8 years ago
mozreview-review

Comment on attachment 8785188 [details]
Bug 1228022 - part 2 - Add support for reading Mac OS Roman encoded names from SFNTNameTables;

https://reviewboard.mozilla.org/r/69724/#review75830

This looks like it should work fine, but I think the usage of CFString functions can be optimized somewhat to avoid extra copying of the string data; see below. r=me with these adjustments.

Oh, you should probably add an #include (ifdef'd for Mac only, obviously) for CoreFoundation.h. I guess it must already be getting pulled in somehow indirectly (through another header, or thanks to unified compilation?), but better not to rely on that.

::: gfx/2d/SFNTNameTable.cpp:328
(Diff revision 3)
> +    gfxWarning() << "Name record too long to decode.";
> +    return false;
> +  }
> +
> +  // length (in bytes) of the Mac Roman encoded string
> +  int encodedLength = static_cast<int>(length);

This seems redundant - you can just keep using 'length' below, surely?

::: gfx/2d/SFNTNameTable.cpp:331
(Diff revision 3)
> +
> +  // length (in bytes) of the Mac Roman encoded string
> +  int encodedLength = static_cast<int>(length);
> +
> +  // pointer to the Mac Roman encoded string in the name record
> +  const char *encodedStr = (char *)(mStringData + offset);

mStringData is already a `const uint8_t*` pointer, so why not stay with that type here? Then you won't need to cast it again for the call to CFStringCreate...

::: gfx/2d/SFNTNameTable.cpp:334
(Diff revision 3)
> +  cfString = CFStringCreateWithBytes(kCFAllocatorDefault,
> +                                     reinterpret_cast<const UInt8*>(encodedStr),
> +                                     encodedLength, kCFStringEncodingMacRoman,
> +                                     false);

As this is such a temporary object, and mStringData won't be going away during its lifetime, I think you can use CFStringCreateWithBytesNoCopy here (passing kCFAllocatorNull for the contentsDeallocator parameter), to avoid forcing CFString to actually copy the text.

::: gfx/2d/SFNTNameTable.cpp:342
(Diff revision 3)
> +  UniquePtr<UniChar[]> u16Buffer = MakeUnique<UniChar[]>(decodedLength);
> +
> +  // convert decoded string to u16 character array
> +  CFStringGetCharacters(cfString, CFRangeMake(0, decodedLength),
> +                        u16Buffer.get());
> +
> +  CFRelease(cfString);
> +
> +  aU16Name.assign(reinterpret_cast<char16_t*>(u16Buffer.get()), decodedLength);

Rather than going through a temporary `u16Buffer` here, it'll be more efficient to just do `aU16Name.SetLength(decodedLength)` and then let CFStringGetCharacters copy directly into it, using something like `reinterpret_cast<char16_t*>(aU16Name.BeginWriting())`.

	Jonathan Kew [:jfkthame]
	Comment 23 • 8 years ago
mozreview-review

Comment on attachment 8785187 [details]
Bug 1228022 - part 1 - Support replay of Mac print stream, adds NativeFontResourceMac, ScaledFontMac::GetCairoFontFace;

https://reviewboard.mozilla.org/r/69722/#review75836

A couple of nits/questions (some may be OK to leave as-is, see what you think), but more important, I think this currently leaks the CGFont object.

::: gfx/2d/Factory.cpp:553
(Diff revision 3)
> +        return NativeFontResourceMac::Create(aData, aSize,
> +                                             /* aNeedsCairo = */ true);

nit: this should be un-indented by two spaces

::: gfx/2d/NativeFontResourceMac.h:24
(Diff revision 3)
> +{
> +public:
> +  MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(NativeFontResourceMac)
> +
> +  static already_AddRefed<NativeFontResourceMac>
> +    Create(uint8_t *aFontData, uint32_t aDataLength, bool aNeedsCairo);

Why do we have the `aNeedsCairo` param here? AFAICS there's only one caller, and it always passes a literal `true`, so the flag doesn't really have a purpose.

Is there a plan for future work that will actually make use of this flag?

::: gfx/2d/NativeFontResourceMac.h:27
(Diff revision 3)
> +
> +  static already_AddRefed<NativeFontResourceMac>
> +    Create(uint8_t *aFontData, uint32_t aDataLength, bool aNeedsCairo);
> +
> +  already_AddRefed<ScaledFont>
> +    CreateScaledFont(uint32_t aIndex, uint32_t aGlyphSize) final;

The `final` specifier is redundant here, isn't it, given that the class as a whole is `final`?

::: gfx/2d/NativeFontResourceMac.h:48
(Diff revision 3)
> +  }
> +
> +  UniquePtr<uint8_t[]>  mFontData;
> +  uint32_t              mDataLength;
> +  bool                  mNeedsCairo;
> +  CGFontRef             mFontRef;

Who is responsible for releasing mFontRef? AFAICS, it's created by the constructor, but never released, so I think this is a leak. We probably need a destructor that calls CGFontRelease(mFontRef), right?

	Jonathan Kew [:jfkthame]
	Comment 24 • 8 years ago
mozreview-review

Comment on attachment 8785187 [details]
Bug 1228022 - part 1 - Support replay of Mac print stream, adds NativeFontResourceMac, ScaledFontMac::GetCairoFontFace;

https://reviewboard.mozilla.org/r/69722/#review75876

	Bob Owen (:bobowen) Reporter
	Comment 25 • 8 years ago
mozreview-review

Comment on attachment 8785187 [details]
Bug 1228022 - part 1 - Support replay of Mac print stream, adds NativeFontResourceMac, ScaledFontMac::GetCairoFontFace;

https://reviewboard.mozilla.org/r/69722/#review75880

::: gfx/2d/NativeFontResourceMac.h:35
(Diff revision 3)
> +  NativeFontResourceMac(uint8_t *aFontData, uint32_t aDataLength,
> +                        bool aNeedsCairo)
> +    : mFontData(MakeUnique<uint8_t[]>(aDataLength)), mDataLength(aDataLength),
> +      mNeedsCairo(aNeedsCairo)
> +  {
> +    std::copy(aFontData, aFontData + aDataLength, mFontData.get());

Sorry, my previous comment on this was confusing, because I changed my mind in a later comment...

As CGFontRef is reference counted I'm guessing that it takes a copy of any data it requires, in which case we don't need to keep our own copy here.

Hopefully jfkthame can confirm?

::: gfx/2d/NativeFontResourceMac.h:37
(Diff revision 3)
> +    CGDataProviderRef provider =
> +      ::CGDataProviderCreateWithData(nullptr, mFontData.get(), mDataLength,
> +                                     nullptr);
> +
> +    mFontRef = ::CGFontCreateWithDataProvider(provider);
> +    ::CGDataProviderRelease(provider);

What happens if any of this fails?
I think the class will be left in an inconsistent state at least.

Perhaps this should be in the static Create, with just the valid CGFontRef (and anything else we need) passed into the constructor.

	Bob Owen (:bobowen) Reporter
	Comment 26 • 8 years ago
mozreview-review

Comment on attachment 8785188 [details]
Bug 1228022 - part 2 - Add support for reading Mac OS Roman encoded names from SFNTNameTables;

https://reviewboard.mozilla.org/r/69724/#review75894

::: gfx/2d/SFNTNameTable.cpp:142
(Diff revision 3)
> +  return false;
> +}
> +#endif
> +
>  static NameRecordMatchers*
>  CreateCanonicalU16Matchers(const BigEndianUint16& aNameID)

We should probaly drop the U16 now.

::: gfx/2d/SFNTNameTable.cpp:144
(Diff revision 3)
> +  // For Windows, we return only Microsoft platform name record
> +  // matchers. On Mac, we return matchers for both Microsoft platform
> +  // records and Mac platform records.

Doing it this way round differs slightly from gfx/thebes/gfxFontUtils.cpp ReadCanonicalName where on Mac it checks for Mac specific first and then MS.

Not sure if that matters.

	Jonathan Kew [:jfkthame]
	Comment 27 • 8 years ago

(In reply to Bob Owen (:bobowen) from comment #25)
> As CGFontRef is reference counted I'm guessing that it takes a copy of any
> data it requires, in which case we don't need to keep our own copy here.

The current patch, at least, creates the font using CGFontCreateWithDataProvider. So the CGFont will retain a reference to the CGDataProvider it was given. The provider, however, is created with CGDataProviderCreateWithData, which means it is just a "wrapper" around a buffer of some kind, and we need to make sure that buffer remains valid for the life of the data provider. (Note that this Create function takes a release callback, which is a clear sign it expects the data to remain valid; it will call the callback when it has finished with it.)

So we do need to hold a copy of the font data here (unless the buffer we were passed is guaranteed to remain valid as long as the CGFont exists -- but I'm guessing that's not the case?)

A more "native" Core Foundation approach to all this would be possible, though: instead of explicitly copying the data to a buffer that we own here, you could do something like (consider this pseudo-code, without checking the exact parameters etc):

  CFDataRef data = CFDataCreate(kCFAllocatorDefault, data, length); // copies the data
  CGDataProviderRef provider = CGDataProviderCreateWithCFData(data); // create a provider
  CFRelease(data); // release our reference to the CFData, provider will keep it alive
  CGFontRef font = CGFontCreateWithDataProvider(provider); // create the font object
  CGDataProviderRelease(provider); // release our reference, font will keep it alive as long as needed

  // now |font| is the only thing we need to hold onto

This avoids the need for an explicit mFontData copy here; that happens internally to the CFData object instead.

So yes, I think I'd suggest rewriting it along these lines.

	Jonathan Kew [:jfkthame]
	Comment 28 • 8 years ago

(In reply to Bob Owen (:bobowen) from comment #26)
> Doing it this way round differs slightly from gfx/thebes/gfxFontUtils.cpp
> ReadCanonicalName where on Mac it checks for Mac specific first and then MS.
> 
> Not sure if that matters.

Only if we're relying on names retrieved by the two pieces of code to match. If this is purely internal to moz2d (which was my assumption -- true?) then it doesn't matter which is given priority, we're just looking for _something_ we can use as an identifier.

	Bob Owen (:bobowen) Reporter
	Comment 29 • 8 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #28)
> (In reply to Bob Owen (:bobowen) from comment #26)
> > Doing it this way round differs slightly from gfx/thebes/gfxFontUtils.cpp
> > ReadCanonicalName where on Mac it checks for Mac specific first and then MS.
> > 
> > Not sure if that matters.
> 
> Only if we're relying on names retrieved by the two pieces of code to match.
> If this is purely internal to moz2d (which was my assumption -- true?) then
> it doesn't matter which is given priority, we're just looking for
> _something_ we can use as an identifier.

Yes, I think this is only used within Moz2D at the moment and thanks for the explanation and fixing the idea over the CGFontRef. :-)

	Haik Aftandilian [:haik] Assignee
	Comment 30 • 8 years ago
mozreview-review-reply

Comment on attachment 8785188 [details]
Bug 1228022 - part 2 - Add support for reading Mac OS Roman encoded names from SFNTNameTables;

https://reviewboard.mozilla.org/r/69724/#review75830

Thanks. Made all the recommended changes except for one regarding a temp buffer.

> As this is such a temporary object, and mStringData won't be going away during its lifetime, I think you can use CFStringCreateWithBytesNoCopy here (passing kCFAllocatorNull for the contentsDeallocator parameter), to avoid forcing CFString to actually copy the text.

Made this change. Note: from my understading the CFRelease(cfString) is still needed and it knows not to free the underlying storage because kCFAllocatorNull is passed to CFStringCreateWithBytesNoCopy().

> Rather than going through a temporary `u16Buffer` here, it'll be more efficient to just do `aU16Name.SetLength(decodedLength)` and then let CFStringGetCharacters copy directly into it, using something like `reinterpret_cast<char16_t*>(aU16Name.BeginWriting())`.

I couldn't figure out how to avoid using the temporary buffer here. I tried using CFStringGetCharactersPtr() as in aU16Name.assign(CFStringGetCharactersPtr(), decodedLength), but CFStringGetCharactersPtr() always returns NULL for this string which the docs say is due to "the internal storage of theString does not allow this to be returned efficiently". I'll mark this issue as dropped, but let me know if you think I'm missing something. (aU16Name is a std::basic_string.)

	Haik Aftandilian [:haik] Assignee
	Comment 31 • 8 years ago
mozreview-review-reply

Comment on attachment 8785188 [details]
Bug 1228022 - part 2 - Add support for reading Mac OS Roman encoded names from SFNTNameTables;

https://reviewboard.mozilla.org/r/69724/#review75894

> Doing it this way round differs slightly from gfx/thebes/gfxFontUtils.cpp ReadCanonicalName where on Mac it checks for Mac specific first and then MS.
> 
> Not sure if that matters.

Although it shouldn't have to be the same, I've made the order the same by putting the Mac matchers first. In the limited testing I've done, putting the Mac matchers first is more efficient. If I put the Mac matchers first, I get 30/32 Mac matches on one font-heavy test page. Putting Microsoft first, I get 10/32 Mac matches. So putting Mac first would be (probably negligibly) faster because we do less failed match tests.

	Jonathan Kew [:jfkthame]
	Comment 34 • 8 years ago
mozreview-review

Comment on attachment 8785188 [details]
Bug 1228022 - part 2 - Add support for reading Mac OS Roman encoded names from SFNTNameTables;

https://reviewboard.mozilla.org/r/69724/#review76414

::: gfx/2d/SFNTNameTable.cpp:342
(Diff revision 4)
> +  // temporary buffer
> +  UniquePtr<UniChar[]> u16Buffer = MakeUnique<UniChar[]>(decodedLength);
> +
> +  CFStringGetCharacters(cfString, CFRangeMake(0, decodedLength),
> +                        u16Buffer.get());
> +
> +  CFRelease(cfString);
> +
> +  aU16Name.assign(reinterpret_cast<char16_t*>(u16Buffer.get()), decodedLength);

To avoid copying through a temp buffer here, I think you should be able to simply do


    aU16Name.SetLength(decodedLength);
    CFStringGetCharacters(cfString, CFRangeMake(0, decodedLength),
                          reinterpret_cast<UniChar*>(aU16Name.BeginWriting()));

to copy the chars directly to the output string.

	Jonathan Kew [:jfkthame]
	Comment 35 • 8 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #34)
> Comment on attachment 8785188 [details]
> Bug 1228022 - part 2 - Add support for reading Mac OS Roman encoded names
> from SFNTNameTables;
> 
> https://reviewboard.mozilla.org/r/69724/#review76414
> 
> ::: gfx/2d/SFNTNameTable.cpp:342
> (Diff revision 4)
> > +  // temporary buffer
> > +  UniquePtr<UniChar[]> u16Buffer = MakeUnique<UniChar[]>(decodedLength);
> > +
> > +  CFStringGetCharacters(cfString, CFRangeMake(0, decodedLength),
> > +                        u16Buffer.get());
> > +
> > +  CFRelease(cfString);
> > +
> > +  aU16Name.assign(reinterpret_cast<char16_t*>(u16Buffer.get()), decodedLength);
> 
> To avoid copying through a temp buffer here, I think you should be able to
> simply do
> 
> 
>     aU16Name.SetLength(decodedLength);
>     CFStringGetCharacters(cfString, CFRangeMake(0, decodedLength),
>                          
> reinterpret_cast<UniChar*>(aU16Name.BeginWriting()));
> 
> to copy the chars directly to the output string.

Oh, sorry, you said aU16Name is a basic_string .... so SetLength() and BeginWriting() from nsString aren't the right method names, I guess; but there should be equivalents you could use, no?

	Haik Aftandilian [:haik] Assignee
	Comment 36 • 8 years ago
mozreview-review-reply

Comment on attachment 8785188 [details]
Bug 1228022 - part 2 - Add support for reading Mac OS Roman encoded names from SFNTNameTables;

https://reviewboard.mozilla.org/r/69724/#review76414

> To avoid copying through a temp buffer here, I think you should be able to simply do
> 
> 
>     aU16Name.SetLength(decodedLength);
>     CFStringGetCharacters(cfString, CFRangeMake(0, decodedLength),
>                           reinterpret_cast<UniChar*>(aU16Name.BeginWriting()));
> 
> to copy the chars directly to the output string.

I don't see a way to do it with std::basic_string. The .begin() method returns an iterator which can't be cast as a UniChar pointer.

	Jonathan Kew [:jfkthame]
	Comment 37 • 8 years ago

(In reply to Haik Aftandilian [:haik] from comment #36)
> Comment on attachment 8785188 [details]
> Bug 1228022 - part 2 - Add support for reading Mac OS Roman encoded names
> from SFNTNameTables;
> 
> https://reviewboard.mozilla.org/r/69724/#review76414
> 
> > To avoid copying through a temp buffer here, I think you should be able to simply do
> > 
> > 
> >     aU16Name.SetLength(decodedLength);
> >     CFStringGetCharacters(cfString, CFRangeMake(0, decodedLength),
> >                           reinterpret_cast<UniChar*>(aU16Name.BeginWriting()));
> > 
> > to copy the chars directly to the output string.
> 
> I don't see a way to do it with std::basic_string. The .begin() method
> returns an iterator which can't be cast as a UniChar pointer.

Hmm, yeah, OK. And data() returns a const charT* pointer, so you can't use that.

So I guess we're stuck with the temp buffer. This isn't going to be hugely perf-critical anyway, so let's not worry about it - sorry for the distraction!

	Haik Aftandilian [:haik] Assignee
	Comment 38 • 8 years ago
mozreview-review-reply

Comment on attachment 8785187 [details]
Bug 1228022 - part 1 - Support replay of Mac print stream, adds NativeFontResourceMac, ScaledFontMac::GetCairoFontFace;

https://reviewboard.mozilla.org/r/69722/#review75880

> Sorry, my previous comment on this was confusing, because I changed my mind in a later comment...
> 
> As CGFontRef is reference counted I'm guessing that it takes a copy of any data it requires, in which case we don't need to keep our own copy here.
> 
> Hopefully jfkthame can confirm?

Followed jfkthame recommendation with the new version which fixes this.

> What happens if any of this fails?
> I think the class will be left in an inconsistent state at least.
> 
> Perhaps this should be in the static Create, with just the valid CGFontRef (and anything else we need) passed into the constructor.

I've moved the allocation calls to the static Create function and am just passing the CGFontRef to the constructor now. That's released in the destructor.

I also changed RecordedFontData::PlayEvent to check for a NULL return from CreateNativeFontResource. I tested trying to print with some test code that simulated the allocation failures in NativeFontResourceMac::Create and it resulted in a print failure dialog:

Print Preview Error
An error occurred while printing.

	Haik Aftandilian [:haik] Assignee
	Comment 39 • 8 years ago
mozreview-review-reply

Comment on attachment 8785187 [details]
Bug 1228022 - part 1 - Support replay of Mac print stream, adds NativeFontResourceMac, ScaledFontMac::GetCairoFontFace;

https://reviewboard.mozilla.org/r/69722/#review75836

Thanks. I've reworked the code following your cleaner recommendation on the bug report, taking care of the leak and handling failure returns from the Core Foundation code.

> Why do we have the `aNeedsCairo` param here? AFAICS there's only one caller, and it always passes a literal `true`, so the flag doesn't really have a purpose.
> 
> Is there a plan for future work that will actually make use of this flag?

No plan here to eventually make use of this. Sorry it was copy and paste code. I've removed that param.

> The `final` specifier is redundant here, isn't it, given that the class as a whole is `final`?

I've removed it.

	Jonathan Kew [:jfkthame]
	Comment 46 • 8 years ago
mozreview-review

Comment on attachment 8785187 [details]
Bug 1228022 - part 1 - Support replay of Mac print stream, adds NativeFontResourceMac, ScaledFontMac::GetCairoFontFace;

https://reviewboard.mozilla.org/r/69722/#review77464

LGTM, thanks. A few suggestions for minor cleanup; r=me with these adjustments (or without them, if it turns out they don't work as I think).

::: gfx/2d/NativeFontResourceMac.h:12
(Diff revision 5)
> +#ifndef mozilla_gfx_NativeFontResourceMac_h
> +#define mozilla_gfx_NativeFontResourceMac_h
> +
> +#include "2D.h"
> +#include "mozilla/AlreadyAddRefed.h"
> +#include "mozilla/UniquePtr.h"

Looks like UniquePtr.h isn't needed here any longer, is it?

::: gfx/2d/NativeFontResourceMac.h:37
(Diff revision 5)
> +  }
> +
> +private:
> +  explicit NativeFontResourceMac(CGFontRef aFontRef) : mFontRef(aFontRef)
> +  {
> +    CFRetain(mFontRef);

As this constructor exists only to be used by the static Create method, I'd suggest that it should take over ownership of the passed-in reference, rather than doing an additional retain (i.e. creating a new reference) of its own.

Then in Create, you won't explicitly release the font; instead, just comment that ownership passes to the NativeFontResourceMac when it is created. No need for the extra retain-and-release dance during creation of this object.

::: gfx/2d/NativeFontResourceMac.cpp:48
(Diff revision 5)
> +  }
> +
> +  RefPtr<NativeFontResourceMac> fontResource =
> +    new NativeFontResourceMac(fontRef);
> +
> +  CFRelease(fontRef);

This is the release call that won't be needed if you let the NativeFontResourceMac constructor take over ownership of the passed-in reference, instead of adding a new one.

::: gfx/2d/NativeFontResourceMac.cpp:57
(Diff revision 5)
> +
> +already_AddRefed<ScaledFont>
> +NativeFontResourceMac::CreateScaledFont(uint32_t aIndex, uint32_t aGlyphSize)
> +{
> +  RefPtr<ScaledFontBase> scaledFont = new ScaledFontMac(mFontRef,
> +     (Float) aGlyphSize);

Do we really need to explicitly cast this? I'd have expected it to happen automatically.

	Haik Aftandilian [:haik] Assignee
	Comment 47 • 8 years ago
mozreview-review-reply

Comment on attachment 8785187 [details]
Bug 1228022 - part 1 - Support replay of Mac print stream, adds NativeFontResourceMac, ScaledFontMac::GetCairoFontFace;

https://reviewboard.mozilla.org/r/69722/#review77464

Thanks. I've made all the suggested changes.

> Do we really need to explicitly cast this? I'd have expected it to happen automatically.

It's not needed. I didn't realize implicit casting from float to uint32_t was permitted. Docs confirm it's standardized.

	Haik Aftandilian [:haik] Assignee
	Comment 51 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c9e2a77f2367a16230b05b231f2bf718f4c759e

	Pulsebot
	Comment 52 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7c4ad68974
part 1 - Support replay of Mac print stream, adds NativeFontResourceMac, ScaledFontMac::GetCairoFontFace; r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/82476bb1bf17
part 2 - Add support for reading Mac OS Roman encoded names from SFNTNameTables; r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee5d5cea6137
part 3 - Flip the print_via_parent pref to true on OS X, enabling remote printing; r=mconley

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 53 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ea7c4ad68974
https://hg.mozilla.org/mozilla-central/rev/82476bb1bf17
https://hg.mozilla.org/mozilla-central/rev/ee5d5cea6137