Closed Bug 1435286 Opened 3 years ago Closed 3 years ago

Stop recording draw events from PrintTarget reference DrawTargets


(Core :: Printing: Output, defect)

Not set



Tracking Status
firefox59 --- verified
firefox60 --- verified


(Reporter: jwatt, Assigned: jwatt)




(1 file)

Currently PrintTarget::GetReferenceDrawTarget and its overrides are passed a DrawEventRecorder which we used to record draw events from the returned reference DrawTarget. This makes no sense. These DTs should simply be used for measuring while doing reflows. There should be no draw commands omitted to them, and no need to waste time or memory passing these DTs over to the parent process during printing.
Attached patch patchSplinter Review
Attachment #8947879 - Flags: review?(bobowencode)
Comment on attachment 8947879 [details] [diff] [review]

Review of attachment 8947879 [details] [diff] [review]:

As discussed on IRC, perhaps a follow-up to come up with some way to ensure the reference DrawTarget isn't used for any actual draw commands.
Attachment #8947879 - Flags: review?(bobowencode) → review+
Pushed by
Stop recording draw events for reference DrawTargets returned from PrintTarget. r=bobowen
(In reply to Bob Owen (:bobowen) from comment #3)
> As discussed on IRC, perhaps a follow-up to come up with some way to ensure
> the reference DrawTarget isn't used for any actual draw commands.

I filed bug 1435357.
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
This fixes bug 1434762.
Blocks: 1434762
Comment on attachment 8947879 [details] [diff] [review]

Requesting beta approval since this fixes topcrash-win bug 1434762.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1354624
[User impact if declined]: topcrash-win bug 1434762
[Is this code covered by automated tests?]: some, but we should have more
[Has the fix been verified in Nightly?]: yes - bug 1434762 comment 5 and 6
[Needs manual test from QE? If yes, steps to reproduce]: myself and philipp have both manually tested the steps in bug 1434762 comment 0, but I think more manual testing of printing in general from QE would be wise
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: it *shouldn't* be
[Why is the change risky/not risky?]: in principle this change is clearly the correct thing to do. However, the printing code is quite unloved and does some bizarre things in places.
[String changes made/needed]: none
Attachment #8947879 - Flags: approval-mozilla-beta?
Agreed that this is a bit scary given the current state of our printing code. I'm going to take this for 59b8 because it fixes the topcrash, but I also agree that we should have QA do some printing smoketesting after it's uplifted.
Flags: qe-verify+
Attachment #8947879 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have reproduced this issue using Firefox 60.0a1(2018.02.02) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 59.0b8 and on 60.0a1 (2018.02.09) on Win 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.13.3.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.