port gfx/thebes to x86_64 Mac OS X

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jaas, Assigned: jaas)

Tracking

Trunk
x86_64
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
We need to port gfx/thebes to 64-bit Mac OS X.
(Assignee)

Comment 1

9 years ago
Created attachment 373857 [details] [diff] [review]
part 1, v1.0
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).
(Assignee)

Comment 3

9 years ago
Created attachment 373901 [details] [diff] [review]
part 1, v1.1
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+
(Assignee)

Updated

9 years ago
Attachment #373901 - Flags: superreview?(roc)
Attachment #373901 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 5

9 years ago
fix v1.1 pushed to mozilla-central

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

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

9 years ago
I shouldn't have marked this fixed, that was only part 1, Jonathan Kew is working on the rest.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

9 years ago
Attachment #373901 - Attachment description: fix v1.1 → part 1, v1.1
Depends on: 493280
Created attachment 377779 [details] [diff] [review]
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.
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.
(Assignee)

Comment 9

9 years ago
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+
Created attachment 381337 [details] [diff] [review]
use Cocoa instead of Appearance Mgr APIs (v2)

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+
(Assignee)

Comment 13

9 years ago
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.)
Keywords: checkin-needed
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/6c8c900be875
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Whiteboard: [needs landing]
(Assignee)

Comment 16

9 years ago
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
(Assignee)

Comment 17

9 years ago
Due to a patch pushed by Jonathan Kew from bug 493280 gfx now builds in a 64-bit config.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.