Closed Bug 355043 Opened 14 years ago Closed 13 years ago

cairo on OS/2 should be initialized before using it

Categories

(Core :: SVG, defect)

1.8 Branch
x86
OS/2
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: mozilla, Assigned: mozilla)

Details

(Keywords: verified1.8.1.5)

Attachments

(1 file, 1 obsolete file)

As discussed on the newsgroup, I don't think using -DCAIRO_BUILD_DLL and the DLL init routine in Mozilla makes much sense, so I left it out of the patch in bug 354963.

Reasons:
- it wouldn't work in static builds
- it may screw up additional DLL initialization in gklayout.dll (or xul.dll)
- it won't work in Thebes builds

For non-Thebes builds this initialization could go into the nsSVGOuterFrame class. For Thebes builds I will address this problem in bug 333235.
Attached patch This seems to do it (obsolete) — Splinter Review
OS/2-cairo takes care of the number of inits and finis internally, so calling it multiple times (for more than one SVG per page or in different tabs) should not hurt. I think this patch is then the best solution.
Assignee: general → mozilla
Status: NEW → ASSIGNED
I guess bug 354866 kind of takes the steam out of this one (because SVG will then only work in Thebes builds). So this also depends on Thebes on OS/2 (and may even completely get solved there).
Depends on: 333235, 354866
Well if it gets solved in Thebes then it isn't an issue but the section of code that was calling the cairo_os2_fini() has been removed in bug 355114.  Not sure if that will cause us to leak from that or not.
(In reply to comment #3)
> Well if it gets solved in Thebes then it isn't an issue but the section of code
> that was calling the cairo_os2_fini() has been removed in bug 355114.  Not sure
> if that will cause us to leak from that or not.
> 

Since this code was not checked in and I no longer required the destructor, I removed it. You can always add the destructor back in if you need it. 
OK, bug 354866 went in, so this is a dupe of the thebes bug now. (I wish I wouldn't get sidetracked by so many things and could concentrate on that...)

*** This bug has been marked as a duplicate of 333235 ***
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
No longer depends on: 333235, 354866
Resolution: --- → DUPLICATE
Actually, now with the new supporting library that uses the OS/2-specific Fontconfig version it makes sense to at least implement this on the 1.8 branch for easier build with SVG/Canvas support. So I reopen and mark it branch.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Version: Trunk → 1.8 Branch
This is the patch for branch. As the cairo_os2_(un)initialize functions were not included in the original check-in because of the reasons mentioned in comment 0, I added a stripped down version back in that would not confuse DLL loading (and without the mutex stuff which isn't needed for Mozilla SVG support).

I will include this in my unofficial builds for testing and if it works ask for review and checkin.
Attachment #240831 - Attachment is obsolete: true
Comment on attachment 267016 [details] [diff] [review]
solution for branch

As this is OS/2 only and the cairo-os2-surface file on trunk basically contains the respective code, this is basically just to confirm that nsSVGOuterSVGFrame is the best place to add this, so that as few as possible inits/uninits occur.
Attachment #267016 - Flags: superreview?(tor)
Attachment #267016 - Flags: review?(tor)
(In reply to comment #8)
> (From update of attachment 267016 [details] [diff] [review])
> As this is OS/2 only and the cairo-os2-surface file on trunk basically contains
> the respective code, this is basically just to confirm that nsSVGOuterSVGFrame
> is the best place to add this, so that as few as possible inits/uninits occur.

Does canvas need this cairo initialization as well?
(In reply to comment #9)
> Does canvas need this cairo initialization as well?

No.
(In reply to comment #10)
> No.

Why not?
Sorry, the important part of the (un)initialization routines is to call FcInit/FcFini so that Fontconfig is started and text can be displayed in SVGs. As Canvas doesn't use text this is not needed.
Attachment #267016 - Flags: superreview?(tor)
Attachment #267016 - Flags: superreview+
Attachment #267016 - Flags: review?(tor)
Attachment #267016 - Flags: review+
Comment on attachment 267016 [details] [diff] [review]
solution for branch

OS/2 only improvement that has been tested in private builds. But as it makes a minor change to a cross-platform file I better ask for approval.
Attachment #267016 - Attachment description: preliminary solution for branch → solution for branch
Attachment #267016 - Flags: approval1.8.1.5?
Comment on attachment 267016 [details] [diff] [review]
solution for branch

approved for 1.8.1.5, a=dveditz for release-drivers
Attachment #267016 - Flags: approval1.8.1.5? → approval1.8.1.5+
Checked into branch.
Status: REOPENED → RESOLVED
Closed: 14 years ago13 years ago
Keywords: fixed1.8.1.5
Resolution: --- → FIXED
I verified this in my own build (the official builds and nightlies don't have SVG activated) but as it hasn't caused any obvious build problems for the branch nightlies I can also mark it as such.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.