Fixing spreadsheets printing.

Fixing spreadsheets printing.
Some plugins needs to have document loaded to decide if printing is supported. This patch makes scripted print preview for plugins to wait DidStopLoading().

BUG=268778
R=cevans@chromium.org, thestig@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221300

[Lei Zhang, 2013-09-03 20:07:26]

https://codereview.chromium.org/23855002/diff/14001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager.cc (right):

https://codereview.chromium.org/23855002/diff/14001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager.cc:177: return;
If we return early, we probably should reset some states, right?

https://codereview.chromium.org/23855002/diff/14001/chrome/renderer/printing/...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/23855002/diff/14001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper.cc:1707:
base::MessageLoop::current()->PostDelayedTask(
I don't think it's worth handling this case. If the plugin is hanging, then
waiting a minute still does not help because we'll go ahead and try to print,
which involves the plugin being responsive.

Maybe simply this to just the following?

if (is_loading_) {
  // wait for DidStopLoading()
} else {
  base::MessageLoop::current()->PostTask(FROM_HERE, task);
}

https://codereview.chromium.org/23855002/diff/14001/chrome/renderer/printing/...
File chrome/renderer/printing/print_web_view_helper.h (right):

https://codereview.chromium.org/23855002/diff/14001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper.h:65: public
base::SupportsWeakPtr<PrintWebViewHelper> {
A couple weeks ago, there was a "let's kill SupportsWeakPtr" thread on
chromium-dev. Can we use a WeakPtrFactory instead?

[Vitaly Buka (NO REVIEWS), 2013-09-04 00:22:39]

https://codereview.chromium.org/23855002/diff/14001/chrome/renderer/printing/...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/23855002/diff/14001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper.cc:1707:
base::MessageLoop::current()->PostDelayedTask(
On 2013/09/03 20:07:26, Lei Zhang wrote:
> I don't think it's worth handling this case. If the plugin is hanging, then
> waiting a minute still does not help because we'll go ahead and try to print,
> which involves the plugin being responsive.
> 
> Maybe simply this to just the following?
> 
> if (is_loading_) {
>   // wait for DidStopLoading()
> } else {
>   base::MessageLoop::current()->PostTask(FROM_HERE, task);
> }

Done.

[Lei Zhang, 2013-09-04 01:06:34]

There's still some un-addressed comments from patch set 2. Some nits below as
well.

https://codereview.chromium.org/23855002/diff/25001/chrome/common/print_messa...
File chrome/common/print_messages.h (right):

https://codereview.chromium.org/23855002/diff/25001/chrome/common/print_messa...
chrome/common/print_messages.h:441: // Shows Preview when is_modifiable is
availible.
nit: Without some context, readers are not going to understand what
"is_modifiable is available" means. How about:

// Tell the browser to show the print preview, when the document is sufficiently
loaded such that the renderer can determine whether it is modifiable or not.

https://codereview.chromium.org/23855002/diff/25001/chrome/renderer/printing/...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/23855002/diff/25001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper.cc:1694: // pumping messages
here.
nit: Align it like:
// 1. Foo ...
//    bar.

https://codereview.chromium.org/23855002/diff/25001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper.cc:1696: // if is_modifiable is
available.
"once the document has been loaded" ?

https://codereview.chromium.org/23855002/diff/25001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper.cc:1698: if (is_modifiable &&
is_loading_ &&
I recommend not checking GetPlugin() or |is_modifiable|, since it's trying to
guess what the PDF plugin is doing, even though the PDF plugin is not exposed
directly at this layer. Checking |is_loading_| alone should be sufficient in
solving our problem.

https://codereview.chromium.org/23855002/diff/25001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper.cc:1700: // Wait for
DidStopLoading. Plugins may change is_modifiable from True
nit: "Plugins may not know the correct |is_modifiable| value until they are
fully loaded, which occurs when DidStopLoading() is called. Defer showing the
preview until then."

[Vitaly Buka (NO REVIEWS), 2013-09-04 01:49:48]

https://codereview.chromium.org/23855002/diff/14001/chrome/renderer/printing/...
File chrome/renderer/printing/print_web_view_helper.h (right):

https://codereview.chromium.org/23855002/diff/14001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper.h:65: public
base::SupportsWeakPtr<PrintWebViewHelper> {
On 2013/09/03 20:07:26, Lei Zhang wrote:
> A couple weeks ago, there was a "let's kill SupportsWeakPtr" thread on
> chromium-dev. Can we use a WeakPtrFactory instead?

Done.

https://codereview.chromium.org/23855002/diff/25001/chrome/common/print_messa...
File chrome/common/print_messages.h (right):

https://codereview.chromium.org/23855002/diff/25001/chrome/common/print_messa...
chrome/common/print_messages.h:441: // Shows Preview when is_modifiable is
availible.
On 2013/09/04 01:06:34, Lei Zhang wrote:
> nit: Without some context, readers are not going to understand what
> "is_modifiable is available" means. How about:
> 
> // Tell the browser to show the print preview, when the document is
sufficiently
> loaded such that the renderer can determine whether it is modifiable or not.

Done.

https://codereview.chromium.org/23855002/diff/25001/chrome/renderer/printing/...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/23855002/diff/25001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper.cc:1694: // pumping messages
here.
On 2013/09/04 01:06:34, Lei Zhang wrote:
> nit: Align it like:
> // 1. Foo ...
> //    bar.

Done.

https://codereview.chromium.org/23855002/diff/25001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper.cc:1696: // if is_modifiable is
available.
On 2013/09/04 01:06:34, Lei Zhang wrote:
> "once the document has been loaded" ?

Done.

https://codereview.chromium.org/23855002/diff/25001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper.cc:1698: if (is_modifiable &&
is_loading_ &&
I afraid that we can get some frames that loading indefinitely, buy modified
itself with scripts.

How about this?

On 2013/09/04 01:06:34, Lei Zhang wrote:
> I recommend not checking GetPlugin() or |is_modifiable|, since it's trying to
> guess what the PDF plugin is doing, even though the PDF plugin is not exposed
> directly at this layer. Checking |is_loading_| alone should be sufficient in
> solving our problem.

https://codereview.chromium.org/23855002/diff/25001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper.cc:1700: // Wait for
DidStopLoading. Plugins may change is_modifiable from True
On 2013/09/04 01:06:34, Lei Zhang wrote:
> nit: "Plugins may not know the correct |is_modifiable| value until they are
> fully loaded, which occurs when DidStopLoading() is called. Defer showing the
> preview until then."

Done.

[Lei Zhang, 2013-09-04 02:18:11]

https://codereview.chromium.org/23855002/diff/25001/chrome/renderer/printing/...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/23855002/diff/25001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper.cc:1698: if (is_modifiable &&
is_loading_ &&
On 2013/09/04 01:49:49, Vitaly Buka wrote:
> I afraid that we can get some frames that loading indefinitely, buy modified
> itself with scripts.

Sure.

https://codereview.chromium.org/23855002/diff/32001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager.cc (right):

https://codereview.chromium.org/23855002/diff/32001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager.cc:100: if (print_preview_state_ !=
NOT_PREVIEWING)
Do you have an example test case where the DCHECK fails?

https://codereview.chromium.org/23855002/diff/32001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager.cc:177: return;
Still need to do cleanup in this case.

[Vitaly Buka (NO REVIEWS), 2013-09-04 02:52:53]

https://codereview.chromium.org/23855002/diff/32001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager.cc (right):

https://codereview.chromium.org/23855002/diff/32001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager.cc:100: if (print_preview_state_ !=
NOT_PREVIEWING)
On 2013/09/04 02:18:12, Lei Zhang wrote:
> Do you have an example test case where the DCHECK fails?
Preview waits for didStopLoading and I click print inside of PDF button

https://codereview.chromium.org/23855002/diff/32001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager.cc:177: return;
On 2013/09/04 02:18:12, Lei Zhang wrote:
> Still need to do cleanup in this case.

Done.

[Lei Zhang, 2013-09-04 03:08:23]

lgtm

[Vitaly Buka (NO REVIEWS), 2013-09-04 03:11:43]

https://codereview.chromium.org/23855002/diff/32001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager.cc (right):

https://codereview.chromium.org/23855002/diff/32001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager.cc:100: if (print_preview_state_ !=
NOT_PREVIEWING)
On 2013/09/04 02:52:53, Vitaly Buka wrote:
> On 2013/09/04 02:18:12, Lei Zhang wrote:
> > Do you have an example test case where the DCHECK fails?
> Preview waits for didStopLoading and I click print inside of PDF button

"call stack" looks something like this, so PrintPreviewForWebNode is earliest
location to detect this.

PrintViewManager::PrintPreviewForWebNode
PrintPreviewMessageHandler::OnRequestPrintPreview
PrintHostMsg_RequestPrintPreview
PrintWebViewHelper::RequestPrintPreview
PrintWebViewHelper::PrintPage

[Vitaly Buka (NO REVIEWS), 2013-09-04 03:14:45]

Lei, thanks for review.

+jln for chrome/common/print_messages.h

[Chris Evans, 2013-09-04 20:27:04]

https://codereview.chromium.org/23855002/diff/13001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager.cc (right):

https://codereview.chromium.org/23855002/diff/13001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager.cc:180:
dialog_controller->PrintPreview(web_contents());
Is it safe to call this multiple times? The untrusted renderer can now spam
multiple OnShowScriptedPrintPreview() in a row.

Previously, when there was only OnSetupScriptedPrintPreview(), this would have
been prevented by the check "if (map.count(rph) != 0)"

[Vitaly Buka (NO REVIEWS), 2013-09-04 20:56:13]

https://codereview.chromium.org/23855002/diff/13001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager.cc (right):

https://codereview.chromium.org/23855002/diff/13001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager.cc:180:
dialog_controller->PrintPreview(web_contents());
Only first call to dialog_controller->PrintPreview creates preview.
PrintPreviewUI::SetInitialParams also could be called multiple times safe.



On 2013/09/04 20:27:04, Chris Evans wrote:
> Is it safe to call this multiple times? The untrusted renderer can now spam
> multiple OnShowScriptedPrintPreview() in a row.
> 
> Previously, when there was only OnSetupScriptedPrintPreview(), this would have
> been prevented by the check "if (map.count(rph) != 0)"

[Chris Evans, 2013-09-04 20:57:36]

On 2013/09/04 20:56:13, Vitaly Buka wrote:
>
https://codereview.chromium.org/23855002/diff/13001/chrome/browser/printing/p...
> File chrome/browser/printing/print_view_manager.cc (right):
> 
>
https://codereview.chromium.org/23855002/diff/13001/chrome/browser/printing/p...
> chrome/browser/printing/print_view_manager.cc:180:
> dialog_controller->PrintPreview(web_contents());
> Only first call to dialog_controller->PrintPreview creates preview.
> PrintPreviewUI::SetInitialParams also could be called multiple times safe.
> 
> 
> 
> On 2013/09/04 20:27:04, Chris Evans wrote:
> > Is it safe to call this multiple times? The untrusted renderer can now spam
> > multiple OnShowScriptedPrintPreview() in a row.
> > 
> > Previously, when there was only OnSetupScriptedPrintPreview(), this would
have
> > been prevented by the check "if (map.count(rph) != 0)"

LGTM

[commit-bot: I haz the power, 2013-09-04 20:59:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/23855002/13001

[commit-bot: I haz the power, 2013-09-04 20:59:37]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2013-09-04 21:07:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/23855002/49001

[Vitaly Buka (NO REVIEWS), 2013-09-04 23:45:34]

Committed patchset #6 manually as r221300 (presubmit successful).