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 297 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.

[Peter Beverloo, 2015/05/13 10:52:42]

This TODO is now redundant.

[Deepak, 2015/05/13 13:21:11]

Done.

   if (options->icon_bitmap) {
     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_url.get()) {
+    gfx::Image app_icon_mask;
+    if (!NotificationConversionHelper::NotificationBitmapToGfxImage(

[Peter Beverloo, 2015/05/13 10:54:04]

On a slightly higher-level note, now that most calls to
NotificationBitmapToGfxImage() are guarded to make sure that the
|notification_bitmap| is non-NULL, it'd be cool if we can change the early
bail-out for NULL values to a DCHECK.

[Deepak, 2015/05/13 13:21:11]

Done.

+            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) {
@@ skipping 13 matching lines @@
       buttons.push_back(button);
     }
     notification->set_buttons(buttons);
   }
 
   if (options->context_message) {
     notification->set_context_message(
         base::UTF8ToUTF16(*options->context_message));
   }
 
   gfx::Image image;

[Peter Beverloo, 2015/05/13 10:52:42]

You can now move this to within the body of the new if-clause.

[Deepak, 2015/05/13 13:21:11]

Done.

-  bool has_image = NotificationConversionHelper::NotificationBitmapToGfxImage(
-      image_scale,
-      bitmap_sizes.image_size,
-      options->image_bitmap.get(),
-      &image);
-  if (has_image) {
+  if (options->image_url.get()) {
+    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