Printing: Catch more error conditions and remove more cruft.

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

Index: chrome/browser/ui/webui/print_preview/print_preview_handler.cc
===================================================================
--- chrome/browser/ui/webui/print_preview/print_preview_handler.cc	(revision 126455)
+++ chrome/browser/ui/webui/print_preview/print_preview_handler.cc	(working copy)
@@ -120,29 +120,15 @@
 
 
 // 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();
 }
 
@@ -283,10 +269,8 @@
 }
 
 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;
@@ -313,10 +297,8 @@
   // 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());
@@ -329,19 +311,16 @@
   }
 
   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) {
@@ -355,6 +334,8 @@
 }
 
 void PrintPreviewHandler::HandlePrint(const ListValue* args) {
+  CHECK_EQ(2U, args->GetSize());
+
   ReportStats();
 
   // Record the number of times the user requests to regenerate preview data
@@ -369,8 +350,6 @@
   }
 
   scoped_ptr<DictionaryValue> settings(GetSettingsDictionary(args));
-  if (!settings.get())
-    return;
 
   // Storing last used settings.
   GetStickySettings()->Store(*settings);
@@ -395,8 +374,7 @@
 
   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);
@@ -491,6 +469,7 @@
 }
 
 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);
@@ -500,10 +479,10 @@
 }
 
 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(),