Print Preview: Make print preview tab modal.

Print Preview: Make print preview tab modal.

BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110056

[Lei Zhang, 2011-10-13 09:49:16]

I'd start by just ignoring the tests and the HtmlDelegate code. The HtmlDelegate
code is being reviewed separately: http://codereview.chromium.org/8220026/ but
included so try jobs work.

[kmadhusu, 2011-10-13 18:52:18]

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.h (left):

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:59: TabContentsWrapperMap;
You can delete this typedef.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:253:
tab_controller->GetInitiatorTab(tab_contents_wrapper());
Store the tab_contents_wrapper() in a local variable and use it in line 245 and
here.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:264:
NavigationController::ReloadType reload_type) {
Do you need to call StopWorker() here?

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

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:49: class
PrintPreviewTabDelegate : public HtmlDialogUIDelegate {
Maybe this class should get moved out of pptc?

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:169:
initiator_tab->tab_contents())->Activate();
Is this required? We are already in initiator tab. Why do you want to activate
it again?

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:227: //
|preview_tab_map_| becase RemoveFooTab() can change |preview_tab_map_|.
typo "because"

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:229: for
(PrintPreviewTabMap::iterator iter = preview_tab_map_.begin();
Rather than reusing the "vec" I would like to use two vectors (one for preview
tabs and one for initiator tabs). Have a single for loop and do the comparison
in parallel.

if (rph == preview_Tab)
  add it to preview tab vector
else if  (rph == initiator_tab)
  add it to initiator tab vector.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:236: for (size_t i = 0;
i < vec.size(); ++i) {
size_t vector_len = vec.size();
for (size_t i = 0; i < vector_len; i++)

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:290:
source_tab_is_preview_tab) {
"source_tab_is_preview_tab" check is not required here.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:446: if
(IsPrintPreviewTab(preview_tab) && preview_tab->web_ui()) {
We already know that this is a print preview tab. Do we need IsPrintPreviewTab()
check here?

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_tab_controller.h (right):

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.h:17: #include <vector>
Why do you need a vector here?

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1063: if ((cr.isMac &&
e.shiftKey && e.metaKey && !e.ctrlKey && !e.altKey) ||
Why do you need this? If we press the key combination for native dialog on a
preview tab, don't we get an event regarding that?

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.cc (right):

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.cc:148:
g_browser_process->background_printing_manager();
Create a helper function to check if the preview tab is owned by background
printing manager and call that function from here and line 159.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui_uitest.cc (right):

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_uitest.cc:68:
ASSERT_TRUE(browser->IsMenuCommandEnabled(IDC_ADVANCED_PRINT, &enabled));
Can you add a test to verify that printing is disabled in other constrained
windows?

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui_unittest.cc (right):

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_unittest.cc:48: EXPECT_EQ(1,
browser()->tab_count());
Can you add a check to confirm that the count of constrained windows is 1?

[kmadhusu, 2011-10-13 19:18:17]

(repeating our conversation): We need to disable the context menu on the preview
tab. In the current code it is enabled. User can view the source and add
javascript code to navigate to some other page. 

Please fix the following bug (repro steps are as follows):
1. Open a preview tab. 
2. Select "Print to PDF"
3. Click Print.
4. Select "Cancel" in file save as dialog.
5. Observe that the cancel button is disabled. 

Expected behavior: Cancel button should be enabled.

[dpapad, 2011-10-13 19:57:33]

On 2011/10/13 19:18:17, kmadhusu wrote:
> (repeating our conversation): We need to disable the context menu on the
preview
> tab. In the current code it is enabled. User can view the source and add
> javascript code to navigate to some other page. 
> 
> Please fix the following bug (repro steps are as follows):
> 1. Open a preview tab. 
> 2. Select "Print to PDF"
> 3. Click Print.
> 4. Select "Cancel" in file save as dialog.
> 5. Observe that the cancel button is disabled. 
> 
> Expected behavior: Cancel button should be enabled.

One thing ti keep in mind. As of now (with this CL) the only way to bring up
devtools is by using the context menu-> Inspect element. We still want to be
able to open devtools (at least for developers).

[Lei Zhang, 2011-10-13 21:12:12]

Still looking at the later comments. We want users to be able to view source if
they want to, but that means the print preview tab can navigate and that needs
to be handled appropriately.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/bac...
File chrome/browser/printing/background_printing_manager.h (left):

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/bac...
chrome/browser/printing/background_printing_manager.h:59: TabContentsWrapperMap;
On 2011/10/13 18:52:18, kmadhusu wrote:
> You can delete this typedef.

Done.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:253:
tab_controller->GetInitiatorTab(tab_contents_wrapper());
On 2011/10/13 18:52:18, kmadhusu wrote:
> Store the tab_contents_wrapper() in a local variable and use it in line 245
and
> here.

Done.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:264:
NavigationController::ReloadType reload_type) {
On 2011/10/13 18:52:18, kmadhusu wrote:
> Do you need to call StopWorker() here?

I don't think so. Why do you say that?

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

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:49: class
PrintPreviewTabDelegate : public HtmlDialogUIDelegate {
On 2011/10/13 18:52:18, kmadhusu wrote:
> Maybe this class should get moved out of pptc?

No, I don't think this delegate makes sense for anyone else.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:169:
initiator_tab->tab_contents())->Activate();
On 2011/10/13 18:52:18, kmadhusu wrote:
> Is this required? We are already in initiator tab. Why do you want to activate
> it again?

One possible reason is tab A is loading, and the user switches to tab B. When
tab A finishes loading, it does a window.print(). In other browsers, this would
have popped up a modal dialog to grab the user's attention, so this would
emulate that. One can argue whether this is good or not from a UI perspective.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:227: //
|preview_tab_map_| becase RemoveFooTab() can change |preview_tab_map_|.
On 2011/10/13 18:52:18, kmadhusu wrote:
> typo "because"

Done.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:229: for
(PrintPreviewTabMap::iterator iter = preview_tab_map_.begin();
On 2011/10/13 18:52:18, kmadhusu wrote:
> Rather than reusing the "vec" I would like to use two vectors (one for preview
> tabs and one for initiator tabs). Have a single for loop and do the comparison
> in parallel.
> 
> if (rph == preview_Tab)
>   add it to preview tab vector
> else if  (rph == initiator_tab)
>   add it to initiator tab vector. 

Done.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:236: for (size_t i = 0;
i < vec.size(); ++i) {
On 2011/10/13 18:52:18, kmadhusu wrote:
> size_t vector_len = vec.size();
> for (size_t i = 0; i < vector_len; i++)

I'll leave the optimization up to the compiler. We do: for (size_t i =0; i <
vec.size()...) all over the place.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:290:
source_tab_is_preview_tab) {
On 2011/10/13 18:52:18, kmadhusu wrote:
> "source_tab_is_preview_tab" check is not required here.

Done.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:446: if
(IsPrintPreviewTab(preview_tab) && preview_tab->web_ui()) {
On 2011/10/13 18:52:18, kmadhusu wrote:
> We already know that this is a print preview tab. Do we need
IsPrintPreviewTab()
> check here?

This was originally added for the case where the print preview tab has the
button to open the closed initiator tab.  Since that case doesn't happen
anymore, I don't think we need this check.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_tab_controller.h (right):

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.h:17: #include <vector>
On 2011/10/13 18:52:18, kmadhusu wrote:
> Why do you need a vector here?

Removed.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1063: if ((cr.isMac &&
e.shiftKey && e.metaKey && !e.ctrlKey && !e.altKey) ||
On 2011/10/13 18:52:18, kmadhusu wrote:
> Why do you need this? If we press the key combination for native dialog on a
> preview tab, don't we get an event regarding that?

When the constrained window has focus, the browser doesn't get the keystrokes.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.cc (right):

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.cc:148:
g_browser_process->background_printing_manager();
On 2011/10/13 18:52:18, kmadhusu wrote:
> Create a helper function to check if the preview tab is owned by background
> printing manager and call that function from here and line 159.

The amount of savings is really minimal here. It's only 2 lines of trivial code.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui_uitest.cc (right):

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_uitest.cc:68:
ASSERT_TRUE(browser->IsMenuCommandEnabled(IDC_ADVANCED_PRINT, &enabled));
On 2011/10/13 18:52:18, kmadhusu wrote:
> Can you add a test to verify that printing is disabled in other constrained
> windows?
> 

Sure, I can add it in a later CL.

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui_unittest.cc (right):

http://codereview.chromium.org/8136027/diff/36046/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_unittest.cc:48: EXPECT_EQ(1,
browser()->tab_count());
On 2011/10/13 18:52:18, kmadhusu wrote:
> Can you add a check to confirm that the count of constrained windows is 1?

Done. I can add it to other tests later.

[Lei Zhang, 2011-10-13 22:16:32]

On 2011/10/13 19:18:17, kmadhusu wrote:
> (repeating our conversation): We need to disable the context menu on the
preview
> tab. In the current code it is enabled. User can view the source and add
> javascript code to navigate to some other page. 
> 
> Please fix the following bug (repro steps are as follows):
> 1. Open a preview tab. 
> 2. Select "Print to PDF"
> 3. Click Print.
> 4. Select "Cancel" in file save as dialog.
> 5. Observe that the cancel button is disabled. 
> 
> Expected behavior: Cancel button should be enabled.

Took care of both of these issues.

[kmadhusu, 2011-10-14 00:21:08]

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

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:74: const gfx::Size
min_size(800, 480);
Define const int variables for all magic numbers.

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:97: return
GURL(chrome::kChromeUIPrintURL);
nit: remove 2 spaces.

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:238:
closed_preview_tabs.push_back(initiator_tab);
closed_preview_tabs => closed_initiator_tabs

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:251:
RemoveInitiatorTab(closed_initiator_tabs[i]);
(repeating our conversation): I am not sure whether the initiator tab and
preview tab can share the same rph. If they share the same rph, we will try to
remove the observers of initiator tab again in RemoveInitiatorTab()
(RemovePreviewTab already removed the observers for both initiator tab and
preview tab).

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:423:
preview_tab_map_[preview_tab] = NULL;
better to add a DCHECK to validate the preview_tab.

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_tab_controller_browsertest.cc
(right):

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller_browsertest.cc:64: // yet
have the same address as |preview_tab|. http://crbug.com/XXX
Add a valid bug #.

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1051:
chrome.send('DialogClose', []);
You can do "chrome.send('DialogClose');". Any specific reason for passing an
empty array?

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1064: if (e.keyCode ==
80 || e.keyCode == 112) {
keycode 112 check is not required.

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

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4527: if (controller &&
controller->GetPrintPreviewForTab(wrapper)) {
nit: {} not required

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/browser_l...
File chrome/browser/ui/browser_list_unittest.cc (right):

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_list_unittest.cc:162: // TODO(thestig) Fix or remove
this test. http://crbug.com/XXXXX
Add valid bug number.

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

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:57: CANCEL,  // UNUSED
Add a todo to fix this later. We need to record the cancel actions and initiator
tab crashes.

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.h (right):

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:117: void HidePreviewTab();
nit: OnHidePreviewTab() and OnClosePrintPreviewTab().

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui_unittest.cc (right):

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_unittest.cc:43:
ASSERT_TRUE(initiator_tab);
EXPECT_EQ(0U, GetConstrainedWindowCount(initiator_tab));

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_unittest.cc:53: EXPECT_EQ(1,
browser()->tab_count());
EXPECT_EQ(1U, GetConstrainedWindowCount(initiator_tab));

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1065: if ((cr.isMac &&
e.shiftKey && e.metaKey && !e.ctrlKey && !e.altKey) ||
On mac, we use cmd+opt+P and not cmd+shift+p

[kmadhusu, 2011-10-14 01:11:38]

I tested with your patch and saw some segfaults and crashes.

Test 1:
Segmentation fault occurs when you close the initiator tab while the preview tab
is loading.
Repro steps:
1. Open a chrome://newtab (make sure your browser has only one tab)
2. Press Ctrl+P
3. Close the initiator tab using 'X' close button.
4. Segmentation fault occurs.

(or)

1. Open a newtab page. (browser can have multiple tabs)
2. Press Ctrl+P
3. Close the initiator tab using 'X' close button while the preview tab is
loading (before you see loading... screen).
4. Segmentation fault occurs.

Test 2:
I saw the following crash once. Sorry, I dont have a repro step. I just
navigated back and forth and created preview tab several times.

[5562:5562:877569145343:FATAL:notification_registrar.cc(68)] Check failed:
false. Trying to remove unregistered observer of type 32 from list of size 3.
Backtrace:
	base::debug::StackTrace::StackTrace() [0x7f47a183eeee]
	logging::LogMessage::~LogMessage() [0x7f47a1864fb6]
	NotificationRegistrar::Remove() [0x7f47a2541263]
	printing::PrintPreviewTabController::RemoveObservers() [0x7f47a121a5a7]
	printing::PrintPreviewTabController::RemovePreviewTab() [0x7f47a121a791]
	printing::PrintPreviewTabController::OnRendererProcessClosed() [0x7f47a1219b5c]
	printing::PrintPreviewTabController::Observe() [0x7f47a121987e]
	NotificationService::Notify() [0x7f47a2543395]
	BrowserRenderProcessHost::ProcessDied() [0x7f47a3a02957]
	BrowserRenderProcessHost::FastShutdownIfPossible() [0x7f47a3a01dda]
	browser_shutdown::OnShutdownStarting() [0x7f47a0ef7669]
	Browser::OnWindowClosing() [0x7f47a0c5de64]
	BrowserWindowGtk::CanClose() [0x7f47a0c8e787]
	BrowserWindowGtk::Close() [0x7f47a0c8bc12]
	BrowserTitlebar::OnButtonClicked() [0x7f47a13500ce]
	BrowserTitlebar::OnButtonClickedThunk() [0x7f47a1350de3]

Test 3:
I saw the following crash twice.

Steps:
Open google.com
Press Ctrl+P
Navigate to Yahoo.com
Press Ctrl+P
Press forward and back button consecutively and press ctrl+P after each click.
Close the tab using the 'x' button.

[7856:7856:878293592746:FATAL:notification_registrar.cc(68)] Check failed:
false. Trying to remove unregistered observer of type 32 from list of size 16.
Backtrace:
	base::debug::StackTrace::StackTrace() [0x7f280dc1deee]
	logging::LogMessage::~LogMessage() [0x7f280dc43fb6]
	NotificationRegistrar::Remove() [0x7f280e920263]
	printing::PrintPreviewTabController::RemoveObservers() [0x7f280d5f95a7]
	printing::PrintPreviewTabController::RemovePreviewTab() [0x7f280d5f9791]
	printing::PrintPreviewTabController::OnTabContentsDestroyed() [0x7f280d5f8ce7]
	printing::PrintPreviewTabController::Observe() [0x7f280d5f88e7]
	NotificationService::Notify() [0x7f280e922395]
	TabContents::~TabContents() [0x7f280fd8691f]
	scoped_ptr<>::~scoped_ptr() [0x7f280d1021d9]
	TabContentsWrapper::~TabContentsWrapper() [0x7f280d0ffc02]
	scoped_ptr<>::~scoped_ptr() [0x7f280cf61e3f]
	ConstrainedHtmlDelegateGtk::~ConstrainedHtmlDelegateGtk() [0x7f280d73be8d]
	ConstrainedHtmlDelegateGtk::DeleteDelegate() [0x7f280d73bfc0]
	ConstrainedWindowGtk::CloseConstrainedWindow() [0x7f280d0767b1]
	ConstrainedWindowTabHelper::CloseConstrainedWindows() [0x7f280d70fffa]
	ConstrainedWindowTabHelper::TabContentsDestroyed() [0x7f280d7103b4]
	TabContentsObserver::TabContentsDestroyed() [0x7f280fd9253b]
	TabContents::~TabContents() [0x7f280fd86b15]
	scoped_ptr<>::~scoped_ptr() [0x7f280d1021d9]
	TabContentsWrapper::~TabContentsWrapper() [0x7f280d0ffc02]
	TabStripModel::InternalCloseTab() [0x7f280d005800]
	TabStripModel::InternalCloseTabs() [0x7f280d0056ce]
	TabStripModel::CloseTabContentsAt() [0x7f280d001d56]
	Browser::CloseContents() [0x7f280d04672c]
	TabContents::Close() [0x7f280fd8bfaa]
	RenderViewHost::ClosePageIgnoringUnloadEvents() [0x7f280fd28a47]
	RenderViewHost::OnMsgClosePageACK() [0x7f280fd2c592]
	IPC::Message::Dispatch<>() [0x7f280fd2f3a3]
	RenderViewHost::OnMessageReceived() [0x7f280fd2a976]
	BrowserRenderProcessHost::OnMessageReceived() [0x7f280fde175f]
	IPC::ChannelProxy::Context::OnDispatchMessage() [0x7f280e96d312]
	DispatchToMethod<>() [0x7f280e96f8fc]
	RunnableMethod<>::Run() [0x7f280e96f626]
	base::subtle::TaskClosureAdapter::Run() [0x7f280dc90497]
	base::internal::Invoker1<>::DoInvoke() [0x7f280dc4c69a]
	base::Callback<>::Run() [0x7f280d56f691]
	MessageLoop::RunTask() [0x7f280dc4957b]
	MessageLoop::DeferOrRunPendingTask() [0x7f280dc496b5]
	MessageLoop::DoWork() [0x7f280dc49ecb]
	base::MessagePumpGlib::HandleDispatch() [0x7f280dcadf19]
	(anonymous namespace)::WorkSourceDispatch() [0x7f280dcad46b]
	0x7f280ab4e8c2
	0x7f280ab52748
	0x7f280ab528fc
	base::MessagePumpGtk::RunOnce() [0x7f280dcaf781]
	base::MessagePumpGlib::RunWithDispatcher() [0x7f280dcadbcc]
	base::MessagePumpGlib::Run() [0x7f280dcadff6]
	MessageLoop::RunInternal() [0x7f280dc49209]
	MessageLoop::RunHandler() [0x7f280dc490bc]
	MessageLoopForUI::Run() [0x7f280dc4a558]
	ChromeBrowserMainParts::MainMessageLoopRun() [0x7f280d85b811]
	content::BrowserMainParts::RunMainMessageLoopParts() [0x7f280fc3687d]
	BrowserMain() [0x7f280fc36bdf]
	(anonymous namespace)::RunNamedProcessTypeMain() [0x7f280db6acdf]
	content::ContentMain() [0x7f280db6b2d0]
	ChromeMain [0x7f280cd6274d]
	main [0x7f280cd6270c]
	0x7f28050c4c4d
	0x7f280cd62619

Sometimes, while navigating back and forth, preview tab shows "loading..."
screen forever and the navigation doesn't complete.

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1060: if (e.keyCode ==
27 && !e.shiftKey && !e.ctrlKey && !e.altKey && !e.metaKey) {
Several times, I tried to close the preview tab and initiator tab using ctrl+Q
and ctrl+W.  Can we listen for those key press events?

[vandebo (ex-Chrome), 2011-10-14 07:14:05]

It seems like you need additional reviewers. i.e. I don't think any of your
current reviewers can really review the nested message loop.  Generally LGTM
though.

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

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:236: }
nit: } else if

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (left):

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:197: function
onInitiatorTabCrashed(initiatorTabURL) {
The initiator tab can still crash.  What happens now?  Oh, does the entire tab
(including the print preview constrained dialog) just sad tab?

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/ui/browser_l...
File chrome/browser/ui/browser_list_unittest.cc (right):

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_list_unittest.cc:162: // TODO(thestig) Fix or remove
this test. http://crbug.com/XXXXX
At least file the bug, if you don't resolve the comment before commit

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

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:57: CANCEL,  // UNUSED
The cancel bucket should still be used - if they hit the cancel button (or
otherwise close the constrained window).

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:62: INITIATOR_TAB_CLOSED,
INITIATOR_TAB_CLOSED may be unsed now...

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.h (right):

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:100: void OnPrintPreviewTabCrashed();
You removed the other tab crashed code, do you still want/need this?

[Lei Zhang, 2011-10-14 18:26:06]

Darin: can you take a look at the renderer side changes for adding the nested
message loop? I know this doesn't correctly handle the case of two tabs sharing
a renderer both doing Javascript window.print(), but fixing that properly is
going to require more effort. This at least fixes most common use cases where
window.print() is followed by some JS triggered navigation.

[Lei Zhang, 2011-10-14 20:31:26]

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

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:74: const gfx::Size
min_size(800, 480);
On 2011/10/14 00:21:09, kmadhusu wrote:
> Define const int variables for all magic numbers.

But this is the constant. I renamed |min_size| to |kMinDialogSize|.

How is this any better?
const int kWidth = 800;
const int kHeight = 480;
const gfx:Size min_size(kWidth, kHeight)

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:97: return
GURL(chrome::kChromeUIPrintURL);
On 2011/10/14 00:21:09, kmadhusu wrote:
> nit: remove 2 spaces.

Done.

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:238:
closed_preview_tabs.push_back(initiator_tab);
On 2011/10/14 00:21:09, kmadhusu wrote:
> closed_preview_tabs => closed_initiator_tabs

Done.

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:251:
RemoveInitiatorTab(closed_initiator_tabs[i]);
On 2011/10/14 00:21:09, kmadhusu wrote:
> (repeating our conversation): I am not sure whether the initiator tab and
> preview tab can share the same rph. If they share the same rph, we will try to
> remove the observers of initiator tab again in RemoveInitiatorTab()
> (RemovePreviewTab already removed the observers for both initiator tab and
> preview tab).

I changed the if on line 237 to an else if.

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:423:
preview_tab_map_[preview_tab] = NULL;
On 2011/10/14 00:21:09, kmadhusu wrote:
> better to add a DCHECK to validate the preview_tab.

Done.

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_tab_controller_browsertest.cc
(right):

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller_browsertest.cc:64: // yet
have the same address as |preview_tab|. http://crbug.com/XXX
On 2011/10/14 00:21:09, kmadhusu wrote:
> Add a valid bug #. 

Done.

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1051:
chrome.send('DialogClose', []);
On 2011/10/14 00:21:09, kmadhusu wrote:
> You can do "chrome.send('DialogClose');". Any specific reason for passing an
> empty array?

Done. Copy/pasted from elsewhere I think.

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1064: if (e.keyCode ==
80 || e.keyCode == 112) {
On 2011/10/14 00:21:09, kmadhusu wrote:
> keycode 112 check is not required.

Done.

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

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4527: if (controller &&
controller->GetPrintPreviewForTab(wrapper)) {
On 2011/10/14 00:21:09, kmadhusu wrote:
> nit: {} not required

I like it for readability for nested if blocks.

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/browser_l...
File chrome/browser/ui/browser_list_unittest.cc (right):

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_list_unittest.cc:162: // TODO(thestig) Fix or remove
this test. http://crbug.com/XXXXX
On 2011/10/14 00:21:09, kmadhusu wrote:
> Add valid bug number.

Done.

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

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:57: CANCEL,  // UNUSED
On 2011/10/14 00:21:09, kmadhusu wrote:
> Add a todo to fix this later. We need to record the cancel actions and
initiator
> tab crashes.

I put it back to use.

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.h (right):

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:117: void HidePreviewTab();
On 2011/10/14 00:21:09, kmadhusu wrote:
> nit: OnHidePreviewTab() and OnClosePrintPreviewTab().

Done.

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui_unittest.cc (right):

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_unittest.cc:43:
ASSERT_TRUE(initiator_tab);
On 2011/10/14 00:21:09, kmadhusu wrote:
> EXPECT_EQ(0U, GetConstrainedWindowCount(initiator_tab));

Done.

http://codereview.chromium.org/8136027/diff/51006/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_unittest.cc:53: EXPECT_EQ(1,
browser()->tab_count());
On 2011/10/14 00:21:09, kmadhusu wrote:
> EXPECT_EQ(1U, GetConstrainedWindowCount(initiator_tab));

Done.

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

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:236: }
On 2011/10/14 07:14:05, vandebo wrote:
> nit: } else if

Done.

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (left):

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:197: function
onInitiatorTabCrashed(initiatorTabURL) {
On 2011/10/14 07:14:05, vandebo wrote:
> The initiator tab can still crash.  What happens now?  Oh, does the entire tab
> (including the print preview constrained dialog) just sad tab?

Sad tab takes over the tab and we lose the constrained windows. Try it.

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1060: if (e.keyCode ==
27 && !e.shiftKey && !e.ctrlKey && !e.altKey && !e.metaKey) {
On 2011/10/14 01:11:38, kmadhusu wrote:
> Several times, I tried to close the preview tab and initiator tab using ctrl+Q
> and ctrl+W.  Can we listen for those key press events?

I'd prefer not too. This starts us down a slippery slope where we start
listening for all keys. The user needs to realize they are in a print dialog and
they need to interact with it.

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:1065: if ((cr.isMac &&
e.shiftKey && e.metaKey && !e.ctrlKey && !e.altKey) ||
On 2011/10/14 00:21:09, kmadhusu wrote:
> On mac, we use cmd+opt+P and not cmd+shift+p

Shows you how much I know about macs. :)
_
 \_ is apparently not a shift key...

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/ui/browser_l...
File chrome/browser/ui/browser_list_unittest.cc (right):

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_list_unittest.cc:162: // TODO(thestig) Fix or remove
this test. http://crbug.com/XXXXX
On 2011/10/14 07:14:05, vandebo wrote:
> At least file the bug, if you don't resolve the comment before commit

Done.

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

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:57: CANCEL,  // UNUSED
On 2011/10/14 07:14:05, vandebo wrote:
> The cancel bucket should still be used - if they hit the cancel button (or
> otherwise close the constrained window).

Sure, undeleted some code.

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:62: INITIATOR_TAB_CLOSED,
On 2011/10/14 07:14:05, vandebo wrote:
> INITIATOR_TAB_CLOSED may be unsed now...

Still used at line 300.

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.h (right):

http://codereview.chromium.org/8136027/diff/58005/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:100: void OnPrintPreviewTabCrashed();
On 2011/10/14 07:14:05, vandebo wrote:
> You removed the other tab crashed code, do you still want/need this?

Yes, we can't leave the user with a sad tab in a constrained dialog with no way
to close it.

[kmadhusu, 2011-10-14 22:48:13]

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

http://codereview.chromium.org/8136027/diff/59002/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:74: const gfx::Size
kMinDialogSize(800, 480);
sorry.. i missed the const keyword in my previous review.

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

http://codereview.chromium.org/8136027/diff/67007/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:411:
initiator_tab->print_view_manager()->PrintPreviewDone();
How do you exit the nested loop in other cases?

[kmadhusu, 2011-10-14 22:51:25]

I didn't see any of the above said crashes using the latest patch. But I saw the
following crashes:

Crash 1:
ASSERTION FAILED: !m_queuedEvents.isEmpty()
third_party/WebKit/Source/WebCore/dom/EventQueue.cpp(126) : void
WebCore::EventQueue::pendingEventTimerFired()

Crash 2:
[31284:31284:952519060906:FATAL:printer_query.cc(27)] Check failed:
!worker_.get(). 
Backtrace:
	base::debug::StackTrace::StackTrace() [0x7f65c0d10a76]
	logging::LogMessage::~LogMessage() [0x7f65c0d36b3e]
	printing::PrinterQuery::~PrinterQuery() [0x7f65c06fb8c6]

I am not sure whether crash 1 is related to your changes.

Thanks.

[Lei Zhang, 2011-10-14 23:22:07]

I've seen both of those debug-only crashes and I'm not too worried about them.

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

http://codereview.chromium.org/8136027/diff/67007/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:411:
initiator_tab->print_view_manager()->PrintPreviewDone();
On 2011/10/14 22:48:13, kmadhusu wrote:
> How do you exit the nested loop in other cases?

The other cases all close the print preview dialog at some point I believe. I'm
testing the first block on cros but there's some other issues there.

[Lei Zhang, 2011-10-15 18:59:32]

On 2011/10/14 23:22:07, Lei Zhang wrote:
> I've seen both of those debug-only crashes and I'm not too worried about them.
> 
>
http://codereview.chromium.org/8136027/diff/67007/chrome/browser/ui/webui/pri...
> File chrome/browser/ui/webui/print_preview_handler.cc (right):
> 
>
http://codereview.chromium.org/8136027/diff/67007/chrome/browser/ui/webui/pri...
> chrome/browser/ui/webui/print_preview_handler.cc:411:
> initiator_tab->print_view_manager()->PrintPreviewDone();
> On 2011/10/14 22:48:13, kmadhusu wrote:
> > How do you exit the nested loop in other cases?
> 
> The other cases all close the print preview dialog at some point I believe.
I'm
> testing the first block on cros but there's some other issues there.

Flushed out the CrOS cloud printer issues. The dialog closes itself there too.

I also added a check to prevent some double window closes in PrintPreviewUI and
fixed the flaky browser tests.

[kmadhusu, 2011-10-16 07:31:34]

lgtm

Please ask albert to review the CrOS cloud print code.

[Lei Zhang, 2011-10-17 05:39:09]

Albert: Currently on CrOS, CP reloads the print preview page on sign in. This
doesn't work with the tab modal print preview dialog because the reload causes
the constrained window to lose its handlers. Instead, I just clear and
re-populate the printer list. This goes from
CloudPrintSigninFlowHandler::Observe() ->
PrintPreviewUI::OnReloadCloudPrintersList() -> JS reloadCloudPrintersList().

[Albert Bodenhamer, 2011-10-17 16:43:42]

lgtm

http://codereview.chromium.org/8136027/diff/73001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8136027/diff/73001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:267: function
reloadCloudPrintersList() {
Nit:  Wont this reload all printers?  I know you're only using it to refresh
after cloud print sign in, but the name should be "reloadPrintersList"

[Lei Zhang, 2011-10-17 18:18:57]

http://codereview.chromium.org/8136027/diff/73001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8136027/diff/73001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:267: function
reloadCloudPrintersList() {
On 2011/10/17 16:43:42, Albert Bodenhamer wrote:
> Nit:  Wont this reload all printers?  I know you're only using it to refresh
> after cloud print sign in, but the name should be "reloadPrintersList"

Done.

[darin (slow to review), 2011-10-17 19:41:39]

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

http://codereview.chromium.org/8136027/diff/59007/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1279: MessageLoop* message_loop =
print_preview_context_.message_loop();
why do you need the MessageLoop member here?  just use MessageLoop::current()

[Lei Zhang, 2011-10-17 20:07:06]

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

http://codereview.chromium.org/8136027/diff/59007/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1279: MessageLoop* message_loop =
print_preview_context_.message_loop();
On 2011/10/17 19:41:39, darin wrote:
> why do you need the MessageLoop member here?  just use MessageLoop::current()

Done.

[darin (slow to review), 2011-10-17 22:16:32]

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

http://codereview.chromium.org/8136027/diff/81002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1532: return in_nested_message_loop_;
can you explain why you need this member variable?  do you receive spurious
calls to OnPrintPreviewDone?

[Lei Zhang, 2011-10-17 22:26:05]

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

http://codereview.chromium.org/8136027/diff/81002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1532: return in_nested_message_loop_;
On 2011/10/17 22:16:33, darin wrote:
> can you explain why you need this member variable?  do you receive spurious
> calls to OnPrintPreviewDone?

Yes, it is possible to receive spurious calls to OnPrintPreviewDone.

Also, a webpage that contains JS code like window.onresize = function(event) {
window.print(); } will hit the check at line 472.

[Lei Zhang, 2011-11-02 08:14:19]

In patch set 14, I reduced the scope of this CL to just the UI changes. Pages
that do "window.print(), Javascript navigation" will remain broken.
"window.print(), window.close()" will break since I'm removing the hack.

I'll follow up with another CL to do the nested message loop in a way where we
shut off all the timers to fix the broken window.print() cases listed above.
After that I will work on modal dialogs for window.print().

[vandebo (ex-Chrome), 2011-11-02 20:50:30]

LGTM from what I can tell.

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:261: }
What should happen if either of these fail?

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

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:59:
std::allocator<WebUIMessageHandler*> >*) const OVERRIDE;
nit: the arguments should be named.

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:114: void
PrintPreviewTabDelegate::OnDialogClosed(const std::string& /* unused */) {
nit: I think the name should match the header.

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:248:
print_preview_ui->OnPrintPreviewTabCrashed();
Is this a crash case?

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_tab_controller_unittest.cc (right):

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller_unittest.cc:46:
EXPECT_EQ(1, browser()->tab_count());
Hmm, will this CL fix 83456 as well?

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/ui/browser_l...
File chrome/browser/ui/browser_list_unittest.cc (right):

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_list_unittest.cc:162: // TODO(thestig) Fix or remove
this test. http://crbug.com/100309
Do you know if this should be fixed or removed yet?

[Lei Zhang, 2011-11-02 22:50:03]

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:261: }
On 2011/11/02 20:50:30, vandebo wrote:
> What should happen if either of these fail?

Kausalya was wondering where we unset this, and it turns out this is redundant.
In a separate CL, I changed tab (un)blocking to update the printing state.

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

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:59:
std::allocator<WebUIMessageHandler*> >*) const OVERRIDE;
On 2011/11/02 20:50:30, vandebo wrote:
> nit: the arguments should be named.

Done.

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:114: void
PrintPreviewTabDelegate::OnDialogClosed(const std::string& /* unused */) {
On 2011/11/02 20:50:30, vandebo wrote:
> nit: I think the name should match the header.

Done.

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:248:
print_preview_ui->OnPrintPreviewTabCrashed();
On 2011/11/02 20:50:30, vandebo wrote:
> Is this a crash case?

It covers crashes, but we can also receive RendererProcessClosed notifications
for other reasons. PrintPreviewUI::OnPrintPreviewTabCrashed is poorly named,
just like OnInitiatorTabCrashed in the existing code. I'll change it.

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_tab_controller_unittest.cc (right):

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller_unittest.cc:46:
EXPECT_EQ(1, browser()->tab_count());
On 2011/11/02 20:50:30, vandebo wrote:
> Hmm, will this CL fix 83456 as well?

I suppose it does, since we don't create a new tab any more.

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/ui/browser_l...
File chrome/browser/ui/browser_list_unittest.cc (right):

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_list_unittest.cc:162: // TODO(thestig) Fix or remove
this test. http://crbug.com/100309
On 2011/11/02 20:50:30, vandebo wrote:
> Do you know if this should be fixed or removed yet?

I haven't given it much thought so it remains a TODO.

[vandebo (ex-Chrome), 2011-11-02 22:52:35]

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_tab_controller_unittest.cc (right):

http://codereview.chromium.org/8136027/diff/95004/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller_unittest.cc:46:
EXPECT_EQ(1, browser()->tab_count());
On 2011/11/02 22:50:03, Lei Zhang wrote:
> On 2011/11/02 20:50:30, vandebo wrote:
> > Hmm, will this CL fix 83456 as well?
> 
> I suppose it does, since we don't create a new tab any more.

You may want to take ownership of the bug, or note that you have a likely fix in
the works.

[kmadhusu, 2011-11-02 22:56:09]

lgtm