Reuse PrintContext to excessively triggering matchMedia('print') listeners.

Reuse PrintContext to excessively triggering matchMedia('print') listeners.

BUG=85013
TEST=TBD

[Lei Zhang, 2011-09-22 21:48:30]

http://codereview.chromium.org/7993005/diff/1/chrome/renderer/print_web_view_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7993005/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper.cc:835: if (!RenderPagesForPrint(frame,
node, prepare.get())) {
Not sure if this is right. If you use the |prepare| from
InitPrintSettingsAndPrepareFrame(), you are going to be using the default
settings in |print_pages_params_| from there and not the new settings generated
in GetPrintSettingsFromUser().

[dominicc (has gone to gerrit), 2011-10-24 08:45:13]

I think this is good to go now.

This needs to land in conjunction with rolling in the fix to WK68614.

http://codereview.chromium.org/7993005/diff/1/chrome/renderer/print_web_view_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7993005/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper.cc:835: if (!RenderPagesForPrint(frame,
node, prepare.get())) {
On 2011/09/22 21:48:30, Lei Zhang wrote:
> Not sure if this is right. If you use the |prepare| from
> InitPrintSettingsAndPrepareFrame(), you are going to be using the default
> settings in |print_pages_params_| from there and not the new settings
generated
> in GetPrintSettingsFromUser().

This code is quite twisty, but don’t the settings get updated via:

GetPrintSettingsFromUser updates print_pages_params_
RenderPagesForPrint passes print_pages_params_ to PrintPages
UpdatePrintableSizeInPrintParameters updates the existing "prepare"

[vandebo (ex-Chrome), 2011-10-24 20:53:47]

http://codereview.chromium.org/7993005/diff/1/chrome/renderer/print_web_view_...
File chrome/renderer/print_web_view_helper.cc (left):

http://codereview.chromium.org/7993005/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper.cc:819: prepare.reset();
I think the reason this is done is not to make sure that the prepare is updated
with new settings, but to make the source page responsive while the print dialog
is displayed. IIRC, while the print dialog is displayed, there's a nested
message loop, so if the page gets resized it will reflow and without the
prepare.reset(), it will be set to the print size and aspect ratio.

http://codereview.chromium.org/7993005/diff/6001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/7993005/diff/6001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_browsertest.cc:161:
PrintWebViewHelper::Get(view_)->OnPrintPages();
Does this test fail without your changes?  I'm not sure that it will.

[dominicc (has gone to gerrit), 2011-10-25 04:44:38]

Thanks for the feedback. I have a couple of questions inline.

http://codereview.chromium.org/7993005/diff/1/chrome/renderer/print_web_view_...
File chrome/renderer/print_web_view_helper.cc (left):

http://codereview.chromium.org/7993005/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper.cc:819: prepare.reset();
On 2011/10/24 20:53:47, vandebo wrote:
> I think the reason this is done is not to make sure that the prepare is
updated
> with new settings, but to make the source page responsive while the print
dialog
> is displayed. IIRC, while the print dialog is displayed, there's a nested
> message loop, so if the page gets resized it will reflow and without the
> prepare.reset(), it will be set to the print size and aspect ratio.

Is that platform-specific, or specific to the new print preview page? Because on
Mac I get a modal dialog.

http://codereview.chromium.org/7993005/diff/6001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/7993005/diff/6001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_browsertest.cc:161:
PrintWebViewHelper::Get(view_)->OnPrintPages();
On 2011/10/24 20:53:47, vandebo wrote:
> Does this test fail without your changes?  I'm not sure that it will.

It does, at least on my Mac. Should I split this into two changes and commit the
test first with a failure expectation so we can verify?

[Lei Zhang, 2011-10-25 06:40:22]

http://codereview.chromium.org/7993005/diff/1/chrome/renderer/print_web_view_...
File chrome/renderer/print_web_view_helper.cc (left):

http://codereview.chromium.org/7993005/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper.cc:819: prepare.reset();
On 2011/10/25 04:44:39, dominicc wrote:
> On 2011/10/24 20:53:47, vandebo wrote:
> > I think the reason this is done is not to make sure that the prepare is
> updated
> > with new settings, but to make the source page responsive while the print
> dialog
> > is displayed. IIRC, while the print dialog is displayed, there's a nested
> > message loop, so if the page gets resized it will reflow and without the
> > prepare.reset(), it will be set to the print size and aspect ratio.
> 
> Is that platform-specific, or specific to the new print preview page? Because
on
> Mac I get a modal dialog.

Unless you changed things in about:flags, in M15+ print preview is turned on
everywhere, but you can still access the native print dialog via ctrl+shift+p.

[vandebo (ex-Chrome), 2011-10-25 17:03:11]

http://codereview.chromium.org/7993005/diff/1/chrome/renderer/print_web_view_...
File chrome/renderer/print_web_view_helper.cc (left):

http://codereview.chromium.org/7993005/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper.cc:819: prepare.reset();
On 2011/10/25 04:44:39, dominicc wrote:
> On 2011/10/24 20:53:47, vandebo wrote:
> > I think the reason this is done is not to make sure that the prepare is
> updated
> > with new settings, but to make the source page responsive while the print
> dialog
> > is displayed. IIRC, while the print dialog is displayed, there's a nested
> > message loop, so if the page gets resized it will reflow and without the
> > prepare.reset(), it will be set to the print size and aspect ratio.
> 
> Is that platform-specific, or specific to the new print preview page? Because
on
> Mac I get a modal dialog.

As Lei noted, this code path is only used for the native print flow - follow the
link from the print preview tab or ctrl-shift-p.  This code path is used on all
platforms and what has been in Chrome for years.

http://codereview.chromium.org/7993005/diff/6001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/7993005/diff/6001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_browsertest.cc:161:
PrintWebViewHelper::Get(view_)->OnPrintPages();
On 2011/10/25 04:44:39, dominicc wrote:
> On 2011/10/24 20:53:47, vandebo wrote:
> > Does this test fail without your changes?  I'm not sure that it will.
> 
> It does, at least on my Mac. Should I split this into two changes and commit
the
> test first with a failure expectation so we can verify?

That's not necessary, I just wasn't sure if calling once OnPrintPages would
trigger the Media query code path multiple times.  As long as you've confirmed
it fails without the fix, then it sounds good.

[dominicc (has gone to gerrit), 2011-12-08 08:24:51]

> On 2011/10/25 04:44:39, dominicc wrote:
> > On 2011/10/24 20:53:47, vandebo wrote:
> > > I think the reason this is done is not to make sure that the prepare is
> > updated
> > > with new settings, but to make the source page responsive while the print
> > dialog
> > > is displayed. IIRC, while the print dialog is displayed, there's a nested
> > > message loop, so if the page gets resized it will reflow and without the
> > > prepare.reset(), it will be set to the print size and aspect ratio.
> > 
> > Is that platform-specific, or specific to the new print preview page?
Because
> on
> > Mac I get a modal dialog.
> 
> As Lei noted, this code path is only used for the native print flow - follow
the
> link from the print preview tab or ctrl-shift-p.  This code path is used on
all
> platforms and what has been in Chrome for years.

Right. But how can the page be resized when a modal dialog is blocking
interaction with the window?

[vandebo (ex-Chrome), 2011-12-08 17:59:14]

On 2011/12/08 08:24:51, dominicc wrote:
> > On 2011/10/25 04:44:39, dominicc wrote:
> > > On 2011/10/24 20:53:47, vandebo wrote:
> > > > I think the reason this is done is not to make sure that the prepare is
> > > updated
> > > > with new settings, but to make the source page responsive while the
print
> > > dialog
> > > > is displayed. IIRC, while the print dialog is displayed, there's a
nested
> > > > message loop, so if the page gets resized it will reflow and without the
> > > > prepare.reset(), it will be set to the print size and aspect ratio.
> > > 
> > > Is that platform-specific, or specific to the new print preview page?
> Because
> > on
> > > Mac I get a modal dialog.
> > 
> > As Lei noted, this code path is only used for the native print flow - follow
> the
> > link from the print preview tab or ctrl-shift-p.  This code path is used on
> all
> > platforms and what has been in Chrome for years.
> 
> Right. But how can the page be resized when a modal dialog is blocking
> interaction with the window?

For one, it's not modal on Linux.  I can resize the window with the native print
dialog displayed.