1432651 - Disentangle RemotePrintJobChild from nsPrintSettings, and kill off nsPrintSession

Status: RESOLVED FIXED

(Core :: Printing: Output, task, P2)

Description

[Jonathan Watt [:jwatt]]

Right now we have an nsIPrintSettings instance hold onto an nsIPrintSession instance, which in turn holds a weak ref to a RemotePrintJobChild. For the most part, whenever we need to get access to the RemotePrintJobChild we do so via an nsIPrintSettings object.

This design doesn't make much sense. nsIPrintSettings should be decoupled from any specific print session. Furthermore, obtaining the RemotePrintJobChild from an nsIPrintSettings makes it really hard to reason about the code since under certain circumstances we replace or overwrite nsIPrintSettings objects.

It would be significantly cleaner and easier to think about the code if the nsPrintJob owned the RemotePrintJobChild, and if we could access it via the document's nsIWebBrowserPrint (i.e. nsDocumentViewer).

Additionally it would be nice to kill off nsIPrintSession, since it's simply a wrapper for RemotePrintJobChild in the current setup.

Comment 1

[Jonathan Watt [:jwatt]]

My approach here is to first make sure that whenever we store a new RemotePrintJobChild on an nsIPrintSession that we also store it on the nsPrintJob. I'll then make all the locations that get their RemotePrintJobChild from an nsIPrintSettings get it from the nsPrintJob instead. Finally we can remove all the nsIPrintSession code.

It's worth providing some background for the first step since that will probably help save time trying to figure out the validity of the change during review:

nsIPrintSession::SetRemotePrintJob is only called by nsPrintSettingsService::DeserializeToPrintSettings. That has only two callers in the child process:

  1) TabChild::RecvPrint
  2) nsPrintingProxy::ShowPrintDialog

#1 on the very next line calls nsDocumentViewer::Print (via the nsIWebBrowserPrint interface) which results in us calling into nsPrintJob::DoCommonPrint and then into #2:

  nsIPrintSession::SetRemotePrintJob
  nsPrintSettingsService::DeserializeToPrintSettings
  nsPrintingProxy::ShowPrintDialog    (only content process caller of ^)
  nsPrintJob::DoCommonPrint
  nsPrintJob::CommonPrint
  nsPrintJob::Print
  nsDocumentViewer::Print             (constructs new nsPrintJob)
  nsIWebBrowserPrint::Print
  TabChild::RecvPrint

#2 waits for a sync response from the parent and gets the returned settings (containing a new RemotePrintJobChild if one wasn't passed in due to the call coming via #1). It's only child process caller is nsPrintJob::DoCommonPrint (the settings are stored in its member printData->mPrintSettings).

So basically the RemotePrintJobChild either comes from TabChild::RecvPrint, or if the nsPrintingProxy::ShowPrintDialog came from somewhere else, the RemotePrintJobChild will be created under the nsPrintingProxy::ShowPrintDialog call.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: Bug 1432651 part 1 - Pass the RemotePrintJobChild through from TabChild::RecvPrint to nsPrintJob::DoCommonPrint.

Currently RemotePrintJobChild instances are created in two places, one of
those being under TabChild::RecvPrint.  Currently we pass the
RemotePrintJobChild through to nsPrintJob::DoCommonPrint via the
nsIPrintSettings.  This patch lays part of the groundwork for disentangling
RemotePrintJobChild from nsPrintSettings by passing it directly and having
nsPrintJob store a strong ref (in its nsPrintData).

Review commit: https://reviewboard.mozilla.org/r/215084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215084/

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: Bug 1432651 part 2 - If nsPrintingProxy::ShowPrintDialog creates an RemotePrintJobChild, store it on the nsPrintJob.

Following on from part 1, the second of the two places where we can create an
RemotePrintJobChild is under nsPrintingProxy::ShowPrintDialog.  This patch
makes nsPrintingProxy::ShowPrintDialog set the RemotePrintJobChild on the
nsPrintJob for the print operation so that we can now get our
RemotePrintJobChild from the nsPrintJob rather than from an nsIPrintSettings.

Since nsPrintingProxy can't access the nsPrintJob directly, this patch adds
methods to the XPIDL interface nsIWebBrowserPrint (implemented by the
document's nsDocumentViewer) to pass the RemotePrintJobChild through to the
nsPrintJob.  This isn't ideal, but unfortunately it's hard to avoid since we
use XPIDL heavily in the printing code to allow it to be scripted by the
browser UI.

Review commit: https://reviewboard.mozilla.org/r/215086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215086/

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: Bug 1432651 part 3 - Stop using nsIPrintSession::GetRemotePrintJob in nsPrintingProxy.

This makes nsPrintingProxy::ShowProgress get the RemotePrintJobChild from the
current nsPrintJob (indirectly via the new
nsIWebBrowserPrint::GetRemotePrintJob API).

Review commit: https://reviewboard.mozilla.org/r/215088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215088/

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: Bug 1432651 part 4 - Stop using nsIPrintSession::GetRemotePrintJob in nsPrintJob.

This makes nsPrintJob methods obtain the RemotePrintJobChild from its
nsPrintData instead of from the nsIPrintSettings' nsIPrintSession.

Review commit: https://reviewboard.mozilla.org/r/215090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215090/

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: Bug 1432651 part 5 - Stop using nsIPrintSession::GetRemotePrintJob in nsDeviceContextSpecProxy.

This makes nsPrintJob::DoPrintCommon pass the RemotePrintJobChild to the
nsDeviceContextSpecProxy constructor so that we don't need to call
nsIPrintSession::GetRemotePrintJob in nsDeviceContextSpecProxy::Init.

This also moves the construction of nsDeviceContextSpecProxy in
nsPrintJob::DoCommonPrint down to just about where we call Init() on the
new object.  This is important since the RemotePrintJobChild may not be
created until after the ShowPrintDialog call so we should wait until after
that to try and obtain it.

Review commit: https://reviewboard.mozilla.org/r/215092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215092/

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: Bug 1432651 part 6 - Stop using nsIPrintSession::GetRemotePrintJob in nsPrintSettingsService.

Review commit: https://reviewboard.mozilla.org/r/215094/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215094/

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: Bug 1432651 part 7 - Remove nsIPrintSession and all the code that uses it.

Review commit: https://reviewboard.mozilla.org/r/215096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215096/

Comment 9

[Code Review Bot [:reviewbot]]

Comment on attachment 8944944 [details]
Bug 1432651 part 4 - Stop using nsIPrintSession::GetRemotePrintJob in nsPrintJob.

https://reviewboard.mozilla.org/r/215090/#review220708


Static analysis found 1 defect in this patch.
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/printing/nsPrintJob.cpp:890
(Diff revision 1)
> -              RefPtr<mozilla::layout::RemotePrintJobChild> remotePrintJob;
> +              printData->mPrintProgressListeners.AppendElement(printData->mRemotePrintJob);
> -              printSession->GetRemotePrintJob(getter_AddRefs(remotePrintJob));
> -              if (NS_SUCCEEDED(rv) && remotePrintJob) {
> -                printData->mPrintProgressListeners.AppendElement(
> -                                                     remotePrintJob);
> -                remotePrintJobListening = true;
> +              remotePrintJobListening = true;

Warning: Value stored to 'remoteprintjoblistening' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment 10

[Code Review Bot [:reviewbot]]

Comment on attachment 8944945 [details]
Bug 1432651 part 5 - Stop using nsIPrintSession::GetRemotePrintJob in nsDeviceContextSpecProxy.

https://reviewboard.mozilla.org/r/215092/#review220712


Static analysis found 1 defect in this patch.
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: widget/nsDeviceContextSpecProxy.h:28
(Diff revision 1)
>  public:
> +  typedef mozilla::layout::RemotePrintJobChild RemotePrintJobChild;
> +
>    NS_DECL_ISUPPORTS
>  
> +  nsDeviceContextSpecProxy(NotNull<RemotePrintJobChild*> aRemotePrintJob)

Error: Bad implicit conversion constructor for 'nsdevicecontextspecproxy' [clang-tidy: mozilla-implicit-constructor]

Comment 11

[Bob Owen (:bobowen)]

Comment on attachment 8944941 [details]
Bug 1432651 part 1 - Pass the RemotePrintJobChild through from TabChild::RecvPrint to nsPrintJob::DoCommonPrint.

https://reviewboard.mozilla.org/r/215084/#review220848

::: layout/printing/nsPrintData.h:11
(Diff revision 1)
>  
>  #ifndef nsPrintData_h___
>  #define nsPrintData_h___
>  
>  #include "mozilla/Attributes.h"
> +#include "mozilla/layout/RemotePrintJobChild.h"

Couldn't we forward declare instead?

Comment 12

[Bob Owen (:bobowen)]

Comment on attachment 8944942 [details]
Bug 1432651 part 2 - If nsPrintingProxy::ShowPrintDialog creates an RemotePrintJobChild, store it on the nsPrintJob.

https://reviewboard.mozilla.org/r/215086/#review220908

Comment 13

[Bob Owen (:bobowen)]

Comment on attachment 8944943 [details]
Bug 1432651 part 3 - Stop using nsIPrintSession::GetRemotePrintJob in nsPrintingProxy.

https://reviewboard.mozilla.org/r/215088/#review220912

Comment 14

[Bob Owen (:bobowen)]

Comment on attachment 8944944 [details]
Bug 1432651 part 4 - Stop using nsIPrintSession::GetRemotePrintJob in nsPrintJob.

https://reviewboard.mozilla.org/r/215090/#review220916

Comment 15

[Bob Owen (:bobowen)]

Comment on attachment 8944945 [details]
Bug 1432651 part 5 - Stop using nsIPrintSession::GetRemotePrintJob in nsDeviceContextSpecProxy.

https://reviewboard.mozilla.org/r/215092/#review220918

Comment 16

[Bob Owen (:bobowen)]

Comment on attachment 8944946 [details]
Bug 1432651 part 6 - Stop using nsIPrintSession::GetRemotePrintJob in nsPrintSettingsService.

https://reviewboard.mozilla.org/r/215094/#review220924

Comment 17

[Bob Owen (:bobowen)]

Comment on attachment 8944947 [details]
Bug 1432651 part 7 - Remove nsIPrintSession and all the code that uses it.

https://reviewboard.mozilla.org/r/215096/#review220934

Comment 18

[Jet Villegas (inactive)]

[ Triage 2017/02/20: P3 ]

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: Bug 1432651 part 1 - Pass the RemotePrintJobChild through from BrowserChild::RecvPrint to nsPrintJob::DoCommonPrint. r=bobowen

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: Bug 1432651 part 2 - If nsPrintingProxy::ShowPrintDialog creates an RemotePrintJobChild, store it on the nsPrintJob. r=bobowen

Following on from part 1, the second of the two places where we can create an
RemotePrintJobChild is under nsPrintingProxy::ShowPrintDialog. This patch
makes nsPrintingProxy::ShowPrintDialog set the RemotePrintJobChild on the
nsPrintJob for the print operation so that we can now get our
RemotePrintJobChild from the nsPrintJob rather than from an nsIPrintSettings.

Since nsPrintingProxy can't access the nsPrintJob directly, this patch adds
methods to the XPIDL interface nsIWebBrowserPrint (implemented by the
document's nsDocumentViewer) to pass the RemotePrintJobChild through to the
nsPrintJob. This isn't ideal, but unfortunately it's hard to avoid since we
use XPIDL heavily in the printing code to allow it to be scripted by the
browser UI.

MozReview-Commit-ID: CwJwGfCcUjZ

Depends on D55545

Comment 21

[Jonathan Watt [:jwatt]]

Attachment: Bug 1432651 part 3 - Stop using nsIPrintSession::GetRemotePrintJob in nsPrintingProxy. r=bobowen

This makes nsPrintingProxy::ShowProgress get the RemotePrintJobChild from the
current nsPrintJob (indirectly via the new
nsIWebBrowserPrint::GetRemotePrintJob API).

MozReview-Commit-ID: JXYNZ3QHFRe

Depends on D55546

Comment 22

[Jonathan Watt [:jwatt]]

Attachment: Bug 1432651 part 4 - Stop using nsIPrintSession::GetRemotePrintJob in nsPrintJob. r=bobowen

This makes nsPrintJob methods obtain the RemotePrintJobChild from its
nsPrintData instead of from the nsIPrintSettings' nsIPrintSession.

MozReview-Commit-ID: 4JAbGp4fQkE

Depends on D55547

Comment 23

[Jonathan Watt [:jwatt]]

Attachment: Bug 1432651 part 5 - Stop using nsIPrintSession::GetRemotePrintJob in nsDeviceContextSpecProxy. r=bobowen

Depends on D55548

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:jwatt, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 25

[Chris Peterson [:cpeterson]]

Deferring to Fission Nightly (M6) because printing bugs don't need to block M5 dogfooding.

Comment 26

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6c)

Comment 27

[Neha Kochar [:neha]]

Jonathan, can we get these ready to land soon?

Comment 28

[Jonathan Watt [:jwatt]]

We need bug 1140929 to happen first. I'm aiming to get that done for v81.

Comment 29

[Sean Voisen (:svoisen)]

With all the recent work for the new print UI in bug 1653340, bug 1636728, and bug 1602410 our approach has changed. I'll leave this open for now but removing it from Fission milestones.

Comment 30

[Jonathan Watt [:jwatt]]

We do still want to land these patches, but it will be a lot easier to do that once the old nested event loop spinning code in DoCommonPrint that's currently still needed for the old print UI is gone.

Comment 31

[Jonathan Watt [:jwatt]]

Attachment: Bug 1432651 p1 - Pass RemotePrintJobChild through to the places where it's needed. r=emilio

Given how nsIPrintSettings is passed around, stored and copied all over the
place, it's very hard to reason about where and when a RemotePrintJobChild is
needed or valid. This patch avoids all that by explicitly passing a
RemotePrintJobChild when it's needed.

Another reason to make this change is because RemotePrintJobChild really does
not belong on nsIPrintSettings. That interface is supposed to represent a
collection of settings for laying out the document that is to be printed.

Comment 32

[Jonathan Watt [:jwatt]]

Attachment: Bug 1432651 p2 - Remove nsIPrintSession and all the code that uses it. r=emilio

Lately nsIPrintSession was only used to pass around RemotePrintJobChild objects.
Now that we pass those objects explicitly where needed (part 1), this class
serves no purpose.

Another reason to want to get rid of this class is that having it as a member
of nsIPrintSettings made no sense and was confusing.

Depends on D146380

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce7f1bd80fa0
p1 - Pass RemotePrintJobChild through to the places where it's needed. r=emilio
https://hg.mozilla.org/integration/autoland/rev/9482edb5ae07
p2 - Remove nsIPrintSession and all the code that uses it. r=emilio,geckoview-reviewers,m_kato

Comment 34

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ce7f1bd80fa0
https://hg.mozilla.org/mozilla-central/rev/9482edb5ae07