Block printing from blocked popup.

Block printing from blocked popup.
Adds a check in the window.print call to make window.print fail if called from a blocked popup.
Also adds handling to print_preview_tab_controller.cc to mark preview as done if the creation of the print preview ui fails.


BUG=117955
TEST=Open test page attached to bug 117955.  Allow the blocked popup.  Ctrl-P should work normally.

[Albert Bodenhamer, 2012-03-16 17:33:26]

Actually, hold off on reviewing this.  There's a test that I missed updating and
problems building on Mac I need to sort out first.

[Albert Bodenhamer, 2012-03-19 16:44:30]

Fixed.  Please take a look when you get a chance.

[Lei Zhang, 2012-03-19 20:19:32]

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

http://codereview.chromium.org/9705084/diff/11001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:218: if
(tab_controller->GetOrCreatePreviewTab(tab) == NULL)
nit: It's easier to just write !foo.

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/mock_printer.h
File chrome/renderer/mock_printer.h (right):

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/mock_printe...
chrome/renderer/mock_printer.h:76: void SetScriptedPrintAllowed(bool allowed);
nit: alphabetical order.

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:671: msg->EnableMessagePumping();
This is not necessary. Just send the message.

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:317: WebKit::WebFrame* frame);
nit: this fits on one line.

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:192:
PrintWebViewHelper::Get(view_)->ResetScriptedPrintCount();
You are missing a test here to make sure resetting the count works.

[Albert Bodenhamer, 2012-03-19 23:16:17]

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

http://codereview.chromium.org/9705084/diff/11001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:218: if
(tab_controller->GetOrCreatePreviewTab(tab) == NULL)
On 2012/03/19 20:19:32, Lei Zhang wrote:
> nit: It's easier to just write !foo.
I have a strong preference for "== NULL" as it avoids a sloppy implicit
typecast, BUT the rest of this file is using "!foo", so I'll go with that for
consistency.
Done.

Done.

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/mock_printer.h
File chrome/renderer/mock_printer.h (right):

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/mock_printe...
chrome/renderer/mock_printer.h:76: void SetScriptedPrintAllowed(bool allowed);
On 2012/03/19 20:19:32, Lei Zhang wrote:
> nit: alphabetical order.

Done.

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:671: msg->EnableMessagePumping();
On 2012/03/19 20:19:32, Lei Zhang wrote:
> This is not necessary. Just send the message.
Actually it is.  Attempting to Send a sync message here without first calling
EnableMessagePumping results in a DCHECK firing that warns of potential
deadlock.

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:317: WebKit::WebFrame* frame);
On 2012/03/19 20:19:32, Lei Zhang wrote:
> nit: this fits on one line.

Done.

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:192:
PrintWebViewHelper::Get(view_)->ResetScriptedPrintCount();
On 2012/03/19 20:19:32, Lei Zhang wrote:
> You are missing a test here to make sure resetting the count works.

Good catch.  I've split this into 2 different tests.

[Lei Zhang, 2012-03-20 20:34:02]

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:671: msg->EnableMessagePumping();
On 2012/03/19 23:16:17, Albert Bodenhamer wrote:
> On 2012/03/19 20:19:32, Lei Zhang wrote:
> > This is not necessary. Just send the message.
> Actually it is.  Attempting to Send a sync message here without first calling
> EnableMessagePumping results in a DCHECK firing that warns of potential
> deadlock.

Oh right, that goes to the UI thread and you'd get the warning on Windows. Umm,
we should avoid using nested message loops as much as possible. If you want a
lecture on why people hate them, talk to jar@.

I slept on this and came up with an alternate way of doing this, without nested
message loops. WDYT?
https://chromiumcodereview.appspot.com/9762004

[Albert Bodenhamer, 2012-03-20 20:52:18]

On Tue, Mar 20, 2012 at 1:34 PM, <thestig@chromium.org> wrote:

>
> http://codereview.chromium.**org/9705084/diff/11001/chrome/**
>
renderer/print_web_view_**helper.cc<http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_view_helper.cc>
> File chrome/renderer/print_web_**view_helper.cc (right):
>
> http://codereview.chromium.**org/9705084/diff/11001/chrome/**
>
renderer/print_web_view_**helper.cc#newcode671<http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_view_helper.cc#newcode671>
> chrome/renderer/print_web_**view_helper.cc:671:
> msg->EnableMessagePumping();
> On 2012/03/19 23:16:17, Albert Bodenhamer wrote:
>
>> On 2012/03/19 20:19:32, Lei Zhang wrote:
>> > This is not necessary. Just send the message.
>> Actually it is.  Attempting to Send a sync message here without first
>>
> calling
>
>> EnableMessagePumping results in a DCHECK firing that warns of
>>
> potential
>
>> deadlock.
>>
>
> Oh right, that goes to the UI thread and you'd get the warning on
> Windows. Umm, we should avoid using nested message loops as much as
> possible. If you want a lecture on why people hate them, talk to jar@.
>

Lots of good reasons to hate them.  If nothing else there's the deadlock
risk, overhead, and debugging difficulty.  I think I'll pass on the
jar@lecture. :-)


>
> I slept on this and came up with an alternate way of doing this, without
> nested message loops. WDYT?
>
https://chromiumcodereview.**appspot.com/9762004<https://chromiumcodereview.a...



I like how it moves the control of the blockage into
BlockedContentTabHelper. Nice way to avoid the nested Send.

I'll incorporate this into my CL.  Thanks!


>
>
>
http://codereview.chromium.**org/9705084/<http://codereview.chromium.org/9705...
>



-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org

[Albert Bodenhamer, 2012-03-20 22:02:44]

On Tue, Mar 20, 2012 at 1:51 PM, Albert Bodenhamer <abodenha@chromium.org>wrote:

> On Tue, Mar 20, 2012 at 1:34 PM, <thestig@chromium.org> wrote:
>
>>
>> http://codereview.chromium.**org/9705084/diff/11001/chrome/**
>>
renderer/print_web_view_**helper.cc<http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_view_helper.cc>
>> File chrome/renderer/print_web_**view_helper.cc (right):
>>
>> http://codereview.chromium.**org/9705084/diff/11001/chrome/**
>>
renderer/print_web_view_**helper.cc#newcode671<http://codereview.chromium.org/9705084/diff/11001/chrome/renderer/print_web_view_helper.cc#newcode671>
>> chrome/renderer/print_web_**view_helper.cc:671:
>> msg->EnableMessagePumping();
>> On 2012/03/19 23:16:17, Albert Bodenhamer wrote:
>>
>>> On 2012/03/19 20:19:32, Lei Zhang wrote:
>>> > This is not necessary. Just send the message.
>>> Actually it is.  Attempting to Send a sync message here without first
>>>
>> calling
>>
>>> EnableMessagePumping results in a DCHECK firing that warns of
>>>
>> potential
>>
>>> deadlock.
>>>
>>
>> Oh right, that goes to the UI thread and you'd get the warning on
>> Windows. Umm, we should avoid using nested message loops as much as
>> possible. If you want a lecture on why people hate them, talk to jar@.
>>
>
> Lots of good reasons to hate them.  If nothing else there's the deadlock
> risk, overhead, and debugging difficulty.  I think I'll pass on the
jar@lecture. :-)
>
>
>>
>> I slept on this and came up with an alternate way of doing this, without
>> nested message loops. WDYT?
>>
https://chromiumcodereview.**appspot.com/9762004<https://chromiumcodereview.a...
>
>
>
> I like how it moves the control of the blockage into
> BlockedContentTabHelper. Nice way to avoid the nested Send.
>
> I'll incorporate this into my CL.  Thanks!
>

Something was bugging me about this.  It just hit me what it is.  I like
that BlockedContentTabHelper is driving the blocked state, but I DON'T like
that it needs to talk directly to PrintViewManager.

What do you think about adding a new notification?  We could create
NOTIFICATION_CONTENT_BLOCKED with the source set to the TabContentsWrapper
of the blocked content and then observe the notification in
PrintViewManager.

Thoughts?


>
>
>>
>>
>>
http://codereview.chromium.**org/9705084/<http://codereview.chromium.org/9705...
>>
>
>
>
> --
> Albert Bodenhamer | Software Engineer |
abodenha@chromium.<abodenha@google.com>
> org
>



-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org

[Lei Zhang, 2012-03-20 22:17:05]

On 2012/03/20 22:02:44, Albert Bodenhamer wrote:
> Something was bugging me about this.  It just hit me what it is.  I like
> that BlockedContentTabHelper is driving the blocked state, but I DON'T like
> that it needs to talk directly to PrintViewManager.
> 
> What do you think about adding a new notification?  We could create
> NOTIFICATION_CONTENT_BLOCKED with the source set to the TabContentsWrapper
> of the blocked content and then observe the notification in
> PrintViewManager.
> 
> Thoughts?

That's fine by me. Just pretend my CL was a PoC.