Closed
Bug 355043
Opened 18 years ago
Closed 17 years ago
cairo on OS/2 should be initialized before using it
Categories
(Core :: SVG, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: mozilla, Assigned: mozilla)
Details
(Keywords: verified1.8.1.5)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
tor
:
review+
tor
:
superreview+
dveditz
:
approval1.8.1.5+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•18 years ago
|
||
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
Assignee | ||
Comment 2•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
(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.
Assignee | ||
Comment 5•18 years ago
|
||
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 ***
Assignee | ||
Comment 6•17 years ago
|
||
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
Assignee | ||
Comment 7•17 years ago
|
||
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
Assignee | ||
Comment 8•17 years ago
|
||
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?
Assignee | ||
Comment 10•17 years ago
|
||
(In reply to comment #9)
> Does canvas need this cairo initialization as well?
No.
Comment 11•17 years ago
|
||
(In reply to comment #10)
> No.
Why not?
Assignee | ||
Comment 12•17 years ago
|
||
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+
Assignee | ||
Comment 13•17 years ago
|
||
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 14•17 years ago
|
||
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+
Assignee | ||
Comment 15•17 years ago
|
||
Checked into branch.
Status: REOPENED → RESOLVED
Closed: 18 years ago → 17 years ago
Keywords: fixed1.8.1.5
Resolution: --- → FIXED
Assignee | ||
Comment 16•17 years ago
|
||
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
Keywords: fixed1.8.1.5 → verified1.8.1.5
You need to log in
before you can comment on or make changes to this bug.
Description
•