Print Preview: Handle a crashed initiator tab by showing a message in PP.

Print Preview: Handle a crashed initiator tab by showing a message in PP.

Allow the user to reopen the crashed tab.

Also refactor Observe notification handling in PrintPreviewTabController while I'm here.

Review URL: http://codereview.chromium.org/7326036/

BUG=83656, 92066, 84764
TEST=Added new PrintPreviewTabControllerBrowserTest

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

[dpapad, 2011-08-08 22:01:57]

http://codereview.chromium.org/7550063/diff/2002/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7550063/diff/2002/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:5843: Print is unavailable because the page
you were trying to print has crashed.
Nit: Printing is unavailable....

http://codereview.chromium.org/7550063/diff/2002/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:5846: Print is unavailable because the page
you were trying to print has been closed.
Nit: same here.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:171: function() {
chrome.send('reloadCrashedInitiatorTab') });
Nit: add a semicolon after chrome.send('reloadCrashedInitiatorTab')

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:656: const ListValue* args) {
According to the style guide, unused parameter names should be commented out in
function definitions.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

[Lei Zhang, 2011-08-09 20:51:18]

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_message_handler.cc:92: if
(!print_preview_tab || !print_preview_tab->web_ui())
Everything in this file can go into the cleanup CL as well.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_tab_controller.cc (right):

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_tab_controller.cc:92: case
content::NOTIFICATION_RENDERER_PROCESS_CLOSED: {
If we just close a tab, do we get this notification? Or do we get
NOTIFICATION_TAB_CONTENTS_DESTORYED first?

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_tab_controller.cc:127: if (!initiator_tab)
In this case, do you need to call RemoveObservers() for the print preview tab? I
don't know if observers are automatically removed for a crashed tab.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_tab_controller.cc:133:
print_preview_ui->OnInitiatorTabCrashed();
Do you need to check |preview_tab| or |print_preview_ui| to see if they are
NULL?

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_tab_controller.cc:150: if
(IsPrintPreviewTab(tab) && tab->web_ui()) {
If the print preview tab is navigating to its initiator tab URL, shouldn't that
be handled in OnNavEntryCommitted() below?

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_tab_controller.cc:196: if
(preview_tab->web_ui()) {
nit: combine with the if statement above.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_tab_controller.h (right):

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_tab_controller.h:99: // Mapping between
initiator tab and the corresponding print preview tab.
When I read this comment, I thought the key was the initiator tab, but the
typedef above says otherwise. Can you just remove this comment and move the
PrintPreviewTabMap typedef to right above this instead?

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_tab_controller_unittest.cc (left):

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_tab_controller_unittest.cc:42:
static_cast<RenderViewHostDelegate*>(initiator_tab)->Activate();
Just curious why you are removing the Activate() calls.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.h (right):

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.h:63: // Gets the default printer.
|args| is unused.
Can you put all these comment changes into a separate cleanup CL?

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.h:129: void SendPrinterList(const
base::ListValue& printers);
Bad merge?

[kmadhusu, 2011-08-10 16:46:59]

Addressed review comments.

I am still working on unit test failure.

http://codereview.chromium.org/7550063/diff/2002/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7550063/diff/2002/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:5843: Print is unavailable because the page
you were trying to print has crashed.
On 2011/08/08 22:01:58, dpapad wrote:
> Nit: Printing is unavailable....

Marcin gave the error msg in line #5486. This is similar to that msg. Therefore,
I would like to leave the way it is.

http://codereview.chromium.org/7550063/diff/2002/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:5846: Print is unavailable because the page
you were trying to print has been closed.
On 2011/08/08 22:01:58, dpapad wrote:
> Nit: same here.

Marcin gave this error message. I would like to retain it.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_message_handler.cc:92: if
(!print_preview_tab || !print_preview_tab->web_ui())
On 2011/08/09 20:51:19, Lei Zhang wrote:
> Everything in this file can go into the cleanup CL as well.

Done.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_tab_controller.cc (right):

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_tab_controller.cc:92: case
content::NOTIFICATION_RENDERER_PROCESS_CLOSED: {
On 2011/08/09 20:51:19, Lei Zhang wrote:
> If we just close a tab, do we get this notification? Or do we get
> NOTIFICATION_TAB_CONTENTS_DESTORYED first?

When a tab is closed, we do not get this notification. We only get
NOTIFICATION_TAB_CONTENTS_DESTROYED. This notification is only to notify crashed
tabs.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_tab_controller.cc:127: if (!initiator_tab)
On 2011/08/09 20:51:19, Lei Zhang wrote:
> In this case, do you need to call RemoveObservers() for the print preview tab?
I
> don't know if observers are automatically removed for a crashed tab.

We should not remove the observers for the preview tab because when the user
navigates from/to the preview tab, we should be able to identify the preview tab
and prevent the print operation on that tab.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_tab_controller.cc:133:
print_preview_ui->OnInitiatorTabCrashed();
On 2011/08/09 20:51:19, Lei Zhang wrote:
> Do you need to check |preview_tab| or |print_preview_ui| to see if they are
> NULL?

They will be valid. If you want I can add a DCHECK.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_tab_controller.cc:150: if
(IsPrintPreviewTab(tab) && tab->web_ui()) {
On 2011/08/09 20:51:19, Lei Zhang wrote:
> If the print preview tab is navigating to its initiator tab URL, shouldn't
that
> be handled in OnNavEntryCommitted() below?

The comment is misleading. Here, the preview tab is destroyed, so we are
notifying |PrintPreviewUI| to abort the initiator tab preview requests.

Fixed the comment.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_tab_controller.cc:196: if
(preview_tab->web_ui()) {
On 2011/08/09 20:51:19, Lei Zhang wrote:
> nit: combine with the if statement above.

Done.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_tab_controller.h (right):

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_tab_controller.h:99: // Mapping between
initiator tab and the corresponding print preview tab.
On 2011/08/09 20:51:19, Lei Zhang wrote:
> When I read this comment, I thought the key was the initiator tab, but the
> typedef above says otherwise. Can you just remove this comment and move the
> PrintPreviewTabMap typedef to right above this instead?

As per chromium style guide, I cannot move typedef right about this statement.
Instead, I have fixed the comment.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
File chrome/browser/printing/print_preview_tab_controller_unittest.cc (left):

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/printing/prin...
chrome/browser/printing/print_preview_tab_controller_unittest.cc:42:
static_cast<RenderViewHostDelegate*>(initiator_tab)->Activate();
On 2011/08/09 20:51:19, Lei Zhang wrote:
> Just curious why you are removing the Activate() calls.

Previously, we were creating print preview tab for the current active tab. But
we changed the code to "GetOrCreatePreviewTabForTab"(for a given tab). So we no
longer require these activate calls.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:171: function() {
chrome.send('reloadCrashedInitiatorTab') });
On 2011/08/08 22:01:58, dpapad wrote:
> Nit: add a semicolon after chrome.send('reloadCrashedInitiatorTab')

Done.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:656: const ListValue* args) {
On 2011/08/08 22:01:58, dpapad wrote:
> According to the style guide, unused parameter names should be commented out
in
> function definitions.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.h (right):

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.h:63: // Gets the default printer.
|args| is unused.
On 2011/08/09 20:51:19, Lei Zhang wrote:
> Can you put all these comment changes into a separate cleanup CL?

Done.

http://codereview.chromium.org/7550063/diff/2002/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.h:129: void SendPrinterList(const
base::ListValue& printers);
On 2011/08/09 20:51:19, Lei Zhang wrote:
> Bad merge?

my bad.. fixed.

[kmadhusu, 2011-08-10 21:23:24]

thestig: Updated print_preview_ui.h

[dpapad, 2011-08-10 22:37:02]

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

http://codereview.chromium.org/7550063/diff/14001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:763:
disableInputElementsInSidebar();
Should disableInputElementsInSidebar() be within displayErrorMessage() in order
to disable the elements even when the error message has no button?

[Lei Zhang, 2011-08-11 01:30:50]

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

http://codereview.chromium.org/7550063/diff/14001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:120: if
(iter->second->render_view_host()->process() == rph) {
If multiple tabs can share the same RendererProcessHost, then isn't it possible
for us to have multiple initiator tabs with the same |rph| here?

That is, initiator tab A and initiator tab B are in the same renderer process.
Both A and B open print preview tabs. The process crashes -> both print preview
tabs should display an error.

http://codereview.chromium.org/7550063/diff/14001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.h (right):

http://codereview.chromium.org/7550063/diff/14001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.h:56: void OnNavigation();
Based on the comment, shouldn't this be renamed to OnTabDestroyed() or
something?

[kmadhusu, 2011-08-15 18:12:49]

Addressed review comments.

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

http://codereview.chromium.org/7550063/diff/14001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:120: if
(iter->second->render_view_host()->process() == rph) {
On 2011/08/11 01:30:51, Lei Zhang wrote:
> If multiple tabs can share the same RendererProcessHost, then isn't it
possible
> for us to have multiple initiator tabs with the same |rph| here?
> 
> That is, initiator tab A and initiator tab B are in the same renderer process.
> Both A and B open print preview tabs. The process crashes -> both print
preview
> tabs should display an error.
Yes. We should display the error on both print preview tabs.

Made code changes accordingly.

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

http://codereview.chromium.org/7550063/diff/14001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:763:
disableInputElementsInSidebar();
On 2011/08/10 22:37:02, dpapad wrote:
> Should disableInputElementsInSidebar() be within displayErrorMessage() in
order
> to disable the elements even when the error message has no button?

Done.

http://codereview.chromium.org/7550063/diff/14001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.h (right):

http://codereview.chromium.org/7550063/diff/14001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.h:56: void OnNavigation();
On 2011/08/11 01:30:51, Lei Zhang wrote:
> Based on the comment, shouldn't this be renamed to OnTabDestroyed() or
> something?

Fixed.

[Lei Zhang, 2011-08-16 00:59:23]

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

http://codereview.chromium.org/7550063/diff/26002/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:50: 
nit: do we need all these extra blank lines?

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

http://codereview.chromium.org/7550063/diff/26002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:745:
disableInputElementsInSidebar();
This will change the behavior for printPreviewFailed(). Let's not do this right
now and fix printPreviewFailed in a separate CL.

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

http://codereview.chromium.org/7550063/diff/26002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:657: const ListValue* /* args
*/) {
nit: just leave off /* args */ like we do in the rest of the file.

http://codereview.chromium.org/7550063/diff/26002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:659:
ReportUserActionHistogram(PREVIEW_FAILED);
Can we use a different histogram value so we can differentiate between failures
and crashes?

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

http://codereview.chromium.org/7550063/diff/26002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:65: // Notifies the Web UI that a
navigation has occurred in this tab. This is the
This comment is out of date.

[kmadhusu, 2011-08-16 01:31:37]

thestig: Addressed review comments.

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

http://codereview.chromium.org/7550063/diff/26002/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_tab_controller.cc:50: 
On 2011/08/16 00:59:23, Lei Zhang wrote:
> nit: do we need all these extra blank lines?

Removed.

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

http://codereview.chromium.org/7550063/diff/26002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:745:
disableInputElementsInSidebar();
On 2011/08/16 00:59:23, Lei Zhang wrote:
> This will change the behavior for printPreviewFailed(). Let's not do this
right
> now and fix printPreviewFailed in a separate CL.

Done.

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

http://codereview.chromium.org/7550063/diff/26002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:657: const ListValue* /* args
*/) {
On 2011/08/16 00:59:23, Lei Zhang wrote:
> nit: just leave off /* args */ like we do in the rest of the file.

Done.

http://codereview.chromium.org/7550063/diff/26002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:659:
ReportUserActionHistogram(PREVIEW_FAILED);
On 2011/08/16 00:59:23, Lei Zhang wrote:
> Can we use a different histogram value so we can differentiate between
failures
> and crashes?

+ INITIATOR_TAB_CRASHED

Once you are fine with the name, I will add it to the histograms list.

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

http://codereview.chromium.org/7550063/diff/26002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:65: // Notifies the Web UI that a
navigation has occurred in this tab. This is the
On 2011/08/16 00:59:23, Lei Zhang wrote:
> This comment is out of date.

Fixed.

[Lei Zhang, 2011-08-16 03:30:40]

LGTM

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

http://codereview.chromium.org/7550063/diff/31002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:76: INITIATOR_TAB_CRASHED,
do we need a corresponding change in some UMA .xml file somewhere?

[kmadhusu, 2011-08-16 16:46:47]

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

http://codereview.chromium.org/7550063/diff/31002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:76: INITIATOR_TAB_CRASHED,
On 2011/08/16 03:30:41, Lei Zhang wrote:
> do we need a corresponding change in some UMA .xml file somewhere?

Yeah. That will be a seperate CL.

[James Hawkins, 2011-08-16 18:37:05]

LGTM. Was there a particular reason you chose to use a browsertest for the added
tests?

[kmadhusu, 2011-08-16 18:43:04]

On 2011/08/16 18:37:05, James Hawkins wrote:
> LGTM. Was there a particular reason you chose to use a browsertest for the
added
> tests?

This way I was able to get the NOTIFICATION_NAV_ENTRY_COMMITTED events.