Closed Bug 144114 Opened 23 years ago Closed 7 years ago

Change nsPrintOptionsImpl to PrintSettingsService

Categories

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

Product:
Core
Component:
Printing: Output
Platform:
x86
Windows 2000
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: rods, Assigned: jwatt)

Details

Attachments

(4 files)

Bug 144114 part 1 - Rename the files for nsPrintOptions and its subclasses.
59 bytes, text/x-review-board-request
bobowen
: review+
Details
Bug 144114 part 2 - Rename nsPrintOptions and its subclasses to nsPrintSettingsService*.
59 bytes, text/x-review-board-request
bobowen
: review+
Details
Bug 144114 part 3 - Clean up the headers of the nsPrintSettingsService subclasses.
59 bytes, text/x-review-board-request
bobowen
: review+
Details
Bug 144114 part 4 - Remove NS_PRINTOPTIONS_CID.
59 bytes, text/x-review-board-request
bobowen
: review+
Details
PrintOptionsImpl will soon implement the nsIPrintOPtions interface and nsIPrintSettingsService Interface The nsIPrintOptions iface is our private interface The nsIPrintSettingsService is the public interface We need to change the impl over to PrintSettingsService.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Future
this bug seems to have been forgotten and should be reassessed now that bug 144128 has been fixed. (should bug 144128 have been marked as blocker?)
Assignee: rods → printing
Status: ASSIGNED → NEW
QA Contact: sujay
Assignee: printing → nobody
QA Contact: printing
Assignee: nobody → jwatt
Comment on attachment 8942395 [details] Bug 144114 part 1 - Rename the files for nsPrintOptions and its subclasses. https://reviewboard.mozilla.org/r/212696/#review218402 ::: widget/cocoa/nsPrintSettingsServiceX.mm:9 (Diff revision 1) > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #include "nsCOMPtr.h" > #include "nsQueryObject.h" > #include "nsIServiceManager.h" > -#include "nsPrintOptionsX.h" > +#include "nsPrintSettingsServiceX.h" nit: might as well move this up to the first include on its own to match the style guide while we're at it ::: widget/gtk/nsPrintSettingsServiceGTK.cpp:12 (Diff revision 1) > #include "nsPrintSettingsGTK.h" > > using namespace mozilla::embedding; > > /** --------------------------------------------------- > * See documentation in nsPrintOptionsWin.h These would need renaming, but actually I think only one of these eight such comments actually points to something that has a comment (WritePrefs) and it's pretty useless. GetEffectivePageSize has one in the idl file, which is somewhat useful assuming it's correct. So, I think we should probably just delete all of these to avoid false hope. As an aside I notice that the second argument in the WritePrefs declaration is misnamed. ::: widget/windows/nsPrintSettingsServiceWin.cpp:6 (Diff revision 1) > /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > /* 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/. */ > #include "nsCOMPtr.h" > -#include "nsPrintOptionsWin.h" > +#include "nsPrintSettingsServiceWin.h" nit: up to first include as well
Attachment #8942395 - Flags: review?(bobowencode) → review+
Comment on attachment 8942396 [details] Bug 144114 part 2 - Rename nsPrintOptions and its subclasses to nsPrintSettingsService*. https://reviewboard.mozilla.org/r/212698/#review218404 ::: widget/android/nsPrintSettingsServiceAndroid.h:5 (Diff revision 1) > /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > /* 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 nsPrintOptionsAndroid_h__ > +#ifndef nsPrintSettingsServiceAndroid_h__ nit: no trailing underscores (same for other files) ::: widget/android/nsPrintSettingsServiceAndroid.h:15 (Diff revision 1) > -//***************************************************************************** > -class nsPrintOptionsAndroid : public nsPrintOptions > { > public: > - nsPrintOptionsAndroid(); > - virtual ~nsPrintOptionsAndroid(); > + nsPrintSettingsServiceAndroid() {} > + virtual ~nsPrintSettingsServiceAndroid() {} nit: I suppose we should lose the virtual here and elsewhere if we're making the class final. (The CppCoreGuidelines suggest not using override or final either on destructors even on non-final classes, but it doesn't give a reason. They also seem to be generally against final, but the main argument for this seems a poor one to me, discouraging extension through inheritance of base classes (as opposed to pure interfaces) seems like a good thing to me.) ::: widget/android/nsPrintSettingsServiceAndroid.h:20 (Diff revision 1) > - virtual ~nsPrintOptionsAndroid(); > + virtual ~nsPrintSettingsServiceAndroid() {} > > nsresult _CreatePrintSettings(nsIPrintSettings** _retval) override; > }; > > -#endif /* nsPrintOptionsAndroid_h__ */ > +#endif /* nsPrintSettingsServiceAndroid_h__ */ nit: // style comment (and elsewhere) (I only say this because the style guide suggests that there are optimizations that expect an exact form and I don't know how picky it is.) ::: widget/nsPrintSettingsService.cpp:1309 (Diff revision 1) > }; > Tester::Tester() > { > nsCOMPtr<nsIPrintSettings> ps; > nsresult rv; > - nsCOMPtr<nsIPrintOptions> printService = > + nsCOMPtr<nsIPrintSettingsService> printService = I guess we should delete this, nsIPrintOptions doesn't even exist anymore. Actually we should probably just delete all DEBUG_rods* code I think, I can't see anyone using it and most of it is probably broken.
Attachment #8942396 - Flags: review?(bobowencode) → review+
Comment on attachment 8942397 [details] Bug 144114 part 3 - Clean up the headers of the nsPrintSettingsService subclasses. https://reviewboard.mozilla.org/r/212700/#review218408 ::: widget/nsPrintSettingsService.h:75 (Diff revision 1) > * method WritePrefs > * @param aPS a pointer to the printer settings > * @param aPrinterName the name of the printer for which to write prefs > * @param aFlags flag specifying which prefs to read > */ > virtual nsresult WritePrefs(nsIPrintSettings* aPS, const nsAString& aPrefName, Perhaps this is the patch in which to s/aPrefName /aPrinterName/. Also the wrapping on ReadPrefs is odd. ::: widget/windows/nsPrintSettingsServiceWin.h:29 (Diff revision 1) > - mozilla::embedding::PrintData* data); > + mozilla::embedding::PrintData* data) override; > + > NS_IMETHODIMP DeserializeToPrintSettings(const mozilla::embedding::PrintData& data, > - nsIPrintSettings* settings); > + nsIPrintSettings* settings) override; > > - virtual nsresult _CreatePrintSettings(nsIPrintSettings **_retval); > + nsresult _CreatePrintSettings(nsIPrintSettings **_retval) override; nit: ** to the left
Attachment #8942397 - Flags: review?(bobowencode) → review+
Comment on attachment 8942398 [details] Bug 144114 part 4 - Remove NS_PRINTOPTIONS_CID. https://reviewboard.mozilla.org/r/212702/#review218410
Attachment #8942398 - Flags: review?(bobowencode) → review+
(In reply to Bob Owen (:bobowen) from comment #8) > Actually we should probably just delete all DEBUG_rods* code I think, I > can't see anyone using it and most of it is probably broken. I addressed all your comments except I didn't remove any DEBUG_rods code. I haven't looked at it, but I half had in mind that it might be worth taking a look at whether it would be worth resurrecting in the form of log module code. (Possibly not.)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: