Comment 27 for bug 526482

Comment on attachment 729488
Save resolution (dpi) and duplex settings v1

I think this patch generally makes sense. Looks like this is just two pieces of data that are configurable in the print dialog, which we don't currently save between print jobs, but which we easily can by storing them in about:config like we do for other data. (and that's what the patch does)

This probably needs review from roc or karlt, the Widget:GTK module owner / peer. (Or someone who's actively working on printing, but unfortunately there have been very few people in that category recently. :))

Feedback below:

># HG changeset patch
># Parent fd5463b65693b5808cd3cde19f8d4c9d0a2c199b
># User Jan Horak <email address hidden>
># Bug #539427 - The Print dialog does not remember any settings, unlike other Gnome/GTK apps
># This patch saves print resolution (dpi) and duplex settings (two sided printing). It only
># stores settings only if the settings is available for printer.

Mozilla commit message convention to describe the change, not the bug, and if possible keep it all on one line (since our RelEng infrastructure generally only prints the first line).

So -- this should look like:
> Bug 539427: Save print resolution (DPI) and duplex settings between print jobs, if they're available.
or something along those lines.

>diff --git a/widget/gtk2/nsPrintSettingsGTK.cpp b/widget/gtk2/nsPrintSettingsGTK.cpp
[...]
>+NS_IMETHODIMP
>+nsPrintSettingsGTK::GetResolution(int32_t *aResolution)

Remove space-at-end-of-line after "NS_IMETHODIMP" here (and in all of your other added "NS_IMETHODIMP" lines in this file.)

>+nsPrintSettingsGTK::GetDuplex(int32_t *aDuplex)
>+{
>+ if (!gtk_print_settings_has_key(mPrintSettings, GTK_PRINT_SETTINGS_DUPLEX))
>+ return NS_ERROR_FAILURE;
>+ *aDuplex = (int16_t) gtk_print_settings_get_duplex(mPrintSettings);
>+ return NS_OK;

Why cast to int16_t here? The outparam is int32_t, not int16_t.

Also: use static_cast, not a c-style cast.

>+NS_IMETHODIMP
>+nsPrintSettingsGTK::SetDuplex(int32_t aDuplex)
>+{
>+ gtk_print_settings_set_duplex(mPrintSettings, (GtkPrintDuplex) aDuplex);

Use a static_cast instead of C-style cast.

Also: assert that we're in-range for a GtkPrintDuplex before doing this cast. e.g.:
 MOZ_ASSERT(aDuplex >= GTK_PRINT_DUPLEX_SIMPLEX &&
            aDuplex <= GTK_PRINT_DUPLEX_VERTICAL,
            "value is out of bounds for GtkPrintDuplex enum");

>@@ -244,16 +244,20 @@ interface nsIPrintSettings : nsISupports
> attribute long printPageDelay; /* in milliseconds */
>
>+ attribute long resolution; /* print resolution (dpi) */
>+
>+ attribute long duplex; /* print in duplex or simplex mode */
>+
  ^^ Drop the 2 space characters on this blank line here.

Also: that last comment makes it sound like "duplex" is a boolean value -- but it's not. (There are 3 possibilities, not 2.)
Maybe replace that comment with just "/* duplex mode */"?