Allow partial update for notification update API

Allow partial update for notification update API

With this change, the user only needs to specify the properties to be changed. This patch does not change the way that the toast is updated by recreating the views. It will be addressed in next patch.

BUG=170924
TEST=new tests added
TBR=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213891

[dewittj, 2013-07-24 23:09:53]

https://codereview.chromium.org/20136004/diff/8002/chrome/browser/extensions/...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/20136004/diff/8002/chrome/browser/extensions/...
chrome/browser/extensions/api/notifications/notifications_api.cc:527: bool
could_create_notification = UpdateNotification(
nit: could_update_notification

https://codereview.chromium.org/20136004/diff/8002/chrome/browser/notificatio...
File chrome/browser/notifications/notification.h (right):

https://codereview.chromium.org/20136004/diff/8002/chrome/browser/notificatio...
chrome/browser/notifications/notification.h:27: Notification();
I don't love this, since it creates an invalid notification.  Can we return a
pointer to the current Notification in the list from DoesIdExist and make a copy
of it in the calling code?  Or return a scoped_ptr<Notification> from
DoesIdExist that contains new Notification(currentNotification)?

https://codereview.chromium.org/20136004/diff/8002/chrome/browser/notificatio...
File chrome/browser/notifications/notification_ui_manager.h (right):

https://codereview.chromium.org/20136004/diff/8002/chrome/browser/notificatio...
chrome/browser/notifications/notification_ui_manager.h:36: Notification*
matched_notification) = 0;
Should we just return Notification* instead of bool?

https://codereview.chromium.org/20136004/diff/8002/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/notifications_custom_bindings.js
(right):

https://codereview.chromium.org/20136004/diff/8002/chrome/renderer/resources/...
chrome/renderer/resources/extensions/notifications_custom_bindings.js:30:
callback(true);
What if all they want to update is the image url, or the button icon urls?  This
logic won't work here.

https://codereview.chromium.org/20136004/diff/8002/ui/message_center/notifica...
File ui/message_center/notification.cc (right):

https://codereview.chromium.org/20136004/diff/8002/ui/message_center/notifica...
ui/message_center/notification.cc:44: Notification::Notification() {
Doesn't this need defaults for all the member variables?

[jianli, 2013-07-25 01:02:41]

https://codereview.chromium.org/20136004/diff/8002/chrome/browser/extensions/...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/20136004/diff/8002/chrome/browser/extensions/...
chrome/browser/extensions/api/notifications/notifications_api.cc:527: bool
could_create_notification = UpdateNotification(
On 2013/07/24 23:09:53, dewittj wrote:
> nit: could_update_notification

Done.

https://codereview.chromium.org/20136004/diff/8002/chrome/browser/notificatio...
File chrome/browser/notifications/notification.h (right):

https://codereview.chromium.org/20136004/diff/8002/chrome/browser/notificatio...
chrome/browser/notifications/notification.h:27: Notification();
On 2013/07/24 23:09:53, dewittj wrote:
> I don't love this, since it creates an invalid notification.  Can we return a
> pointer to the current Notification in the list from DoesIdExist and make a
copy
> of it in the calling code?  Or return a scoped_ptr<Notification> from
> DoesIdExist that contains new Notification(currentNotification)?

Removed.

https://codereview.chromium.org/20136004/diff/8002/chrome/browser/notificatio...
File chrome/browser/notifications/notification_ui_manager.h (right):

https://codereview.chromium.org/20136004/diff/8002/chrome/browser/notificatio...
chrome/browser/notifications/notification_ui_manager.h:36: Notification*
matched_notification) = 0;
On 2013/07/24 23:09:53, dewittj wrote:
> Should we just return Notification* instead of bool?

Changed to return Notification pointer and renamed the method to FindById.

https://codereview.chromium.org/20136004/diff/8002/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/notifications_custom_bindings.js
(right):

https://codereview.chromium.org/20136004/diff/8002/chrome/renderer/resources/...
chrome/renderer/resources/extensions/notifications_custom_bindings.js:30:
callback(true);
On 2013/07/24 23:09:53, dewittj wrote:
> What if all they want to update is the image url, or the button icon urls? 
This
> logic won't work here.

Done.

https://codereview.chromium.org/20136004/diff/8002/ui/message_center/notifica...
File ui/message_center/notification.cc (right):

https://codereview.chromium.org/20136004/diff/8002/ui/message_center/notifica...
ui/message_center/notification.cc:44: Notification::Notification() {
On 2013/07/24 23:09:53, dewittj wrote:
> Doesn't this need defaults for all the member variables?

Removed.

[jianli, 2013-07-25 19:02:02]

Adding miket for idl change.

[dewittj, 2013-07-25 20:16:26]

lgtm if you add a few more tests that cover all the required properties for
create.

https://codereview.chromium.org/20136004/diff/86001/chrome/browser/extensions...
File chrome/browser/extensions/api/notifications/notifications_apitest.cc
(right):

https://codereview.chromium.org/20136004/diff/86001/chrome/browser/extensions...
chrome/browser/extensions/api/notifications/notifications_apitest.cc:275:
"\"iconUrl\": \"an/image/that/does/not/exist.png\""
Can you add some more expectations to this test that cover the entire set of
required properties?

[miket_OOO, 2013-07-25 22:59:35]

chrome/common/extensions/api/notifications.idl OWNER LGTM

[jianli, 2013-07-25 23:06:46]

Adding sky as TBR for ash and chromeos test file changes due to notification
interface change.

[commit-bot: I haz the power, 2013-07-26 03:59:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/20136004/137001

[commit-bot: I haz the power, 2013-07-26 13:40:51]

Change committed as 213891