Closed Bug 1411121 Opened 7 years ago Closed 7 years ago

Remove the printdialog.xul and related files.

Categories

(Toolkit :: Printing, enhancement, P3)

Product:
Toolkit
Component:
Printing
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

References

Details

Attachments

(4 files)

Bug 1411121 - Part 1: Return NS_ERROR_FAILURE if nsIPrintDialogService doesn't exist on GTK when showing print dialog.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1411121 - Part 2: Remove printdialog.xul and printjoboptions.xul.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1411121 - Part 3: Remove the nsIPrintingPromptService::showPrinterProperties()
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1411121 - Part 4. Remove printPageSetup resources from linux package since Linux will display native dialog.
59 bytes, text/x-review-board-request
mconley
: review+
Details 
+++ 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 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)
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 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+
(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 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 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+
Thank you so much!
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)
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
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)
(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 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+
(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
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.

Attachment

General

Creator:
Created:
Updated:
Size: