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

chrome/browser/extensions/api/notifications/notifications_api.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/extensions/api/notifications/notifications_api.h"
 
 #include "base/callback.h"
 #include "base/guid.h"
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 165 matching lines @@
   float image_scale =
       ui::GetScaleForScaleFactor(ui::GetSupportedScaleFactors().back());
 
   // Extract required fields: type, title, message, and icon.
   message_center::NotificationType type =
       MapApiTemplateTypeToType(options->type);
   const base::string16 title(base::UTF8ToUTF16(*options->title));
   const base::string16 message(base::UTF8ToUTF16(*options->message));
   gfx::Image icon;
 
-  if (!NotificationConversionHelper::NotificationBitmapToGfxImage(
-          image_scale,
-          bitmap_sizes.icon_size,
-          options->icon_bitmap.get(),
+  if (options->icon_bitmap.get() &&
+      !NotificationConversionHelper::NotificationBitmapToGfxImage(
+          image_scale, bitmap_sizes.icon_size, options->icon_bitmap.get(),
           &icon)) {
     SetError(kUnableToDecodeIconError);
     return false;
   }
 
   // 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.get(), &optional_fields.small_image)) {
       SetError(kUnableToDecodeIconError);
       return false;
-    }
   }
 
   if (options->priority.get())
     optional_fields.priority = *options->priority;
 
   if (options->event_time.get())
     optional_fields.timestamp = base::Time::FromJsTime(*options->event_time);
 
   if (options->buttons.get()) {
     // Currently we allow up to 2 buttons.
     size_t number_of_buttons = options->buttons->size();
     number_of_buttons = number_of_buttons > 2 ? 2 : number_of_buttons;
 
     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.get(), &info.icon);
+        optional_fields.buttons.push_back(info);
+      }
     }
   }
 
   if (options->context_message) {
     optional_fields.context_message =
         base::UTF8ToUTF16(*options->context_message);
   }
 
-  bool has_image = NotificationConversionHelper::NotificationBitmapToGfxImage(
-      image_scale,
-      bitmap_sizes.image_size,
-      options->image_bitmap.get(),
-      &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);
-    return false;
+  if (options->image_bitmap.get()) {
+    bool has_image = NotificationConversionHelper::NotificationBitmapToGfxImage(
+        image_scale, bitmap_sizes.image_size, options->image_bitmap.get(),
+        &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);
+      return false;
+    }
   }
 
   // We should have list items if and only if the type is a multiple type.
   bool has_list_items = options->items.get() && options->items->size() > 0;
   if (has_list_items != (type == message_center::NOTIFICATION_TYPE_MULTIPLE)) {
     SetError(kExtraListItemsProvided);
     return false;
   }
 
   if (options->progress.get() != NULL) {
@@ skipping 53 matching lines @@
       ui::GetScaleForScaleFactor(ui::GetSupportedScaleFactors().back());
 
   // Update optional fields if provided.
   if (options->type != api::notifications::TEMPLATE_TYPE_NONE)
     notification->set_type(MapApiTemplateTypeToType(options->type));
   if (options->title)
     notification->set_title(base::UTF8ToUTF16(*options->title));
   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()) {

[dewittj, 2015/05/22 16:42:23]

icon is required, we need to set an error if there isn't one.

[Deepak, 2015/05/23 14:03:40]

When I return error code on failing of this if (options->icon_bitmap.get())
condition then many test case like:

interactive_ui_tests (with patch) interactive_ui_tests (with patch) 319 disabled
failed 2
failures:
NotificationsApiTest.TestPartialUpdate
NotificationsApiTest.TestBasicUsage

get failed, as in all the test case either 'iconUrl' is getting passed, or
nothing is passed for icon url.
icon_bitmap is not getting passed in any of the test case.
So returning error on failing of if (options->icon_bitmap.get()) is not feasible
without affecting existing testcases.
Please let me know your opinion on this, and correct me, If I misunderstood
something.

     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.get(),
+            &icon)) {
+      SetError(kUnableToDecodeIconError);
+      return false;
+    }
     notification->set_icon(icon);
   }
 
-  gfx::Image app_icon_mask;
-  if (NotificationConversionHelper::NotificationBitmapToGfxImage(
-          image_scale,
-          bitmap_sizes.app_icon_mask_size,
-          options->app_icon_mask_bitmap.get(),
-          &app_icon_mask)) {
+  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(), &app_icon_mask)) {
+      SetError(kUnableToDecodeIconError);
+      return false;
+    }
     notification->set_small_image(app_icon_mask);
   }
 
   if (options->priority)
     notification->set_priority(*options->priority);
 
   if (options->event_time)
     notification->set_timestamp(base::Time::FromJsTime(*options->event_time));
 
   if (options->buttons) {
     // Currently we allow up to 2 buttons.
     size_t number_of_buttons = options->buttons->size();
     number_of_buttons = number_of_buttons > 2 ? 2 : number_of_buttons;
 
     std::vector<message_center::ButtonInfo> buttons;
     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.get(), &button.icon);
+        buttons.push_back(button);
+      }
     }
     notification->set_buttons(buttons);
   }
 
   if (options->context_message) {
     notification->set_context_message(
         base::UTF8ToUTF16(*options->context_message));
   }
 
-  gfx::Image image;
-  bool has_image = NotificationConversionHelper::NotificationBitmapToGfxImage(
-      image_scale,
-      bitmap_sizes.image_size,
-      options->image_bitmap.get(),
-      &image);
-  if (has_image) {
+  if (options->image_bitmap.get()) {

[dewittj, 2015/05/22 16:42:23]

If the type is 'image', then an image bitmap is required.  Your change allows
there to be no image bitmap in all cases.  Please fix.

[Deepak, 2015/05/23 14:03:40]

Done.

+    gfx::Image image;
+    bool has_image = NotificationConversionHelper::NotificationBitmapToGfxImage(
+        image_scale, bitmap_sizes.image_size, options->image_bitmap.get(),
+        &image);
+    if (!has_image) {
+      SetError(kUnableToDecodeIconError);
+      return false;
+    }
     // We should have an image if and only if the type is an image type.
     if (notification->type() != message_center::NOTIFICATION_TYPE_IMAGE) {
       SetError(kExtraImageProvided);
       return false;
     }
     notification->set_image(image);
   }
 
   if (options->progress) {
     // We should have progress if and only if the type is a progress type.
@@ skipping 218 matching lines @@
           ? api::notifications::PERMISSION_LEVEL_GRANTED
           : api::notifications::PERMISSION_LEVEL_DENIED;
 
   SetResult(new base::StringValue(api::notifications::ToString(result)));
   SendResponse(true);
 
   return true;
 }
 
 }  // namespace extensions