Remove the printdialog.xul and related files.

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mantaroh, Assigned: mantaroh)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(4 attachments)

+++ This bug was initially created as a clone of Bug #1409972 +++

We can remove the printing xul dialog(i.e. printdialog.xul) from printingui/unixshared/nsPrintingPromptService.cpp since this code is unreachable now.
This code introduced bug 193001. In this era, gecko support BeOS and QNX, this platform uses embedding/components/printingui/src/unixshared/nsPrintingPromptService.cpp[1] and these doesn't have implementation of nsIPrintDialogSerice[2].

[1] https://github.com/mozilla/gecko-dev/blob/9e6a8820f7964ed36d63acf13b84b0cde12caaa6/embedding/components/printingui/src/Makefile.in#L46
[2] https://github.com/mozilla/gecko-dev/tree/9e6a8820f7964ed36d63acf13b84b0cde12caaa6/widget/src
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8921331 [details]
Bug 1411121 - Part 1: Return NS_ERROR_FAILURE if nsIPrintDialogService doesn't exist on GTK when showing print dialog.

https://reviewboard.mozilla.org/r/192320/#review197922

::: commit-message-b6ba3:4
(Diff revision 1)
> +are not nsIPrintDialogService. But this code is unreachable code since
> +current gtk widget has nsIPrintDialogService always.

What prevents this code from being called in the Android port?
Attachment #8921331 - Flags: review?(karlt)
Assignee

Comment 6

2 years ago
mozreview-review
Comment on attachment 8921331 [details]
Bug 1411121 - Part 1: Return NS_ERROR_FAILURE if nsIPrintDialogService doesn't exist on GTK when showing print dialog.

https://reviewboard.mozilla.org/r/192320/#review197978

::: commit-message-b6ba3:4
(Diff revision 1)
> +are not nsIPrintDialogService. But this code is unreachable code since
> +current gtk widget has nsIPrintDialogService always.

Thank you so much for review this.

I think that Android platform will not call this nsIPrintingPromptService::ShowPrintDialog/ShowPageSetup. Because android calls silent print always.[1][2]. If silent print is enable, print engine will not call this nsIPrintingPromptService::ShowDialog on layout[3].

[1] http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#3922,3928
[2] http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/mobile/android/chrome/content/PrintHelper.js#31
[3] http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/layout/printing/nsPrintEngine.cpp#600,617

Comment 7

2 years ago
mozreview-review
Comment on attachment 8921331 [details]
Bug 1411121 - Part 1: Return NS_ERROR_FAILURE if nsIPrintDialogService doesn't exist on GTK when showing print dialog.

https://reviewboard.mozilla.org/r/192320/#review198450

Thanks!
Attachment #8921331 - Flags: review+
Assignee

Comment 8

2 years ago
(In reply to Karl Tomlinson (:karlt) from comment #7)
> Comment on attachment 8921331 [details]
> Bug 1411121 - Part 1: Return NS_ERROR_FAILURE if nsIPrintDialogService
> doesn't exist on GTK when showing print dialog.
> 
> https://reviewboard.mozilla.org/r/192320/#review198450
> 
> Thanks!

Thank you for confirming this!
Priority: -- → P3

Comment 9

2 years ago
mozreview-review
Comment on attachment 8921332 [details]
Bug 1411121 - Part 2: Remove printdialog.xul and printjoboptions.xul.

https://reviewboard.mozilla.org/r/192322/#review200198

Great, thanks!
Attachment #8921332 - Flags: review?(mconley) → review+

Comment 10

2 years ago
mozreview-review
Comment on attachment 8921333 [details]
Bug 1411121 - Part 3: Remove the nsIPrintingPromptService::showPrinterProperties()

https://reviewboard.mozilla.org/r/192332/#review200200

Thanks for the clean-up!
Attachment #8921333 - Flags: review?(mconley) → review+
Assignee

Comment 11

2 years ago
Thank you so much!

Comment 12

2 years ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s dc18133ffbc3 -d eb4f3467df5c: rebasing 431361:dc18133ffbc3 "Bug 1411121 - Part 1: Return NS_ERROR_FAILURE if nsIPrintDialogService doesn't exist on GTK when showing print dialog. r=karlt"
merging toolkit/components/printingui/unixshared/nsPrintingPromptService.cpp
warning: conflicts while merging toolkit/components/printingui/unixshared/nsPrintingPromptService.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

2 years ago
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d390f8888d71
Part 1: Return NS_ERROR_FAILURE if nsIPrintDialogService doesn't exist on GTK when showing print dialog. r=karlt
https://hg.mozilla.org/integration/autoland/rev/9d8722dd9693
Part 2: Remove printdialog.xul and printjoboptions.xul. r=mconley
https://hg.mozilla.org/integration/autoland/rev/9f02fcb3d28c
Part 3: Remove the nsIPrintingPromptService::showPrinterProperties() r=mconley
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 21

2 years ago
https://reviewboard.mozilla.org/r/192318/diff/2-3/

This test checks that package contain the correct resources, So I should remove the printdialog.dtd and printjoboptions.dtd from this test.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b18702a1f1380187bbb60848e131d7bf2148546
Flags: needinfo?(mantaroh)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 26

2 years ago
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #21)
> https://reviewboard.mozilla.org/r/192318/diff/2-3/
> 
> This test checks that package contain the correct resources, So I should
> remove the printdialog.dtd and printjoboptions.dtd from this test.
> 
> Try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5b18702a1f1380187bbb60848e131d7bf2148546

https://treeherder.mozilla.org/logviewer.html#?job_id=141518233&repo=try

Linux test is failed. I think that my patch will remove reference of printPageSetup.xul on Linux, as result, this test will fail.
So I need to remove printPageSetup's resources from linux package.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=187044c21482f54f03efd835ff00803975462c5c

Mike,
I'm Sorry to bother you. Would you please confirm last patch?

Comment 27

2 years ago
mozreview-review
Comment on attachment 8924453 [details]
Bug 1411121 - Part 4. Remove printPageSetup resources from linux package since Linux will display native dialog.

https://reviewboard.mozilla.org/r/195716/#review201126

Looks good! Might be worth a try build though just to make sure we didn't miss anything. Thanks!
Attachment #8924453 - Flags: review?(mconley) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 32

2 years ago
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #31)
> Comment on attachment 8924453 [details]
> Bug 1411121 - Part 4. Remove printPageSetup resources from linux package
> since Linux will display native dialog.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/195716/diff/1-2/

Rebased it.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccdadaab42f1bc1851d105a597b7a995d420adcb

Comment 33

2 years ago
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b503682a7c6e
Part 1: Return NS_ERROR_FAILURE if nsIPrintDialogService doesn't exist on GTK when showing print dialog. r=karlt
https://hg.mozilla.org/integration/autoland/rev/1ca360e19880
Part 2: Remove printdialog.xul and printjoboptions.xul. r=mconley
https://hg.mozilla.org/integration/autoland/rev/db6983d22fd3
Part 3: Remove the nsIPrintingPromptService::showPrinterProperties() r=mconley
https://hg.mozilla.org/integration/autoland/rev/110873e70443
Part 4. Remove printPageSetup resources from linux package since Linux will display native dialog. r=mconley
You need to log in before you can comment on or make changes to this bug.