printToFile is busted on Mac

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
8 years ago
11 months ago

People

(Reporter: jruderman, Assigned: cosmo0920)

Tracking

(Depends on 1 bug, {testcase})

Trunk
mozilla54
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments, 11 obsolete attachments)

193 bytes, text/html
Details
12.57 KB, patch
haik
: review+
Details | Diff | Splinter Review
Reporter

Description

8 years ago
Posted file testcase
1. Install https://www.squarefree.com/extensions/domFuzzLite2.xpi
2. Load the testcase.

Result on Windows and Linux: creates "fuzzout.pdf" in the Firefox profile dir :)

Result on Mac: sends my page to the nearest office printer :(

This bug makes it impossible to test Mac printing code, which is why I didn't catch topcrash bug 665218 with my dom fuzzer and why that bug still has no regression test.

(This is an old bug; the problem was mentioned in bug 299129 comment 0.)
Don't know who's working on printing bugs these days...
This is really OSX widget level stuff.
Let me provisionally assign this to myself.  I don't know when I'll get to it, but it sounds like the kind of stuff I normally work on.
Assignee: nobody → smichaud
Whoever gets around to working on this, please take note of the spring cleaning in bug 629500.  I am making this a dependency of that because follow-up work that I have planned for that bug might wind up fixing this as a side-effect, so I don't want to lose it.  However, please don't take this as a statement of intent to fix this bug, and don't hesitate to drop the dependency if you fix it before that bug lands.
Depends on: 629500
Assignee

Comment 5

2 years ago
Dose anyone work this bug in these days?
I've got succeeded obtaining PDF file with my patch when specified printToFile, toFileName, and printSilent in nsIPrintSettings.
And I want to submit my patch.
Nobody is working on this at the moment. I've assigned it to you.
Assignee: smichaud → cosmo0920.oucc
Assignee

Comment 8

2 years ago
Previous patch didn't call nsPrintSettingsX::InitAdjustedPaperSize() when changing page format. This patch calls it when changing page format in nsDeviceContextSpecX::Init.
Attachment #8825782 - Attachment is obsolete: true
Attachment #8825782 - Flags: review?(mstange)
Attachment #8827044 - Flags: review?(mstange)
Assignee

Comment 9

2 years ago
Add missing converting units.
Inch -> Pixels.
Attachment #8827044 - Attachment is obsolete: true
Attachment #8827044 - Flags: review?(mstange)
Attachment #8827066 - Flags: review?(mstange)
Assignee

Comment 10

2 years ago
Fix wrong units descriptions in comment.
Attachment #8827066 - Attachment is obsolete: true
Attachment #8827066 - Flags: review?(mstange)
Attachment #8827310 - Flags: review?(mstange)
Assignee

Updated

2 years ago
Depends on: 385228
Assignee

Comment 11

2 years ago
Current patch implementation does not use `GetEffectivePageSize()`, which depends on cairo context size, because cairo context too large on macOS.
If we want to use correct internal papar size via `GetEffectivePageSize()`, we should fix "Surface size too large" warnings.
Assignee

Comment 12

2 years ago
Avoid to use hardcorded PPI value.
Attachment #8827708 - Flags: review?(mstange)
Assignee

Updated

2 years ago
Attachment #8827310 - Attachment is obsolete: true
Attachment #8827310 - Flags: review?(mstange)
Assignee

Comment 13

2 years ago
I've found another issue when printing to PDF,.
In macOS, unwriteable margin settings does not apply into Core Printing API.
(Although, traditional macOS printing is not affected by this issue because printing modal dialog's advanced settings covers this feature.)
We also should apply this patch if implementing printToFile feature on macOS.
Attachment #8828646 - Flags: review?(mstange)
Assignee

Comment 14

2 years ago
Attachment #8828646 - Attachment is obsolete: true
Attachment #8828646 - Flags: review?(mstange)
Attachment #8828698 - Flags: review?(mstange)
Assignee

Comment 15

2 years ago
I've cleaned up previous patches implementations.
Attachment #8827708 - Attachment is obsolete: true
Attachment #8828698 - Attachment is obsolete: true
Attachment #8827708 - Flags: review?(mstange)
Attachment #8828698 - Flags: review?(mstange)
Attachment #8828892 - Flags: review?(mstange)
Assignee

Updated

2 years ago
No longer depends on: 385228
Assignee

Comment 16

2 years ago
More cleaning up patch.
Attachment #8828892 - Attachment is obsolete: true
Attachment #8828892 - Flags: review?(mstange)
Attachment #8828911 - Flags: review?(mstange)
Assignee

Comment 17

2 years ago
Attachment #8828911 - Attachment is obsolete: true
Attachment #8828911 - Flags: review?(mstange)
Attachment #8829087 - Flags: review?(mstange)
Attachment #8829087 - Flags: review?(mconley)
Assignee

Comment 18

2 years ago
Attachment #8829087 - Attachment is obsolete: true
Attachment #8829087 - Flags: review?(mstange)
Attachment #8829087 - Flags: review?(mconley)
Attachment #8829088 - Flags: review?(mstange)
Attachment #8829088 - Flags: review?(mconley)
Assignee

Updated

2 years ago
Status: NEW → ASSIGNED
Comment on attachment 8829088 [details] [diff] [review]
set-needed-information-when-printing-into-PDF-file-v9.diff

Hey haik - do you have the cycles to review this?
Attachment #8829088 - Flags: review?(mconley) → review?(haftandilian)
(In reply to Mike Conley (:mconley) from comment #19)
> Comment on attachment 8829088 [details] [diff] [review]
> set-needed-information-when-printing-into-PDF-file-v9.diff
> 
> Hey haik - do you have the cycles to review this?

Sure, I'll get to this by the end of the week.
Awesome - thank you. :)
(In reply to Haik Aftandilian [:haik] from comment #20)
> (In reply to Mike Conley (:mconley) from comment #19)
> > Comment on attachment 8829088 [details] [diff] [review]
> > set-needed-information-when-printing-into-PDF-file-v9.diff
> > 
> > Hey haik - do you have the cycles to review this?
> 
> Sure, I'll get to this by the end of the week.

Sorry, need some more time on this one.
Comment on attachment 8829088 [details] [diff] [review]
set-needed-information-when-printing-into-PDF-file-v9.diff

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

This looks OK to me, but I tried Jesse's attached testcase and it didn't work for me. Does it work for you?

(Keep in mind I'm not very knowledgeable about Mac widget code or Objective-C.)
Attachment #8829088 - Flags: review?(haftandilian) → review+
Assignee

Comment 24

2 years ago
Thanks for reviewing this.
Unfortunately, with Firefox 50.0.1, I did not succeed to work Jesse's attached test case on macOS even if Ubuntu 14.04.5.
Assignee

Comment 25

2 years ago
I've also confirmed that e10s-ized content process does not load extension's components and non-e10s window blocks with privileged or cross-origin callable error now.
The testcase is not working anymore....
Assignee

Comment 26

2 years ago
I've found confirmation step with attached addon:

1. Set xpinstall.signatures.required to false
2. Install attached testcase addon
3. Open non-e10s-ized window with New Non-e10s
4. Open new empty tab
5. appmenu -> Developer -> Toggle Tools
6. Type `fuzzPriv.printToFile(true, false, false, false)` in Console
7. Confirm to be created fuzzout.pdf within profile folder
Assignee

Comment 27

2 years ago
Can I add checkin-needed keyword?
Yes, thanks for looking into that.
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 29

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2114a592360d
Create needed API when printing info PDF on macOS. r=haik
https://hg.mozilla.org/integration/mozilla-inbound/rev/d88c174c2d20
Set needed paper size information when printing info PDF on macOS. r=haik
Keywords: checkin-needed
Sorry, I had to back this out for build bustage on OS X because of use of undeclared identifiers:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9436c71aecf1598172748722c48d87a1ac0894d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/31590ab43f619f595a0edef9acf4492e457ed92d

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d88c174c2d20fe3f20b2c7bc5ffe73a154c5079f
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=74604888&repo=mozilla-inbound

 /builds/slave/m-in-m64-000000000000000000000/build/src/widget/cocoa/nsPrintSettingsX.mm:337:35: error: use of undeclared identifier 'NSPaperOrientationPortrait'
/builds/slave/m-in-m64-000000000000000000000/build/src/widget/cocoa/nsPrintSettingsX.mm:352:56: error: use of undeclared identifier 'NSPaperOrientationPortrait'
/builds/slave/m-in-m64-000000000000000000000/build/src/widget/cocoa/nsPrintSettingsX.mm:355:56: error: use of undeclared identifier 'NSPaperOrientationLandscape'
/builds/slave/m-in-m64-000000000000000000000/build/src/widget/cocoa/nsPrintSettingsX.mm:474:35: error: use of undeclared identifier 'NSPaperOrientationPortrait'

Please fix the issue and submit an updated patch.
Flags: needinfo?(cosmo0920.oucc)
Assignee

Comment 31

2 years ago
Attachment #8829088 - Attachment is obsolete: true
Attachment #8829088 - Flags: review?(mstange)
Flags: needinfo?(cosmo0920.oucc)
Attachment #8833840 - Flags: review?(haftandilian)
Attachment #8833840 - Flags: review?(haftandilian) → review+
See Also: → 1330643
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 32

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd49b1d1bf8f
Export the constants for paper orientation. r=haik
Keywords: checkin-needed

Comment 33

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd49b1d1bf8f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
See Also: → 1353064
Depends on: 1467582
You need to log in before you can comment on or make changes to this bug.