Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1135213004:
Returning error when NotificationConversionHelper::NotificationBitmapToGfxImage() get failed in not… (Closed)
|
Created:
5 years, 7 months ago by Deepak Modified:
5 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, peter+watch_chromium.org, mlamouri+watch-notifications_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReturning error when NotificationConversionHelper::NotificationBitmapToGfxImage() get failed in notification_api.cc.
Proper Error code should get returned when their is some error in NotificationBitmapToGfxImage()
call for better error handling.
BUG=487183
Committed: https://crrev.com/24b9c277b2b946591035dd7b3dc0e6f550b2fe90
Cr-Commit-Position: refs/heads/master@{#332540}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 6
Patch Set 5 : Changes as per review comments. #Patch Set 6 : Changes as per review comments. #Patch Set 7 : Changes as per review comments. #
Total comments: 6
Patch Set 8 : Changes as per review comments. #Patch Set 9 : #Patch Set 10 : Changes as per review comments. #Patch Set 11 : Changes as per review comments. #
Total comments: 5
Patch Set 12 : Fixing merge issue. #Patch Set 13 : Changes as per review comments. #
Messages
Total messages: 26 (4 generated)
deepak.m1@samsung.com changed reviewers: + dewittj@chromium.org, rockot@chromium.org
@rockot As dewittj is OOO, Please review.
peter@chromium.org changed reviewers: + peter@chromium.org
A few nits. You probably want Justin's approval despite his OOO since this changes behavior of the extension API... https://codereview.chromium.org/1135213004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/1135213004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.cc:318: // TODO(dewittj): Return error if this fails. This TODO is now redundant. https://codereview.chromium.org/1135213004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.cc:371: gfx::Image image; You can now move this to within the body of the new if-clause.
https://codereview.chromium.org/1135213004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/1135213004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.cc:332: if (!NotificationConversionHelper::NotificationBitmapToGfxImage( 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.
@Peter Thanks for review. Changes done as suggested. PTAL https://codereview.chromium.org/1135213004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/1135213004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.cc:318: // TODO(dewittj): Return error if this fails. On 2015/05/13 10:52:42, Peter Beverloo wrote: > This TODO is now redundant. Done. https://codereview.chromium.org/1135213004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.cc:332: if (!NotificationConversionHelper::NotificationBitmapToGfxImage( On 2015/05/13 10:54:04, Peter Beverloo wrote: > 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. Done. https://codereview.chromium.org/1135213004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.cc:371: gfx::Image image; On 2015/05/13 10:52:42, Peter Beverloo wrote: > You can now move this to within the body of the new if-clause. Done.
this change seems to make a lot of extra changes. If the point is truly to add error messages, just add them in the spots it's necessary. Then we can discuss the other bits such as testing for URL in other changes. Is this change motivated by a particular bug or user experience? I see that the bug referenced is simply one you've created yourself (please add Cr-UI-Notifications label) * Why are you testing for the URLs all over the place? * Don't assume that the existence of a *_url field implies that the *_bitmap field is not null * It seems inconsistent that one place in the conversion function should be a DCHECK and lots of other places should just return false.
On 2015/05/21 17:32:21, dewittj wrote: @dewittj Thanks for review. > this change seems to make a lot of extra changes. If the point is truly to add > error messages, just add them in the spots it's necessary. Then we can discuss > the other bits such as testing for URL in other changes. => These changes are for adding error message when some error occurs while accessing NotificationConversionHelper::NotificationBitmapToGfxImage() as mentioned in the TODO statements. > Is this change motivated by a particular bug or user experience? I see that the > bug referenced is simply one you've created yourself (please add > Cr-UI-Notifications label) => I have added Cr-UI-Notifications label in the issue. > * Why are you testing for the URLs all over the place? =>Correct, I agree, I have changed the check to bitmap.get() as we are returning false at the start of NotificationBitmapToGfxImage() if notification_bitmap is NULL. > * Don't assume that the existence of a *_url field implies that the *_bitmap > field is not null => Agree with you, changes done. > * It seems inconsistent that one place in the conversion function should be a > DCHECK and lots of other places should just return false. => I have done this as it is safe to put DCHECK() here as we are checking bitmap at all places before making call to NotificationBitmapToGfxImage(), and @peter suggested me for making this change. PTAL.
On 2015/05/21 17:32:21, dewittj wrote: > * It seems inconsistent that one place in the conversion function should be a > DCHECK and lots of other places should just return false. Sorry, that was my suggestion. Checking the existence of arguments as provided by Chrome code (DCHECK) vs. their validity when provided by developers (return false).
On 2015/05/22 13:44:24, Peter Beverloo wrote: > On 2015/05/21 17:32:21, dewittj wrote: > > * It seems inconsistent that one place in the conversion function should be a > > DCHECK and lots of other places should just return false. > > Sorry, that was my suggestion. Checking the existence of arguments as provided > by Chrome code (DCHECK) vs. their validity when provided by developers (return > false). @dewittj should we continue DCHECK usage(As All calling points we are checking bitmaps) or fall back to returning false.?
dewittj@chromium.org changed reviewers: - rockot@chromium.org
https://codereview.chromium.org/1135213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/1135213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/notifications_api.cc:314: if (options->icon_bitmap.get()) { icon is required, we need to set an error if there isn't one. https://codereview.chromium.org/1135213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/notifications_api.cc:366: if (options->image_bitmap.get()) { 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. https://codereview.chromium.org/1135213004/diff/120001/chrome/browser/notific... File chrome/browser/notifications/notification_conversion_helper.cc (right): https://codereview.chromium.org/1135213004/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_conversion_helper.cc:145: extensions::api::notifications::NotificationBitmap* notification_bitmap, Let's change this to take a const extensions::api::notifications::NotificationBitmap& instead of a pointer.
PTAL https://codereview.chromium.org/1135213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/1135213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/notifications_api.cc:314: if (options->icon_bitmap.get()) { On 2015/05/22 16:42:23, dewittj wrote: > icon is required, we need to set an error if there isn't one. 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. https://codereview.chromium.org/1135213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/notifications_api.cc:366: if (options->image_bitmap.get()) { On 2015/05/22 16:42:23, dewittj wrote: > 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. Done. https://codereview.chromium.org/1135213004/diff/120001/chrome/browser/notific... File chrome/browser/notifications/notification_conversion_helper.cc (right): https://codereview.chromium.org/1135213004/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_conversion_helper.cc:145: extensions::api::notifications::NotificationBitmap* notification_bitmap, On 2015/05/22 16:42:24, dewittj wrote: > Let's change this to take a const > extensions::api::notifications::NotificationBitmap& instead of a pointer. Done.
@dewittj Thanks for review. Returning error when icon_bitmap is not present while calling UpdateNotification().And Updating test cases to reflect this. PTAL Thanks
@dewittj PTAL
sorry, I don't want the icon_url to be required for update (that would be a breaking change since partial updates are allowed).
On 2015/05/27 18:01:25, dewittj wrote: > sorry, I don't want the icon_url to be required for update (that would be a > breaking change since partial updates are allowed). As icon_url gives us icon_bitmap, and partial updates are allowed, then we should not return error while options->icon_bitmap.get() is null, in updating notification. extra changes removed. PTAL
On 2015/05/28 05:45:55, Deepak wrote: >On 2015/05/27 18:01:25, dewittj wrote: > sorry, I don't want the icon_url to be required for update (that would be a > breaking change since partial updates are allowed). As icon_url gives us icon_bitmap, and partial updates are allowed, then we should not return error while options->icon_bitmap.get() is null, in updating notification. extra changes removed. Please review.
https://codereview.chromium.org/1135213004/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/1135213004/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/notifications_api.cc:186: if (options->icon_bitmap.get() && 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. https://codereview.chromium.org/1135213004/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/notifications_api.cc:370: *(*options->buttons)[i]->icon_bitmap, please pull this out into its own variable, all these dereferences are getting hard to read. https://codereview.chromium.org/1135213004/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/notifications_api.cc:384: bool has_image; This code is a little convoluted. Possibly shorten it into one statement: bool has_image = options->image_bitmap.get() && NotificationConversionHelper::NotificatonBitmapToGfxImage(...)
@dewittj Thanks for review. *Changes done as suggested. *Try bot run successfully. Please review. https://codereview.chromium.org/1135213004/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/1135213004/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/notifications_api.cc:186: if (options->icon_bitmap.get() && On 2015/06/01 16:00:57, dewittj wrote: > 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. correct, I agree with you. https://codereview.chromium.org/1135213004/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/notifications_api.cc:384: bool has_image; On 2015/06/01 16:00:57, dewittj wrote: > This code is a little convoluted. Possibly shorten it into one statement: > > bool has_image = options->image_bitmap.get() && > NotificationConversionHelper::NotificatonBitmapToGfxImage(...) Done.
lgtm
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135213004/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/24b9c277b2b946591035dd7b3dc0e6f550b2fe90 Cr-Commit-Position: refs/heads/master@{#332540}
Message was sent while issue was closed.
On 2015/06/02 17:19:35, dewittj wrote: > lgtm Thanks @dewittj for review. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
