Adding user data to the notification options.

Adding user data to the notification options.

[dharcourt, 2013-02-22 21:35:30]

https://codereview.chromium.org/12334047/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/experimental_notification.idl (right):

https://codereview.chromium.org/12334047/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental_notification.idl:73: DOMString?
userData;
If this object will have the same restriction as the one passed in to
StorageArea.set() (chrome.storage.local and friends), we might want to document
that.  StorageArea.set() documents it as "Object specifying items to augment
storage with. Values that cannot be serialized (functions, etc) will be
ignored.". Sounds reasonable?

https://codereview.chromium.org/12334047/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental_notification.idl:85: 
How about returning a notificationsInfo object which would be a map of
notification IDs to NotificationOptions? Alternatively, we could return an array
of NotificationInfo objects with each having an ID and options. Both seem easier
to work with than two synchronized arrays, and the array-of-info-objects
alternative would be more amenable to returning additional notification
information in the future without breaking APIs.

https://codereview.chromium.org/12334047/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental_notification.idl:110: static void
getAllNotifications(GetAllNotificationsCallback callback);
How about getNotification(selector, callback), where selector is either an array
of notification IDs, a single notification ID string, or ""/undefined/null to
get all the notification IDs?

[vadimt, 2013-02-22 22:38:44]

https://codereview.chromium.org/12334047/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/experimental_notification.idl (right):

https://codereview.chromium.org/12334047/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental_notification.idl:73: DOMString?
userData;
On 2013/02/22 21:35:30, dharcourt wrote:
> If this object will have the same restriction as the one passed in to
> StorageArea.set() (chrome.storage.local and friends), we might want to
document
> that.  StorageArea.set() documents it as "Object specifying items to augment
> storage with. Values that cannot be serialized (functions, etc) will be
> ignored.". Sounds reasonable?

Yes. Object is better than string.

https://codereview.chromium.org/12334047/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental_notification.idl:85: 
On 2013/02/22 21:35:30, dharcourt wrote:
> How about returning a notificationsInfo object which would be a map of
> notification IDs to NotificationOptions? Alternatively, we could return an
array
> of NotificationInfo objects with each having an ID and options. Both seem
easier
> to work with than two synchronized arrays, and the array-of-info-objects
> alternative would be more amenable to returning additional notification
> information in the future without breaking APIs.

Based on the offline discussion, it seems like we'll have a map from ID to user
data.

https://codereview.chromium.org/12334047/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental_notification.idl:110: static void
getAllNotifications(GetAllNotificationsCallback callback);
On 2013/02/22 21:35:30, dharcourt wrote:
> How about getNotification(selector, callback), where selector is either an
array
> of notification IDs, a single notification ID string, or ""/undefined/null to
> get all the notification IDs?

My usecase requires only getting all notifications. But feel free to extend
this.
Actually, strictly and narrowly, I need only the list (or set) of IDs. Not even
user data IN THIS CALL. However, I guess, for symmetry, we should return same
info per notification as get reported to onClosed, onClicked and
onButtonClicked.

[vadimt, 2013-02-26 03:13:01]

Incorporated feedback.

Didn't rename existing functions (like delete => clear) because no clarity with
being able to make a breaking change.

Not sure I expressed correctly in IDL that the param to
GetAllNotificationsCallback is a map.

Also, I didn't dare to add selector parameter to getAllNotifications since I
have no usecase for this.

[vadimt, 2013-02-28 20:53:07]

Adding clearAll/get/getAll

[vadimt1, 2013-02-28 20:53:13]

Mike, thanks for landing the 'clear()'
CL<https://codereview.chromium.org/12326127/>
!

I've updated the proposed IDL change<https://codereview.chromium.org/12334047>,
and my next question is whether the proposal is ready to be implemented,
and what timeline to expect.

Thanks!
Vadim

On Mon, Feb 25, 2013 at 7:13 PM, <vadimt@chromium.org> wrote:

> Incorporated feedback.
>
> Didn't rename existing functions (like delete => clear) because no clarity
> with
> being able to make a breaking change.
>
> Not sure I expressed correctly in IDL that the param to
> GetAllNotificationsCallback is a map.
>
> Also, I didn't dare to add selector parameter to getAllNotifications since
> I
> have no usecase for this.
>
>
https://codereview.chromium.**org/12334047/<https://codereview.chromium.org/1...
>



-- 
Thank you!
Vadim