Share common printingui implementation instead of each platform implementation.

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mantaroh, Assigned: mantaroh)

Tracking

(Blocks 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(8 attachments, 1 obsolete attachment)

After landed the bug 1405210, we can merge toolkit/components/printingui implementation. As result, we can drop each platform code.

A printingui components has 3 classes each platforms:

 * nsPrintProgress : 
   * unix / windows uses this.
   * unix implementation contain the print settings member in order to cancel job.
     But as far as I can see, I guess we need to add this member to the windows.
     (Filed bug 1409971)
 
 * nsPrintProgressParam :
   * unix / windows uses this.
   * There are NOT differnces.

 * nsPrintingPromptService : 
   * All platform uses this. 
   * Unix implementation contain xul print dialog and uxl print job dialog,
     However, after landed the bug 193001, gecko have not used it. So we can drop this.
Assignee

Comment 1

2 years ago
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #0)
>  * nsPrintingPromptService : 
>    * All platform uses this. 
>    * Unix implementation contain xul print dialog and uxl print job dialog,
>      However, after landed the bug 193001, gecko have not used it. So we can
> drop this.

Additional Information:
In QT + KDE environment(like Plasma), gecko try display the native dialog. The appearance of print dialog is same of other application(like KEdit). So I think that we don't need to keep this xul dialog like FilePicker dialog.
Assignee

Comment 2

2 years ago
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #0)
> >  * nsPrintingPromptService : 
> >    * All platform uses this. 
> >    * Unix implementation contain xul print dialog and uxl print job dialog,
> >      However, after landed the bug 193001, gecko have not used it. So we can
> > drop this.
> 
> Additional Information:
> In QT + KDE environment(like Plasma), gecko try display the native dialog.
> The appearance of print dialog is same of other application(like KEdit). So
> I think that we don't need to keep this xul dialog like FilePicker dialog.

karlt,
I'll remove the printdialog.xul from m-c.
This printing xul dialog(printDialog.xul) is used only by nsPrintingPromptService::ShowPrintDialog() of gtk[1]. But I think that this code will not run since current GTK implementation uses nsIPrintDialogService::Show() always[2].

Do you have any problem about removing this dialog?

[1] http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/toolkit/components/printingui/unixshared/nsPrintingPromptService.cpp#85-96
[2] http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/widget/gtk/nsWidgetFactory.cpp#277
Flags: needinfo?(karlt)
I have no problem.  Removing printdialog.xul sounds good to me, thanks.
Flags: needinfo?(karlt)
Assignee

Updated

2 years ago
Depends on: 1411121
Assignee

Comment 4

2 years ago
(In reply to Karl Tomlinson (:karlt) from comment #3)
> I have no problem.  Removing printdialog.xul sounds good to me, thanks.

Thanks karl!
I filed this to bug 1411121.
Priority: -- → P3
Assignee

Updated

2 years ago
Blocks: 1384778
Assignee

Comment 6

2 years ago
This is the diff of Windows and Unixshared implementation for help the review of merge work.

The primary difference is as follow:
 * Windows implementation has original NS_IMPL_ADDREF and NS_IMPL_RELEASE, however this implementation doesn't different from current NS_IMPL_*. I'm not sure that why we use this.
 * Unix has 'nsIPrintSettings' member into nsPrintProgress in order to cancel job.
 * Some of parameter check is different.
 * Windows and unix casts bool to nsresult.(see bug 778106)
 * Windows return always NS_OK in nsIWebProgressListener's implementation, otherwise unix return always NS_ERROR_NOT_IMPLEMENTED. I think that it is ok to return NS_OK since we doesn't use this function.
Assignee

Comment 7

2 years ago
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #6)
> Created attachment 8924072 [details]
> Reference: The diff of Windows and unixshared.
> 
> This is the diff of Windows and Unixshared implementation for help the
> review of merge work.

Previous file is hard to see for me, So I converted to html file.
Attachment #8924072 - Attachment is obsolete: true
Assignee

Comment 8

2 years ago
This is the diff of macOS and Unixshared implementation.

The primary difference is as follow:
  * macOS implementation doesn't have any progress implementation.
  * Some of parameter check is different from other.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

2 years ago
mozreview-review
Comment on attachment 8926202 [details]
Bug 1409972 - Part 1. Apply clang-format to nsPrintingPromptService.

https://reviewboard.mozilla.org/r/197440/#review202668


C/C++ static analysis found 2 defects in this patch (only the first 30 are reported here).

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


::: toolkit/components/printingui/unixshared/nsPrintingPromptService.cpp:27
(Diff revision 1)
> +
> +NS_IMPL_ISUPPORTS(nsPrintingPromptService,
> +                  nsIPrintingPromptService,
> +                  nsIWebProgressListener)
>  
> -NS_IMPL_ISUPPORTS(nsPrintingPromptService, nsIPrintingPromptService, nsIWebProgressListener)
> +nsPrintingPromptService::nsPrintingPromptService() {}

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

nsPrintingPromptService::nsPrintingPromptService() {}
                         ^
                                                   = default;

::: toolkit/components/printingui/unixshared/nsPrintingPromptService.cpp:29
(Diff revision 1)
> +                  nsIPrintingPromptService,
> +                  nsIWebProgressListener)
>  
> -NS_IMPL_ISUPPORTS(nsPrintingPromptService, nsIPrintingPromptService, nsIWebProgressListener)
> +nsPrintingPromptService::nsPrintingPromptService() {}
>  
> -nsPrintingPromptService::nsPrintingPromptService()
> +nsPrintingPromptService::~nsPrintingPromptService() {}

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

nsPrintingPromptService::~nsPrintingPromptService() {}
                         ^
                                                    = default;

Comment 17

2 years ago
mozreview-review
Comment on attachment 8926204 [details]
Bug 1409972 - Part 3. Merge windows printingui.

https://reviewboard.mozilla.org/r/197444/#review202670


C/C++ static analysis found 0 defects in this patch.

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

Comment 18

2 years ago
mozreview-review
Comment on attachment 8926205 [details]
Bug 1409972 - Part 4. Merge mac printingui.

https://reviewboard.mozilla.org/r/197446/#review202674


C/C++ static analysis found 0 defects in this patch.

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

Comment 19

2 years ago
mozreview-review
Comment on attachment 8926206 [details]
Bug 1409972 - Part 5. Use the common printingui implementation.

https://reviewboard.mozilla.org/r/197448/#review202936

::: toolkit/components/printingui/moz.build:18
(Diff revision 1)
>  
>  if CONFIG['NS_PRINTING']:
> -    if toolkit == 'windows':
> -        DIRS += ['win']
> -    elif toolkit == 'cocoa':
> -        DIRS += ['mac']
> +    UNIFIED_SOURCES += [
> +        'nsPrintingPromptService.cpp',
> +    ]
> +    if toolkit == 'windows' or CONFIG['MOZ_PDF_PRINTING']:

Since the local 'toolkit' variable is only used here, you can just do if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows' and remove the toolkit definition above.
Attachment #8926206 - Flags: review?(mshal) → review+

Comment 20

2 years ago
mozreview-review
Comment on attachment 8926202 [details]
Bug 1409972 - Part 1. Apply clang-format to nsPrintingPromptService.

https://reviewboard.mozilla.org/r/197440/#review202958

Clangbot has some issues with this, and I noticed that its feedback can be applied elsewhere. I also know that some of these files are likely to be dropped during the unification, so drop the issues and spend effort where it makes sense. :)

::: toolkit/components/printingui/win/nsPrintingPromptService.cpp:27
(Diff revision 1)
> +
> +NS_IMPL_ISUPPORTS(nsPrintingPromptService,
> +                  nsIPrintingPromptService,
> +                  nsIWebProgressListener)
>  
> -NS_IMPL_ISUPPORTS(nsPrintingPromptService, nsIPrintingPromptService, nsIWebProgressListener)
> +nsPrintingPromptService::nsPrintingPromptService() {}

Strange that the clangbot isn't complaining about these empty constructors as well. I'd imagine that the same feedback can be applied here.
Attachment #8926202 - Flags: review?(mconley) → review+

Comment 21

2 years ago
mozreview-review
Comment on attachment 8926203 [details]
Bug 1409972 - Part 2. Copy printingui implementation from unixshared.

https://reviewboard.mozilla.org/r/197442/#review202992

Thanks!
Attachment #8926203 - Flags: review?(mconley) → review+

Comment 22

2 years ago
mozreview-review
Comment on attachment 8926204 [details]
Bug 1409972 - Part 3. Merge windows printingui.

https://reviewboard.mozilla.org/r/197444/#review203000

Modulo some grammatical nits, this looks good to me. Thanks!

::: commit-message-02246:13
(Diff revision 1)
> +   This is the unexpected case.
> +
> +Original windows implementation has following difference, but this patch will
> +not copy it:
> + * Windows has own NS_IMPL_ADDREF / NS_IMPL_RELEASE implementation, however this
> +   implementation doesn't different from XPCOM implementation. Bug 156318

Grammar nit: "doesn't" -> "isn't"

::: commit-message-02246:15
(Diff revision 1)
> +Original windows implementation has following difference, but this patch will
> +not copy it:
> + * Windows has own NS_IMPL_ADDREF / NS_IMPL_RELEASE implementation, however this
> +   implementation doesn't different from XPCOM implementation. Bug 156318
> +   doesn't show this reason.
> + * Windows implementation lack some of the function parameter check.

"lack" -> "lacks"

"some of the" -> "some", "check" -> "checks"

"this checks" -> "these checks".
Attachment #8926204 - Flags: review?(mconley) → review+

Comment 23

2 years ago
mozreview-review
Comment on attachment 8926205 [details]
Bug 1409972 - Part 4. Merge mac printingui.

https://reviewboard.mozilla.org/r/197446/#review203002
Attachment #8926205 - Flags: review?(mconley) → review+

Comment 24

2 years ago
mozreview-review
Comment on attachment 8926207 [details]
Bug 1409972 - Part 6. Remove printingui implementation of each platform.

https://reviewboard.mozilla.org/r/197450/#review203004

Assuming a green try run, looks good to me!

Because we don't have much in the way of automated testing for printing, we might want to get SoftVision's help to test printing on each platform, if we can.
Attachment #8926207 - Flags: review?(mconley) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 31

2 years ago
mozreview-review-reply
Comment on attachment 8926202 [details]
Bug 1409972 - Part 1. Apply clang-format to nsPrintingPromptService.

https://reviewboard.mozilla.org/r/197440/#review202958

Thanks!
I fixed these clanbot's alert to all platform implementations.
Assignee

Comment 32

2 years ago
mozreview-review-reply
Comment on attachment 8926204 [details]
Bug 1409972 - Part 3. Merge windows printingui.

https://reviewboard.mozilla.org/r/197444/#review203000

Oh,, Thank you very much for confirming this comments.
I updated a commit message!
Assignee

Comment 33

2 years ago
mozreview-review-reply
Comment on attachment 8926206 [details]
Bug 1409972 - Part 5. Use the common printingui implementation.

https://reviewboard.mozilla.org/r/197448/#review202936

> Since the local 'toolkit' variable is only used here, you can just do if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows' and remove the toolkit definition above.

Thanks!
Yes, This definition is not necesarry. I dropped this 'toolkit' variable from this file.

Comment 35

2 years ago
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3d465d9677d8
Part 1. Apply clang-format to nsPrintingPromptService. r=mconley
https://hg.mozilla.org/integration/autoland/rev/96fcac56b31e
Part 2. Copy printingui implementation from unixshared. r=mconley
https://hg.mozilla.org/integration/autoland/rev/fb5f9d6a7772
Part 3. Merge windows printingui. r=mconley
https://hg.mozilla.org/integration/autoland/rev/1d8e9cd8df19
Part 4. Merge mac printingui. r=mconley
https://hg.mozilla.org/integration/autoland/rev/614c8b4d4a0d
Part 5. Use the common printingui implementation. r=mshal
https://hg.mozilla.org/integration/autoland/rev/854a8759f855
Part 6. Remove printingui implementation of each platform. r=mconley
You need to log in before you can comment on or make changes to this bug.