port gfx/thebes to x86_64 Mac OS X

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: jaas, Assigned: jaas)

Tracking

Trunk
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

We need to port gfx/thebes to 64-bit Mac OS X.
Posted patch part 1, v1.0 (obsolete) — Splinter Review
Attachment #373857 - Flags: review?(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).
Posted patch part 1, v1.1Splinter Review
Attachment #373857 - Attachment is obsolete: true
Attachment #373901 - Flags: review?(jfkthame)
Attachment #373857 - Flags: review?(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!
Attachment #373901 - Flags: review?(jfkthame) → review+
Attachment #373901 - Flags: superreview?(roc)
Attachment #373901 - Flags: superreview?(roc) → superreview+
fix v1.1 pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/579f615bb510
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
I shouldn't have marked this fixed, that was only part 1, Jonathan Kew is working on the rest.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #373901 - Attachment description: fix v1.1 → part 1, v1.1
Depends on: 493280
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.
Attachment #377779 - Flags: review?(joshmoz)
(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 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.
Attachment #377779 - Flags: review?(joshmoz) → review+
Updated patch as requested in Josh's review comments.
Attachment #377779 - Attachment is obsolete: true
Attachment #381337 - Flags: superreview?(roc)
(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?
The latter. Even some C APIs that call Obj-C internally are at risk.
Attachment #381337 - Flags: superreview?(roc) → superreview+
Sorry, I was referring to bug 417560.
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.)
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/6c8c900be875
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Whiteboard: [needs landing]
This bug isn't fixed yet, it still has unresolved dependencies that are required to successfully compile 64-bit gfx on Mac OS X.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 498252
Due to a patch pushed by Jonathan Kew from bug 493280 gfx now builds in a 64-bit config.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.