Add NotifyOnPermissionLevelChanged implementation of notification

Add NotifyOnPermissionLevelChanged implementation of notification
provider API

When an extension/app with notificationProvider permission shows a user
information about sources of the notifications, the notifiers. When the
user wants to allow or disallow one notifier, NotifyOnPermissionLevelChanged
can be used to inform the notifier that its permission level was changed.

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

[liyanhou, 2014-08-13 17:38:45]

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

[Pete Williamson, 2014-08-13 19:54:01]

https://codereview.chromium.org/468813002/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc
(right):

https://codereview.chromium.org/468813002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:199:
bool is_application_type =
Let's add a comment about why we only check application type and not web type,
or at least move the comment below up to here.

https://codereview.chromium.org/468813002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:221:
// can change permission levels of application type notifiers.
nit: notifiers. Because -> notifiers, because

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

https://codereview.chromium.org/468813002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/notification_provider.idl:115: NotifierType
notifierType,
Why do we need notifierType here?  Won't the notifier already know its type? 
(Perhaps I've misunderstood something here.)

https://codereview.chromium.org/468813002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js
(right):

https://codereview.chromium.org/468813002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:100:
function allEventesReceived(a, b, c, d, e) {
nit:  Eventes -> Events
Let's use more descriptive names for the events than a, b, c, etc.

[dewittj, 2014-08-13 21:30:36]

https://codereview.chromium.org/468813002/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc
(right):

https://codereview.chromium.org/468813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:210:
? true
Can you please rewrite this statement as multiple lines so it's not so far
indented?

Also you don't need the ternary operator, the result of == will be what you're
looking for.

https://codereview.chromium.org/468813002/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js
(right):

https://codereview.chromium.org/468813002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:175:
.catch(function() { return allEventsReceived(
This is technically racy; imagine an environment where the computer is bogged
down and it takes a while for the on_permission_level_changed_callback to
execute; this could fail even though the test is meant to pass.

I recommend making separate test function for each event, and actually waiting
for the event to occur before calling chrome.test.succeed.

[liyanhou, 2014-08-14 16:15:24]

petewil@, dewittj@, Fixed comments. Fixed and refactored browser test. Could you
take another look?

https://codereview.chromium.org/468813002/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc
(right):

https://codereview.chromium.org/468813002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:199:
bool is_application_type =
On 2014/08/13 19:54:01, Pete Williamson wrote:
> Let's add a comment about why we only check application type and not web type,
> or at least move the comment below up to here.

Done.

https://codereview.chromium.org/468813002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:221:
// can change permission levels of application type notifiers.
On 2014/08/13 19:54:01, Pete Williamson wrote:
> nit: notifiers. Because -> notifiers, because

Done.

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

https://codereview.chromium.org/468813002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/notification_provider.idl:115: NotifierType
notifierType,
On 2014/08/13 19:54:01, Pete Williamson wrote:
> Why do we need notifierType here?  Won't the notifier already know its type? 
> (Perhaps I've misunderstood something here.)

An object of Notifier dictionary type contains NotifierType, but notifier ID is
just a string and doesn't contain the type information. The reason I'm passing
both notifierId and notifierType instead of just passing one Notifier object is
that an Notifier object also contains other information not needed for
notifyOnPermissionLevelChanged, including an image of the notifier's icon.

https://codereview.chromium.org/468813002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js
(right):

https://codereview.chromium.org/468813002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:100:
function allEventesReceived(a, b, c, d, e) {
On 2014/08/13 19:54:01, Pete Williamson wrote:
> nit:  Eventes -> Events
> Let's use more descriptive names for the events than a, b, c, etc.

Done.

https://codereview.chromium.org/468813002/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc
(right):

https://codereview.chromium.org/468813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/notification_provider/notification_provider_api.cc:210:
? true
On 2014/08/13 21:30:36, dewittj wrote:
> Can you please rewrite this statement as multiple lines so it's not so far
> indented?
> 
> Also you don't need the ternary operator, the result of == will be what you're
> looking for.

Done.

https://codereview.chromium.org/468813002/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js
(right):

https://codereview.chromium.org/468813002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:175:
.catch(function() { return allEventsReceived(
On 2014/08/13 21:30:36, dewittj wrote:
> This is technically racy; imagine an environment where the computer is bogged
> down and it takes a while for the on_permission_level_changed_callback to
> execute; this could fail even though the test is meant to pass.
> 
> I recommend making separate test function for each event, and actually waiting
> for the event to occur before calling chrome.test.succeed.

Done.

[Pete Williamson, 2014-08-14 20:24:47]

lgtm

[dewittj, 2014-08-14 20:30:30]

can you create a bug for implementation of this API and add the bug number to
this cl?

https://codereview.chromium.org/468813002/diff/100001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js
(right):

https://codereview.chromium.org/468813002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:111:
chrome.notifications.onClicked.addListener(function(id) {
Please add listeners before you cause the events that you're listening for.
(i.e. above)

[dewittj, 2014-08-14 20:47:20]

lgtm once my other comments are resolved

[liyanhou, 2014-08-15 18:57:59]

https://codereview.chromium.org/468813002/diff/100001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js
(right):

https://codereview.chromium.org/468813002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/notification_provider/basic_usage/background.js:111:
chrome.notifications.onClicked.addListener(function(id) {
On 2014/08/14 20:30:30, dewittj wrote:
> Please add listeners before you cause the events that you're listening for.
> (i.e. above)

Done.

[liyanhou, 2014-08-15 22:00:51]

The CQ bit was checked by liyanhou@chromium.org

[commit-bot: I haz the power, 2014-08-15 22:02:53]

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

[commit-bot: I haz the power, 2014-08-16 06:10:24]

Committed patchset #10 (180001) as 290105