370029 - Stop using gParentWnd in nsDeviceContextSpecWin

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Eli Friedman]

Attachment: Patch

Straigntforward patch.

The primary justification for doing this is the subsequent removal of the aWidget parameter to nsIDeviceContextSpec::Init.  Also, using a global to store an hwnd is very dangerous, and could crash if DisplayPropertiesDlg were ever actually called on Windows.

Comment 1

[Eli Friedman]

(I just noticed the spelling error; fixed in my tree.)

Comment 2

[Ere Maijala (slow)]

Comment on attachment 254704 [details] [diff] [review]
Patch

You could still use the window handle in Init even though it's not stored in a global anymore, no? At least MSDN isn't saying hWnd can be NULL so it's better it isn't.

Make DisplayPropertiesDlg return NS_OK so we're on par with most of the other implementations.

Comment 3

[Eli Friedman]

(In reply to comment #2)
> (From update of attachment 254704 [details] [diff] [review])
> You could still use the window handle in Init even though it's not stored in a
> global anymore, no? At least MSDN isn't saying hWnd can be NULL so it's better
> it isn't.

Passing in NULL is perfectly safe if you're not asking DocumentProperties to display anything.  Logically, it wouldn't make any sense, because the settings for a printer shouldn't change depending on which window you happen to be asking from. Microsoft examples have no problem passing in NULL (i.e. http://support.microsoft.com/kb/140285).

Plus, we aren't even guaranteed that Init() will be called before GetDataFromPrinter (see InitPrintSettingsFromPrinter, and nsGlobalWindow::Print which calls it).  We've been passing in NULL for DocumentProperties already without any problems.

I'd like to get rid of the aWidget parameter to nsIDeviceContextSpec::Init, and that's dependent on not using it anymore on Windows.

> Make DisplayPropertiesDlg return NS_OK so we're on par with most of the other
> implementations.

Okay.

Comment 4

[Eli Friedman]

Attachment: Patch v2

Comment 5

[Ere Maijala (slow)]

Comment on attachment 255553 [details] [diff] [review]
Patch v2

Ok then :) r=emaijala

Comment 6

[Eli Friedman]

Checked in.