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

VERIFIED FIXED in Firefox 58

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

53 Branch
mozilla59
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58- verified, firefox59 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
This allows the web extension saveAsPDF function to work properly.
Attachment #8931654 - Flags: review?(karlt)
Noticed these while testing the logging.
Attachment #8931655 - Flags: review?(karlt)
The is a pretty simple fix for web extension saveAsPDF function, we should try and get it into Fx58.
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?
Flags: needinfo?(bobowencode)
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!
Attachment #8931708 - Flags: review?(mconley) → review+
(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.
Flags: needinfo?(bobowencode)
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.
Flags: needinfo?(bobowencode)
(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.
Flags: needinfo?(bobowencode)
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.
Attachment #8931654 - Flags: review?(karlt) → review-
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.
Attachment #8931655 - Flags: review?(karlt) → review+
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.
Attachment #8932034 - Flags: review?(karlt)
Attachment #8931654 - Attachment is obsolete: true
Attachment #8932034 - Flags: review?(karlt) → review+
Pushed by bobowencode@gmail.com:
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
Please request uplift to beta when you get a chance.  Don't feel like I need to track this.
(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 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
Attachment #8932034 - Flags: approval-mozilla-beta?
Comment on attachment 8931655 [details] [diff] [review]
Part 2: Remove some unused members from nsDeviceContextSpecG

See comment 18.
Attachment #8931655 - Flags: approval-mozilla-beta?
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.
Attachment #8931708 - Flags: approval-mozilla-beta?
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+.
Attachment #8931655 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8931708 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8932034 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Flags: needinfo?(bobowencode)
(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.
Flags: needinfo?(bobowencode)
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.