230157 - nsDeviceContextSpecG prefs issues (wrong api)

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[Boris Zbarsky [:bzbarsky]]

gfx/src/gtk/nsDeviceContextSpecG.cpp misuses prefs badly.  See bug 225686
comment 6 for the leak (and misuse of the api in terms of ignoring the nsresult
return) and see bug 225686 comment 14 for the fact that the deprecated prefs api
is used.

Chances are the xlib version has similar issues.

Comment 1

[Christopher Blizzard (:blizzard)]

That's some impressive code right there, folks.

Comment 2

[Worcester12345]

mlk keyword needed?

Comment 3

[Mikael Parknert]

Attachment: gtk patch

bug 257381 removed the leak-part of this bug. This patch changes to use
nsIPrefBranch + some small string code changes

Comment 4

[Kenneth Herron]

Mikael, just my two cents. There's a file
gfx/src/xlib/nsDeviceContextSpecXlib.cpp which serves the same purpose in the
Xlib toolkit as nsDeviceContextSpecG.cpp does for gtk. You should create a patch
for that file as well to keep the two toolkits in sync. This isn't hard to do;
the two files are virtually identical other than the names of the identifiers.

(There's a bug somewhere about replacing these two files with a single shared
version, if you're looking for your next bug :-)

Comment 5

[Boris Zbarsky [:bzbarsky]]

I don't know when I'll be able to get to this review; it will be at least a week...

Comment 6

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2

Only requesting a review from bz for this one (not sure who to request a sr from).

This patches:
* mozilla/widget/src/gtk2/nsDeviceContextSpecG.cpp
* mozilla/gfx/src/xlib/nsDeviceContextSpecXlib.cpp
and
* mozilla/gfx/src/qt/nsDeviceContextSpecQt.cpp

I'm not sure if the last one is actually wanted here and I'll re-submit the patch without the modifications to that file if needed. If I missed any other files let me know.

Patch based from the last patch submitted by Mikael Parknert.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I don't really know this code that well (either the prefs stuff or the code being patched).

biesi, Neil, do you happen to know who's a good reviewer for the pref api changes?

Comment 8

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2 (Not including nsDeviceContextSpecQt changes)

Removed Windows line endings.

Comment 9

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2 (Inc. nsDeviceContextSpecQt.cpp changes)

Comment 10

[Ryan Jones-Ward [:sciguyryan]]

Comment on attachment 242345 [details] [diff] [review]
Patch v2 (Inc. nsDeviceContextSpecQt.cpp changes)

Per biesi requesting r,sr from roc.

Comment 11

[[e-mail]]

(In reply to comment #0)
>gfx/src/gtk/nsDeviceContextSpecG.cpp misuses prefs badly.
Just looking at the file, nsPrinterFeatures ought to use a pref branch rather than concatenating strings all the time. (Who reads these prefs anyway?)

Comment 12

[Kenneth Herron]

(In reply to comment #11)
>(Who reads these prefs anyway?)

As far as I understand it, the PrinterFeatures prefs (anything starting with "print.tmp.printerfeatures") are created by the the device context spec at the beginning of each print operation to list all of the printers and their capabilities. The prefs are then read by the print dialog to show the list of printers, paper sizes, and so on.

There is an *amazing* amount of cruft in the printing interfaces...

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 242345 [details] [diff] [review]
Patch v2 (Inc. nsDeviceContextSpecQt.cpp changes)

I'd really like to see someone take ownership of this stuff and overhaul it properly, but for now we should definitely take this. Nice work.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

I was going to land this patch, but it doesn't seem to apply cleanly to the current trunk.

Comment 15

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Updated to latest trunk.

Not actually sure what happened with that patch, I've gone through it and updated it to the latest trunk CVS so it should patch correctly now.

Comment 16

[Kenneth Herron]

Checked in:

Checking in gfx/src/qt/nsDeviceContextSpecQt.cpp;
/cvsroot/mozilla/gfx/src/qt/nsDeviceContextSpecQt.cpp,v  <--  nsDeviceContextSpecQt.cpp
new revision: 1.5; previous revision: 1.4
done
Checking in gfx/src/xlib/nsDeviceContextSpecXlib.cpp;
/cvsroot/mozilla/gfx/src/xlib/nsDeviceContextSpecXlib.cpp,v  <--  nsDeviceContextSpecXlib.cpp
new revision: 1.52; previous revision: 1.51
done
Checking in widget/src/gtk2/nsDeviceContextSpecG.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsDeviceContextSpecG.cpp,v  <--  nsDeviceContextSpecG.cpp
new revision: 1.74; previous revision: 1.73

Comment 17

[Ryan Jones-Ward [:sciguyryan]]

Are there any other files that need patches? If not this can be marked as fixed. :)