Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2961863002:
Print Preview: close dialog when system dialog is open (Win) (Closed)
DescriptionPrint Preview: close dialog when system dialog is open (Win)
When the system dialog opens on Windows, this is sent through the normal
print message. However, since the dialog is modal, the promise will not
be resolved until the dialog closes if the message is sent to the
renderer first. Resolve the promise before forwarding settings to the
renderer.
BUG=737035
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2961863002
Cr-Commit-Position: refs/heads/master@{#482855}
Committed: https://chromium.googlesource.com/chromium/src/+/63f156d9c2dfd6a04d1a49a58a1c0178b1e3f6e1
Patch Set 1 #Patch Set 2 : Change to always resolve promise #Patch Set 3 : Adjust comment #
Total comments: 6
Patch Set 4 : Address comments #
Messages
Total messages: 26 (20 generated)
Description was changed from ========== Print Preview: close dialog when system dialog is open (Win) When the system dialog opens on Windows, this is sent through the normal print message. However, since the dialog is modal, the promise will not be resolved until the dialog closes. Hide the print preview dialog from C++ and do not wait for promise resolution on the JS side. BUG=737035 ========== to ========== Print Preview: close dialog when system dialog is open (Win) When the system dialog opens on Windows, this is sent through the normal print message. However, since the dialog is modal, the promise will not be resolved until the dialog closes. Hide the print preview dialog from C++ and do not wait for promise resolution on the JS side. BUG=737035 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Print Preview: close dialog when system dialog is open (Win) When the system dialog opens on Windows, this is sent through the normal print message. However, since the dialog is modal, the promise will not be resolved until the dialog closes. Hide the print preview dialog from C++ and do not wait for promise resolution on the JS side. BUG=737035 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Print Preview: close dialog when system dialog is open (Win) When the system dialog opens on Windows, this is sent through the normal print message. However, since the dialog is modal, the promise will not be resolved until the dialog closes. Hide the print preview dialog from C++ and do not wait for promise resolution on the JS side. BUG=737035 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
rbpotter@chromium.org changed reviewers: + dpapad@chromium.org, thestig@chromium.org
Description was changed from ========== Print Preview: close dialog when system dialog is open (Win) When the system dialog opens on Windows, this is sent through the normal print message. However, since the dialog is modal, the promise will not be resolved until the dialog closes. Hide the print preview dialog from C++ and do not wait for promise resolution on the JS side. BUG=737035 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Print Preview: close dialog when system dialog is open (Win) When the system dialog opens on Windows, this is sent through the normal print message. However, since the dialog is modal, the promise will not be resolved until the dialog closes if the message is sent to the renderer first. Resolve the promise before forwarding settings to the renderer. BUG=737035 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
lgtm https://codereview.chromium.org/2961863002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2961863002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1041: // the renderer. for additional context: the renderer in HandleHidePreview(). https://codereview.chromium.org/2961863002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1088: settings_ = nullptr; Let's call settings_.reset() so it's more obvious that it's a smart pointer and not a raw pointer. https://codereview.chromium.org/2961863002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2961863002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.h:435: // Print settings to use in the print request. Mention this is for a local print request?
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2961863002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2961863002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1041: // the renderer. On 2017/06/27 21:58:48, Lei Zhang wrote: > for additional context: > > the renderer in HandleHidePreview(). Done. https://codereview.chromium.org/2961863002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1088: settings_ = nullptr; On 2017/06/27 21:58:48, Lei Zhang wrote: > Let's call settings_.reset() so it's more obvious that it's a smart pointer and > not a raw pointer. Done. https://codereview.chromium.org/2961863002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2961863002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.h:435: // Print settings to use in the print request. On 2017/06/27 21:58:48, Lei Zhang wrote: > Mention this is for a local print request? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rbpotter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2961863002/#ps60001 (title: "Address comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498616645431980,
"parent_rev": "2f502a91eb07a2c8f68f9c83d250a5d39c76327f", "commit_rev":
"63f156d9c2dfd6a04d1a49a58a1c0178b1e3f6e1"}
Message was sent while issue was closed.
Description was changed from ========== Print Preview: close dialog when system dialog is open (Win) When the system dialog opens on Windows, this is sent through the normal print message. However, since the dialog is modal, the promise will not be resolved until the dialog closes if the message is sent to the renderer first. Resolve the promise before forwarding settings to the renderer. BUG=737035 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Print Preview: close dialog when system dialog is open (Win) When the system dialog opens on Windows, this is sent through the normal print message. However, since the dialog is modal, the promise will not be resolved until the dialog closes if the message is sent to the renderer first. Resolve the promise before forwarding settings to the renderer. BUG=737035 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2961863002 Cr-Commit-Position: refs/heads/master@{#482855} Committed: https://chromium.googlesource.com/chromium/src/+/63f156d9c2dfd6a04d1a49a58a1c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/63f156d9c2dfd6a04d1a49a58a1c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
