Closed Bug 1136855 Opened 9 years ago Closed 9 years ago

Print settings are not saved from print job to print job

Categories

(Toolkit :: Printing, defect)

36 Branch
x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla39
Iteration:
39.1 - 9 Mar
Tracking Status
firefox36 - verified
firefox37 - verified
firefox38 - verified
firefox39 - verified
relnote-firefox --- 36+

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

STR:

1) View an HTML page in Firefox
2) Go to File > Print to print the document
3) Set some print settings, like print in Landscape, or scaling settings
4) Print the page
5) Go to File > Print to print the document again

ER:

The print settings, by default, should be persisted from print job to print job.

Unfortunately, this regression just shipped in 36. 

AR:

Print settings are reset from print job to print job.

I caused this with bug 1082575, I'm afraid.
Flags: firefox-backlog+
Just to give credit where credit's due, dw-dev noticed this in bug 1088070 comment 8: https://bugzilla.mozilla.org/show_bug.cgi?id=1088070#c8.
Comment on attachment 8569406 [details] [diff] [review]
Send a message from the content script when printing has finished so the parent can save print settings. r=?

So this fixes things for the single-process case, and should also apply to Aurora and Beta without too much difficulty.

The multi-process case is still busted (browser-content.js nulls out the printSettings CPOW in the multi-process case), but that's something I think we can solve in a separate bug.

I want to write a regression test for this as well, if I can. I'll do that in a separate patch.
Attachment #8569406 - Flags: review?(dtownsend)
[Tracking Requested - why for this release]:

This is a regression in 36+ that prevents print settings from being saved from print job to print job. I highly doubt that's important enough to spin a point release for, but if a point release ends up going out, it might be a nice additional fix to go in.
Assignee: nobody → mconley
Comment on attachment 8569406 [details] [diff] [review]
Send a message from the content script when printing has finished so the parent can save print settings. r=?

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

::: toolkit/content/browser-content.js
@@ +465,5 @@
> +      print.print(printSettings, null);
> +    } catch(e) {
> +      // Pressing cancel is expressed as an NS_ERROR_ABORT return value,
> +      // causing an exception to be thrown which we catch here.
> +      rv = e;

e will be an exception object here, you probably want e.result.
Attachment #8569406 - Flags: review?(dtownsend) → review+
Thanks! Good catch on the e.result.

Pushed:

remote:   https://hg.mozilla.org/integration/fx-team/rev/92c65f60614a

Marking leave-open, since I plan on writing a regression test.
Keywords: leave-open
Whiteboard: [fixed-in-fx-team]
Tracking this since it's a recent regression and we may want to uplift the fix (and the test you're adding)
This is what landed on central, after I fixed Mossop's issue.
Attachment #8569406 - Attachment is obsolete: true
Attachment #8569942 - Flags: review+
Comment on attachment 8569942 [details] [diff] [review]
Send a message from the content script when printing has finished so the parent can save print settings. r=Mossop

Approval Request Comment
[Feature/regressing bug #]:

Regressed by bug 1082575.

[User impact if declined]:

Print settings will not be saved from print job to print job.

[Describe test coverage new/current, TreeHerder]:

Landed on central, having passed all automated tests. Manually tested, and now writing an automated test for this particular case.

[Risks and why]: 

I don't think this adds much risk, to be honest.

[String/UUID change made/needed]:

None.
Attachment #8569942 - Flags: approval-mozilla-beta?
Attachment #8569942 - Flags: approval-mozilla-aurora?
Hey Sylvestre - I just heard through the grapevine that we're spinning a point release for 36. How do I go about nominating this bug to ride along with that point release?
Flags: needinfo?(sledru)
[Tracking Requested - why for this release]:

This is a regression in 36+ that prevents print settings from being saved from print job to print job. I heard that we're spinning a point release - this might be nice to include in there.
Comment on attachment 8569942 [details] [diff] [review]
Send a message from the content script when printing has finished so the parent can save print settings. r=Mossop

Approval Request Comment

See comment 10.
Attachment #8569942 - Flags: approval-mozilla-release?
Comment on attachment 8569942 [details] [diff] [review]
Send a message from the content script when printing has finished so the parent can save print settings. r=Mossop

For sure, we want that fixed in 37 & 38.
Let's discuss a bit more about 36.
Flags: needinfo?(sledru)
Attachment #8569942 - Flags: approval-mozilla-beta?
Attachment #8569942 - Flags: approval-mozilla-beta+
Attachment #8569942 - Flags: approval-mozilla-aurora?
Attachment #8569942 - Flags: approval-mozilla-aurora+
So writing an automated test is not going to be nearly as easy as I thought. In order for the settings to be saved, the user needs to "OK" the print settings dialog and actually kick off the print.

I can successfully mock out the nsIPrintingPromptService and set some settings in a test, but the problem is that I can't mock out nsIDeviceContextSpec, which is what Gecko will print to. Specifically, nsIDeviceContextSpec has a method called GetSurfaceForPrinter which returns a gfxASurface, which is not something I can mock out in Javascript. :(

I would like to revisit writing automated tests for this stuff, and finding a way around this, but I've got bigger fish to fry, I'm afraid.
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Iteration: --- → 39.1 - 9 Mar
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
Potentially part of the 36.0.1 release, tracking.
Comment on attachment 8569942 [details] [diff] [review]
Send a message from the content script when printing has finished so the parent can save print settings. r=Mossop

Mike confirmed on IRC that the patch is low risk. Taking it.
Attachment #8569942 - Flags: approval-mozilla-release? → approval-mozilla-release+
You pushed this to the MOBILE360_2015022513_RELBRANCH, which was not what you wanted to do. Backed out and landed on the correct branch.

https://hg.mozilla.org/releases/mozilla-release/rev/9f425111aaf1
https://hg.mozilla.org/releases/mozilla-release/rev/5717cc7f29c7
Huh. For next time, how do I avoid doing that? I just pulled from mozilla-release and applied on top of that.
Flags: needinfo?(ryanvm)
And thanks, btw. :)
We discussed on IRC. The issue is that the tip rev wasn't on default, so you need to explicitly update to it first.
Flags: needinfo?(ryanvm)
I verified this issue and this is what I have noticed:

1. The tab crashed using Firefox 39.0a1 (2015-03-04) with e10s On Windows 7 x64. Crash ID: 4ba319ca-e1ee-462d-9178-a1dcc2150303 .

2. "e10s printing is not implemented yet.Bug 927188" message is displayed using Firefox 39.0a1 (2015-03-04) with e10s on Ubuntu 14.04 x32.

3. Only "Orientation" setting is persisted from print job to print job using Firefox 39.0a1 (2015-03-04) without e10s, Firefox 38.0a2 (2015-03-04) and Firefox 37 Beta 2 (20150302192546) on Windows 7 x64.

4. Only the "orientation" and the "paper size" settings are persisted using Firefox 39.0a1 (2015-03-04) without e10s, Firefox 38.0a2 (2015-03-04) and Firefox 37 Beta 2 (20150302192546) on Ubuntu 14.04 x32.

5. Print settings are saved from print job to print job using Firefox 39.0a1 (2015-03-04) with/without e10s, Firefox 38.0a2 (2015-03-04) and Firefox 37 Beta 2 (20150302192546) on Mac 10.9.5.


Based on the above mentions should I reopen this bug or file separate ones?
Flags: needinfo?(mconley)
Hey Vasilica,

Thanks for testing this!

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #25)
> 1. The tab crashed using Firefox 39.0a1 (2015-03-04) with e10s On Windows 7
> x64. Crash ID: 4ba319ca-e1ee-462d-9178-a1dcc2150303 .
> 

This should get a new bug filed for it.

> 2. "e10s printing is not implemented yet.Bug 927188" message is displayed
> using Firefox 39.0a1 (2015-03-04) with e10s on Ubuntu 14.04 x32.
> 

Known, and blocked on bug 1090448.

> 3. Only "Orientation" setting is persisted from print job to print job using
> Firefox 39.0a1 (2015-03-04) without e10s, Firefox 38.0a2 (2015-03-04) and
> Firefox 37 Beta 2 (20150302192546) on Windows 7 x64.
> 

Is this also true for Firefox 36?

> 4. Only the "orientation" and the "paper size" settings are persisted using
> Firefox 39.0a1 (2015-03-04) without e10s, Firefox 38.0a2 (2015-03-04) and
> Firefox 37 Beta 2 (20150302192546) on Ubuntu 14.04 x32.
> 

Same as above - true for Firefox 36 as well?

> Based on the above mentions should I reopen this bug or file separate ones?

The only "known" in the group you just listed is 2. New bugs should be filed for the other issues you found.
Flags: needinfo?(mconley) → needinfo?(vasilica.mihasca)
I think here we should verify on a current 36 build from https://hg.mozilla.org/releases/mozilla-release.
Keywords: qawanted
Added to the release notes with "36.0.1 - Printing preferences could be lost (1136855)"
> This should get a new bug filed for it.

For this Nightly crash I have commented in Bug 1019063.

> Is this also true for Firefox 36?

The same Print behavior persists on Firefox 36.0.1 using Windows 7 x64 and Ubuntu 14.04 x32, only Paper Size, Scale and Orientation settings are kept (exception: This 2 options from Orientation are not persisted: Rotated Landscape and Rotated Portrait)   

> The only "known" in the group you just listed is 2. New bugs should be filed
> for the other issues you found.

Before filling new bugs for the remaining issues, I want to make sure which are the expected results:
 
Which settings from Print should persist from print job to print job? All of them? 
For example, the Page Order, the Pages per Sheet or Number of Copies settings are not persistent from print job to print job. These settings should also be saved?
Flags: needinfo?(vasilica.mihasca) → needinfo?(mconley)
Keywords: qawanted
QA Contact: vasilica.mihasca
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #29)
> Before filling new bugs for the remaining issues, I want to make sure which
> are the expected results:
>  
> Which settings from Print should persist from print job to print job? All of
> them? 
> For example, the Page Order, the Pages per Sheet or Number of Copies
> settings are not persistent from print job to print job. These settings
> should also be saved?

At this point, I'd be happy with parity with Firefox 35 (which was before any of the e10s printing work was done).

Unfortunately, we don't have much in the way of automated tests for printing, so it's not easy for me to tell which settings are supposed to be persisted. It would not surprise me if it's not all of them - I suspect our generic print settings infrastructure is somewhat "leaky" from platform to platform.

Would it be possible for you to do a comparison with Firefox 35 to see if there is anything new that we're not persisting? Any regressions from 35 would probably be highest priority. Anything that 35 didn't already persist should probably also be filed, but are lesser priority, since we've lived with it for this long.

Does that make sense?
Flags: needinfo?(mconley) → needinfo?(vasilica.mihasca)
I made a comparison between Firefox 35 (20150108202552) and Firefox 36.0.1 (20150305021524) using Windows 7 x64 and Ubuntu 14.04 x32, but I did not encountered new regressions.

Settings that persist in Firefox 35, persist in Firefox 36.0.1 as well.
Flags: needinfo?(vasilica.mihasca)
So it seems we do have parity with Firefox 35, and issues that also show in 35 can be filed separately, and will be lower priority. Vasilica can you please file those?
Flags: needinfo?(vasilica.mihasca)
Mike, this is the behavior of print settings before and after the print (reproduced on Firefox 37 Beta 7 build id:20150319212106 using Windows 7 X64):
 
 - I set this properties : http://i.imgur.com/qTh88cp.jpg
 - After printing: http://i.imgur.com/m1KdZvX.jpg

Should I file a separate bug for these unsaved settings?
Flags: needinfo?(vasilica.mihasca) → needinfo?(mconley)
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #34)
> Mike, this is the behavior of print settings before and after the print
> (reproduced on Firefox 37 Beta 7 build id:20150319212106 using Windows 7
> X64):
>  
>  - I set this properties : http://i.imgur.com/qTh88cp.jpg
>  - After printing: http://i.imgur.com/m1KdZvX.jpg
> 
> Should I file a separate bug for these unsaved settings?

Yes please! Also, if you wouldn't mind, could you also include in that bug whether or not this is reproducible in Firefox 35?
Flags: needinfo?(mconley)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #35)

> Yes please! Also, if you wouldn't mind, could you also include in that bug
> whether or not this is reproducible in Firefox 35?

I log this issue Bug 1146814 and I specified that this bug is also reproducible on Firefox 35.

Closing this bug as the remaining issue is tracked in the followup bug: Bug 1146814.

Thanks Mike for your guidance!
Untracking this for all channels since it's verified fixed.
You need to log in before you can comment on or make changes to this bug.