Returning error when NotificationConversionHelper::NotificationBitmapToGfxImage() get failed in not…

chrome/browser/extensions/api/notifications/notifications_api.cc

Index: chrome/browser/extensions/api/notifications/notifications_api.cc
diff --git a/chrome/browser/extensions/api/notifications/notifications_api.cc b/chrome/browser/extensions/api/notifications/notifications_api.cc
index 71fb9122a375d0082b8a09c53efa7e975eab4f38..f36ea6ef2175414504ced7705698b553a07a3467 100644
--- a/chrome/browser/extensions/api/notifications/notifications_api.cc
+++ b/chrome/browser/extensions/api/notifications/notifications_api.cc
@@ -183,10 +183,11 @@ bool NotificationsApiFunction::CreateNotification(
   const base::string16 message(base::UTF8ToUTF16(*options->message));
   gfx::Image icon;
 
-  if (!NotificationConversionHelper::NotificationBitmapToGfxImage(
+  if (options->icon_bitmap.get() &&

[dewittj, 2015/06/01 16:00:57]

this line should read 
if (!options->icon_bitmap.get() ||
   !NotificationConversionHelper::NotificationBitmapToGfxImage...

I think the confusion is that there are two very similar methods in this file;
one for creating a notification and one for updating it.  In this line we are in
the creation routine - so it should fail if there is no icon (or the icon is
invalid).

Below (at line 385) there is no strict requirement for icon_bitmap's existence
so we don't need to change the logic there.

[Deepak, 2015/06/02 07:36:35]

correct, I agree with you.

+      !NotificationConversionHelper::NotificationBitmapToGfxImage(
           image_scale,
           bitmap_sizes.icon_size,
-          options->icon_bitmap.get(),
+          *options->icon_bitmap,
           &icon)) {
     SetError(kUnableToDecodeIconError);
     return false;
@@ -194,15 +195,14 @@ bool NotificationsApiFunction::CreateNotification(
 
   // Then, handle any optional data that's been provided.
   message_center::RichNotificationData optional_fields;
-  if (options->app_icon_mask_url.get()) {
-    if (!NotificationConversionHelper::NotificationBitmapToGfxImage(
-            image_scale,
-            bitmap_sizes.app_icon_mask_size,
-            options->app_icon_mask_bitmap.get(),
-            &optional_fields.small_image)) {
+  if (options->app_icon_mask_bitmap.get() &&
+      !NotificationConversionHelper::NotificationBitmapToGfxImage(
+          image_scale,
+          bitmap_sizes.app_icon_mask_size,
+          *options->app_icon_mask_bitmap,
+          &optional_fields.small_image)) {
       SetError(kUnableToDecodeIconError);
       return false;
-    }
   }
 
   if (options->priority.get())
@@ -219,12 +219,14 @@ bool NotificationsApiFunction::CreateNotification(
     for (size_t i = 0; i < number_of_buttons; i++) {
       message_center::ButtonInfo info(
           base::UTF8ToUTF16((*options->buttons)[i]->title));
-      NotificationConversionHelper::NotificationBitmapToGfxImage(
-          image_scale,
-          bitmap_sizes.button_icon_size,
-          (*options->buttons)[i]->icon_bitmap.get(),
-          &info.icon);
-      optional_fields.buttons.push_back(info);
+      if ((*options->buttons)[i]->icon_bitmap.get()) {
+        NotificationConversionHelper::NotificationBitmapToGfxImage(
+            image_scale,
+            bitmap_sizes.button_icon_size,
+            *(*options->buttons)[i]->icon_bitmap,
+            &info.icon);
+        optional_fields.buttons.push_back(info);
+      }
     }
   }
 
@@ -233,11 +235,16 @@ bool NotificationsApiFunction::CreateNotification(
         base::UTF8ToUTF16(*options->context_message);
   }
 
-  bool has_image = NotificationConversionHelper::NotificationBitmapToGfxImage(
-      image_scale,
-      bitmap_sizes.image_size,
-      options->image_bitmap.get(),
-      &optional_fields.image);
+  bool has_image;
+  if (!options->image_bitmap.get()) {
+    has_image = false;
+  } else {
+    has_image = NotificationConversionHelper::NotificationBitmapToGfxImage(
+        image_scale,
+        bitmap_sizes.image_size,
+        *options->image_bitmap,
+        &optional_fields.image);
+  }
   // We should have an image if and only if the type is an image type.
   if (has_image != (type == message_center::NOTIFICATION_TYPE_IMAGE)) {
     SetError(kExtraImageProvided);
@@ -315,20 +322,29 @@ bool NotificationsApiFunction::UpdateNotification(
   if (options->message)
     notification->set_message(base::UTF8ToUTF16(*options->message));
 
-  // TODO(dewittj): Return error if this fails.
-  if (options->icon_bitmap) {
+  if (options->icon_bitmap.get()) {
     gfx::Image icon;
-    NotificationConversionHelper::NotificationBitmapToGfxImage(
-        image_scale, bitmap_sizes.icon_size, options->icon_bitmap.get(), &icon);
+    if (!NotificationConversionHelper::NotificationBitmapToGfxImage(
+          image_scale,
+          bitmap_sizes.icon_size,
+          *options->icon_bitmap,
+          &icon)) {
+      SetError(kUnableToDecodeIconError);
+      return false;
+    }
     notification->set_icon(icon);
   }
 
-  gfx::Image app_icon_mask;
-  if (NotificationConversionHelper::NotificationBitmapToGfxImage(
+  if (options->app_icon_mask_bitmap.get()) {
+    gfx::Image app_icon_mask;
+    if (!NotificationConversionHelper::NotificationBitmapToGfxImage(
           image_scale,
           bitmap_sizes.app_icon_mask_size,
-          options->app_icon_mask_bitmap.get(),
+          *options->app_icon_mask_bitmap,
           &app_icon_mask)) {
+      SetError(kUnableToDecodeIconError);
+      return false;
+    }
     notification->set_small_image(app_icon_mask);
   }
 
@@ -347,12 +363,14 @@ bool NotificationsApiFunction::UpdateNotification(
     for (size_t i = 0; i < number_of_buttons; i++) {
       message_center::ButtonInfo button(
           base::UTF8ToUTF16((*options->buttons)[i]->title));
-      NotificationConversionHelper::NotificationBitmapToGfxImage(
-          image_scale,
-          bitmap_sizes.button_icon_size,
-          (*options->buttons)[i]->icon_bitmap.get(),
-          &button.icon);
-      buttons.push_back(button);
+      if ((*options->buttons)[i]->icon_bitmap.get()) {
+        NotificationConversionHelper::NotificationBitmapToGfxImage(
+            image_scale,
+            bitmap_sizes.button_icon_size,
+            *(*options->buttons)[i]->icon_bitmap,

[dewittj, 2015/06/01 16:00:57]

please pull this out into its own variable, all these dereferences are getting
hard to read.

+            &button.icon);
+        buttons.push_back(button);
+      }
     }
     notification->set_buttons(buttons);
   }
@@ -363,11 +381,16 @@ bool NotificationsApiFunction::UpdateNotification(
   }
 
   gfx::Image image;
-  bool has_image = NotificationConversionHelper::NotificationBitmapToGfxImage(
-      image_scale,
-      bitmap_sizes.image_size,
-      options->image_bitmap.get(),
-      &image);
+  bool has_image;

[dewittj, 2015/06/01 16:00:57]

This code is a little convoluted.  Possibly shorten it into one statement:

bool has_image = options->image_bitmap.get() &&
NotificationConversionHelper::NotificatonBitmapToGfxImage(...)

[Deepak, 2015/06/02 07:36:35]

Done.

+  if (!options->image_bitmap.get()) {
+    has_image = false;
+  } else {
+    has_image = NotificationConversionHelper::NotificationBitmapToGfxImage(
+        image_scale,
+        bitmap_sizes.image_size,
+        *options->image_bitmap,
+        &image);
+  }
   if (has_image) {
     // We should have an image if and only if the type is an image type.
     if (notification->type() != message_center::NOTIFICATION_TYPE_IMAGE) {