Closed Bug 792314 Opened 7 years ago Closed 7 years ago

Make print preview support display lists properly

Categories

(Core :: Print Preview, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(3 files, 4 obsolete files)

Attached patch WIP fix (obsolete) — Splinter Review
For DLBI to work correctly, we need the printing code (or reftests at least) to be building full display lists instead of recursing into nsLayoutUtils::PaintFrame.

Looks like this patch works correctly when printing a page to pdf on Mac (with a page zoom forced via pref).

Does anyone know how to test with a non-1.0 page sequence resolution? It doesn't appear to be used at all when printing on mac, and Mac doesn't have built-in print preview.

Think there's still a few issues with reftests currently, working on it.
Attachment #662421 - Flags: feedback?(roc)
For testing print preview on OSX there is the addon https://addons.mozilla.org/en-US/firefox/addon/print-print-preview-update/?src=ap .  It's not the exact same thing, but seems to use most the print preview code.
Attachment #662421 - Attachment is obsolete: true
Attachment #662739 - Flags: review?(roc)
Attachment #662740 - Flags: review?(roc)
Attachment #662741 - Flags: review?(roc)
Comment on attachment 662739 [details] [diff] [review]
Make nsDisplayTransform support untransformed frames

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

::: layout/base/nsDisplayList.h
@@ +2505,5 @@
>   */ 
>  class nsDisplayTransform: public nsDisplayItem
>  {
>  public:
> +  typedef gfx3DMatrix (* ComputeTransformFunction)(nsIFrame* aFrame, float aAppUnitsPerPixel);

You need to document how the matrix returned by this function is interpreted.
Attachment #662739 - Flags: review?(roc) → review+
Comment on attachment 662740 [details] [diff] [review]
Build full display lists for nsPageFrame

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

::: layout/generic/nsPageFrame.cpp
@@ +405,4 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  if (PresContext()->GetPageScale() != 1.0) {
> +    rv = content.AppendNewToTop(new (aBuilder) nsDisplayTransform(aBuilder, this, &content, ::ComputePageTransform));

Simpler to just always build this. No point in having multiple code paths to cause confusion later.
Attachment #662740 - Flags: review?(roc) → review+
Comment on attachment 662741 [details] [diff] [review]
Build full display lists for nsSimplePageSequence

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

::: layout/generic/nsSimplePageSequence.cpp
@@ +807,3 @@
>  {
> +  float scale = aFrame->PresContext()->GetPrintPreviewScale();
> +  return gfx3DMatrix::ScalingMatrix(scale, scale, 1);

Why don't we need to change the basis here? Are we assuming that the nsSimplePageSequenceFrame is always at 0,0? That might not be a good assumption.

@@ +825,3 @@
>  
> +  if (PresContext()->GetPrintPreviewScale() != 1.0) {
> +    rv = content.AppendNewToTop(new (aBuilder) nsDisplayTransform(aBuilder, this, &content, ::ComputePageSequenceTransform));

Make this unconditional too.
Attachment #662740 - Attachment is obsolete: true
Attachment #662761 - Flags: review?(roc)
Test change forces active layers above it, just like the file it's being compared to, which has an active <canvas> object.
Attachment #662741 - Attachment is obsolete: true
Attachment #662741 - Flags: review?(roc)
Attachment #662762 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Comment on attachment 662741 [details] [diff] [review]
> Build full display lists for nsSimplePageSequence
> 
> Review of attachment 662741 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsSimplePageSequence.cpp
> @@ +807,3 @@
> >  {
> > +  float scale = aFrame->PresContext()->GetPrintPreviewScale();
> > +  return gfx3DMatrix::ScalingMatrix(scale, scale, 1);
> 
> Why don't we need to change the basis here? Are we assuming that the
> nsSimplePageSequenceFrame is always at 0,0? That might not be a good
> assumption.

This was because nsPageFrame was transforming it's child, not itself and needed to adjust for that. But i've changed that now so the transform is created for the child directly.

We still change the basis in both cases inside nsDisplayTransform::GetTransform.
Comment on attachment 662761 [details] [diff] [review]
Build full display lists for nsPageFrame v2

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

::: layout/generic/nsPageFrame.cpp
@@ +366,5 @@
>  }
>  
> +static gfx3DMatrix ComputePageTransform(nsIFrame* aFrame, float aAppUnitsPerPixel)
> +{
> +  float scale = aFrame->GetParent()->PresContext()->GetPageScale();

This GetParent() is superfluous.
Attachment #662761 - Flags: review?(roc) → review+
Added back clipping code that I removed. This fixes a few reftest failures on tinderbox and exposed a regression from bug 788044.
Attachment #662761 - Attachment is obsolete: true
Attachment #662785 - Flags: review?(roc)
Landed the first 2 patches, the nsPageSequence one is still causing android fuzz failures.

https://hg.mozilla.org/integration/mozilla-inbound/rev/724f2a728a6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8e84de2291e

Try push:

https://tbpl.mozilla.org/?tree=Try&rev=030841d5fa13
Whiteboard: [leave open]
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e7a1b07944
Assignee: nobody → matt.woodrow
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/e6e7a1b07944
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 794709
Depends on: 816603
Depends on: 820494
You need to log in before you can comment on or make changes to this bug.