Printing: Catch more error conditions and remove more cruft.

chrome/browser/ui/webui/print_preview/print_preview_handler.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/webui/print_preview/print_preview_handler.h"
 
 #include <ctype.h>
 
 #include <string>
 #include <vector>
@@ skipping 102 matching lines @@
 // locale.
 const char kMeasurementSystem[] = "measurementSystem";
 // Name of a dictionary field holding the number format according to the locale.
 const char kNumberFormat[] = "numberFormat";
 // Name of a dictionary field specifying whether to print automatically in
 // kiosk mode. See http://crbug.com/31395.
 const char kPrintAutomaticallyInKioskMode[] = "printAutomaticallyInKioskMode";
 
 
 // Get the print job settings dictionary from |args|. The caller takes
-// ownership of the returned DictionaryValue. Returns NULL on failure.
+// ownership of the returned DictionaryValue.
 DictionaryValue* GetSettingsDictionary(const ListValue* args) {
   std::string json_str;
-  if (!args->GetString(0, &json_str)) {
-    NOTREACHED() << "Could not read JSON argument";
-    return NULL;
-  }
-  if (json_str.empty()) {
-    NOTREACHED() << "Empty print job settings";
-    return NULL;
-  }
+  CHECK(args->GetString(0, &json_str));
+  CHECK(!json_str.empty());

[kmadhusu, 2012/03/14 18:22:20]

'if' condition is better than CHECK because we are on the browser code and we
don't want to crash the browser. 

[Lei Zhang, 2012/03/14 20:09:11]

But we want to crash if this ever happens, so we know about it through a crash
report, or through a frustrated user. Though it should never happen, thus
there's nothing to worry about. ;)

[kmadhusu, 2012/03/14 20:28:33]

Sorry, you are adding ~15 CHECK conditions here. If any one of the conditions is
met, it will blow up the entire browser. We should handle the error cases
gracefully and say "Print Preview Failed" message rather than crashing it. 

   scoped_ptr<DictionaryValue> settings(static_cast<DictionaryValue*>(
       base::JSONReader::Read(json_str, false)));
-  if (!settings.get() || !settings->IsType(Value::TYPE_DICTIONARY)) {
-    NOTREACHED() << "Print job settings must be a dictionary.";
-    return NULL;
-  }
-
-  if (settings->empty()) {
-    NOTREACHED() << "Print job settings dictionary is empty";
-    return NULL;
-  }
-
+  CHECK(settings.get() && settings->IsType(Value::TYPE_DICTIONARY));
+  CHECK(!settings->empty());
   return settings.release();
 }
 
 int GetPageCountFromSettingsDictionary(const DictionaryValue& settings) {
   int count = 0;
   ListValue* page_range_array;
   if (settings.GetList(printing::kSettingPageRange, &page_range_array)) {
     for (size_t index = 0; index < page_range_array->GetSize(); ++index) {
       DictionaryValue* dict;
       if (!page_range_array->GetDictionary(index, &dict))
@@ skipping 120 matching lines @@
                                print_backend_.get(),
                                has_logged_printers_count_);
   has_logged_printers_count_ = true;
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&PrintSystemTaskProxy::EnumeratePrinters, task.get()));
 }
 
 void PrintPreviewHandler::HandleGetPreview(const ListValue* args) {
-  DCHECK_EQ(3U, args->GetSize());
+  CHECK_EQ(3U, args->GetSize());
   scoped_ptr<DictionaryValue> settings(GetSettingsDictionary(args));
-  if (!settings.get())
-    return;
   int request_id = -1;
   if (!settings->GetInteger(printing::kPreviewRequestID, &request_id))
     return;
 
   PrintPreviewUI* print_preview_ui = static_cast<PrintPreviewUI*>(
       web_ui()->GetController());
   print_preview_ui->OnPrintPreviewRequest(request_id);
   // Add an additional key in order to identify |print_preview_ui| later on
   // when calling PrintPreviewUI::GetCurrentPrintPreviewStatus() on the IO
   // thread.
   settings->SetString(printing::kPreviewUIAddr,
                       print_preview_ui->GetPrintPreviewUIAddress());
 
   // Increment request count.
   ++regenerate_preview_request_count_;
 
   TabContentsWrapper* initiator_tab = GetInitiatorTab();
   if (!initiator_tab) {
     ReportUserActionHistogram(INITIATOR_TAB_CLOSED);
     print_preview_ui->OnClosePrintPreviewTab();
     return;
   }
 
   // Retrieve the page title and url and send it to the renderer process if
   // headers and footers are to be displayed.
   bool display_header_footer = false;
-  if (!settings->GetBoolean(printing::kSettingHeaderFooterEnabled,
-                            &display_header_footer)) {
-    NOTREACHED();
-  }
+  CHECK(settings->GetBoolean(printing::kSettingHeaderFooterEnabled,
+                             &display_header_footer));
   if (display_header_footer) {
     settings->SetString(printing::kSettingHeaderFooterTitle,
                         initiator_tab->web_contents()->GetTitle());
     std::string url;
     NavigationEntry* entry =
         initiator_tab->web_contents()->GetController().GetActiveEntry();
     if (entry)
       url = entry->GetVirtualURL().spec();
     settings->SetString(printing::kSettingHeaderFooterURL, url);
   }
 
   bool generate_draft_data = false;
-  bool success = settings->GetBoolean(printing::kSettingGenerateDraftData,
-                                      &generate_draft_data);
-  DCHECK(success);
+  CHECK(settings->GetBoolean(printing::kSettingGenerateDraftData,
+                             &generate_draft_data));
 
   if (!generate_draft_data) {
     double draft_page_count_double = -1;
-    success = args->GetDouble(1, &draft_page_count_double);
-    DCHECK(success);
+    CHECK(args->GetDouble(1, &draft_page_count_double));
     int draft_page_count = static_cast<int>(draft_page_count_double);
 
     bool preview_modifiable = false;
-    success = args->GetBoolean(2, &preview_modifiable);
-    DCHECK(success);
+    CHECK(args->GetBoolean(2, &preview_modifiable));
 
     if (draft_page_count != -1 && preview_modifiable &&
         print_preview_ui->GetAvailableDraftPageCount() != draft_page_count) {
       settings->SetBoolean(printing::kSettingGenerateDraftData, true);
     }
   }
 
   VLOG(1) << "Print preview request start";
   RenderViewHost* rvh = initiator_tab->web_contents()->GetRenderViewHost();
   rvh->Send(new PrintMsg_PrintPreview(rvh->GetRoutingID(), *settings));
 }
 
 void PrintPreviewHandler::HandlePrint(const ListValue* args) {
+  CHECK_EQ(2U, args->GetSize());
+
   ReportStats();
 
   // Record the number of times the user requests to regenerate preview data
   // before printing.
   UMA_HISTOGRAM_COUNTS("PrintPreview.RegeneratePreviewRequest.BeforePrint",
                        regenerate_preview_request_count_);
 
   TabContentsWrapper* initiator_tab = GetInitiatorTab();
   if (initiator_tab) {
     RenderViewHost* rvh = initiator_tab->web_contents()->GetRenderViewHost();
     rvh->Send(new PrintMsg_ResetScriptedPrintCount(rvh->GetRoutingID()));
   }
 
   scoped_ptr<DictionaryValue> settings(GetSettingsDictionary(args));
-  if (!settings.get())
-    return;
 
   // Storing last used settings.
   GetStickySettings()->Store(*settings);
 
   // Never try to add headers/footers here. It's already in the generated PDF.
   settings->SetBoolean(printing::kSettingHeaderFooterEnabled, false);
 
   bool print_to_pdf = false;
   bool is_cloud_printer = false;
   bool is_cloud_dialog = false;
 
   bool open_pdf_in_preview = false;
 #if defined(OS_MACOSX)
   open_pdf_in_preview = settings->HasKey(printing::kSettingOpenPDFInPreview);
 #endif
 
   if (!open_pdf_in_preview) {
     settings->GetBoolean(printing::kSettingPrintToPDF, &print_to_pdf);
     settings->GetBoolean(printing::kSettingCloudPrintDialog, &is_cloud_dialog);
     is_cloud_printer = settings->HasKey(printing::kSettingCloudPrintId);
   }
 
   if (is_cloud_printer) {
     std::string print_ticket;
-    bool res = args->GetString(1, &print_ticket);
-    DCHECK(res);
+    CHECK(args->GetString(1, &print_ticket));
     SendCloudPrintJob(*settings, print_ticket);
   } else if (print_to_pdf) {
     HandlePrintToPdf(*settings);
   } else if (is_cloud_dialog) {
     HandlePrintWithCloudPrint();
   } else {
     ReportPrintSettingsStats(*settings);
     ReportUserActionHistogram(PRINT_TO_PRINTER);
     UMA_HISTOGRAM_COUNTS("PrintPreview.PageCount.PrintToPrinter",
                          GetPageCountFromSettingsDictionary(*settings));
@@ skipping 74 matching lines @@
     if (browser)
       initiator_tab = browser->GetSelectedTabContentsWrapper();
   }
 
   if (initiator_tab)
     initiator_tab->print_view_manager()->PreviewPrintingRequestCancelled();
   delete preview_tab_wrapper();
 }
 
 void PrintPreviewHandler::HandleSaveLastPrinter(const ListValue* args) {
+  CHECK_EQ(2U, args->GetSize());
   std::string data_to_save;
   if (args->GetString(0, &data_to_save) && !data_to_save.empty())
     GetStickySettings()->StorePrinterName(data_to_save);
 
   if (args->GetString(1, &data_to_save) && !data_to_save.empty())
     GetStickySettings()->StoreCloudPrintData(data_to_save);
 }
 
 void PrintPreviewHandler::HandleGetPrinterCapabilities(const ListValue* args) {
+  CHECK_EQ(1U, args->GetSize());
   std::string printer_name;
-  bool ret = args->GetString(0, &printer_name);
-  if (!ret || printer_name.empty())
-    return;
+  CHECK(args->GetString(0, &printer_name));
+  CHECK(!printer_name.empty());
 
   scoped_refptr<PrintSystemTaskProxy> task =
       new PrintSystemTaskProxy(AsWeakPtr(),
                                print_backend_.get(),
                                has_logged_printers_count_);
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&PrintSystemTaskProxy::GetPrinterCapabilities, task.get(),
                  printer_name));
@@ skipping 346 matching lines @@
     return;
 
   // We no longer require the initiator tab details. Remove those details
   // associated with the preview tab to allow the initiator tab to create
   // another preview tab.
   printing::PrintPreviewTabController* tab_controller =
       printing::PrintPreviewTabController::GetInstance();
   if (tab_controller)
     tab_controller->EraseInitiatorTabInfo(preview_tab_wrapper());
 }