Print Preview: Print in the background.

Print Preview: Print in the background.

BUG=79531
TEST=Print preview tab closes print button is pressed. Printing succeeds.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=84539

[Lei Zhang, 2011-05-05 23:52:57]

For print preview, we need to hide the preview tab once the user clicks the
print button. Yet the print preview tab needs to stay alive for a bit longer in
the background to finish printing.

sky: Would you mind taking a first look at this?
cbentzel: I imagine you had to do something similar for pre-rendering. If you
have any comments on this, please let me know.

Also, feel free to suggest additional reviewers.

[cbentzel, 2011-05-06 16:04:50]

On 2011/05/05 23:52:57, Lei Zhang wrote:
> For print preview, we need to hide the preview tab once the user clicks the
> print button. Yet the print preview tab needs to stay alive for a bit longer
in
> the background to finish printing.
> 
> sky: Would you mind taking a first look at this?
> cbentzel: I imagine you had to do something similar for pre-rendering. If you
> have any comments on this, please let me know.

Well, this is pretty much the reverse of the prerender case [we start out in the
background and bring to the foreground]. However, this generally likes what we
are doing with our new version of prerender which uses TabContents for the
background pages, rather than a new kind of RenderViewHostDelegate.

> 
> Also, feel free to suggest additional reviewers.

[sky, 2011-05-06 16:37:55]

http://codereview.chromium.org/6930034/diff/6003/chrome/browser/printing/back...
File chrome/browser/printing/background_printing_manager.cc (right):

http://codereview.chromium.org/6930034/diff/6003/chrome/browser/printing/back...
chrome/browser/printing/background_printing_manager.cc:22: DCHECK_EQ(0U,
printing_contents_.size());
Its the browsers that keep the process alive, not the tabcontents. So, if you
print the last tab in the background and close all browsers then this could hit.

If you want to make sure chrome doesn't exit in this case HideTabContents needs
to up the ref count of the browser_process. Having said that, it seems if the
user closes all tabs and there is a print process under you way you would want
to show some dialog.

http://codereview.chromium.org/6930034/diff/6003/chrome/browser/printing/back...
File chrome/browser/printing/background_printing_manager.h (right):

http://codereview.chromium.org/6930034/diff/6003/chrome/browser/printing/back...
chrome/browser/printing/background_printing_manager.h:25: : public
base::NonThreadSafe,
Does this class really need to be ref counted? If so, make the destructor
private and add the necessary friend so it can be deleted.

http://codereview.chromium.org/6930034/diff/6003/chrome/browser/printing/back...
chrome/browser/printing/background_printing_manager.h:33: void
HideTabContents(TabContentsWrapper* content);
Did you consider using OwnTabContents? At a minimum document this takes
ownership of the tab and deletes it when done.

[Lei Zhang, 2011-05-06 18:49:01]

http://codereview.chromium.org/6930034/diff/6003/chrome/browser/printing/back...
File chrome/browser/printing/background_printing_manager.cc (right):

http://codereview.chromium.org/6930034/diff/6003/chrome/browser/printing/back...
chrome/browser/printing/background_printing_manager.cc:22: DCHECK_EQ(0U,
printing_contents_.size());
On 2011/05/06 16:37:55, sky wrote:
> Its the browsers that keep the process alive, not the tabcontents. So, if you
> print the last tab in the background and close all browsers then this could
hit.
>
> If you want to make sure chrome doesn't exit in this case HideTabContents
needs
> to up the ref count of the browser_process. Having said that, it seems if the
> user closes all tabs and there is a print process under you way you would want
> to show some dialog.

Indeed, thanks for mentioning this corner case. I'll look into fixing this.

I don't think we show a dialog right now when the user quits and printing is in
progress. I can certainly try to improve this, but I don't think the behavior is
any worse than what we have right now.

http://codereview.chromium.org/6930034/diff/6003/chrome/browser/printing/back...
File chrome/browser/printing/background_printing_manager.h (right):

http://codereview.chromium.org/6930034/diff/6003/chrome/browser/printing/back...
chrome/browser/printing/background_printing_manager.h:25: : public
base::NonThreadSafe,
On 2011/05/06 16:37:55, sky wrote:
> Does this class really need to be ref counted? If so, make the destructor
> private and add the necessary friend so it can be deleted.

No, it doesn't. Removed.

http://codereview.chromium.org/6930034/diff/6003/chrome/browser/printing/back...
chrome/browser/printing/background_printing_manager.h:33: void
HideTabContents(TabContentsWrapper* content);
On 2011/05/06 16:37:55, sky wrote:
> Did you consider using OwnTabContents? At a minimum document this takes
> ownership of the tab and deletes it when done.

Do you mean renaming this to OwnTabContents? I can't find "OwnTabContents" in
code search.

[sky, 2011-05-06 19:21:11]

On Fri, May 6, 2011 at 11:49 AM,  <thestig@chromium.org> wrote:
>
>
http://codereview.chromium.org/6930034/diff/6003/chrome/browser/printing/back...
> File chrome/browser/printing/background_printing_manager.cc (right):
>
>
http://codereview.chromium.org/6930034/diff/6003/chrome/browser/printing/back...
> chrome/browser/printing/background_printing_manager.cc:22: DCHECK_EQ(0U,
> printing_contents_.size());
> On 2011/05/06 16:37:55, sky wrote:
>>
>> Its the browsers that keep the process alive, not the tabcontents. So,
>
> if you
>>
>> print the last tab in the background and close all browsers then this
>
> could hit.
>
>> If you want to make sure chrome doesn't exit in this case
>
> HideTabContents needs
>>
>> to up the ref count of the browser_process. Having said that, it seems
>
> if the
>>
>> user closes all tabs and there is a print process under you way you
>
> would want
>>
>> to show some dialog.
>
> Indeed, thanks for mentioning this corner case. I'll look into fixing
> this.
>
> I don't think we show a dialog right now when the user quits and
> printing is in progress. I can certainly try to improve this, but I
> don't think the behavior is any worse than what we have right now.

Are you going to try to deal with it in this patch, or the next one?
I'm fine with doing it in the future if you prefer, just add a TODO
and remove/comment-out the DCHECK.

>
http://codereview.chromium.org/6930034/diff/6003/chrome/browser/printing/back...
> File chrome/browser/printing/background_printing_manager.h (right):
>
>
http://codereview.chromium.org/6930034/diff/6003/chrome/browser/printing/back...
> chrome/browser/printing/background_printing_manager.h:25: : public
> base::NonThreadSafe,
> On 2011/05/06 16:37:55, sky wrote:
>>
>> Does this class really need to be ref counted? If so, make the
>
> destructor
>>
>> private and add the necessary friend so it can be deleted.
>
> No, it doesn't. Removed.
>
>
http://codereview.chromium.org/6930034/diff/6003/chrome/browser/printing/back...
> chrome/browser/printing/background_printing_manager.h:33: void
> HideTabContents(TabContentsWrapper* content);
> On 2011/05/06 16:37:55, sky wrote:
>>
>> Did you consider using OwnTabContents? At a minimum document this
>
> takes
>>
>> ownership of the tab and deletes it when done.
>
> Do you mean renaming this to OwnTabContents? I can't find
> "OwnTabContents" in code search.
>
> http://codereview.chromium.org/6930034/
>

Naming it OwnTabContents makes it clear what's going on. If you prefer
Hide, that's fine, just document that it takes ownership of the tab
contents and deletes it when done.

Is there a dcheck you could add to make sure the tabcontents is in the
process of printing when Hide is invoked?

  -Scott

[Lei Zhang, 2011-05-06 22:19:03]

On 2011/05/06 19:21:11, sky wrote:
> On Fri, May 6, 2011 at 11:49 AM,  <mailto:thestig@chromium.org> wrote:
> > I don't think we show a dialog right now when the user quits and
> > printing is in progress. I can certainly try to improve this, but I
> > don't think the behavior is any worse than what we have right now.
> 
> Are you going to try to deal with it in this patch, or the next one?
> I'm fine with doing it in the future if you prefer, just add a TODO
> and remove/comment-out the DCHECK.

I added a TODO and removed the DCHECK.

> > Do you mean renaming this to OwnTabContents? I can't find
> > "OwnTabContents" in code search.
> >
> > http://codereview.chromium.org/6930034/
> >
> 
> Naming it OwnTabContents makes it clear what's going on. If you prefer
> Hide, that's fine, just document that it takes ownership of the tab
> contents and deletes it when done.
> 
> Is there a dcheck you could add to make sure the tabcontents is in the
> process of printing when Hide is invoked?

I changed it to OwnTabContents and updated the comment. I don't have a way to
check to see if it's printing, but I did add a check to make sure the
TabContents it's hiding is at the chrome://print url.

[sky, 2011-05-06 22:25:08]

LGTM