Closed Bug 1425178 Opened 5 years ago Closed 5 years ago

Rename nsPrintEngine to nsPrintJob


(Core :: Printing: Setup, enhancement)

Not set



Tracking Status
firefox59 --- fixed


(Reporter: jwatt, Assigned: jwatt)



(1 file)

"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.
Attached patch patchSplinter Review
Attachment #8936726 - Flags: review?(bobowencode)
Comment on attachment 8936726 [details] [diff] [review]

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:
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 */
> +#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 /* */
Attachment #8936726 - Flags: review?(bobowencode) → review+
(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.
Pushed by
Rename nsPrintEngine to nsPrintJob. r=bobowen
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.