Move native printing dialog code of windows to widget from toolkit.

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mantaroh, Assigned: mantaroh)

Tracking

(Blocks 1 bug)

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

A printingui toolkit has native print dialog code[1]. I think we can move this code to widget and we can share common printingui implementation for all platform[2].

[1] http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/toolkit/components/printingui/win/nsPrintingPromptService.cpp
[2] http://searchfox.org/mozilla-central/source/toolkit/components/printingui
Assignee

Updated

2 years ago
Attachment #8914619 - Attachment description: WIP:make-dialogservice → [WIP]make-dialogservice
Assignee

Comment 2

2 years ago
Posted patch [WIP]move-native-dialog (obsolete) — Splinter Review
Assignee

Updated

2 years ago
Attachment #8914619 - Attachment is patch: true
Assignee

Updated

2 years ago
Attachment #8914620 - Attachment is patch: true
Assignee

Comment 3

2 years ago
Assignee: nobody → mantaroh
Attachment #8914620 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee

Comment 4

2 years ago
Comment on attachment 8914621 [details] [diff] [review]
[WIP]move-native-dialog

This patch will change the SetWindowText() and CreateWindow() to SetWindowTextW() and CreateWindowW() in order to treat localized string correctly as well.

We can reproduce this localized string bug if print any page with <frame> tag on windows environment.

[1] http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/toolkit/components/printingui/win/nsPrintDialogUtil.cpp#126-127
Attachment #8914621 - Attachment is patch: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

2 years ago
Priority: -- → P3

Comment 8

2 years ago
mozreview-review
Comment on attachment 8914642 [details]
Bug 1405210 - Part 1: Add PrintDialogService to windows widget.

https://reviewboard.mozilla.org/r/185966/#review195450

::: widget/moz.build
(Diff revision 1)
>  toolkit = CONFIG['MOZ_WIDGET_TOOLKIT']
>  
>  if toolkit in ('cocoa', 'android', 'uikit'):
>      DIRS += [toolkit]
> -if toolkit in ('android', 'cocoa', 'gtk2', 'gtk3'):
> -    EXPORTS += ['nsIPrintDialogService.h']

note build changes will require a build config peer r+.

::: widget/windows/nsWindowBase.cpp:14
(Diff revision 1)
>  #include "mozilla/MiscEvents.h"
>  #include "KeyboardLayout.h"
>  #include "WinUtils.h"
>  #include "npapi.h"
>  #include "nsAutoPtr.h"
> +#include "nsIPresShell.h"

did we need this?
Attachment #8914642 - Flags: review?(jmathies) → review+

Comment 9

2 years ago
mozreview-review
Comment on attachment 8914643 [details]
Bug 1405210 - Part 2: Move native printing dialog code to windows widget.

https://reviewboard.mozilla.org/r/185968/#review195452

::: widget/windows/nsPrintDialogWin.cpp:31
(Diff revision 1)
>  
>  using namespace mozilla;
> +using namespace mozilla::widget;
> +
> +/****************************************************************
> + ************************* ParamBlock ***************************

slim this big ugly header comment down plz.

::: widget/windows/nsPrintDialogWin.cpp:37
(Diff revision 1)
> + ****************************************************************/
> +
> +class ParamBlock {
> +
> +public:
> +    ParamBlock()

nit - spacing is off

::: widget/windows/nsPrintDialogWin.cpp:95
(Diff revision 1)
> +  NS_ENSURE_ARG(aNSSettings);
> +
> +  ParamBlock block;
> +  nsresult rv = block.Init();
> +  if (NS_FAILED(rv))
> +    return rv;

nit - brace me

::: widget/windows/nsPrintDialogWin.cpp:103
(Diff revision 1)
> +  rv = DoDialog(aParent, block, aNSSettings, kPageSetupDialogURL);
> +
> +  // if aWebBrowserPrint is not null then we are printing
> +  // so we want to pass back NS_ERROR_ABORT on cancel
> +  if (NS_SUCCEEDED(rv))
> +  {

nit - move '{' up one line

::: widget/windows/nsPrintDialogWin.cpp:131
(Diff revision 1)
> +  // (though we'd rather this didn't fail, it's OK if it does. so there's
> +  // no failure or null check.)
> +  // retain ownership for method lifetime
> +  nsCOMPtr<mozIDOMWindowProxy> activeParent;
> +  if (!aParent)
> +  {

nit - move '{' up
Attachment #8914643 - Flags: review?(jmathies) → review+

Comment 10

2 years ago
mozreview-review
Comment on attachment 8914644 [details]
Bug 1405210 - Part 3: Apply clang-format to added print dialog widget code.

https://reviewboard.mozilla.org/r/185970/#review195454
Attachment #8914644 - Flags: review?(jmathies) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 14

2 years ago
mozreview-review-reply
Comment on attachment 8914642 [details]
Bug 1405210 - Part 1: Add PrintDialogService to windows widget.

https://reviewboard.mozilla.org/r/185966/#review195450

Thank you so much for reviewing.

> note build changes will require a build config peer r+.

OK.

ted,
Could you please review this spart?

> did we need this?

This will prevent to unified build failure.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 18

2 years ago
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #14)
> Comment on attachment 8914642 [details]
> Bug 1405210 - Part 1: Add PrintDialogService to windows widget.
> 
> https://reviewboard.mozilla.org/r/185966/#review195450
> 
> Thank you so much for reviewing.
> 
> > note build changes will require a build config peer r+.
> 
> OK.
> 
> ted,
> Could you please review this spart?

Oh, sorry. This r? is my mistake.

mshal,
Could you please confirm this part?

Comment 19

2 years ago
mozreview-review
Comment on attachment 8914642 [details]
Bug 1405210 - Part 1: Add PrintDialogService to windows widget.

https://reviewboard.mozilla.org/r/185966/#review196512

Build changes LGTM!
Attachment #8914642 - Flags: review?(mshal) → review+
Assignee

Comment 20

2 years ago
mozreview-review-reply
Comment on attachment 8914642 [details]
Bug 1405210 - Part 1: Add PrintDialogService to windows widget.

https://reviewboard.mozilla.org/r/185966/#review196512

Thank you!

Comment 22

2 years ago
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/64beab4cf557
Part 1: Add PrintDialogService to windows widget. r=jimm,mshal
https://hg.mozilla.org/integration/autoland/rev/bbc8f61facef
Part 2: Move native printing dialog code to windows widget. r=jimm
https://hg.mozilla.org/integration/autoland/rev/fbdb84f3a8c4
Part 3: Apply clang-format to added print dialog widget code. r=jimm
You need to log in before you can comment on or make changes to this bug.