Don't show the header and footer in print preview of PDFs

VERIFIED FIXED in mozilla29

Status

()

VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: spammaaja, Assigned: dev_oskar)

Tracking

(Blocks: 1 bug)

Trunk
mozilla29
x86
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Bug 743252 removed the header and footer from PDFs. However, they are still shown in print preview.
(Reporter)

Updated

6 years ago
Blocks: 743252

Comment 1

6 years ago
Confirmed.
http://hg.mozilla.org/mozilla-central/rev/08a034e1d43a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130223 Firefox/22.0 ID:20130223031157
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

5 years ago
Created attachment 8350885 [details] [diff] [review]
PrintPreview moznomarginboxes code

The patch simple copies the logic of the print code, so both are sync.

Changing the footer and header in the ui doesn't work anyway with moznomarginboxes. At least now the printpreview and printout are the same, and the PrintPreview is affected by user changes for footers and headers
(Assignee)

Comment 3

5 years ago
Created attachment 8350886 [details] [diff] [review]
PrintPreview moznomarginboxes code fixed

Missed a line in patch file
Attachment #8350885 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 947125
Comment on attachment 8350886 [details] [diff] [review]
PrintPreview moznomarginboxes code fixed

(bringing conversation over from bug 650966)

> When Bug 844473 doesn't even get attention I have no idea how I can help, if
> even simple stuff won't get fixed or at least a reason why not.

You have tripped over a long-standing problem with our process.  If you just post a patch to a bug, no one will notice.  You have to select a specific person to ask for a code review.  Worse, there is no automatic or even fully reliable way to know who understands the code well enough to review *and* is procedurally entitled to do so.  Even worse, there is a lot of code understood *only* by a small handful of chronically-overworked "super-reviewers", or in some cases, not truly understood by anyone at all.  (Be glad you aren't trying to patch the table layout engine. :-/ )

In this case, based on `hg annotate` for this area of nsDocumentViewer.cpp, I have decided that Robert O'Callahan is at least plausibly appropriate as the reviewer.  I can also confirm that my own (limited) understanding of this code says that this is the correct change.  (That's what 'feedback+' means.)
Attachment #8350886 - Flags: review?(roc)
Attachment #8350886 - Flags: feedback+
FYI, once you get a positive review from roc (or whoever he reassigns it to), you can then go ahead and push the patch to mozilla-inbound.  If you don't have access to do that yet, you instead put "checkin-needed" in the "keywords" box at the top of the bug and someone will do it for you.

Updated

5 years ago
Assignee: nobody → dev_oskar
Status: NEW → ASSIGNED
Comment on attachment 8350886 [details] [diff] [review]
PrintPreview moznomarginboxes code fixed

https://hg.mozilla.org/integration/mozilla-inbound/rev/7418f487bb8b
Attachment #8350886 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/7418f487bb8b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(Assignee)

Comment 8

5 years ago
Created attachment 8369195 [details] [diff] [review]
Honor mozdisallowselectionprint and moznomarginboxes in print preview, use child document.

With the first PrintPreview call mDocument is likely not the document that belongs to aChildDOMWin. The patch switches from mDocument to doc of aChildDOMWin. (as used by the mPrintEngine->Initialize call)
Additional it adds debug log, as the manipulation of header footer settings aren't saved anymore. (The current saving code for this is broken anyway)

Technical nsIPrintSettings should gain a new setting for enableMarginBoxes and the code for moznomarginboxes rewritten to change this setting once when starting printPreview so the user can control this by ui, I will create a new bug for this.
Attachment #8350886 - Attachment is obsolete: true
Attachment #8369195 - Flags: review?(roc)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla29 → ---
Comment on attachment 8369195 [details] [diff] [review]
Honor mozdisallowselectionprint and moznomarginboxes in print preview, use child document.

https://hg.mozilla.org/integration/mozilla-inbound/rev/543e8c09e7b0

Is this something we could write a test for?
Attachment #8369195 - Flags: checkin? → checkin+
Flags: needinfo?(dev_oskar)
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/543e8c09e7b0
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Assignee)

Comment 11

5 years ago
Hmm, creating a test is quite complicate.

moznomarginboxes influences the print settings when Print or PrintPreview gets called. (in a very odd way, and only then)

Reftest assumes currently fixed settings, and directly uses setPageMode(true, ps).
So header and footer would need to be enabled for the first part and PrintPreview mode somehow activated.
For the reference page the settings would need to be reset to zero, and again PrintPreview mode activated, then comparing both. 

Not sure how to write such code...
Flags: needinfo?(dev_oskar)
OK, thanks.
Flags: in-testsuite? → in-testsuite-
correcting target milestone, sorry for the bugmail spam
Target Milestone: mozilla30 → mozilla29

Updated

5 years ago
Blocks: 967105

Updated

5 years ago
No longer blocks: 967105
Filed follow-up bug 967105 for the difficulty of writing reftests for this sort of thing in general.

Updated

5 years ago
Keywords: verifyme
Verified as fixed with Firefox 29 beta 1 on Win 7 x86 using the page provided in bug 743252: https://bugzilla.mozilla.org/attachment.cgi?id=714383
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.