1420171 - Don't get the default printer on Linux if we're printing to a file

Status: VERIFIED FIXED

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

Description

[Bob Owen (:bobowen)]

Bug 1329216 moved getting the default printer to the parent because of sandboxing.

It does this whenever the name in the settings is blank.
It is legitimate for the name to be blank if printing to a file, so we should check for that in the settings.

Comment 1

[Bob Owen (:bobowen)]

Attachment: Part 1: Only default the print-to file name on Linux if not already set

This allows the web extension saveAsPDF function to work properly.

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 2: Remove some unused members from nsDeviceContextSpecG

Noticed these while testing the logging.

Comment 3

[Bob Owen (:bobowen)]

The is a pretty simple fix for web extension saveAsPDF function, we should try and get it into Fx58.

Comment 4

[dw-dev]

Bob,

I just want to clarify how this change will work for tabs.saveAsPDF().


My understanding is that this change:

- prevents 'printSettings.toFileName' being reset to an empty string, if it is already set to a non-empty string.

- does not prevent 'printSettings.isInitializedFromPrinter' from being reset in line 146 of PrintingParent.cpp.


I propose in tabs.saveAsPDF() to:

- set printSettings.printerName = "";  (I assume this setting is ignored for print to file)
  
- set printSettings.isInitializedFromPrefs = true;  (to prevent the settings being re-initialized from preferences)

- not set printSettings.isInitializedFromPrinter  (since it will be reset in line 146 of PrintingParent.cpp)


Are these settings okay?

Comment 5

[Bob Owen (:bobowen)]

Attachment: Part 3: Don't default the settings printer name on Linux when printing to file

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8931708 [details] [diff] [review]
Part 3: Don't default the settings printer name on Linux when printing to file

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

Thanks!

Comment 7

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #4)
> Bob,
> 
> I just want to clarify how this change will work for tabs.saveAsPDF().
> 
> 
> My understanding is that this change:
> 
> - prevents 'printSettings.toFileName' being reset to an empty string, if it
> is already set to a non-empty string.

Not quite, it now won't overwrite with the default when the print-to name is already set.

> - does not prevent 'printSettings.isInitializedFromPrinter' from being reset
> in line 146 of PrintingParent.cpp.

I was going to leave this for a follow-up, but I think you're right we should do it now, so I've added another patch.
Oh look, mconley has already r+ it, thanks. :-)

> I propose in tabs.saveAsPDF() to:
> 
> - set printSettings.printerName = "";  (I assume this setting is ignored for
> print to file)
>   
> - set printSettings.isInitializedFromPrefs = true;  (to prevent the settings
> being re-initialized from preferences)
> 
> - not set printSettings.isInitializedFromPrinter  (since it will be reset in
> line 146 of PrintingParent.cpp)

You can set isInitializedFromPrinter now as well if you like.

Comment 8

[dw-dev]

Bob,

With Part 3 now added, does this means that it is okay for tabs.savePDF() to set the printerName to a null string?

Assuming that is the case, I will go ahead and submit the patch for Bug 1415507.

Comment 9

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #8)
> Bob,
> 
> With Part 3 now added, does this means that it is okay for tabs.savePDF() to
> set the printerName to a null string?
> 
> Assuming that is the case, I will go ahead and submit the patch for Bug
> 1415507.

Yes, I think that's fine or just not set it at all, which is what happens now I think.
Happy to test on Linux, Mac and Windows on Monday with the changes here and your patches.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8931654 [details] [diff] [review]
Part 1: Only default the print-to file name on Linux if not already set

>-  aPrintSettings->SetToFileName(NS_ConvertUTF8toUTF16(filename));
>+      filename.AssignASCII(path);

Please don't change the charset used to decode PWD, at least not without an
explanation.

Assuming UTF-8 may not be quite right on all systems, but it will at least
be reasonably good on systems using UTF-8 charmaps.

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8931655 [details] [diff] [review]
Part 2: Remove some unused members from nsDeviceContextSpecG

Thank you for cleaning up.  This printing code is a maze, even without deceptive tricks like this.

Comment 12

[Bob Owen (:bobowen)]

Attachment: Part 1: Only default the print-to file name on Linux if not already set

This allows the web extension saveAsPDF function to work properly.

(In reply to Karl Tomlinson (:karlt) from comment #10)
...
> >-  aPrintSettings->SetToFileName(NS_ConvertUTF8toUTF16(filename));
> >+      filename.AssignASCII(path);
> 
> Please don't change the charset used to decode PWD, at least not without an
> explanation.

Whoops, fixed thanks.

Comment 13

[Bob Owen (:bobowen)]

Thanks Karl.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a10ab196a44e66c9382007f621d72fe60f787fc8

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d602eb4467cd
Part 1: Only default the print-to file name on Linux if not already set. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c4514cb4120
Part 2: Remove some unused members from nsDeviceContextSpecG. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/85b0368a8c80
Part 3: Don't default the settings printer name on Linux when printing to file. r=mconley

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/d602eb4467cd
https://hg.mozilla.org/mozilla-central/rev/8c4514cb4120
https://hg.mozilla.org/mozilla-central/rev/85b0368a8c80

Comment 16

[Julien Cristau [:jcristau]]

Please request uplift to beta when you get a chance.  Don't feel like I need to track this.

Comment 17

[Bob Owen (:bobowen)]

(In reply to Julien Cristau [:jcristau] from comment #16)
> Please request uplift to beta when you get a chance.  Don't feel like I need
> to track this.

Will do, going to let it bake for a couple of days because this is printing. :-)

Comment 18

[Bob Owen (:bobowen)]

Comment on attachment 8932034 [details] [diff] [review]
Part 1: Only default the print-to file name on Linux if not already set

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1329216

[User impact if declined]:
Extensions using saveAsPDF function don't save to the user chosen file.

[Is this code covered by automated tests?]:
Not currently, but bug 1415507 is close to landing a test for the saveAsPDF

[Has the fix been verified in Nightly?]:
Tested with Print Edit extension on a local build.

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes, manual testing would be good.
saveAsPDF can be tested via Print Edit web extension, it would be good to test normal Linux save to PDF as well.

[List of other uplifts needed for the feature/fix]:
Other two parts from this bug.

[Is the change risky?]:
Fairly low risk.

[Why is the change risky/not risky?]:
Very simple changes to not override the given file name with the default one.
Also, to not set the default printer name if printing to a file.

[String changes made/needed]:
None

Comment 19

[Bob Owen (:bobowen)]

Comment on attachment 8931655 [details] [diff] [review]
Part 2: Remove some unused members from nsDeviceContextSpecG

See comment 18.

Comment 20

[Bob Owen (:bobowen)]

Comment on attachment 8931708 [details] [diff] [review]
Part 3: Don't default the settings printer name on Linux when printing to file

See comment 18.

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8931655 [details] [diff] [review]
Part 2: Remove some unused members from nsDeviceContextSpecG

Fix a printing issue for extensions using saveAsPDF function. Beta58+.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b7d6ba8ab96b
https://hg.mozilla.org/releases/mozilla-beta/rev/072ce042c1d8
https://hg.mozilla.org/releases/mozilla-beta/rev/a447794c0dd0

Comment 23

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I tried to reproduced this issue using Firefox 59.0a1 (ID=20171123100420) on Ubuntu 14.04 x64.
I followed the above steps to reproduced it:
 1. Install add-on : https://addons.mozilla.org/en-US/firefox/addon/print-edit-we/
 2. Access a pdf file (ex. http://www23.statcan.gc.ca/imdb-bmdi/pub/instrument/3901_Q2_V3-eng.pdf#page=1&zoom=auto,-14,792) and edit with the "Print Edit WE"
 3.Click on Save as PDF and Save
 4.The pdf document was saved in Downloads folder with 0 byte size
I done the same steps using Firefox 58.0b10 and the pdf file was saved correctly.
If I am wrong with steps, please help me.

Comment 24

[Bob Owen (:bobowen)]

(In reply to Timea Zsoldos from comment #23)
> I tried to reproduced this issue using Firefox 59.0a1 (ID=20171123100420) on
> Ubuntu 14.04 x64.
> I followed the above steps to reproduced it:
>  1. Install add-on :
> https://addons.mozilla.org/en-US/firefox/addon/print-edit-we/
>  2. Access a pdf file (ex.
> http://www23.statcan.gc.ca/imdb-bmdi/pub/instrument/3901_Q2_V3-eng.
> pdf#page=1&zoom=auto,-14,792) and edit with the "Print Edit WE"
>  3.Click on Save as PDF and Save
>  4.The pdf document was saved in Downloads folder with 0 byte size
> I done the same steps using Firefox 58.0b10 and the pdf file was saved
> correctly.
> If I am wrong with steps, please help me.

It sounds like you are confirming that it is fixed, yes?
In "4." above you should find that it has downloaded the pdf, but it is in your home dir (or possibly the dir you launched Firefox from) named mozilla.pdf.

Comment 25

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

Based on comment 24, I have reproduced this issue using Firefox 59.0a1 (build ID=20171123100420) on Ubuntu 14.04 x64.
I can confirm this issue is fixed, I verified using Firefox 58.0b11 and nightly 59.0a1 (2017.12.12) on Ubuntu 16.04 x64.