Notification Provider API

Notification Provider API

The Notification provider API will reroute the notifications that are supposed to be sent to the Chrome Notification Center, so an app can get the notifications and have its own implementation of notification center.

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

[liyanhou, 2014-06-26 18:31:21]

PTAL. This is the skeleton of the new API, with browser tests for the events.
Thank you!

[Pete Williamson, 2014-06-30 19:03:49]

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/notification_getter/notification_getter_api.cc
(right):

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/notification_getter/notification_getter_api.cc:66:
->DispatchEventToExtension(extension_id, event.Pass());
This seems to be sending the OnCreated event back to the extension that created
the notification instead of the extension which is becoming the new notification
center.  Is that what we think should happen, or something we are doing for
testing?  A comment here might help me understand the intent of the code.  I
have the same concern for Update and Close below.

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/notification_getter/notification_getter_api.h
(right):

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/notification_getter/notification_getter_api.h:5:
#ifndef
CHROME_BROWSER_EXTENSIONS_API_NOTIFICATION_GETTER_NOTIFICATION_GETTER_API_H_
You already know this, but as a reminder, let's change the name of
NotificationGetter everywhere (including filenames) to
CustomNotificationCenter/custom_notification_center.

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/notification_getter/notification_getter_api.h:19:
class NotificationGetterEventRouter {
Let's add a class level comment here: A quick description of the purpose and
responsibilities of the class will help to make the code more readable.  Please
do that for all classes in every .h file.

I realize that the functions below in this file are largely
boilerplate/template, but I still think it is a good idea to add comments.

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/notification_getter/notification_getter_apitest.cc
(right):

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/notification_getter/notification_getter_apitest.cc:14:
std::string sender_id1_ = "SenderId1", notification_id1_ = "NotificationId1";
Let's use two separate lines for this, it makes code easier to read.

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/notification_getter/notification_getter_apitest.cc:15:
base::DictionaryValue* options = new base::DictionaryValue();
We have options (declared here) and options_ (declared on line 22).  Let's use
names that are a bit further apart, since these are two different things.  Also,
we normally use trailing underscores only for member variables, and options_ is
not a member variable here.  Maybe the first one could be options_args and the
second one options?

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/notification_getter/notification_getter_apitest.cc:42:
extension->id(), sender_id1_, notification_id1_);
You are calling three functions in this one test.  If any one of them fails,
will the test fail?  (Have you tested that?) The callbacks seem to be all
returning success, I wonder what would happen if one returned failure, would
this unit test report a failure?

https://codereview.chromium.org/356673003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/notification_getter.idl (right):

https://codereview.chromium.org/356673003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/notification_getter.idl:49: dictionary
NotificationOptions {
Can we find a more descriptive name than notification options?  (Also, please
remember to incorporate my other comments from the api doc).

https://codereview.chromium.org/356673003/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/notification_getter/events/test.js
(right):

https://codereview.chromium.org/356673003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/notification_getter/events/test.js:7:
notificationId,
Nit - indentation is off in this function, both this line, the next.

https://codereview.chromium.org/356673003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/notification_getter/events/test.js:19:
notificationId) {
Nit - indentation off here.

[liyanhou, 2014-07-14 17:44:44]

PTAL. Fixed comments from petewil.

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/notification_getter/notification_getter_api.cc
(right):

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/notification_getter/notification_getter_api.cc:66:
->DispatchEventToExtension(extension_id, event.Pass());
On 2014/06/30 19:03:48, Pete Williamson wrote:
> This seems to be sending the OnCreated event back to the extension that
created
> the notification instead of the extension which is becoming the new
notification
> center.  Is that what we think should happen, or something we are doing for
> testing?  A comment here might help me understand the intent of the code.  I
> have the same concern for Update and Close below.

It's sending the event to the new notification center. Yes I agree that the
variable names extension_id (custom notification center extension/app id) and
sender_id (sender of the notification) are confusing. I changed the naming to
make it clearer.

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/notification_getter/notification_getter_api.h
(right):

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/notification_getter/notification_getter_api.h:5:
#ifndef
CHROME_BROWSER_EXTENSIONS_API_NOTIFICATION_GETTER_NOTIFICATION_GETTER_API_H_
On 2014/06/30 19:03:48, Pete Williamson wrote:
> You already know this, but as a reminder, let's change the name of
> NotificationGetter everywhere (including filenames) to
> CustomNotificationCenter/custom_notification_center.

Done.

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/notification_getter/notification_getter_api.h:19:
class NotificationGetterEventRouter {
On 2014/06/30 19:03:48, Pete Williamson wrote:
> Let's add a class level comment here: A quick description of the purpose and
> responsibilities of the class will help to make the code more readable. 
Please
> do that for all classes in every .h file.
> 
> I realize that the functions below in this file are largely
> boilerplate/template, but I still think it is a good idea to add comments.

Done.

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/notification_getter/notification_getter_apitest.cc
(right):

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/notification_getter/notification_getter_apitest.cc:14:
std::string sender_id1_ = "SenderId1", notification_id1_ = "NotificationId1";
On 2014/06/30 19:03:49, Pete Williamson wrote:
> Let's use two separate lines for this, it makes code easier to read.

Done.

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/notification_getter/notification_getter_apitest.cc:15:
base::DictionaryValue* options = new base::DictionaryValue();
On 2014/06/30 19:03:49, Pete Williamson wrote:
> We have options (declared here) and options_ (declared on line 22).  Let's use
> names that are a bit further apart, since these are two different things. 
Also,
> we normally use trailing underscores only for member variables, and options_
is
> not a member variable here.  Maybe the first one could be options_args and the
> second one options?

Done.

https://codereview.chromium.org/356673003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/notification_getter/notification_getter_apitest.cc:42:
extension->id(), sender_id1_, notification_id1_);
On 2014/06/30 19:03:49, Pete Williamson wrote:
> You are calling three functions in this one test.  If any one of them fails,
> will the test fail?  (Have you tested that?) The callbacks seem to be all
> returning success, I wonder what would happen if one returned failure, would
> this unit test report a failure?

I just tried it. Yes, if any of them fails, the whole browser test will fail.
After the API is implemented, the test will do more than just returning a
success message.

https://codereview.chromium.org/356673003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/notification_getter.idl (right):

https://codereview.chromium.org/356673003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/notification_getter.idl:49: dictionary
NotificationOptions {
On 2014/06/30 19:03:49, Pete Williamson wrote:
> Can we find a more descriptive name than notification options?  (Also, please
> remember to incorporate my other comments from the api doc).

Done.

https://codereview.chromium.org/356673003/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/notification_getter/events/test.js
(right):

https://codereview.chromium.org/356673003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/notification_getter/events/test.js:7:
notificationId,
On 2014/06/30 19:03:49, Pete Williamson wrote:
> Nit - indentation is off in this function, both this line, the next.

Done.

https://codereview.chromium.org/356673003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/notification_getter/events/test.js:19:
notificationId) {
On 2014/06/30 19:03:49, Pete Williamson wrote:
> Nit - indentation off here.

Done.

[Pete Williamson, 2014-07-14 19:47:49]

LGTM with nits (You have my OK once the nits are fixed.)

https://codereview.chromium.org/356673003/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/custom_notification_center.idl (right):

https://codereview.chromium.org/356673003/diff/100001/chrome/common/extension...
chrome/common/extensions/api/custom_notification_center.idl:8: // icon, title,
message, expandedMessage, up to two buttons
Comments should be complete sentences, starting with a capital letter and ending
with a period.  (Please check the whole file and fix all comments.)

[liyanhou, 2014-07-15 16:53:32]

Fixed last comment. PTAL. Thank you!

https://codereview.chromium.org/356673003/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/custom_notification_center.idl (right):

https://codereview.chromium.org/356673003/diff/100001/chrome/common/extension...
chrome/common/extensions/api/custom_notification_center.idl:8: // icon, title,
message, expandedMessage, up to two buttons
On 2014/07/14 19:47:48, Pete Williamson wrote:
> Comments should be complete sentences, starting with a capital letter and
ending
> with a period.  (Please check the whole file and fix all comments.)

Done.

[liyanhou, 2014-07-28 20:56:09]

Could you take a look at this? It looks long but it's mostly boilerplate code.
Thank you so much!

[Alexei Svitkine (slow), 2014-07-28 20:56:48]

https://codereview.chromium.org/356673003/diff/240001/extensions/browser/exte...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/356673003/diff/240001/extensions/browser/exte...
extensions/browser/extension_function_histogram_value.h:917:
NOTIFICATIONPROVIDER_GETALLNOTIFIERS,
Please update histograms.xml with these.

[not at google - send to devlin, 2014-07-28 21:32:40]

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
File chrome/common/extensions/api/api.gyp (right):

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/api/api.gyp:84: 'notification_provider.idl',
should "notification_provider" come before "notifications" in sort order?

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
File chrome/common/extensions/api/notification_provider.idl (right):

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/api/notification_provider.idl:5: // Use the
<code>chrome.notificationProvider</code> API to get notifications.
This description is what is shown in the generated documentation. It could be
more helpful; "get notifications" could equally apply to the notifications API.

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/api/notification_provider.idl:8: // TODO(liyanhou):
Refactoer this to using notifications.PermissionLevel after
Refactor

also note that this will become the documentation for the
NotifierPermissionLevel so you might want to actually go:

// TODO(liyanhou): Use notifications.PermissionLevel everywhere and delete this
type. See http://crbug.com/etc.

// <some documentation about the permission level>

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/api/notification_provider.idl:40: callback
NotifyOnPermissionLevelChangedCallback = void (boolean notifierExists);
could you line-break after the =?

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/api/notification_provider.idl:104: static void
onCreated(DOMString notifierId,
it would be good to either provide a method to get the details of a specific
notifier, or pass in the full Notifier rather than its ID. or both.

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/api/notification_provider.idl:105: DOMString
notificaitonId,
notificationId

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
File chrome/common/extensions/permissions/chrome_api_permissions.cc (right):

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/permissions/chrome_api_permissions.cc:294:
{APIPermission::kNotificationProvider, "notificationProvider"},
this isn't just a platform app permission - at least, according to the features
file.

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
File chrome/common/extensions/permissions/permission_set_unittest.cc (right):

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/permissions/permission_set_unittest.cc:658:
skip.insert(APIPermission::kNotification);
FYI I'm going to rename kNotification to kNotifications.

https://codereview.chromium.org/356673003/diff/240001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/notification_provider/events/test.js
(right):

https://codereview.chromium.org/356673003/diff/240001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/notification_provider/events/test.js:4: var
EventsTest = function() {
eventsTest? or better, testEvents?

https://codereview.chromium.org/356673003/diff/260001/chrome/browser/extensio...
File
chrome/browser/extensions/api/notification_provider/notification_provider_api.h
(right):

https://codereview.chromium.org/356673003/diff/260001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_api.h:63:
: public ChromeAsyncExtensionFunction {
Implementing ChromeAsyncExtensionFunction is deprecated, extend
ChromeUIThreadExtensionFunction instead.

It amounts to overriding a Run() method rather than a RunAsync() method; there
are detailed comments in extension_function.h.

[liyanhou, 2014-07-29 18:26:54]

Could you take a look at this? Thank you so much!

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
File chrome/common/extensions/api/api.gyp (right):

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/api/api.gyp:84: 'notification_provider.idl',
On 2014/07/28 21:32:39, kalman wrote:
> should "notification_provider" come before "notifications" in sort order?

Done.

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
File chrome/common/extensions/api/notification_provider.idl (right):

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/api/notification_provider.idl:5: // Use the
<code>chrome.notificationProvider</code> API to get notifications.
On 2014/07/28 21:32:39, kalman wrote:
> This description is what is shown in the generated documentation. It could be
> more helpful; "get notifications" could equally apply to the notifications
API.

Done.

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/api/notification_provider.idl:8: // TODO(liyanhou):
Refactoer this to using notifications.PermissionLevel after
On 2014/07/28 21:32:39, kalman wrote:
> Refactor
> 
> also note that this will become the documentation for the
> NotifierPermissionLevel so you might want to actually go:
> 
> // TODO(liyanhou): Use notifications.PermissionLevel everywhere and delete
this
> type. See http://crbug.com/etc.
> 
> // <some documentation about the permission level>

Done.

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/api/notification_provider.idl:40: callback
NotifyOnPermissionLevelChangedCallback = void (boolean notifierExists);
On 2014/07/28 21:32:39, kalman wrote:
> could you line-break after the =?

Done.

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/api/notification_provider.idl:104: static void
onCreated(DOMString notifierId,
On 2014/07/28 21:32:39, kalman wrote:
> it would be good to either provide a method to get the details of a specific
> notifier, or pass in the full Notifier rather than its ID. or both.

Done.

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/api/notification_provider.idl:105: DOMString
notificaitonId,
On 2014/07/28 21:32:39, kalman wrote:
> notificationId

Done.

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
File chrome/common/extensions/permissions/chrome_api_permissions.cc (right):

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/permissions/chrome_api_permissions.cc:294:
{APIPermission::kNotificationProvider, "notificationProvider"},
On 2014/07/28 21:32:39, kalman wrote:
> this isn't just a platform app permission - at least, according to the
features
> file.

Done.

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
File chrome/common/extensions/permissions/permission_set_unittest.cc (right):

https://codereview.chromium.org/356673003/diff/240001/chrome/common/extension...
chrome/common/extensions/permissions/permission_set_unittest.cc:658:
skip.insert(APIPermission::kNotification);
On 2014/07/28 21:32:39, kalman wrote:
> FYI I'm going to rename kNotification to kNotifications.

Acknowledged.Thank you!

https://codereview.chromium.org/356673003/diff/240001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/notification_provider/events/test.js
(right):

https://codereview.chromium.org/356673003/diff/240001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/notification_provider/events/test.js:4: var
EventsTest = function() {
On 2014/07/28 21:32:39, kalman wrote:
> eventsTest? or better, testEvents?

Done.

https://codereview.chromium.org/356673003/diff/240001/extensions/browser/exte...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/356673003/diff/240001/extensions/browser/exte...
extensions/browser/extension_function_histogram_value.h:917:
NOTIFICATIONPROVIDER_GETALLNOTIFIERS,
On 2014/07/28 20:56:48, Alexei Svitkine wrote:
> Please update histograms.xml with these.

Done.

https://codereview.chromium.org/356673003/diff/260001/chrome/browser/extensio...
File
chrome/browser/extensions/api/notification_provider/notification_provider_api.h
(right):

https://codereview.chromium.org/356673003/diff/260001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_api.h:63:
: public ChromeAsyncExtensionFunction {
On 2014/07/28 21:32:40, kalman wrote:
> Implementing ChromeAsyncExtensionFunction is deprecated, extend
> ChromeUIThreadExtensionFunction instead.
> 
> It amounts to overriding a Run() method rather than a RunAsync() method; there
> are detailed comments in extension_function.h.

Done.

[not at google - send to devlin, 2014-07-29 20:09:39]

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
File
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc
(right):

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:111:
base::FundamentalValue* result = new base::FundamentalValue(true);
for every function: it's better to use the generated return types if possible.
this would look like:

return RespondNow(ArgumentList(
    api::notification_provider::NotifyOnCleared::Result::Create(true)
    .ToValue()));

modulo not-inlining it.

the repetition of "api::notification_provider::NotifyOnCleared" is pretty
painful, I often see people aliasing that to just "NotifyOnCleared", up to you.
I'm going to have another go at fixing that, but in the meantime... that's what
you have.

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:218:
result->Append(dictionary.release());
for example, here the result could be nicer:

return RespondNow(ArgumentList(
    api::notification_provider::GetAllNotifiers::Result::Create(
        // might need to make_scoped_ptr() here?
        new base::DictionaryValue(),
        new base::ListValue(),
    .ToValue()));

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
File
chrome/browser/extensions/api/notification_provider/notification_provider_api.h
(right):

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_api.h:71:
scoped_ptr<api::notification_provider::NotifyOnCleared::Params> params_;
for every function: It's unlikely that you'll actually need to hold onto params_
as a member variable. most of the time you only need to declare on stack.

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
File
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc
(right):

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc:18:
options_args->SetWithoutPathExpansion("inconUrl",
typo. but hopefully moot.

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc:25:
new extensions::api::notifications::NotificationOptions();
this is new'd but I don't see it free'd anywhere? easier to just declare on the
stack.

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc:26:
extensions::api::notifications::NotificationOptions::Populate(*options_args,
setting up a dictionary and then using Populate on it is rather roundabout. you
can just set the properties directly, like options.type = ..., options.icon_url
= ...;

https://codereview.chromium.org/356673003/diff/320001/chrome/common/extension...
File chrome/common/extensions/api/notification_provider.idl (right):

https://codereview.chromium.org/356673003/diff/320001/chrome/common/extension...
chrome/common/extensions/api/notification_provider.idl:115: static void
onCreated(DOMString notifierId,
[following on from previous comment]

what's the expected behaviour here? if the notificationProvider is just going to
turn around and request the Notifier given |notifierId| then you might as well
save the round trip, and annoyance of needing to do that, and pass it in right
away.

if not, then this is fine I suppose. the only downside would be the expense of
serializing notifications.NotificationBitmap every event - but events should be
pretty rare.

[liyanhou, 2014-07-30 17:19:25]

Could you take a look at this? Thank you so much!

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
File
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc
(right):

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:111:
base::FundamentalValue* result = new base::FundamentalValue(true);
On 2014/07/29 20:09:38, kalman wrote:
> for every function: it's better to use the generated return types if possible.
> this would look like:
> 
> return RespondNow(ArgumentList(
>     api::notification_provider::NotifyOnCleared::Result::Create(true)
>     .ToValue()));
> 
> modulo not-inlining it.
> 
> the repetition of "api::notification_provider::NotifyOnCleared" is pretty
> painful, I often see people aliasing that to just "NotifyOnCleared", up to
you.
> I'm going to have another go at fixing that, but in the meantime... that's
what
> you have.

Done.

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:218:
result->Append(dictionary.release());
On 2014/07/29 20:09:38, kalman wrote:
> for example, here the result could be nicer:
> 
> return RespondNow(ArgumentList(
>     api::notification_provider::GetAllNotifiers::Result::Create(
>         // might need to make_scoped_ptr() here?
>         new base::DictionaryValue(),
>         new base::ListValue(),
>     .ToValue()));

Done.

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
File
chrome/browser/extensions/api/notification_provider/notification_provider_api.h
(right):

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_api.h:71:
scoped_ptr<api::notification_provider::NotifyOnCleared::Params> params_;
On 2014/07/29 20:09:38, kalman wrote:
> for every function: It's unlikely that you'll actually need to hold onto
params_
> as a member variable. most of the time you only need to declare on stack.

Done.

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
File
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc
(right):

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc:18:
options_args->SetWithoutPathExpansion("inconUrl",
On 2014/07/29 20:09:38, kalman wrote:
> typo. but hopefully moot.

Done.

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc:25:
new extensions::api::notifications::NotificationOptions();
On 2014/07/29 20:09:38, kalman wrote:
> this is new'd but I don't see it free'd anywhere? easier to just declare on
the
> stack.

Done.

https://codereview.chromium.org/356673003/diff/320001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_apitest.cc:26:
extensions::api::notifications::NotificationOptions::Populate(*options_args,
On 2014/07/29 20:09:38, kalman wrote:
> setting up a dictionary and then using Populate on it is rather roundabout.
you
> can just set the properties directly, like options.type = ...,
options.icon_url
> = ...;

Done.

https://codereview.chromium.org/356673003/diff/320001/chrome/common/extension...
File chrome/common/extensions/api/notification_provider.idl (right):

https://codereview.chromium.org/356673003/diff/320001/chrome/common/extension...
chrome/common/extensions/api/notification_provider.idl:115: static void
onCreated(DOMString notifierId,
On 2014/07/29 20:09:38, kalman wrote:
> [following on from previous comment]
> 
> what's the expected behaviour here? if the notificationProvider is just going
to
> turn around and request the Notifier given |notifierId| then you might as well
> save the round trip, and annoyance of needing to do that, and pass it in right
> away.
> 
> if not, then this is fine I suppose. the only downside would be the expense of
> serializing notifications.NotificationBitmap every event - but events should
be
> pretty rare.

The expected behavior is to store notifier ID, notification ID, and data of the
notification together, then when a user clicks on or close a notification, the
app/extension can use the notifier ID and notification ID to notify the
notifier.

The intent is that Notifier information is more for displaying the notifiers in
the settings page, for users to block and unblock notifications from certain
notifiers. I don't know if apps would want information about the notifier every
time they get a notification. If so, I suppose they can keep a mapping of
notifier IDs to Notifier information. In that case, they may also keep converted
images in the mapping, to avoid converting bitmaps of the same notifiers.

That's just my thought on the scenarios, feel free if you think of use cases
that apps would want to get full Notifier information when it gets a
notification.

[not at google - send to devlin, 2014-07-30 19:24:37]

lgtm

https://codereview.chromium.org/356673003/diff/320001/chrome/common/extension...
File chrome/common/extensions/api/notification_provider.idl (right):

https://codereview.chromium.org/356673003/diff/320001/chrome/common/extension...
chrome/common/extensions/api/notification_provider.idl:115: static void
onCreated(DOMString notifierId,
On 2014/07/30 17:19:25, liyanhou wrote:
> On 2014/07/29 20:09:38, kalman wrote:
> > [following on from previous comment]
> > 
> > what's the expected behaviour here? if the notificationProvider is just
going
> to
> > turn around and request the Notifier given |notifierId| then you might as
well
> > save the round trip, and annoyance of needing to do that, and pass it in
right
> > away.
> > 
> > if not, then this is fine I suppose. the only downside would be the expense
of
> > serializing notifications.NotificationBitmap every event - but events should
> be
> > pretty rare.
> 
> The expected behavior is to store notifier ID, notification ID, and data of
the
> notification together, then when a user clicks on or close a notification, the
> app/extension can use the notifier ID and notification ID to notify the
> notifier.
> 
> The intent is that Notifier information is more for displaying the notifiers
in
> the settings page, for users to block and unblock notifications from certain
> notifiers. I don't know if apps would want information about the notifier
every
> time they get a notification. If so, I suppose they can keep a mapping of
> notifier IDs to Notifier information. In that case, they may also keep
converted
> images in the mapping, to avoid converting bitmaps of the same notifiers.
> 
> That's just my thought on the scenarios, feel free if you think of use cases
> that apps would want to get full Notifier information when it gets a
> notification.

Perhaps when onCreated() is called the provider will want to show the notifier's
name along with the notification?

but given this event is async anyway it's pretty easy for the app to turn around
and get the name if it wants, so this is fine.

https://codereview.chromium.org/356673003/diff/380001/chrome/browser/extensio...
File
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc
(right):

https://codereview.chromium.org/356673003/diff/380001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:72:
const extensions::api::notifications::NotificationOptions& options) {
extensions:: also unnecessary in this file

https://codereview.chromium.org/356673003/diff/380001/chrome/browser/extensio...
File
chrome/browser/extensions/api/notification_provider/notification_provider_api.h
(right):

https://codereview.chromium.org/356673003/diff/380001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_api.h:30:
const extensions::api::notifications::NotificationOptions& options);
extensions:: unnecessary (for all occurrences of extensions::).

[liyanhou, 2014-07-30 22:21:51]

Could you please take another look? Thank you so much!

https://codereview.chromium.org/356673003/diff/380001/chrome/browser/extensio...
File
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc
(right):

https://codereview.chromium.org/356673003/diff/380001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:72:
const extensions::api::notifications::NotificationOptions& options) {
On 2014/07/30 19:24:37, kalman wrote:
> extensions:: also unnecessary in this file

Done.

https://codereview.chromium.org/356673003/diff/380001/chrome/browser/extensio...
File
chrome/browser/extensions/api/notification_provider/notification_provider_api.h
(right):

https://codereview.chromium.org/356673003/diff/380001/chrome/browser/extensio...
chrome/browser/extensions/api/notification_provider/notification_provider_api.h:30:
const extensions::api::notifications::NotificationOptions& options);
On 2014/07/30 19:24:37, kalman wrote:
> extensions:: unnecessary (for all occurrences of extensions::).

Done.

[not at google - send to devlin, 2014-07-30 23:20:38]

lgtm

[Alexei Svitkine (slow), 2014-07-31 18:33:02]

lgtm

[liyanhou, 2014-07-31 18:34:23]

The CQ bit was checked by liyanhou@chromium.org

[commit-bot: I haz the power, 2014-07-31 18:36:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/liyanhou@chromium.org/356673003/420001

[commit-bot: I haz the power, 2014-07-31 22:10:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-01 11:51:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-01 11:51:58]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[liyanhou, 2014-08-01 17:53:10]

The CQ bit was checked by liyanhou@chromium.org

[commit-bot: I haz the power, 2014-08-01 17:56:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/liyanhou@chromium.org/356673003/420001

[commit-bot: I haz the power, 2014-08-02 05:45:41]

Change committed as 287155