Open Bug 1432651 Opened 4 years ago Updated 1 year ago

Disentangle RemotePrintJobChild from nsPrintSettings, and kill off nsPrintSession

Categories

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

task

Tracking

()

ASSIGNED

People

(Reporter: jwatt, Assigned: jwatt)

References

(Depends on 1 open bug)

Details

(Whiteboard: [print2020])

Attachments

(12 files)

59 bytes, text/x-review-board-request
bobowen
: review+
Details
59 bytes, text/x-review-board-request
bobowen
: review+
Details
59 bytes, text/x-review-board-request
bobowen
: review+
Details
59 bytes, text/x-review-board-request
bobowen
: review+
Details
59 bytes, text/x-review-board-request
bobowen
: review+
Details
59 bytes, text/x-review-board-request
bobowen
: review+
Details
59 bytes, text/x-review-board-request
bobowen
: review+
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
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.
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 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 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 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?
Attachment #8944941 - Flags: review?(bobowencode) → review+
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
Attachment #8944942 - Flags: review?(bobowencode) → review+
Comment on attachment 8944943 [details]
Bug 1432651 part 3 - Stop using nsIPrintSession::GetRemotePrintJob in nsPrintingProxy.

https://reviewboard.mozilla.org/r/215088/#review220912
Attachment #8944943 - Flags: review?(bobowencode) → review+
Comment on attachment 8944944 [details]
Bug 1432651 part 4 - Stop using nsIPrintSession::GetRemotePrintJob in nsPrintJob.

https://reviewboard.mozilla.org/r/215090/#review220916
Attachment #8944944 - Flags: review?(bobowencode) → review+
Comment on attachment 8944945 [details]
Bug 1432651 part 5 - Stop using nsIPrintSession::GetRemotePrintJob in nsDeviceContextSpecProxy.

https://reviewboard.mozilla.org/r/215092/#review220918
Attachment #8944945 - Flags: review?(bobowencode) → review+
Comment on attachment 8944946 [details]
Bug 1432651 part 6 - Stop using nsIPrintSession::GetRemotePrintJob in nsPrintSettingsService.

https://reviewboard.mozilla.org/r/215094/#review220924
Attachment #8944946 - Flags: review?(bobowencode) → review+
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
Attachment #8944947 - Flags: review?(bobowencode) → review+
[ Triage 2017/02/20: P3 ]
Priority: -- → P3

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

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

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

MozReview-Commit-ID: 4JAbGp4fQkE

Depends on D55547

Blocks: 1557645
Status: NEW → ASSIGNED
Fission Milestone: --- → M5
Priority: P3 → P2

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.

Flags: needinfo?(jwatt)
Flags: needinfo?(jwatt)

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

Fission Milestone: M5 → M6
Type: enhancement → task
Whiteboard: [print2020]

Tracking for Fission Nightly (M6c)

Fission Milestone: M6 → M6c

Jonathan, can we get these ready to land soon?

Flags: needinfo?(jwatt)

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

Depends on: 1140929
Flags: needinfo?(jwatt)

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.

No longer blocks: 1557645
Fission Milestone: M6c → ---

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.

You need to log in before you can comment on or make changes to this bug.