Be able to print items that do window.print(); window.close() (airline tix e.g.)

Be able to print items that do window.print(); window.close() (airline tix e.g.)

This is not commitable due to too many logs, and could stand some clean up.
Please review locations of code and provide feedback of how it's done.

Basic gist - hide; don't close initiator tabs, "own" them in the background printing manager,
release them when they're no longer needed, deleting if owned.

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

[Lei Zhang, 2011-08-11 09:47:16]

It's probably not too hard to add a test for this by calling
Browser::CloseContents() from the test.

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

http://codereview.chromium.org/7574002/diff/3001/chrome/browser/printing/back...
chrome/browser/printing/background_printing_manager.h:68: 
If you add a second set for the initiator tabs we are holding, rather than
mixing initiator tabs and print preview tabs in |printing_contents_|, this CL
will likely be more robust and readable.

[Sheridan Rawlins, 2011-08-16 06:11:54]

Added a test too.

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

http://codereview.chromium.org/7574002/diff/3001/chrome/browser/printing/back...
chrome/browser/printing/background_printing_manager.h:68: 
On 2011/08/11 09:47:16, Lei Zhang wrote:
> If you add a second set for the initiator tabs we are holding, rather than
> mixing initiator tabs and print preview tabs in |printing_contents_|, this CL
> will likely be more robust and readable.

Done.

[Sheridan Rawlins, 2011-08-18 23:27:38]

Adding Kausalya as reviewer since I resolved some conflicts with her commits
while rebasing.

[Sheridan Rawlins, 2011-08-18 23:33:38]

Removing WIP.  Please review.

[kmadhusu, 2011-08-18 23:45:50]

http://codereview.chromium.org/7574002/diff/15001/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.cc (right):

http://codereview.chromium.org/7574002/diff/15001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:102: 
lines 99-113 and 128-140 are exactly the same. They can go into a helper
function.

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

http://codereview.chromium.org/7574002/diff/15001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:183:
ReleaseInitiatorTabContents(initiator_wrapper);
You need to delete the initiator tab request after aborting its preview request.
We currently abort the previous request in line #191. So, release the contents
after that code.

[Sheridan Rawlins, 2011-08-19 00:24:25]

http://codereview.chromium.org/7574002/diff/15001/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.cc (right):

http://codereview.chromium.org/7574002/diff/15001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:102: 
On 2011/08/18 23:45:50, kmadhusu wrote:
> lines 99-113 and 128-140 are exactly the same. They can go into a helper
> function. 

Done.

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

http://codereview.chromium.org/7574002/diff/15001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:183:
ReleaseInitiatorTabContents(initiator_wrapper);
On 2011/08/18 23:45:50, kmadhusu wrote:
> You need to delete the initiator tab request after aborting its preview
request.
> We currently abort the previous request in line #191. So, release the contents
> after that code.

Done.

[kmadhusu, 2011-08-19 17:13:04]

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

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:165:
print_preview_ui->OnInitiatorTabCrashed();
If the hidden initiator tab is crashed, you need to release the initiator tab
contents right?

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:191: TabContentsWrapper*
initiator_wrapper =
You can move line #179 to here.

[Lei Zhang, 2011-08-19 19:00:23]

Crazy thought - what if we have JS code that looks like:

window.print();
window.close();
alert('hi');

That alert() should never fire, but it might if we are just hiding the tab.
Would we need to stop JS from executing at the moment of the window.close()?

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.cc (left):

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:33:
DCHECK(CalledOnValidThread());
I would keep this here and do the check up front in the public
OwnFooTabContents(), rather in the private OwnTabContents() in your CL.

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.cc (right):

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:43: if (tab_controller)
I highly prefer to handle the style where we handle error cases and return
early. If |tab_controller| is NULL, in the previous case you'd just stop,
whereas now you'd keep reading the code and wonder if we handle it below
somehow.

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:51:
!ReleaseInitiatorTabContents(initiator_tab_wrapper);
Why do you call ReleaseInitiatorTabContents() here? Can't you wait for the print
preview tab to get destroyed, and delete the hidden initiator tab then?

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:155: if (source_contents)
{
When is |source_contents| ever NULL? It seems you always pass in a valid
|source_contents|.

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.h (right):

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:40: bool
OwnInitiatorTabContents(TabContentsWrapper* content);
You might want to say it does not take ownership if it returns false.

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:50: // Let others iterate
over the list of background initiator tabs.
are these used anywhere?

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:57: // Returns true if
|initiator_contents_| contains |entry|.
also not used anywhere?

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:78: TabContentsWrapper*
source_content,
document |source_content|

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:85:
std::set<TabContentsWrapper*> initiator_contents_;
change these to TabContentsWrapperSet since you defined it above.

[Sheridan Rawlins, 2011-08-25 22:52:54]

Lei - as we discussed, that case can happen (audio, video, and javascript
continue to run).

This patch doesn't address that, but is still valuable stop-gap.

Also as we discussed, we will still need the ownership for the case when we copy
the DOM into a new TabContents, so that it is ultimately deleted.  Therefore
this patch is not throw away in the long-term case.

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.cc (left):

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:33:
DCHECK(CalledOnValidThread());
On 2011/08/19 19:00:23, Lei Zhang wrote:
> I would keep this here and do the check up front in the public
> OwnFooTabContents(), rather in the private OwnTabContents() in your CL.

Done.

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.cc (right):

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:43: if (tab_controller)
On 2011/08/19 19:00:23, Lei Zhang wrote:
> I highly prefer to handle the style where we handle error cases and return
> early. If |tab_controller| is NULL, in the previous case you'd just stop,
> whereas now you'd keep reading the code and wonder if we handle it below
> somehow.

Done.

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:51:
!ReleaseInitiatorTabContents(initiator_tab_wrapper);
Well, this is the place where we hope to switch to the initiator tab.  If it
will be destroyed, then we don't want to switch to it, so we might as well
delete it too since we know we don't need it anymore.

On 2011/08/19 19:00:23, Lei Zhang wrote:
> Why do you call ReleaseInitiatorTabContents() here? Can't you wait for the
print
> preview tab to get destroyed, and delete the hidden initiator tab then?

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:155: if (source_contents)
{
On 2011/08/19 19:00:23, Lei Zhang wrote:
> When is |source_contents| ever NULL? It seems you always pass in a valid
> |source_contents|.

Done.

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.h (right):

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:40: bool
OwnInitiatorTabContents(TabContentsWrapper* content);
On 2011/08/19 19:00:23, Lei Zhang wrote:
> You might want to say it does not take ownership if it returns false.

Done.

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:50: // Let others iterate
over the list of background initiator tabs.
No; removed.

On 2011/08/19 19:00:23, Lei Zhang wrote:
> are these used anywhere?

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:57: // Returns true if
|initiator_contents_| contains |entry|.
Removed.

On 2011/08/19 19:00:23, Lei Zhang wrote:
> also not used anywhere?

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:78: TabContentsWrapper*
source_content,
On 2011/08/19 19:00:23, Lei Zhang wrote:
> document |source_content|

Done.

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:85:
std::set<TabContentsWrapper*> initiator_contents_;
On 2011/08/19 19:00:23, Lei Zhang wrote:
> change these to TabContentsWrapperSet since you defined it above.

Done.

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

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:165:
print_preview_ui->OnInitiatorTabCrashed();
On 2011/08/19 17:13:04, kmadhusu wrote:
> If the hidden initiator tab is crashed, you need to release the initiator tab
> contents right?

Done.

http://codereview.chromium.org/7574002/diff/18001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:191: TabContentsWrapper*
initiator_wrapper =
On 2011/08/19 17:13:04, kmadhusu wrote:
> You can move line #179 to here.

Done.

[Lei Zhang, 2011-08-26 10:01:54]

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.cc (right):

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:33: TabContentsWrapper*
content) {
This should be "contents" to be consistent with the rest of the file and the
rest of the project.

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:66:
printing::PrintPreviewTabController::GetInstance();
check the return value

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:153:
CHECK(wrapper_set->find(contents) == wrapper_set->end());
You should either just do this CHECK here, do it in OwnFoo and not here.

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:157: if (source_contents)
{
I don't think this will ever be false in this private method. I bet you can
assert this by changing line 150 to a CHECK.

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.h (right):

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:53: bool
HasTabContents(TabContentsWrapper* entry);
Probably want to rename this to HasPreviewTabContents.

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:78: // The set of tabs
managed by BackgroundPrintingManager.
update comment

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

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:13: #include
"chrome/browser/printing/background_printing_manager.h"
nit: alphabetical order

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:162: TabContents*
initiator_tab = iter->second;
move this up to between line 159/160 and then you can also make the if statement
more readable.

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:164: if (initiator_tab)
{
this will always be true, we already tested this condition on line 160.

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

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3235: TabContentsWrapper* tab_wrapper =
Please add a comment here to explain what's going on.

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:934:
g_browser_process->background_printing_manager()->
GetBackgroundPrintingManager() is ever so slightly shorter.

http://codereview.chromium.org/7574002/diff/24001/chrome/test/data/webui/test...
File chrome/test/data/webui/test_api.js (right):

http://codereview.chromium.org/7574002/diff/24001/chrome/test/data/webui/test...
chrome/test/data/webui/test_api.js:85: noPrint: false,
How about changing this to print: true to avoid the double negative and update
WebUIBrowserTest::BrowsePrintPreload() accordingly?

[Sheridan Rawlins, 2011-08-26 23:45:39]

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.cc (right):

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:33: TabContentsWrapper*
content) {
On 2011/08/26 10:01:54, Lei Zhang wrote:
> This should be "contents" to be consistent with the rest of the file and the
> rest of the project.

Done.

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:66:
printing::PrintPreviewTabController::GetInstance();
On 2011/08/26 10:01:54, Lei Zhang wrote:
> check the return value

Done.

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:153:
CHECK(wrapper_set->find(contents) == wrapper_set->end());
On 2011/08/26 10:01:54, Lei Zhang wrote:
> You should either just do this CHECK here, do it in OwnFoo and not here.

Done.

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:157: if (source_contents)
{
On 2011/08/26 10:01:54, Lei Zhang wrote:
> I don't think this will ever be false in this private method. I bet you can
> assert this by changing line 150 to a CHECK.

Done.

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.h (right):

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:53: bool
HasTabContents(TabContentsWrapper* entry);
On 2011/08/26 10:01:54, Lei Zhang wrote:
> Probably want to rename this to HasPreviewTabContents.

Done.

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:78: // The set of tabs
managed by BackgroundPrintingManager.
On 2011/08/26 10:01:54, Lei Zhang wrote:
> update comment

Done.

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

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:13: #include
"chrome/browser/printing/background_printing_manager.h"
On 2011/08/26 10:01:54, Lei Zhang wrote:
> nit: alphabetical order

Done.

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:162: TabContents*
initiator_tab = iter->second;
On 2011/08/26 10:01:54, Lei Zhang wrote:
> move this up to between line 159/160 and then you can also make the if
statement
> more readable.

Done.

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:164: if (initiator_tab)
{
Yeah, the previous comment made this obvious :)

Done.

On 2011/08/26 10:01:54, Lei Zhang wrote:
> this will always be true, we already tested this condition on line 160.

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

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3235: TabContentsWrapper* tab_wrapper =
On 2011/08/26 10:01:54, Lei Zhang wrote:
> Please add a comment here to explain what's going on.

Done.

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7574002/diff/24001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:934:
g_browser_process->background_printing_manager()->
On 2011/08/26 10:01:54, Lei Zhang wrote:
> GetBackgroundPrintingManager() is ever so slightly shorter.

Done.

http://codereview.chromium.org/7574002/diff/24001/chrome/test/data/webui/test...
File chrome/test/data/webui/test_api.js (right):

http://codereview.chromium.org/7574002/diff/24001/chrome/test/data/webui/test...
chrome/test/data/webui/test_api.js:85: noPrint: false,
On 2011/08/26 10:01:54, Lei Zhang wrote:
> How about changing this to print: true to avoid the double negative and update
> WebUIBrowserTest::BrowsePrintPreload() accordingly?

Done.

[Lei Zhang, 2011-08-27 01:05:06]

http://codereview.chromium.org/7574002/diff/29001/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.cc (right):

http://codereview.chromium.org/7574002/diff/29001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.cc:52:
!ReleaseInitiatorTabContents(initiator_tab_wrapper);
Why do you attempt to destroy the initiator tab here? Don't we already try to do
this when the print preview tab prints and goes away? (in
PrintPreviewTabController::OnTabContentsDestroyed)

http://codereview.chromium.org/7574002/diff/29001/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.h (right):

http://codereview.chromium.org/7574002/diff/29001/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:72: TabContentsWrapper*
source_content,
I don't think you need to pass in |source_content|. You can derive it inside
OwnTabContents() by calling PrintPreviewTabController::GetPrintPreviewForTab().