Closed
Bug 1420171
Opened 6 years ago
Closed 6 years ago
Don't get the default printer on Linux if we're printing to a file
Categories
(Core :: Printing: Setup, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla59
People
(Reporter: bobowen, Assigned: bobowen)
References
Details
Attachments
(3 files, 1 obsolete file)
1.90 KB,
patch
|
karlt
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.56 KB,
patch
|
mconley
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.11 KB,
patch
|
karlt
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
This allows the web extension saveAsPDF function to work properly.
Attachment #8931654 -
Flags: review?(karlt)
Assignee | ||
Comment 2•6 years ago
|
||
Noticed these while testing the logging.
Assignee | ||
Updated•6 years ago
|
Attachment #8931655 -
Flags: review?(karlt)
Assignee | ||
Comment 3•6 years ago
|
||
The is a pretty simple fix for web extension saveAsPDF function, we should try and get it into Fx58.
status-firefox57:
--- → affected
status-firefox58:
--- → affected
status-firefox59:
--- → affected
tracking-firefox58:
--- → ?
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)
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #8931708 -
Flags: review?(mconley)
Comment 6•6 years ago
|
||
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+
Assignee | ||
Comment 7•6 years ago
|
||
(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)
Assignee | ||
Comment 9•6 years ago
|
||
(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 10•6 years ago
|
||
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 11•6 years ago
|
||
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+
Assignee | ||
Comment 12•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Attachment #8931654 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #8932034 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 13•6 years ago
|
||
Thanks Karl. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a10ab196a44e66c9382007f621d72fe60f787fc8
Comment 14•6 years ago
|
||
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
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d602eb4467cd https://hg.mozilla.org/mozilla-central/rev/8c4514cb4120 https://hg.mozilla.org/mozilla-central/rev/85b0368a8c80
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 16•6 years ago
|
||
Please request uplift to beta when you get a chance. Don't feel like I need to track this.
Assignee | ||
Comment 17•6 years ago
|
||
(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. :-)
Assignee | ||
Comment 18•6 years ago
|
||
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?
Assignee | ||
Comment 19•6 years ago
|
||
Comment on attachment 8931655 [details] [diff] [review] Part 2: Remove some unused members from nsDeviceContextSpecG See comment 18.
Attachment #8931655 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 20•6 years ago
|
||
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 21•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8931708 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•6 years ago
|
Attachment #8932034 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 22•6 years ago
|
||
bugherder uplift |
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
Updated•6 years ago
|
Flags: qe-verify+
Comment 23•6 years ago
|
||
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)
Assignee | ||
Comment 24•6 years ago
|
||
(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)
Comment 25•6 years ago
|
||
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.
Description
•