Rename nsPrintEngine to nsPrintJob

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

"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.
Posted patch patchSplinter Review
Attachment #8936726 - Flags: review?(bobowencode)
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 /* */
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 jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31be43353751
Rename nsPrintEngine to nsPrintJob. r=bobowen
https://hg.mozilla.org/mozilla-central/rev/31be43353751
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.