Closed Bug 1401899 Opened 3 years ago Closed 5 months ago

[e10s] Print doesn't use the corresponded print preference on macOS.

Categories

(Toolkit :: Printing, enhancement, P3)

enhancement

Tracking

()

RESOLVED DUPLICATE of bug 117233

People

(Reporter: mantaroh, Assigned: mantaroh)

Details

Attachments

(3 files, 1 obsolete file)

Currently, Gecko will not store printer settings into corresponded preference on macOS.

In macOS, gecko will load print settings from global printer settings(i.e. print.print_**. '**' is preference name like print.print_footerleft) since nsPrintOptionsX doesn't implement the nsIPrinterEnumerator. So gecko can't get the printer name.[1]

However gecko will store the print setting into printer preference when we push the 'OK' button of native print dialog. In this case, gecko will use printer name which gets from native print dialog. So gecko store this print option per printer. (i.e. print.<printer_name>.print_**) [2]

As result, Gecko doesn't carry the print settings from previous job.

I think that some bugs related to this bug.

[1] https://dxr.mozilla.org/mozilla-central/rev/319a34bea9e4f3459886b5b9e835bd338320f1fd/widget/nsPrintOptionsImpl.cpp#988
[2] https://dxr.mozilla.org/mozilla-central/rev/319a34bea9e4f3459886b5b9e835bd338320f1fd/widget/cocoa/nsPrintOptionsX.mm#75-78

Step To Reproduce:
 1) Open Print dialog
 2) Change print settings. (e.g. footer center to 'Title')
 3) Print or Save as PDF
 4) Re-Open Print dialog

Actually Result:
 Print settings is default, and generated print settings per printer(print.<printer_name>.print).
Hi mantaroh, sorry it took me so long to get to this!

I'm not sure I fully understand, so maybe you can clear some things up for me:

1) Based on my read of the code, and your comment 0, it sounds like we're storing printing preferences keyed on printer names. Is that correct?

2) It also sounds like when we open up the print dialog, we're either not restoring those original settings from prefs, or we have the wrong printer selected. Is that correct?

Is my conception of the problem more or less accurate?
Flags: needinfo?(mantaroh)
Comment on attachment 8911033 [details]
Bug 1401899 - Don't use the printer name which gets from NSPrintInfo for storing the printer settings on macOS.

https://reviewboard.mozilla.org/r/182514/#review191062

Clearing review until I get some questions cleared up.
Attachment #8911033 - Flags: review?(mconley)
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews from comment #2)
> Hi mantaroh, sorry it took me so long to get to this!
> 

Mike, 
Thank you for confirming it.


> I'm not sure I fully understand, so maybe you can clear some things up for
> me:
> 
> 1) Based on my read of the code, and your comment 0, it sounds like we're
> storing printing preferences keyed on printer names. Is that correct?
> 

Yes, current code store printing preferences per printer. This printer name is propagated via NSPrintInfo when serializing printer settings after closing print dialog, and gecko use this printer name for storing preferences. 

This preference which related to printer name is introduced by bug 128142. And we should implemet nsIPrinterEnumerator on macOS, however we doesn't implement it yet (Bug 117233). So I think that we should use global print setting on macOS.

> 2) It also sounds like when we open up the print dialog, we're either not
> restoring those original settings from prefs, or we have the wrong printer
> selected. Is that correct?
> 

Yes, when we open the print dialog, nsPrintOptionImpl restoring global print preferences(i.e. print.printer_** key), not related to printer name(i.e. print.<printer_name>.printer_** key). In this dialog, selected printer is depended on macOS's printers&scanner preference. (e.g. If user set 'Default Printer' to 'Last printer used', selected printer is last used printer.)
Flags: needinfo?(mantaroh)
I see. It seems then that maybe the better solution is to try to (finally!) fix bug 117233 instead of wallpapering over it. Would you be willing to try that?
Flags: needinfo?(mantaroh)
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews from comment #5)
> I see. It seems then that maybe the better solution is to try to (finally!)
> fix bug 117233 instead of wallpapering over it. Would you be willing to try
> that?

I investigated what we need for implementing this feature:
 * Implementation of nsIPrinterEnumerator
   * we can use PMServerCreatePrinterList or PMSessionCreatePrinterList for getting printer lists. [1][2]
     I'm prefer PMSessionCreatePrinterList since we can get current printer(i.e. default printer).
   * run this service on main process only, maybe we can't use this code on content process.
 * Implementation of reading/storing print preference per printer.
   * nsIPrintSettingsService use printer name for reading/storing print preferences if we can get the default printer name via nsIPrinterEnumerator[3][4][5]. So I guess that we don't need to change around this mechanism.

[1] https://developer.apple.com/documentation/applicationservices/1463247-pmcreatesession?language=objc
[2] https://developer.apple.com/documentation/applicationservices/1460119-pmsessioncreateprinterlist?language=objc
[3] http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/widget/nsPrintOptionsImpl.cpp#1153,1195
[4] http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/widget/nsPrintOptionsImpl.cpp#1108
[5] http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/widget/nsPrintOptionsImpl.cpp#1018-1052
Note to self.
 * nsIPrinterEnumerator::GetDefaultPrinterName should use PMPSessionCreatePrinterList() in order to distinguish the selected printer(i.e. default printer).
 * We might create PMPrintSession during this GetDefaultPrinterName function. I.e. We don't need have the PMPrintSession as class member.
 * nsIPrinterEnumerator::GetPrinterNameList should use PMServerCreatePrinterList() since we don't need get the default printer.
 * nsIPrinterEnumerator::InitPrintSettingsFromPrinter should set PMPrintSettings and PMPageFormat.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #6)
> (In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews from
> comment #5)
> > I see. It seems then that maybe the better solution is to try to (finally!)
> > fix bug 117233 instead of wallpapering over it. Would you be willing to try
> > that?
> 
> I investigated what we need for implementing this feature:
>  * Implementation of nsIPrinterEnumerator
>    * we can use PMServerCreatePrinterList or PMSessionCreatePrinterList for
> getting printer lists. [1][2]
>      I'm prefer PMSessionCreatePrinterList since we can get current
> printer(i.e. default printer).
>    * run this service on main process only, maybe we can't use this code on
> content process.
>  * Implementation of reading/storing print preference per printer.
>    * nsIPrintSettingsService use printer name for reading/storing print
> preferences if we can get the default printer name via
> nsIPrinterEnumerator[3][4][5]. So I guess that we don't need to change
> around this mechanism.
> 
> [1]
> https://developer.apple.com/documentation/applicationservices/1463247-
> pmcreatesession?language=objc
> [2]
> https://developer.apple.com/documentation/applicationservices/1460119-
> pmsessioncreateprinterlist?language=objc
> [3]
> http://searchfox.org/mozilla-central/rev/
> 7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/widget/nsPrintOptionsImpl.cpp#1153,
> 1195
> [4]
> http://searchfox.org/mozilla-central/rev/
> 7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/widget/nsPrintOptionsImpl.cpp#1108
> [5]
> http://searchfox.org/mozilla-central/rev/
> 7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/widget/nsPrintOptionsImpl.cpp#1018-
> 1052

Furthermore..

The page setup preference (e.g. scaling / paper size / orientation) save to preferences as binary data[1].
So these preferences of each printer doesn't affect print (see 1346649), maybe we should store these settings to each printer's preferences.
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews from comment #5)
> I see. It seems then that maybe the better solution is to try to (finally!)
> fix bug 117233 instead of wallpapering over it. Would you be willing to try
> that?

OK. I've implemented the prototype for this, and clarify the problems(comment 6 / comment 7 / comment 8).

I think I need a little time for implementing it, So I want to choose the workaround until landed this 'each printer preferences' implementation. What do you think?
Flags: needinfo?(mantaroh) → needinfo?(mconley)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #9)
> I think I need a little time for implementing it, So I want to choose the
> workaround until landed this 'each printer preferences' implementation. What
> do you think?

Hey mantaroh,

How much time do you need? If the scale of time is "days", then we should probably just take that instead of landing the workaround. If the scale of time is "months", then we should probably land this workaround.

So do you have a sense of how long it'll take?
Flags: needinfo?(mconley) → needinfo?(mantaroh)
Priority: -- → P3
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #10)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #9)
> > I think I need a little time for implementing it, So I want to choose the
> > workaround until landed this 'each printer preferences' implementation. What
> > do you think?
> 
> Hey mantaroh,
> 
> How much time do you need? If the scale of time is "days", then we should
> probably just take that instead of landing the workaround. If the scale of
> time is "months", then we should probably land this workaround.
> 
> So do you have a sense of how long it'll take?

OK. Maybe it will take few days, I think.
So I'll skip this workaround, and I try to implement it.
Thanks.
Flags: needinfo?(mantaroh)
Attachment #8911033 - Attachment is obsolete: true
Assignee: nobody → mantaroh
Current (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #13)
> Created attachment 8920939 [details] [diff] [review]
> WIP: Part2. Implement printerEnumeratorService for macOS

This patch will not apply creating printer settings from real printer information(i.e. not implemented InitPrintSettingsFromPrinter) yet.

Note to self:
Before implement this bug, We need to stop overwrite the printer name from NSPrintInfo on content process.[1]
Now, we have 2 nsIPrintSettings (i.e. parent / content processes). As Result of it, we don't sent corresponded printer name from content process to parent process even if we implement printer enumerator.

[1] http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/widget/cocoa/nsPrintOptionsX.mm#75-78

Erik, can you check if your patches for bug 117233 fixed this bug also, or if we have further work to do?

Flags: needinfo?(enordin)

The functionality described in this bug appears to work correctly both in the current nightly 78.0a1 (2020-05-28) and in my patch.

Step To Reproduce

  1. Open Print dialog
  2. Change print settings. (e.g. footer center to 'Title')
  3. Print or Save as PDF
  4. Re-Open Print dialog

Expected Results
The print dialog retains the setting from the previous print.

Actual Results
Same as expected

Flags: needinfo?(enordin)
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 117233
You need to log in before you can comment on or make changes to this bug.