Created initial multi-item notification view

Created initial multi-item notification view
    
This view (type="multiple") currently displays a predefined list of
notification itmes, with ugly fonts and spacing. It has no notion of a
collapsed/expanded state.

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

[dharcourt, 2012-11-15 18:14:34]

Not sure if this is enough to commit, but this is where I'm headed with the
multiple-item notification.

Thanks for the review!

    - Charles

[miket_OOO, 2012-11-15 18:35:32]

Pretty close, but we need a test. Right now the notification_apitest tests are
pretty pointless, but they do at least ensure that the code is callable. At the
moment there's zero test coverage for the new MessageViewMultiple class, so
there's nothing stopping someone else from breaking it.

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
File ui/message_center/message_view_multiple.cc (right):

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:20: // Temporarily here, this will
later get merged into the Notification class.
Use TODO format.

Can you do this work now, rather than leaving it for later? I don't see the
advantage of postponing this work.

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:54: UTF8ToUTF16("This is an imporant
message!")));
spelling

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:63: UTF8ToUTF16("I ate Cleron's
sandwhich!")));
spelling

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:72: // TODO(dharcourt): If possible,
us GridLayout fill functionality to make the
spelling

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
File ui/message_center/message_view_multiple.h (right):

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
ui/message_center/message_view_multiple.h:15: // TODO: Rename MessageCenter and
MessageView* to NotificationCenter
The standard TODO format is TODO(dharcourt)

This can be wrapped closer to 80 columns.

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
ui/message_center/message_view_multiple.h:32: struct NotificationItem; // To be
moved later to the Notification class.
Always two spaces before an inline comment delimiter.

Use TODO format rather than free-form notes for future work.

[benwells, 2012-11-15 22:27:08]

(i just took a quick look to understand the message center a bit better)

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
File ui/message_center/message_view_multiple.cc (right):

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:26: details(details) {
drive by suggestion: you can use {} on one line here and for the
MessageViewMultiple constructor / destructor below.

[dharcourt, 2012-11-16 02:37:00]

Implemented review suggestions (adding a browser test, moving NotificationItem
to notification_list.h, and various formatting/typo fixes).

Thanks for the review!

    - Charles

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
File ui/message_center/message_view_multiple.cc (right):

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:20: // Temporarily here, this will
later get merged into the Notification class.
On 2012/11/15 18:35:32, miket wrote:
> Use TODO format.
> 
> Can you do this work now, rather than leaving it for later? I don't see the
> advantage of postponing this work.

Done.

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:26: details(details) {
On 2012/11/15 22:27:08, benwells wrote:
> drive by suggestion: you can use {} on one line here and for the
> MessageViewMultiple constructor / destructor below.

Thanks for the review & the suggestion.

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:63: UTF8ToUTF16("I ate Cleron's
sandwhich!")));
On 2012/11/15 18:35:32, miket wrote:
> spelling

Funny. The mockup had a typo which I kepth even though it looked strange to
me... Clearly I didn't need to as I can come up with plenty of typos on my own
:-)

Fixed now.

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:72: // TODO(dharcourt): If possible,
us GridLayout fill functionality to make the
On 2012/11/15 18:35:32, miket wrote:
> spelling

Done.

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
File ui/message_center/message_view_multiple.h (right):

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
ui/message_center/message_view_multiple.h:15: // TODO: Rename MessageCenter and
MessageView* to NotificationCenter
On 2012/11/15 18:35:32, miket wrote:
> The standard TODO format is TODO(dharcourt)
> 
> This can be wrapped closer to 80 columns.

Done.

https://codereview.chromium.org/11410008/diff/2001/ui/message_center/message_...
ui/message_center/message_view_multiple.h:32: struct NotificationItem; // To be
moved later to the Notification class.
On 2012/11/15 18:35:32, miket wrote:
> Always two spaces before an inline comment delimiter.
> 
> Use TODO format rather than free-form notes for future work.

Done. But then this line disappeared as the TODO was done :-).

[miket_OOO, 2012-11-16 17:27:14]

LGTM

https://codereview.chromium.org/11410008/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/notification/notification_apitest.cc (right):

https://codereview.chromium.org/11410008/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/notification/notification_apitest.cc:108: //
TODO(dharcourt): [...], items = [{title: foo, message: bar}, ...], [...]
By the way, if you want to figure out a less ugly way to do this (e.g., creating
a DictionaryValue and converting it to JSON), go ahead.

https://codereview.chromium.org/11410008/diff/8001/ui/message_center/message_...
File ui/message_center/message_view_multiple.cc (right):

https://codereview.chromium.org/11410008/diff/8001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:40:
notification_.items->push_back(NotificationList::NotificationItem(
Before committing this, please remove these names and replace them with
obviously fictitious names (something like John Doe, etc.). As a policy we don't
include contributors names in the source, and by extension these names shouldn't
be included either.

[commit-bot: I haz the power, 2012-11-16 17:42:12]

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

[dharcourt, 2012-11-16 17:42:42]

Applied suggested changes and checkes commit!

    - C=

https://codereview.chromium.org/11410008/diff/8001/chrome/browser/extensions/...
File chrome/browser/extensions/api/notification/notification_apitest.cc (right):

https://codereview.chromium.org/11410008/diff/8001/chrome/browser/extensions/...
chrome/browser/extensions/api/notification/notification_apitest.cc:108: //
TODO(dharcourt): [...], items = [{title: foo, message: bar}, ...], [...]
On 2012/11/16 17:27:14, miket wrote:
> By the way, if you want to figure out a less ugly way to do this (e.g.,
creating
> a DictionaryValue and converting it to JSON), go ahead.

I'll try to do that in my next change list.

https://codereview.chromium.org/11410008/diff/8001/ui/message_center/message_...
File ui/message_center/message_view_multiple.cc (right):

https://codereview.chromium.org/11410008/diff/8001/ui/message_center/message_...
ui/message_center/message_view_multiple.cc:40:
notification_.items->push_back(NotificationList::NotificationItem(
On 2012/11/16 17:27:14, miket wrote:
> Before committing this, please remove these names and replace them with
> obviously fictitious names (something like John Doe, etc.). As a policy we
don't
> include contributors names in the source, and by extension these names
shouldn't
> be included either.

Done.

[commit-bot: I haz the power, 2012-11-16 19:40:04]

Change committed as 168263

[miket_OOO, 2012-11-20 23:54:52]

https://chromiumcodereview.appspot.com/11410008/diff/10002/ui/message_center/...
File ui/message_center/notification_list.h (right):

https://chromiumcodereview.appspot.com/11410008/diff/10002/ui/message_center/...
ui/message_center/notification_list.h:57: std::vector<NotificationItem>* items;
Sorry, I must have missed this earlier. I'm not seeing where this pointer gets
freed. Moreover, because the container is a struct, we would look askance at a
destructor that was freeing one of its members.

Change this to a vector<...> rather than a pointer to one, fix the application
code, and you should be in better shape.