Print Preview: Hold on to tabs that do window.print(); window.close() until print preview finishes.

Print Preview: Hold on to tabs that do window.print(); window.close() until print preview finishes.

BUG=87362
TEST=Create html file with window.print(); window.close(); and then print, cancel or close the print tab.

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

[Lei Zhang, 2011-09-06 17:06:04]

This is the follow up to http://codereview.chromium.org/7574002/

- Already checked in some of the cleanup bits in a separate CL.
- The test doesn't work. It succeeds even without this CL.
- The basic premise of this CL is that for an initiator tab that calls
window.close(), it is associated with a print preview tab. The print preview tab
will eventually go away, so make the initiator tab have the same life time.
- This version of the code keeps a preview tab : initiator tab mapping
internally, so it doesn't have to rely on outside callers to tell it to release
the initiator tabs. I believe this to be less confusing and less fragile.

[kmadhusu, 2011-09-07 23:21:09]

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
File chrome/browser/printing/background_printing_manager.cc (right):

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
chrome/browser/printing/background_printing_manager.cc:143:
MessageLoop::current()->DeleteSoon(FROM_HERE, tab);
When exactly multiple preview tabs share the same render_view_host?

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
chrome/browser/printing/background_printing_manager.cc:160:
(printing_tabs_.find(preview_tab) != printing_tabs_.end());
= HasPrintPreviewTab(preview_tab);

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
File chrome/browser/printing/background_printing_manager.h (right):

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
chrome/browser/printing/background_printing_manager.h:70: TabContentsWrapperSet
initiator_tabs_;
(repeating our in-person conversation): initiator_tabs_ data structure is not
required.

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
chrome/browser/printing/background_printing_manager.h:73: // |initiator_tabs_|.
Document Key and Value.

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
chrome/browser/printing/background_printing_manager.h:74:
std::map<TabContentsWrapper*, TabContentsWrapper*> map_;
In the .cc file, I did not see any code to erase the added map entries. Please
fix it.

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
chrome/browser/printing/background_printing_manager.h:74:
std::map<TabContentsWrapper*, TabContentsWrapper*> map_;
Add a comment explaining the scenario at which map_ will be populated.

http://codereview.chromium.org/7800036/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7800036/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:3460: OwnInitiatorTab(source_wrapper)) {
nit: Fix indentation.

[Lei Zhang, 2011-09-07 23:41:16]

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
File chrome/browser/printing/background_printing_manager.cc (right):

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
chrome/browser/printing/background_printing_manager.cc:143:
MessageLoop::current()->DeleteSoon(FROM_HERE, tab);
On 2011/09/07 23:21:10, kmadhusu wrote:
> When exactly multiple preview tabs share the same render_view_host?

This happens quite often. For example, open two tabs to google.com and print
from both. There's a good change both print preview tabs will be the same
process.

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
chrome/browser/printing/background_printing_manager.cc:160:
(printing_tabs_.find(preview_tab) != printing_tabs_.end());
On 2011/09/07 23:21:10, kmadhusu wrote:
> = HasPrintPreviewTab(preview_tab);

Done.

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
File chrome/browser/printing/background_printing_manager.h (right):

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
chrome/browser/printing/background_printing_manager.h:70: TabContentsWrapperSet
initiator_tabs_;
On 2011/09/07 23:21:10, kmadhusu wrote:
> (repeating our in-person conversation): initiator_tabs_ data structure is not
> required.

Done.

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
chrome/browser/printing/background_printing_manager.h:73: // |initiator_tabs_|.
On 2011/09/07 23:21:10, kmadhusu wrote:
> Document Key and Value.

Done.

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
chrome/browser/printing/background_printing_manager.h:74:
std::map<TabContentsWrapper*, TabContentsWrapper*> map_;
On 2011/09/07 23:21:10, kmadhusu wrote:
> In the .cc file, I did not see any code to erase the added map entries. Please
> fix it. 

Done.

http://codereview.chromium.org/7800036/diff/1/chrome/browser/printing/backgro...
chrome/browser/printing/background_printing_manager.h:74:
std::map<TabContentsWrapper*, TabContentsWrapper*> map_;
On 2011/09/07 23:21:10, kmadhusu wrote:
> Add a comment explaining the scenario at which map_ will be populated. 

Done.

http://codereview.chromium.org/7800036/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7800036/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:3460: OwnInitiatorTab(source_wrapper)) {
On 2011/09/07 23:21:10, kmadhusu wrote:
> nit: Fix indentation.

Done.

[kmadhusu, 2011-09-08 00:55:49]

LGTM

[commit-bot: I haz the power, 2011-09-08 20:34:55]

Change committed as 100235