Print preview not showing if default print is invalid.

Print preview not showing if default print is invalid.

BUG=94082
TEST=none


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

[kmadhusu, 2011-08-25 17:33:35]

http://codereview.chromium.org/7740005/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7740005/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:5889: <message
name="IDS_PRINT_PREVIEW_INVALID_DEFAULT_PRINTER" desc="Message to display when
there is no default printer or default printer settings are invalid.">
(Repeating our in-person conversation): Please change all the function names,
ipc message and error description to say that the selection printer default
settings is invalid. 

The current code says "Default printer - settings is invalid. " But we want
"Selected printer - default settings is invalid".

[arthurhsu, 2011-08-26 00:17:36]

On 2011/08/25 17:33:35, kmadhusu wrote:
>
http://codereview.chromium.org/7740005/diff/1/chrome/app/generated_resources.grd
> File chrome/app/generated_resources.grd (right):
> 
>
http://codereview.chromium.org/7740005/diff/1/chrome/app/generated_resources....
> chrome/app/generated_resources.grd:5889: <message
> name="IDS_PRINT_PREVIEW_INVALID_DEFAULT_PRINTER" desc="Message to display when
> there is no default printer or default printer settings are invalid.">
> (Repeating our in-person conversation): Please change all the function names,
> ipc message and error description to say that the selection printer default
> settings is invalid. 
> 
> The current code says "Default printer - settings is invalid. " But we want
> "Selected printer - default settings is invalid".

I have updated the namings and messages.  Also added corresponding unit tests.

[kmadhusu, 2011-08-26 18:50:40]

http://codereview.chromium.org/7740005/diff/12/chrome/app/generated_resources...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7740005/diff/12/chrome/app/generated_resources...
chrome/app/generated_resources.grd:5890: Selected printer is not available or
not installed correctly.  Please select another printer.
nit: UI people said that we never want to say "Please" in error message. Can you
rephrase this?

Eg:
Selected printer is not available or not installed correctly. Check your printer
or select a different printer.

http://codereview.chromium.org/7740005/diff/12/chrome/browser/printing/print_...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7740005/diff/12/chrome/browser/printing/print_...
chrome/browser/printing/print_preview_message_handler.cc:223:
static_cast<PrintPreviewUI*>(print_preview_tab->web_ui());
If the preview tab is hidden for printing, we want to show an alert dialog with
an error message.

Refer to lines #187-190.

http://codereview.chromium.org/7740005/diff/12/chrome/browser/resources/print...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7740005/diff/12/chrome/browser/resources/print...
chrome/browser/resources/print_preview/print_preview.js:811: * Display an error
message when encountered invalid default printer.
nit: Update the comment.

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/mock_printer.cc
File chrome/renderer/mock_printer.cc (right):

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/mock_printer.c...
chrome/renderer/mock_printer.cc:90: void MockPrinter::SetBadSettings(bool
be_evil) {
How about UseInvalidSettings()? This function will always set
|use_invalid_settings_| to true. 

void MockPrinter::UseInvalidSettings() {
  use_invalid_settings_ = true;
}

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/mock_printer.c...
chrome/renderer/mock_printer.cc:243: return evil_ ? 0 :
++current_document_cookie_;
document_cookie is not a printer setting. So its better to set "dpi"  or 
"page_size" or "printable_size" values to zero to represent invalid printer
settings.

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/mock_printer.h
File chrome/renderer/mock_printer.h (right):

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/mock_printer.h...
chrome/renderer/mock_printer.h:146: bool evil_;
bool use_invalid_settings_;

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/print_web_view...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/print_web_view...
chrome/renderer/print_web_view_helper.cc:635: routing_id(),
nit: Fix indentation

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/print_web_view...
chrome/renderer/print_web_view_helper.cc:1030:
l10n_util::GetStringUTF16(IDS_DEFAULT_PRINTER_NOT_FOUND_WARNING));
If the printer settings is empty/invalid, we don't want to display "Could not
find default printer" error message. Please change this error.

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/print_web_view...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/print_web_view...
chrome/renderer/print_web_view_helper_browsertest.cc:441:
TEST_F(PrintWebViewHelperPreviewTest, OnPrintPreviewInvalidSettingsFail) {
You also need a separate test to verify "OnPrintForPrintPreview" failure if the
printer has invalid settings.

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/print_web_view...
chrome/renderer/print_web_view_helper_browsertest.cc:446: // Set mock printer to
provide invalid settings.
The purpose of orig_params and wrong_params is not clear here. 

How about the following test?

TEST_F(PrintWebViewHelperPreviewTest, OnPrintPreviewInvalidSettingsFail) {
  LoadHTML(kPrintPreviewHTML);

  PrintWebViewHelper::Get(view_)->OnInitiatePrintPreview();

  // Set the mock printer to provide invalid settings.
  render_thread_.printer()->UseInvalidPrinterSettings();

  // Fill in some dummy values.
  DictionaryValue dict;
  CreatePrintSettingsDictionary(&dict);
  PrintWebViewHelper::Get(view_)->OnPrintPreview(dict);

  // We should have received invalid printer settings from |printer_|.
  VerifyPrintPreviewInvalidPrinterSettings(true);

  EXPECT_EQ(0, render_thread_.print_preview_pages_reamining());
  VerifyPrintPreviewFailed(true);
  VerifyPrintPreviewGenerated(false);
  VertifyPagesPrinted(false);   
}

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/print_web_view...
chrome/renderer/print_web_view_helper_browsertest.cc:460:
VerifyPrintFailed(false);
We are verifying that the print preview failed.

[arthurhsu, 2011-08-29 22:53:16]

http://codereview.chromium.org/7740005/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7740005/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:5889: <message
name="IDS_PRINT_PREVIEW_INVALID_DEFAULT_PRINTER" desc="Message to display when
there is no default printer or default printer settings are invalid.">
On 2011/08/25 17:33:35, kmadhusu wrote:
> (Repeating our in-person conversation): Please change all the function names,
> ipc message and error description to say that the selection printer default
> settings is invalid. 
> 
> The current code says "Default printer - settings is invalid. " But we want
> "Selected printer - default settings is invalid".

Done.

http://codereview.chromium.org/7740005/diff/12/chrome/app/generated_resources...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7740005/diff/12/chrome/app/generated_resources...
chrome/app/generated_resources.grd:5890: Selected printer is not available or
not installed correctly.  Please select another printer.
On 2011/08/26 18:50:40, kmadhusu wrote:
> nit: UI people said that we never want to say "Please" in error message. Can
you
> rephrase this?
> 
> Eg:
> Selected printer is not available or not installed correctly. Check your
printer
> or select a different printer.

Done.

http://codereview.chromium.org/7740005/diff/12/chrome/browser/printing/print_...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7740005/diff/12/chrome/browser/printing/print_...
chrome/browser/printing/print_preview_message_handler.cc:223:
static_cast<PrintPreviewUI*>(print_preview_tab->web_ui());
On 2011/08/26 18:50:40, kmadhusu wrote:
> If the preview tab is hidden for printing, we want to show an alert dialog
with
> an error message.
> 
> Refer to lines #187-190. 

Done.

http://codereview.chromium.org/7740005/diff/12/chrome/browser/resources/print...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7740005/diff/12/chrome/browser/resources/print...
chrome/browser/resources/print_preview/print_preview.js:811: * Display an error
message when encountered invalid default printer.
On 2011/08/26 18:50:40, kmadhusu wrote:
> nit: Update the comment.

Done.

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/mock_printer.cc
File chrome/renderer/mock_printer.cc (right):

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/mock_printer.c...
chrome/renderer/mock_printer.cc:90: void MockPrinter::SetBadSettings(bool
be_evil) {
On 2011/08/26 18:50:40, kmadhusu wrote:
> How about UseInvalidSettings()? This function will always set
> |use_invalid_settings_| to true. 
> 
> void MockPrinter::UseInvalidSettings() {
>   use_invalid_settings_ = true;
> }

Added a value in parameter, since we need to flip the bit back for other test
cases.

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/mock_printer.c...
chrome/renderer/mock_printer.cc:243: return evil_ ? 0 :
++current_document_cookie_;
On 2011/08/26 18:50:40, kmadhusu wrote:
> document_cookie is not a printer setting. So its better to set "dpi"  or 
> "page_size" or "printable_size" values to zero to represent invalid printer
> settings.

It is used to determine if the printer settings is invalid in
print_web_view_helper.cc:102.

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/mock_printer.h
File chrome/renderer/mock_printer.h (right):

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/mock_printer.h...
chrome/renderer/mock_printer.h:146: bool evil_;
On 2011/08/26 18:50:40, kmadhusu wrote:
> bool use_invalid_settings_;

Done.

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/print_web_view...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/print_web_view...
chrome/renderer/print_web_view_helper.cc:635: routing_id(),
On 2011/08/26 18:50:40, kmadhusu wrote:
> nit: Fix indentation

Done.

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/print_web_view...
chrome/renderer/print_web_view_helper.cc:1030:
l10n_util::GetStringUTF16(IDS_DEFAULT_PRINTER_NOT_FOUND_WARNING));
On 2011/08/26 18:50:40, kmadhusu wrote:
> If the printer settings is empty/invalid, we don't want to display "Could not
> find default printer" error message. Please change this error.

Done.

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/print_web_view...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/print_web_view...
chrome/renderer/print_web_view_helper_browsertest.cc:441:
TEST_F(PrintWebViewHelperPreviewTest, OnPrintPreviewInvalidSettingsFail) {
On 2011/08/26 18:50:40, kmadhusu wrote:
> You also need a separate test to verify "OnPrintForPrintPreview" failure if
the
> printer has invalid settings. 

Done.

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/print_web_view...
chrome/renderer/print_web_view_helper_browsertest.cc:446: // Set mock printer to
provide invalid settings.
On 2011/08/26 18:50:40, kmadhusu wrote:
> The purpose of orig_params and wrong_params is not clear here. 
> 
> How about the following test?
> 
> TEST_F(PrintWebViewHelperPreviewTest, OnPrintPreviewInvalidSettingsFail) {
>   LoadHTML(kPrintPreviewHTML);
> 
>   PrintWebViewHelper::Get(view_)->OnInitiatePrintPreview();
> 
>   // Set the mock printer to provide invalid settings.
>   render_thread_.printer()->UseInvalidPrinterSettings();
> 
>   // Fill in some dummy values.
>   DictionaryValue dict;
>   CreatePrintSettingsDictionary(&dict);
>   PrintWebViewHelper::Get(view_)->OnPrintPreview(dict);
> 
>   // We should have received invalid printer settings from |printer_|.
>   VerifyPrintPreviewInvalidPrinterSettings(true);
> 
>   EXPECT_EQ(0, render_thread_.print_preview_pages_reamining());
>   VerifyPrintPreviewFailed(true);
>   VerifyPrintPreviewGenerated(false);
>   VertifyPagesPrinted(false);   
> }

Done.

http://codereview.chromium.org/7740005/diff/12/chrome/renderer/print_web_view...
chrome/renderer/print_web_view_helper_browsertest.cc:460:
VerifyPrintFailed(false);
On 2011/08/26 18:50:40, kmadhusu wrote:
> We are verifying that the print preview failed.

Done.

[kmadhusu, 2011-08-30 02:11:26]

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

http://codereview.chromium.org/7740005/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:216:
StopWorker(document_cookie);
You need to stop the worker only if the document_cookie is valid.

http://codereview.chromium.org/7740005/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:233:
print_preview_ui->OnInvalidPrinterSettings(show_as_alert);
If you are going to display the alert from javascript, you can easily identify
whether the tab is hidden or not in print_preview.js using |isTabHidden|
variable. Therefore, you can remove |show_as_alert| param here and hence lines
222-229 is not required here.


Having said that, we still want to delete the background printing tab (which is
the hidden preview tab) after the alert is shown and the user has clicked "Ok"
button. You may want to listen for the user action and then send a message from
javascript to c++ to close/delete the hidden preview tab wrapper.

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

http://codereview.chromium.org/7740005/diff/12001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:820: function
invalidPrinterSettings() {
Using "isTabHidden" variable, you can merge invalidPrinterSettingsAlert()
function definition with this function. 

if (isTabHidden)
  alert(...);
else
  displayErrorMessage(...);

http://codereview.chromium.org/7740005/diff/12001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:824: function
invalidPrinterSettingsAlert() {
This function is not required as per my previous comment.

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

http://codereview.chromium.org/7740005/diff/12001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.cc:235: void
PrintPreviewUI::OnInvalidPrinterSettings(bool show_alert_dialog) {
As I said earlier, |show_alert_dialog| is not required.

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/mock_printer.h
File chrome/renderer/mock_printer.h (right):

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/mock_printe...
chrome/renderer/mock_printer.h:146: bool use_invalid_settings_;
initialize this member variable to false.

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:441:
TEST_F(PrintWebViewHelperPreviewTest, OnPrintPreviewInvalidDefaultPrinter) {
How about? 

OnPrintPreviewInvalidDefaultPrinter => OnPrintPreviewUsingInvalidPrinterSettings

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:457: EXPECT_EQ(0,
render_thread_.print_preview_pages_remaining());
print_preview_pages_remaining() return 1 because we haven't rendered any pages
for preview.

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:462:
VerifyPrintPreviewGenerated(false);
nit: Remove the blank line above this statement.

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:465:
TEST_F(PrintWebViewHelperPreviewTest, OnPrintForPrintPreviewInvalidPrinter) {
How about?

OnPrintForPrintPreviewInvalidPrinter =>
OnPrintForPrintPreviewUsingInvalidPrinterSetting

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:470:
render_thread_.printer()->ResetPrinter();
Is ResetPrinter() required? I think mockprinter is not shared among tests.

[arthurhsu, 2011-08-30 20:43:02]

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

http://codereview.chromium.org/7740005/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:216:
StopWorker(document_cookie);
On 2011/08/30 02:11:27, kmadhusu wrote:
> You need to stop the worker only if the document_cookie is valid.

Done.

http://codereview.chromium.org/7740005/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:233:
print_preview_ui->OnInvalidPrinterSettings(show_as_alert);
Per discussion, changed to follow the OnPrintPreviewFailed pattern.

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

http://codereview.chromium.org/7740005/diff/12001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:820: function
invalidPrinterSettings() {
On 2011/08/30 02:11:27, kmadhusu wrote:
> Using "isTabHidden" variable, you can merge invalidPrinterSettingsAlert()
> function definition with this function. 
> 
> if (isTabHidden)
>   alert(...);
> else
>   displayErrorMessage(...);

Done.

http://codereview.chromium.org/7740005/diff/12001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:824: function
invalidPrinterSettingsAlert() {
On 2011/08/30 02:11:27, kmadhusu wrote:
> This function is not required as per my previous comment.

Done.

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

http://codereview.chromium.org/7740005/diff/12001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.cc:235: void
PrintPreviewUI::OnInvalidPrinterSettings(bool show_alert_dialog) {
On 2011/08/30 02:11:27, kmadhusu wrote:
> As I said earlier, |show_alert_dialog| is not required. 

Done.

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/mock_printer.h
File chrome/renderer/mock_printer.h (right):

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/mock_printe...
chrome/renderer/mock_printer.h:146: bool use_invalid_settings_;
On 2011/08/30 02:11:27, kmadhusu wrote:
> initialize this member variable to false.

Done.

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:441:
TEST_F(PrintWebViewHelperPreviewTest, OnPrintPreviewInvalidDefaultPrinter) {
On 2011/08/30 02:11:27, kmadhusu wrote:
> How about? 
> 
> OnPrintPreviewInvalidDefaultPrinter =>
OnPrintPreviewUsingInvalidPrinterSettings

Done.

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:457: EXPECT_EQ(0,
render_thread_.print_preview_pages_remaining());
On 2011/08/30 02:11:27, kmadhusu wrote:
> print_preview_pages_remaining() return 1 because we haven't rendered any pages
> for preview. 

It should be zero since it can't tell how many pages are there (there's no
printer settings for it to determine).

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:462:
VerifyPrintPreviewGenerated(false);
On 2011/08/30 02:11:27, kmadhusu wrote:
> nit: Remove the blank line above this statement.

Done.

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:465:
TEST_F(PrintWebViewHelperPreviewTest, OnPrintForPrintPreviewInvalidPrinter) {
On 2011/08/30 02:11:27, kmadhusu wrote:
> How about?
> 
> OnPrintForPrintPreviewInvalidPrinter =>
> OnPrintForPrintPreviewUsingInvalidPrinterSetting

Done.

http://codereview.chromium.org/7740005/diff/12001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:470:
render_thread_.printer()->ResetPrinter();
On 2011/08/30 02:11:27, kmadhusu wrote:
> Is ResetPrinter() required? I think mockprinter is not shared among tests.

Done.

[kmadhusu, 2011-08-31 01:23:47]

LGTM

[commit-bot: I haz the power, 2011-08-31 01:55:02]

Try job failure for 7740005-18001 (retry) on linux for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-08-31 22:03:01]

Try job failure for 7740005-22001 (retry) on linux for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-09-01 04:25:24]

Change committed as 99123

[Lei Zhang, 2011-09-01 04:52:08]

http://codereview.chromium.org/7740005/diff/26001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7740005/diff/26001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5961: Selected printer is not available or
not installed correctly.  Check your printer or select another printer.
nit: The selected printer ... or try selecting another printer.

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

http://codereview.chromium.org/7740005/diff/26001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:212: void
PrintPreviewMessageHandler::OnInvalidPrinterSettings(int document_cookie) {
You can probably merge some of this code with
PrintPreviewMessageHandler::OnPrintPreviewFailed.