1425178 - Rename nsPrintEngine to nsPrintJob

Status: RESOLVED FIXED

(Core :: Printing: Setup, enhancement)

Description

[Jonathan Watt [:jwatt]]

"Print engine" sounds like a singleton, or at least something that lasts across more than one print. Really this class represents a single print job.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Bob Owen (:bobowen) - PTO until 3rd April]

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

Review of attachment 8936726 [details] [diff] [review]:
-----------------------------------------------------------------

Having looked though the patch it does generally feels more natural with it being called nsPrintJob.
It feels a bit clunky when they are print preview related things, but no worse than with nsPrintEngine.

Should we CC some people who've recently touched this stuff to let them know about the change?

I spotted a few extra textual references (sometimes without the ns) in:
docshell/base/nsIContentViewerContainer.idl
layout/base/nsIDocumentViewerPrint.h
toolkit/components/printingui/ipc/PrintProgressDialogChild.cpp
toolkit/content/browser-content.js
layout/tools/reftest/reftest.jsm (although I imagine this one shouldn't change)

::: layout/base/nsDocumentViewer.cpp
@@ +4166,2 @@
>      // XXX shouldn't this be GetDoingPrint() ?
> +    return mPrintJob->GetDoingPrintPreview(aDoingPrint);

Not related to this change, but that comment is a really good question.
Looks like it's only used in one place in JS when we have a PrintingError event, so I'm not entirely sure what problems it might be causing and whether other code is just allowing for the fact it is wrong.

nsPrintJob::GetDoingPrint isn't currently used because of this.

::: layout/printing/nsPrintJob.cpp
@@ +232,5 @@
>    DisconnectPagePrintTimer();
>  }
>  
>  //-------------------------------------------------------
> +void nsPrintJob::Destroy()

nit: given that we're touching all of these, should we fix all these function where the return type is on the same line?

Also the few cases where the argument names are justified using spaces and the stars are in the wrong place.

::: layout/printing/nsPrintJob.h
@@ +2,5 @@
>  /* vim: set ts=8 sts=2 et sw=2 tw=80: */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#ifndef nsPrintJob_h___

nit: should be nsPrintJob_h to match the current style guide.

@@ +282,4 @@
>    void PageDone(nsresult aResult);
>  };
>  
> +#endif /* nsPrintJob_h___ */

nit: // instead of /* */

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Bob Owen (:bobowen) from comment #2)
> Should we CC some people who've recently touched this stuff to let them know
> about the change?

Sure. CC'ing Haik and C.J. Feel free to CC anyone else.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31be43353751
Rename nsPrintEngine to nsPrintJob. r=bobowen

Comment 5

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/31be43353751