355043 - cairo on OS/2 should be initialized before using it

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Peter Weilbacher]

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.

Comment 1

[Peter Weilbacher]

Attachment: This seems to do it

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.

Comment 2

[Peter Weilbacher]

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).

Comment 3

[Andy Willis (abwillis)]

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.

Comment 4

[Robert Longson [:longsonr]]

(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. 

Comment 5

[Peter Weilbacher]

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 ***

Comment 6

[Peter Weilbacher]

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.

Comment 7

[Peter Weilbacher]

Attachment: solution for 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.

Comment 8

[Peter Weilbacher]

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.

Comment 9

[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?

Comment 10

[Peter Weilbacher]

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

No.

Comment 11

[tor]

(In reply to comment #10)
> No.

Why not?

Comment 12

[Peter Weilbacher]

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.

Comment 13

[Peter Weilbacher]

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.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 267016 [details] [diff] [review]
solution for branch

approved for 1.8.1.5, a=dveditz for release-drivers

Comment 15

[Peter Weilbacher]

Checked into branch.

Comment 16

[Peter Weilbacher]

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.