Add API function chrome.notifications.getAll

Add API function chrome.notifications.getAll

This function returns an object whose keys are the notification
IDs of all notifications created by that extension.

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

[dewittj, 2013-05-14 23:44:14]

miket: OWNERS for chrome/browser/extensions, chrome/common/extensions
dimich: notification ui managers in particular

[Dmitry Titov, 2013-05-16 20:39:15]

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

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/api/notifications/notifications_api.cc:387: iter++) {
nit: Most often, this loop is split in 2 lines, not 3...

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/api/notifications/notifications_api.cc:389:
StripScopeFromIdentifier(extension_->id(), *iter), true);
Just a note: this makes me think we might want to clean up the way we use
various 'id's in Notifications. It would be better to store/roundtrip the values
without combining/splitting them in strings, and explicitly consider the
required equality checks. For example, we have 2  implementations for 'update
match' - one that checks extension_id+id (here, in ClearFunction for example),
another also checks associated profile equality (in
MessageCenterNotificationManager::UpdateNotification). This means at least one
of these places has a bug :-)

But this can be done in a separate patch[es]. Here you use the current profile
which I think is correct.

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

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/api/notifications/notifications_apitest.cc:332:
ASSERT_FALSE(return_value->GetBoolean("test", &dictionary_bool));
What does this line check?

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
File chrome/browser/notifications/balloon_notification_ui_manager.cc (right):

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
chrome/browser/notifications/balloon_notification_ui_manager.cc:73: iter !=
balloons.end();
this loop can be in 2 lines.

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.cc:85:
(*curiter).second->profile() == profile) {
Should it be == or IsSameProfile() ?

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
File chrome/browser/notifications/notification_ui_manager.h (right):

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
chrome/browser/notifications/notification_ui_manager.h:45:
std::set<std::string>* notification_ids) = 0;
This can just return a set, this is ok (fast) with current compilers.

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
File chrome/browser/notifications/notification_ui_manager_impl.cc (right):

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
chrome/browser/notifications/notification_ui_manager_impl.cc:103: for
(NotificationDeque::iterator loopiter = show_queue_.begin();
just 'iter'?

[dewittj, 2013-05-16 23:40:33]

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

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/api/notifications/notifications_api.cc:387: iter++) {
On 2013/05/16 20:39:15, Dmitry Titov wrote:
> nit: Most often, this loop is split in 2 lines, not 3...

ALL HAIL CLANG-FORMAT-DIFF

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/api/notifications/notifications_api.cc:389:
StripScopeFromIdentifier(extension_->id(), *iter), true);
On 2013/05/16 20:39:15, Dmitry Titov wrote:
> Just a note: this makes me think we might want to clean up the way we use
> various 'id's in Notifications. It would be better to store/roundtrip the
values
> without combining/splitting them in strings, and explicitly consider the
> required equality checks. For example, we have 2  implementations for 'update
> match' - one that checks extension_id+id (here, in ClearFunction for example),
> another also checks associated profile equality (in
> MessageCenterNotificationManager::UpdateNotification). This means at least one
> of these places has a bug :-)
> 
> But this can be done in a separate patch[es]. Here you use the current profile
> which I think is correct.

I agree, but it is indeed better done in a separate patch.

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

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/extensions...
chrome/browser/extensions/api/notifications/notifications_apitest.cc:332:
ASSERT_FALSE(return_value->GetBoolean("test", &dictionary_bool));
On 2013/05/16 20:39:15, Dmitry Titov wrote:
> What does this line check?

Probably it is unnecessary but I was checking that it doesn't have any
unexpected values.  I'll remove.

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
File chrome/browser/notifications/balloon_notification_ui_manager.cc (right):

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
chrome/browser/notifications/balloon_notification_ui_manager.cc:73: iter !=
balloons.end();
On 2013/05/16 20:39:15, Dmitry Titov wrote:
> this loop can be in 2 lines.

ALL HAIL CLANG-FORMAT-DIFF

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.cc:85:
(*curiter).second->profile() == profile) {
On 2013/05/16 20:39:15, Dmitry Titov wrote:
> Should it be == or IsSameProfile() ?
> 

Good catch, probably the same extension would handle incognito/non-incognito
profiles.

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
File chrome/browser/notifications/notification_ui_manager.h (right):

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
chrome/browser/notifications/notification_ui_manager.h:45:
std::set<std::string>* notification_ids) = 0;
On 2013/05/16 20:39:15, Dmitry Titov wrote:
> This can just return a set, this is ok (fast) with current compilers.

Done.

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
File chrome/browser/notifications/notification_ui_manager_impl.cc (right):

https://codereview.chromium.org/14767029/diff/23001/chrome/browser/notificati...
chrome/browser/notifications/notification_ui_manager_impl.cc:103: for
(NotificationDeque::iterator loopiter = show_queue_.begin();
On 2013/05/16 20:39:15, Dmitry Titov wrote:
> just 'iter'?

okay.

[Dmitry Titov, 2013-05-17 00:14:41]

lgtm with a nit:

https://codereview.chromium.org/14767029/diff/44002/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/14767029/diff/44002/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.cc:84:
NotificationMap::iterator curiter = loopiter++;
This loop does not need curiter/loopiter treatment since it doesn't destroy
*curiter, which is usually what this is used for. Can be just a simple iter.

[miket_OOO, 2013-05-20 21:14:44]

lgtm

https://codereview.chromium.org/14767029/diff/56001/chrome/browser/notificati...
File chrome/browser/notifications/notification_ui_manager_impl.cc (right):

https://codereview.chromium.org/14767029/diff/56001/chrome/browser/notificati...
chrome/browser/notifications/notification_ui_manager_impl.cc:107:
notification_ids.insert((*iter)->notification().notification_id());
It's a code smell that I've now seen the same block of code in three places in
this CL.

[commit-bot: I haz the power, 2013-05-21 18:12:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/14767029/70001

[dewittj, 2013-05-21 18:14:31]

On 2013/05/20 21:14:44, miket wrote:
> lgtm
> 
>
https://codereview.chromium.org/14767029/diff/56001/chrome/browser/notificati...
> File chrome/browser/notifications/notification_ui_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/14767029/diff/56001/chrome/browser/notificati...
> chrome/browser/notifications/notification_ui_manager_impl.cc:107:
> notification_ids.insert((*iter)->notification().notification_id());
> It's a code smell that I've now seen the same block of code in three places in
> this CL.

In this case, there are 2 places we need to check for notifications: the
NotificationUIManagerImpl (which can queue notifications if the screen is
locked), and the message center or balloon collection which has the actual list
of active notifications.  Hopefully soon balloon collection will go away (once
we support message center on all platforms) but we'll still have to loop through
multiple collections to reliably find all the notifications in the system.

[commit-bot: I haz the power, 2013-05-21 20:33:19]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-05-21 22:36:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/14767029/70001

[commit-bot: I haz the power, 2013-05-22 00:20:57]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-05-22 23:07:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/14767029/70001

[commit-bot: I haz the power, 2013-05-22 23:07:09]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 528.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
6c009b46f62e08d3e11e7e05549688b35c7d8331..d41073289ed651648343fc90eb178c932b988124
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -528,6 +528,7 @@ enum HistogramValue {
   EXPERIMENTAL_ACCESSIBILITY_SETNATIVEACCESSIBILITYENABLED,
   RUNTIME_GETPLATFORMINFO,
   WEBVIEW_INSERTCSS,
+  NOTIFICATIONS_GET_ALL,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[commit-bot: I haz the power, 2013-05-23 01:27:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/14767029/101001

[commit-bot: I haz the power, 2013-05-23 03:43:43]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-05-23 20:07:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/14767029/123001

[commit-bot: I haz the power, 2013-05-23 20:07:57]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 529.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
8bc1a745926823241543df2a6cc5f63b80f6cd33..6da4b4f6b48c08212c55017832be4f029920574e
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -529,6 +529,7 @@ enum HistogramValue {
   RUNTIME_GETPLATFORMINFO,
   WEBVIEW_INSERTCSS,
   METRICSPRIVATE_GETISCRASHRECORDINGENABLED,
+  NOTIFICATIONS_GET_ALL,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[commit-bot: I haz the power, 2013-05-23 20:13:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/14767029/132001

[commit-bot: I haz the power, 2013-05-24 00:33:57]

Change committed as 201932