151628 - API - Freeze - nsIPrintSettings needs assignment method

Status: VERIFIED FIXED

(Core :: Printing: Output, defect, P1)

Description

[rods (gone)]

Comment 1

[rods (gone)]

Attachment: patch

Add "Assign" method to nsIPrintSettings to assign values from one PS to
another.

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 87559 [details] [diff] [review]
patch

+// Assignment helper macros
+#define AssignDBL(_name) \
+if (NS_SUCCEEDED(aPS->Get##_name(&dbl))) { \
...

There's a few of these macros, please rename them so that they match the normal
macro naming convention ni mozilla, i.e. all upper case, so in stead of
AssignDBL(), make it ASSIGN_DBL(), otherwise the call-sites look too much like
they're calling functions.

- In nsPrintSettingsMac::AssignObj():

+  if (psMac) {
+	  if (mPrintRecord)
+		  ::DisposeHandle((Handle)mPrintRecord);

Stick to 2-space indentation.

- In nsPrintSettingsX::AssignObj()

+  NS_ENSURE_ARG_POINTER(aPS);
+
+  nsCOMPtr<nsIPrintSettingsX> psX = do_QueryInterface(aPS);
+  if (psX) {
+    if (mPageFormat != kPMNoPageFormat) {
+      ::PMDisposePageFormat(mPageFormat);
+      mPageFormat = kPMNoPageFormat;
+    }
+
+    psX->GetPMPageFormat(&mPageFormat);
+    psX->GetPMPrintSettings(&mPrintSettings);
+
+  }

Why the empty line above this closing brace?

+
+  return NS_ERROR_FAILURE;
+}

Why not start out with:

+  nsCOMPtr<nsIPrintSettingsX> psX = do_QueryInterface(aPS);
+  if (!psX) {
+    return NS_ERROR_FAILURE;
+  }

and then go on with the normal code...? Then you can loose the
NS_ENSURE_ARG_POINTER() too.

- In nsPrintSettingsWin::AssignObj():

+    if (mDeviceName) nsCRT::free(mDeviceName);
+    if (mDriverName) nsCRT::free(mDriverName);
+    if (mDevMode) free(mDevMode);

This looks inconsistent, why not nsCRT::free(mDevMode)? And please split those
if's onto two lines, and add braces while you're at it.

Oh, and same thing here, check for the bad input first and return the error
early in stead of putting all the code inside an if.

sr=jst if you fix that.

Comment 3

[rods (gone)]

Attachment: alt. patch

This is an alternative patch that uses an assignment operator.

Comment 4

[rods (gone)]

Attachment: alternative patch

This is the correct alt. patch that uses the assigment operator

Comment 5

[Conrad Carlen (not reading bugmail)]

Comment on attachment 87952 [details] [diff] [review]
alternative patch 

r=ccarlen

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 87952 [details] [diff] [review]
alternative patch 

sr=jst, but don't you want 

  nsPrintSettings& operator=(const nsPrintSettings& rhs);

to be virtual to make sure the right one is called regardless of the type of
the reference you happen to have to one of these things when you call that
operator?

Comment 7

[rods (gone)]

The other platforms need it

fixed

Comment 8

[Kevin McCluskey (gone)]

topembed+. This is needed to freeze the printing API's.

Comment 9

[rods (gone)]

I added virtual to the assignment operator per jst's suggestion.

Comment 10

[Olga]

Verified on trunk 06/20/02 Win2K, Linux.
I checked Page Setup functionality, no regressions.

Comment 11

[Kevin McCluskey (gone)]

adt: This is a low risk patch that is needed for embedding only. This change is
needed before we can freeze the printing API's. Without this change the printing
API's will remain unfrozen.

Comment 12

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 13

[rods (gone)]

ADT: Although the patch is large, the important part of the change is just
adding the the argument to the call. The rest of the patch, the "Assign" method
and copy constructor which made it large, will be used on on the trunk very
shortly, but is not used on the branch.

So the patch is still low risk.

Comment 14

[Steve Dagley]

This bug appears to have gotten branch landing approval on 6/25 - any chance
it's going to land there soon?

Comment 15

[Paul Wyskoczka]

Adding adt1.0.1+ on behalf of adt.  Please check into the Mozilla branch asap.

Comment 16

[Judson Valeski]

1.0.1 driver aproval still stnads.

Comment 17

[rods (gone)]

fixed on branch