Print Preview: Display a throbber when the user requests the system print

Print Preview: Display a throbber when the user requests the system print
dialog.

BUG=83256
TEST=none

R=thestig@chromium.org,dpapad@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91001

[James Hawkins, 2011-06-20 23:52:46]

New CL routes a message to the PrintPreviewHandler that the print dialog has
been shown. The PrintPreviewHandler waits for this message before closing
itself.

[dpapad, 2011-06-21 01:08:46]

On 2011/06/20 23:52:46, James Hawkins wrote:
> New CL routes a message to the PrintPreviewHandler that the print dialog has
> been shown. The PrintPreviewHandler waits for this message before closing
> itself.

LGTM on the JS/CSS/HTML part. I could not test it though because the patch was
failing.

[James Hawkins, 2011-06-21 01:10:01]

On 2011/06/21 01:08:46, dpapad wrote:
> On 2011/06/20 23:52:46, James Hawkins wrote:
> > New CL routes a message to the PrintPreviewHandler that the print dialog has
> > been shown. The PrintPreviewHandler waits for this message before closing
> > itself.
> 
> LGTM on the JS/CSS/HTML part. I could not test it though because the patch was
> failing.

Please update to trunk and try again.

[dpapad, 2011-06-21 01:39:00]

On 2011/06/21 01:10:01, James Hawkins wrote:
> On 2011/06/21 01:08:46, dpapad wrote:
> > On 2011/06/20 23:52:46, James Hawkins wrote:
> > > New CL routes a message to the PrintPreviewHandler that the print dialog
has
> > > been shown. The PrintPreviewHandler waits for this message before closing
> > > itself.
> > 
> > LGTM on the JS/CSS/HTML part. I could not test it though because the patch
was
> > failing.
> 
> Please update to trunk and try again.

Summarizing our online conversation, we should remove all event listeners
(including that of the "Advanced" link) once the user clicks on it. Also I think
that people from the UI leads should also take a look (after landing, on canary
is ok with me).

[Lei Zhang, 2011-06-21 01:54:46]

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.cc:132:
delegate_->OnPrintDialogShown();
set |delegate_| to NULL here instead of below?

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager.h (right):

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.h:45: bool
PrintNow(PrintViewManagerDelegate* delegate);
Since we expect |delegate| to be NULL most of the time, can we leave PrintNow()
as it was before and add PrintNowWithDelegate(PVMD* delegate)?

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager_delegate.h (right):

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager_delegate.h:12: class
PrintViewManagerDelegate {
What's your thought on printing::PrintViewManagerDelegate vs.
printing::PrintViewManager::Delegate ?

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:633: if (!initiator_tab)
FWIW, I don't think is can happen.

http://codereview.chromium.org/7202012/diff/6001/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7202012/diff/6001/chrome/common/print_messages...
chrome/common/print_messages.h:182:
IPC_MESSAGE_ROUTED0(PrintHostMsg_DidShowPrintDialog)
You can avoid adding a new message, the additional IPC and the associated
PrintWebViewHelper / PrintViewManager changes, if you just instead add a call to
print_job_manager_->OnDidShowPrintDialog() in
PrintingMessageFilter::OnScriptedPrint() to keep this all on the browser side.

[James Hawkins, 2011-06-22 23:47:24]

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.cc:132:
delegate_->OnPrintDialogShown();
On 2011/06/21 01:54:46, Lei Zhang wrote:
> set |delegate_| to NULL here instead of below?

No, why would we remove the delegate here? That's relying on the assumption that
the delegate wants no more messages after this call, or that this object will
never make another call to the delegate (which may not always be true).

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager.h (right):

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.h:45: bool
PrintNow(PrintViewManagerDelegate* delegate);
On 2011/06/21 01:54:46, Lei Zhang wrote:
> Since we expect |delegate| to be NULL most of the time, can we leave
PrintNow()
> as it was before and add PrintNowWithDelegate(PVMD* delegate)?

I'd rather not, as that complicates the API unnecessarily by adding another
method.

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager_delegate.h (right):

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager_delegate.h:12: class
PrintViewManagerDelegate {
On 2011/06/21 01:54:46, Lei Zhang wrote:
> What's your thought on printing::PrintViewManagerDelegate vs.
> printing::PrintViewManager::Delegate ?

As per off-line discussion, leaving as is.

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:633: if (!initiator_tab)
On 2011/06/21 01:54:46, Lei Zhang wrote:
> FWIW, I don't think is can happen.

It can (just tested).

http://codereview.chromium.org/7202012/diff/6001/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7202012/diff/6001/chrome/common/print_messages...
chrome/common/print_messages.h:182:
IPC_MESSAGE_ROUTED0(PrintHostMsg_DidShowPrintDialog)
On 2011/06/21 01:54:46, Lei Zhang wrote:
> You can avoid adding a new message, the additional IPC and the associated
> PrintWebViewHelper / PrintViewManager changes, if you just instead add a call
to
> print_job_manager_->OnDidShowPrintDialog() in
> PrintingMessageFilter::OnScriptedPrint() to keep this all on the browser side.

There are several reasons why this doesn't work:
* PrintJobManager has no access to the PrintViewManagerDelegate.
* Currently OnScriptedPrint is the only initiator of the code path to show the
native print dialog for settings, but that may not always be the case in the
future: this notification is designed to be sent in all cases where the native
print dialog is shown. Moving the notification to OnScriptedPrint is fragile in
this sense.

[James Hawkins, 2011-06-22 23:49:32]

Forgot to mention: the latest CL disables the controls per dpapad's comments.

[James Hawkins, 2011-06-23 01:36:33]

Addresses some of dpapad's concerns re: UI.

[dpapad, 2011-06-23 02:06:46]

LGTM with one comment.

http://codereview.chromium.org/7202012/diff/13006/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.css (right):

http://codereview.chromium.org/7202012/diff/13006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.css:178:
#system-dialog-link {
You need to add here
-webkit-padding-start: 0
to override the padding of the button class, so that the text is displayed
aligned with the other labels.

[Lei Zhang, 2011-06-23 17:08:45]

How do you manage the delegate's lifetime? If you add a delegate, and destroy
the delegate (aka close the preview tab) before you get to
PrintViewManager::OnDidShowPrintDialog(), you end up with a dangling pointer.

In the case where there is no PDF plugin, the main view has a button to launch
the native print dialog. When you press that button, the throbber shows up next
to the advanced link, which is a bit weird.

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:633: if (!initiator_tab)
On 2011/06/22 23:47:24, James Hawkins wrote:
> On 2011/06/21 01:54:46, Lei Zhang wrote:
> > FWIW, I don't think is can happen.
> 
> It can (just tested).

How?

[James Hawkins, 2011-06-27 21:58:17]

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7202012/diff/6001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:633: if (!initiator_tab)
On 2011/06/23 17:08:45, Lei Zhang wrote:
> On 2011/06/22 23:47:24, James Hawkins wrote:
> > On 2011/06/21 01:54:46, Lei Zhang wrote:
> > > FWIW, I don't think is can happen.
> > 
> > It can (just tested).
> 
> How?

Just tested again and it's now not possible, though I don't know what
necessarily changed. Made this a DCHECK.

http://codereview.chromium.org/7202012/diff/13006/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.css (right):

http://codereview.chromium.org/7202012/diff/13006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.css:178:
#system-dialog-link {
On 2011/06/23 02:06:47, dpapad wrote:
> You need to add here
> -webkit-padding-start: 0
> to override the padding of the button class, so that the text is displayed
> aligned with the other labels.

Done.

[James Hawkins, 2011-06-27 21:59:48]

On 2011/06/23 17:08:45, Lei Zhang wrote:
> How do you manage the delegate's lifetime? If you add a delegate, and destroy
> the delegate (aka close the preview tab) before you get to
> PrintViewManager::OnDidShowPrintDialog(), you end up with a dangling pointer.
> 

PrintPreviewHandler disconnects itself in a method OnNavigate, which is called
for any navigations which would destroy said object or when the tab is closing. 
Ideally we'd hook this up in the destructor, but the PrintPreviewTabController
destroys the association before the destructor is reached.

> In the case where there is no PDF plugin, the main view has a button to launch
> the native print dialog. When you press that button, the throbber shows up
next
> to the advanced link, which is a bit weird.
> 

Fixed.

>
http://codereview.chromium.org/7202012/diff/6001/chrome/browser/ui/webui/prin...
> File chrome/browser/ui/webui/print_preview_handler.cc (right):
> 
>
http://codereview.chromium.org/7202012/diff/6001/chrome/browser/ui/webui/prin...
> chrome/browser/ui/webui/print_preview_handler.cc:633: if (!initiator_tab)
> On 2011/06/22 23:47:24, James Hawkins wrote:
> > On 2011/06/21 01:54:46, Lei Zhang wrote:
> > > FWIW, I don't think is can happen.
> > 
> > It can (just tested).
> 
> How?

[dpapad, 2011-06-27 22:17:07]

Would it look better to have the throbber within the button instead of next to
it?

http://codereview.chromium.org/7202012/diff/21014/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7202012/diff/21014/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:212: * Similar to
onAdvancedClick(), but specific to the
s/onAdvancedClick/onSystemDialogLinkClicked?

[James Hawkins, 2011-06-27 22:21:32]

http://codereview.chromium.org/7202012/diff/21014/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7202012/diff/21014/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:212: * Similar to
onAdvancedClick(), but specific to the
On 2011/06/27 22:17:07, dpapad wrote:
> s/onAdvancedClick/onSystemDialogLinkClicked?

Done.

[James Hawkins, 2011-06-27 22:21:48]

On 2011/06/27 22:17:07, dpapad wrote:
> Would it look better to have the throbber within the button instead of next to
> it?
> 

No.

[James Hawkins, 2011-06-28 02:19:45]

gong

[dpapad, 2011-06-28 02:25:30]

reLGTM with one comment.

http://codereview.chromium.org/7202012/diff/22021/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7202012/diff/22021/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:73:
$('print-button').focus();
Why remove the autofocus attribute from the html and add this here?

[James Hawkins, 2011-06-28 04:02:34]

Demetrios: Fixed the crasher your noticed.

Lei: Waiting on your LGTM.

http://codereview.chromium.org/7202012/diff/22021/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7202012/diff/22021/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:73:
$('print-button').focus();
On 2011/06/28 02:25:30, dpapad wrote:
> Why remove the autofocus attribute from the html and add this here?

autofocus is somewhat crappy and focuses disabled fields; we'd have to call
blur() when disabling input fields otherwise, which is even more hacky. Also,
all other WebUI explicitly sets focus using JS focus() (except for a loner in
bookmark manager).

[Lei Zhang, 2011-06-28 09:15:38]

1. If you open print preview, and close the initiator tab. The 'advanced' link
still works and clicking it displays the throbber forever.


If popping up the print dialog takes a long time, say, with an artificial sleep,
you run into a few more corner cases:

2. Closing the initiator tab means the print dialog never opens. This leaves the
print preview tab with the throbber running forever.

3. Trying to navigate in the initiator tab results in a print dialog, but
printing to file doesn't do anything.

http://codereview.chromium.org/7202012/diff/25002/chrome/browser/printing/pri...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/7202012/diff/25002/chrome/browser/printing/pri...
chrome/browser/printing/print_view_manager.cc:86: DCHECK(!observer ||
!observer_);
Both |observer| and |observer_| can be NULL, which doesn't match the comment in
the header.

http://codereview.chromium.org/7202012/diff/25002/chrome/browser/printing/pri...
File chrome/browser/printing/print_view_manager_observer.h (right):

http://codereview.chromium.org/7202012/diff/25002/chrome/browser/printing/pri...
chrome/browser/printing/print_view_manager_observer.h:12: // dialog is shown.
Register the observer via PrintViewManager::SetObserver.
s/SetObserver/set_observer/

http://codereview.chromium.org/7202012/diff/25002/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.h (right):

http://codereview.chromium.org/7202012/diff/25002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.h:46: //
comment?

[James Hawkins, 2011-06-28 18:15:36]

http://codereview.chromium.org/7202012/diff/25002/chrome/browser/printing/pri...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/7202012/diff/25002/chrome/browser/printing/pri...
chrome/browser/printing/print_view_manager.cc:86: DCHECK(!observer ||
!observer_);
On 2011/06/28 09:15:38, Lei Zhang wrote:
> Both |observer| and |observer_| can be NULL, which doesn't match the comment
in
> the header.

Thanks, done.

http://codereview.chromium.org/7202012/diff/25002/chrome/browser/printing/pri...
File chrome/browser/printing/print_view_manager_observer.h (right):

http://codereview.chromium.org/7202012/diff/25002/chrome/browser/printing/pri...
chrome/browser/printing/print_view_manager_observer.h:12: // dialog is shown.
Register the observer via PrintViewManager::SetObserver.
On 2011/06/28 09:15:38, Lei Zhang wrote:
> s/SetObserver/set_observer/

Done.

http://codereview.chromium.org/7202012/diff/25002/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.h (right):

http://codereview.chromium.org/7202012/diff/25002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.h:46: //
On 2011/06/28 09:15:38, Lei Zhang wrote:
> comment?

Whoops, done.

[James Hawkins, 2011-06-28 18:17:34]

On 2011/06/28 09:15:38, Lei Zhang wrote:
> 1. If you open print preview, and close the initiator tab. The 'advanced' link
> still works and clicking it displays the throbber forever.
> 

Fixed.

> 
> If popping up the print dialog takes a long time, say, with an artificial
sleep,
> you run into a few more corner cases:
> 
> 2. Closing the initiator tab means the print dialog never opens. This leaves
the
> print preview tab with the throbber running forever.
> 

Fixed.

> 3. Trying to navigate in the initiator tab results in a print dialog, but
> printing to file doesn't do anything.
> 

I don't understand the repro steps here.

[James Hawkins, 2011-06-28 20:05:47]

http://codereview.chromium.org/7202012/diff/25002/chrome/browser/printing/pri...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/7202012/diff/25002/chrome/browser/printing/pri...
chrome/browser/printing/print_view_manager.cc:86: DCHECK(!observer ||
!observer_);
On 2011/06/28 18:15:36, James Hawkins wrote:
> On 2011/06/28 09:15:38, Lei Zhang wrote:
> > Both |observer| and |observer_| can be NULL, which doesn't match the comment
> in
> > the header.
> 
> Thanks, done.

Actually, what I really wanted to do was just fix the comment. Done.

[James Hawkins, 2011-06-28 20:39:04]

Please take another look.

[Lei Zhang, 2011-06-28 20:55:35]

I can show you case 3 in person.

http://codereview.chromium.org/7202012/diff/22043/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_tab_controller.cc (right):

http://codereview.chromium.org/7202012/diff/22043/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:142:
static_cast<PrintPreviewUI*>(source_tab->web_ui());
For the case of: open print preview; close initiator tab; click "reopen the
page", normally you would get NULL here and skip the OnNavigation() call.
However, if the initiator tab is a WebUI page, e.g. the NTP, then you would get
a WebUI back and incorrectly cast it to a PrintPreviewUI.

[Lei Zhang, 2011-06-28 20:58:04]

http://codereview.chromium.org/7202012/diff/22043/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_tab_controller.cc (right):

http://codereview.chromium.org/7202012/diff/22043/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:142:
static_cast<PrintPreviewUI*>(source_tab->web_ui());
On 2011/06/28 20:55:35, Lei Zhang wrote:
> For the case of: open print preview; close initiator tab; click "reopen the
> page", normally you would get NULL here and skip the OnNavigation() call.
> However, if the initiator tab is a WebUI page, e.g. the NTP, then you would
get
> a WebUI back and incorrectly cast it to a PrintPreviewUI.

Probably just want to check with IsPrintPreviewTab(source_tab) here.

[Lei Zhang, 2011-06-29 02:15:39]

LGTM