Added plumbing to get and transmit multiple-item notification data

Added plumbing to get and transmit multiple-item notification data

BUG=161101
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168966

[dharcourt, 2012-11-20 00:45:04]

Thanks for the review,

    - Charles

[miket_OOO, 2012-11-20 23:22:05]

lgtm

https://codereview.chromium.org/11413077/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/notification/notification_api.cc (right):

https://codereview.chromium.org/11413077/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/notification/notification_api.cc:81: typedef
api::experimental_notification::NotificationItem NotificationItem;
What you're trying to do is actually this:

  using api::experimental_notification::NotificationItem;

but if you did that, I'd reply with
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp....

TL;DR: remove this line and spell it out below.

https://codereview.chromium.org/11413077/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/notification/notification_api.cc:154:
UTF8ToUTF16((*i).get()->title));
Can (*i). be i-> instead?

https://codereview.chromium.org/11413077/diff/2001/ui/message_center/notifica...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/11413077/diff/2001/ui/message_center/notifica...
ui/message_center/notification_list.cc:65: notification.priority = 0;
It looks like I forgot to initialize unread_count to zero. Would you mind doing
that here for me? Thanks.

https://codereview.chromium.org/11413077/diff/2001/ui/message_center/notifica...
ui/message_center/notification_list.cc:116:
optional_fields->GetList(ui::notifications::kItemsKey, &items);
Consider CHECKing the result of GetList().

This code feels slightly riskier than the surrounding code that does GetString()
operations without CHECKs because you haven't initialized items, and the rest of
the code assumes items is valid. This is probably OK because you know that you
built the list yourself, but in the future if someone creates a Notification
(the other chrome/ kind of Notification) and foolishly puts the wrong type in
the items key, this code will crash, and at least your CHECK will crash more
informatively.

https://codereview.chromium.org/11413077/diff/2001/ui/message_center/notifica...
ui/message_center/notification_list.cc:123:
item->GetString(ui::notifications::kItemMessageKey, &message);
See, no CHECK needed here because title/message have default initializers.

https://codereview.chromium.org/11413077/diff/2001/ui/notifications/notificat...
File ui/notifications/notification_types.cc (right):

https://codereview.chromium.org/11413077/diff/2001/ui/notifications/notificat...
ui/notifications/notification_types.cc:23: const char kItemTitleKey[] =
"item_title";
If these live inside the items dictionary, do we need the redundant item_
prefix?

[miket_OOO, 2012-11-20 23:56:33]

https://chromiumcodereview.appspot.com/11413077/diff/2001/ui/message_center/n...
File ui/message_center/notification_list.cc (right):

https://chromiumcodereview.appspot.com/11413077/diff/2001/ui/message_center/n...
ui/message_center/notification_list.cc:114: notification.items = new
std::vector<NotificationList::NotificationItem>();
See my posthumous comment to the review that introduced the items member of
NotificationList. This block leaks.

[dharcourt, 2012-11-21 00:43:08]

Suggestions implemented. Committing. Thanks for the feedback.

    - Charles

https://codereview.chromium.org/11413077/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/notification/notification_api.cc (right):

https://codereview.chromium.org/11413077/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/notification/notification_api.cc:81: typedef
api::experimental_notification::NotificationItem NotificationItem;
On 2012/11/20 23:22:05, miket wrote:
> What you're trying to do is actually this:
> 
>   using api::experimental_notification::NotificationItem;
> 
> but if you did that, I'd reply with
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp....
> 
> TL;DR: remove this line and spell it out below.

Done.

https://codereview.chromium.org/11413077/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/notification/notification_api.cc:154:
UTF8ToUTF16((*i).get()->title));
On 2012/11/20 23:22:05, miket wrote:
> Can (*i). be i-> instead?

Yes. Fixed.

https://codereview.chromium.org/11413077/diff/2001/ui/message_center/notifica...
File ui/message_center/notification_list.cc (right):

https://codereview.chromium.org/11413077/diff/2001/ui/message_center/notifica...
ui/message_center/notification_list.cc:65: notification.priority = 0;
On 2012/11/20 23:22:05, miket wrote:
> It looks like I forgot to initialize unread_count to zero. Would you mind
doing
> that here for me? Thanks.

Done.

https://codereview.chromium.org/11413077/diff/2001/ui/message_center/notifica...
ui/message_center/notification_list.cc:114: notification.items = new
std::vector<NotificationList::NotificationItem>();
On 2012/11/20 23:56:33, miket wrote:
> See my posthumous comment to the review that introduced the items member of
> NotificationList. This block leaks.

Fixed as suggested.

https://codereview.chromium.org/11413077/diff/2001/ui/message_center/notifica...
ui/message_center/notification_list.cc:116:
optional_fields->GetList(ui::notifications::kItemsKey, &items);
On 2012/11/20 23:22:05, miket wrote:
> Consider CHECKing the result of GetList().
> 
> This code feels slightly riskier than the surrounding code that does
GetString()
> operations without CHECKs because you haven't initialized items, and the rest
of
> the code assumes items is valid. This is probably OK because you know that you
> built the list yourself, but in the future if someone creates a Notification
> (the other chrome/ kind of Notification) and foolishly puts the wrong type in
> the items key, this code will crash, and at least your CHECK will crash more
> informatively.

Done.

https://codereview.chromium.org/11413077/diff/2001/ui/notifications/notificat...
File ui/notifications/notification_types.cc (right):

https://codereview.chromium.org/11413077/diff/2001/ui/notifications/notificat...
ui/notifications/notification_types.cc:23: const char kItemTitleKey[] =
"item_title";
On 2012/11/20 23:22:05, miket wrote:
> If these live inside the items dictionary, do we need the redundant item_
> prefix?

No. Fixed.

[commit-bot: I haz the power, 2012-11-21 00:43:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/11413077/4002

[commit-bot: I haz the power, 2012-11-21 00:43:41]

Failed to apply patch for ui/message_center/notification_list.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ui/message_center/notification_list.cc
  Hunk #1 FAILED at 4.
  Hunk #2 succeeded at 65 (offset 2 lines).
  Hunk #3 succeeded at 113 (offset 2 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
ui/message_center/notification_list.cc.rej

Patch:       ui/message_center/notification_list.cc
Index: ui/message_center/notification_list.cc
diff --git a/ui/message_center/notification_list.cc
b/ui/message_center/notification_list.cc
index
3ed47b63329451751bba7379180914034c5e9024..88363b4bb63cd81959da53acb9edd28165fc04ad
100644
--- a/ui/message_center/notification_list.cc
+++ b/ui/message_center/notification_list.cc
@@ -4,6 +4,7 @@
 
 #include "ui/message_center/notification_list.h"
 
+#include "base/logging.h"
 #include "base/time.h"
 #include "base/values.h"
 
@@ -63,6 +64,7 @@ void NotificationList::AddNotification(
   // Initialize primitive fields before unpacking optional fields.
   // timestamp initializes to default NULL time.
   notification.priority = 0;
+  notification.unread_count = 0;
 
   UnpackOptionalFields(optional_fields, notification);
 
@@ -110,6 +112,19 @@ void NotificationList::UnpackOptionalFields(
   if (optional_fields->HasKey(ui::notifications::kImageUrlKey))
     optional_fields->GetString(ui::notifications::kImageUrlKey,
                                &notification.image_url);
+  if (optional_fields->HasKey(ui::notifications::kItemsKey)) {
+    const ListValue* items;
+    CHECK(optional_fields->GetList(ui::notifications::kItemsKey, &items));
+    for (size_t i = 0; i < items->GetSize(); ++i) {
+      string16 title;
+      string16 message;
+      const base::DictionaryValue* item;
+      items->GetDictionary(i, &item);
+      item->GetString(ui::notifications::kItemTitleKey, &title);
+      item->GetString(ui::notifications::kItemMessageKey, &message);
+      notification.items.push_back(NotificationItem(title, message));
+    }
+  }
 }
 
 void NotificationList::UpdateNotificationMessage(const std::string& old_id,

[commit-bot: I haz the power, 2012-11-21 00:54:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/11413077/2010

[commit-bot: I haz the power, 2012-11-21 04:07:42]

Change committed as 168966