Make print preview support display lists properly

RESOLVED FIXED in mozilla18

Status

()

Core
Print Preview
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

unspecified
mozilla18
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 662421 [details] [diff] [review]
WIP fix

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.
(Assignee)

Comment 2

5 years ago
Created attachment 662739 [details] [diff] [review]
Make nsDisplayTransform support untransformed frames
Attachment #662421 - Attachment is obsolete: true
Attachment #662739 - Flags: review?(roc)
(Assignee)

Comment 3

5 years ago
Created attachment 662740 [details] [diff] [review]
Build full display lists for nsPageFrame
Attachment #662740 - Flags: review?(roc)
(Assignee)

Comment 4

5 years ago
Created attachment 662741 [details] [diff] [review]
Build full display lists for nsSimplePageSequence
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.
(Assignee)

Comment 8

5 years ago
Created attachment 662761 [details] [diff] [review]
Build full display lists for nsPageFrame v2
Attachment #662740 - Attachment is obsolete: true
Attachment #662761 - Flags: review?(roc)
(Assignee)

Comment 9

5 years ago
Created attachment 662762 [details] [diff] [review]
Build full display lists for nsSimplePageSequence v2

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)
(Assignee)

Comment 10

5 years ago
(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+
(Assignee)

Comment 12

5 years ago
Created attachment 662785 [details] [diff] [review]
Build full display lists for nsPageFrame v3

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)
(Assignee)

Comment 13

5 years ago
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]
(Assignee)

Comment 15

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

5 years ago
Depends on: 794709

Updated

5 years ago
Depends on: 816603

Updated

5 years ago
Depends on: 820494
You need to log in before you can comment on or make changes to this bug.