489354 - port gfx/thebes to x86_64 Mac OS X

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Josh Aas]

We need to port gfx/thebes to 64-bit Mac OS X.

Comment 1

[Josh Aas]

Attachment: part 1, v1.0

Comment 2

[Jonathan Kew [:jfkthame]]

Comment on attachment 373857 [details] [diff] [review]
part 1, v1.0

One issue here:

+    CGFontRef cgFont = ::CGFontCreateWithPlatformFont((void*)mATSFont);

needs to be

+    CGFontRef cgFont = ::CGFontCreateWithPlatformFont(&mATSFont);

instead. I feel the CGFont documentation is less than perfectly clear on this: it says "you should pass a pointer to an ATS font", and it's easy to assume that an ATSFontRef *is* such a "pointer to an ATS font". But it's not, and casting it to void* won't make it one. You really do have to pass a *pointer to* an ATSFontRef.

With that change, the patch works for me on 10.5 with Core Text (32-bit).

Comment 3

[Josh Aas]

Attachment: part 1, v1.1

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 373901 [details] [diff] [review]
part 1, v1.1

Looks fine, r+ from me ... though I don't think that counts for much officially!

Comment 5

[Josh Aas]

fix v1.1 pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/579f615bb510

Comment 6

[Josh Aas]

I shouldn't have marked this fixed, that was only part 1, Jonathan Kew is working on the rest.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: use Cocoa instead of Appearance Mgr APIs

The right way forward here seems to be to switch to Cocoa APIs, so I have rewritten nsSystemFontsMac to use these. In initial testing, the results look fine, but I have not checked that all the options are being exercised, just aimed to replicate the old groups of constants that mapped to the same font.

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to comment #7)

> The right way forward here seems to be to switch to Cocoa APIs...

I'm not sure if this is adequate in the context of localization; it would be good to test it under various locales.

If this approach does not work adequately in non-Latin locales, an alternative would be to use CTFontCreateUIFontForLanguage and then extract the family name, size, and font traits from the resulting CTFontRef. This would be for 10.5+ only, though, and involves more verbose code, so I'd prefer to avoid it.

Comment 9

[Josh Aas]

Comment on attachment 377779 [details] [diff] [review]
use Cocoa instead of Appearance Mgr APIs

> ifneq (,$(filter $(MOZ_WIDGET_TOOLKIT),mac cocoa))

Change this to "ifeq" and just compare against "cocoa" while you're here.

>+            font = [NSFont systemFontOfSize: 0.0];

I'd prefer it if we don't put a space after the ":" for Obj-C arguments. We don't do that anywhere else in the tree. This happens a bunch in this patch.

Other than that it looks good, thanks! At some point we'll need to add Obj-C exception wrappers, bug 489354.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: use Cocoa instead of Appearance Mgr APIs (v2)

Updated patch as requested in Josh's review comments.

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to comment #9)

> At some point we'll need to add Obj-C
> exception wrappers, bug 489354.

Did you mean to refer to a different bug here?

I'm not really sure what the implications of this are, or whether there's any real risk of the Cocoa methods here throwing exceptions at us. Does Apple document this, or do we just have to assume it's always a risk?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The latter. Even some C APIs that call Obj-C internally are at risk.

Comment 13

[Josh Aas]

Sorry, I was referring to bug 417560.

Comment 14

[Jonathan Kew [:jfkthame]]

OK, I've been reading some of the bugs related to Obj-C exceptions.

In this case, I suspect exceptions are pretty unlikely, unless the OS font system is thoroughly messed up, but it looks like the right thing to do would be to simply wrap the main body of nsSystemFontsMac::GetSystemFont with NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT...NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT.

Right now that won't compile because of the -fno-exceptions option which causes try and catch to be #define'd as empty. Could hack around that by #undef'ing them at the top of the file, but I'm inclined to leave it for now as it looks like the Obj-C extension stuff is somewhat in flux, and we can pick this up later along with the rest of bug 417560 once the macro names and compilation options are more settled.

Meanwhile it'd be nice to land this on trunk so the new approach can get some wider testing. (Mostly I'm concerned whether it will behave nicely for all locale settings - for the app and/or the host OS - but I'm not readily able to test everything personally.)

Comment 15

[Jeff Muizelaar [:jrmuizel]]

http://hg.mozilla.org/mozilla-central/rev/6c8c900be875

Comment 16

[Josh Aas]

This bug isn't fixed yet, it still has unresolved dependencies that are required to successfully compile 64-bit gfx on Mac OS X.

Comment 17

[Josh Aas]

Due to a patch pushed by Jonathan Kew from bug 493280 gfx now builds in a 64-bit config.