1661873 - Audit whether it's right thing to do that we do create nsRootPresContext for subdocuments in print preview (maybe also in printing)

Status: RESOLVED FIXED

(Core :: Print Preview, task)

Description

[Hiroyuki Ikezoe (:hiro)]

In bug 1661608 Botond noticed that we do create nsRootPresContext for subdocuments in print previewing documents. To me, it's simply wrong but I am not 100% sure right now, if it's wrong we should stop doing it.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

It comes from inside nsPrintJob::ReflowPrintObject, and it was introduced in bug 468568 comment 173 to avoid happening DidPaint in the subdocument's pres contexts. The bug 468568 aimed to be able to print downloadable fonts in print documents. In these days do we still need the hack introduced in the bug?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

From a review comment by Olli in bug 468568 comment 177 (it's not well-formatted though);

• aPO->mPresContext = aPO->mParent ?

•  new nsPresContext(aPO->mDocument, type) :
  

•  new nsRootPresContext(aPO->mDocument, type);
  

  NS_ENSURE_TRUE(aPO->mPresContext, NS_ERROR_OUT_OF_MEMORY);

In printpreview case if there isn't parent, shouldn't you check
DocumentViewerImpl::FindContainerView() so that we don't end up creating
rootprescontext in case pp is shown as a tab (like it usually is).

Whereas the current code is;

  nsView* parentView = aPO->mParent && aPO->mParent->PrintingIsEnabled()        
                           ? nullptr                                            
                           : GetParentViewForRoot();                            
  aPO->mPresContext = parentView ? new nsPresContext(aPO->mDocument, type)      
                                 : new nsRootPresContext(aPO->mDocument, type);

So it looks it's not been what we wanted. In the current code, we do create nsRootPresContext in most cases.

Instead what we wanted is if (aPo->mParent && aPO->mParent->PrintingIsEnabled()) or GetParentViewForRoot() is not null, then we do create nsPresContext?

CCing, Olli.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Olli, is my understanding in comment 2 correct? I've confirmed with the correction downloadable fonts is printed/print-previewed properly in an iframe.

If my understanding is correct, I am going to add a print reftest with a downloadable font (and add a print-preview test if I could).

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

hmm, my comment was even before e10s.
But yes, your suggestion sounds right. Top level thing should be RootPrescontext

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Thank you, Olli!

Comment 6

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69f34109f76c84510ce6b53ddf7e1fcb92425891

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1661873 - Add wpts and mochitests to make sure downloadable font is properly printed or rendered in print preview. r?jfkthame

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1661873 - Generate nsRootPresShell for printing ducuments only if the document has no parent OR the parent is not going to be printed AND GetParentViewForRoot() is not null. r?smaug

Though there is no automated test for this change, I think the test cases in the
previous commit well cover the original issue (bug 468568) that downloadable
fonts are not rendered in printing.

Depends on D89095

Comment 9

[Hiroyuki Ikezoe (:hiro)]

test_printpreview.xhtml failed on a TV run with "application terminated with exit code 2" but it actually failed before newly adding tests here;

https://treeherder.mozilla.org/#/jobs?repo=try&revision=914915a848efa0c5e92e99cbca994a9b4ea78246&selectedTaskRun=Oe8aoHsvT3uYGcg3JVJdDA.0

So I think it's a pre-existing issue. (And I noticed there are a bunch of intermittent test failures on test_printpreview.xhtml, we should take a look of them).

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/986578cbad09
Add wpts and mochitests to make sure downloadable font is properly printed or rendered in print preview. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/69fdf7a9f0ce
Generate nsRootPresShell for printing ducuments only if the document has no parent OR the parent is not going to be printed AND GetParentViewForRoot() is not null. r=smaug

Comment 11

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/25372 for changes under testing/web-platform/tests

Comment 12

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/986578cbad09
https://hg.mozilla.org/mozilla-central/rev/69fdf7a9f0ce

Comment 13

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot