Route newly created notifications to notification provider API.

Route newly created notifications to notification provider API.

Whenever a new notification is created, route it to an
extension/app that has notification provider permission.
Currently, if there's an app with notificationProvider
permission, notifications will go to both message center
and that app. In the future, when additional sections in
Chrome Settings is implemented, this will send notifications
to only one party based on the user's selection.

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

[liyanhou, 2014-08-05 00:40:14]

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

[dewittj, 2014-08-05 17:38:09]

needs tests

[Pete Williamson, 2014-08-05 22:03:53]

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:59: bool
GfxImageToNotificationBitmapArgs(
Why return bool when this has no failure case?  Maybe we missed a return false,
or maybe this function should be void.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:72: size_t
data_size = data_area * BYTES_PER_PIXEL;
Do we know for sure that all input bitmaps will be 4 bytes per pixel?

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:77: for
(size_t t = 0; t < data_area; ++t) {
nit - "i" is normally used as an iterator variable in our code base, not "t".

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:81:
data[rgba_index + 0] = (((bitmap_pixels[t]) >> 16) & 0xFF);
Have you checked elsewhere in the code to see if there is a utility function to
do this?  It seems likely.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:115: //
Extract required fileds: type, title, message, and icon.
nit: fileds: fields

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:120:
"title", new base::StringValue(notification.title()));
can notification.title() (or any of the other methods we call on it) return
null?  If so, we can have a crash.  The notification may be coming from an
untrusted source, we don't want it to be able to crash chrome, which might be a
security hole.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:145: if
(!base::UTF16ToUTF8(rich_data->context_message).empty())
Can we check emptyness without doing a conversion?  It would be simpler and
faster.  (or maybe this function has a side effect of converting in place?  It
seems to be throwing away any return value)

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:168: if
(type == "image" && !rich_data->image.IsEmpty()) {
maybe log if we find one that violates this assumption (same for progress)

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:184: for
(size_t i = 0; i < rich_data->items.size(); i++) {
use "j" instead of "i" here to keep it distinct from the one using "i" above. 
It can also help in the debugger in some circumstances.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:197: //
change choosing a random app with this permission to a user selected app.
nit:  We're not actually getting a random app here, we're getting the last one
to be returned from the search (which might actually be stable instead of
random, so it would return the same one every time).

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:199:
extensions::ExtensionRegistry* registry =
Can this return null (maybe early in setup before the extension registry is set
up).

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:661: 
We should not check this in turned on.  It is OK to check it in behind a flag,
or in trunk or canary only (not stable, beta, or dev).  We don't want to turn
the feature on for everybody until the settings story is figured out and turned
on.

[dewittj, 2014-08-07 22:49:09]

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:660: //
notifications will go to both Chrome Notification Center and that app.
I don't believe this code should live in AddProfileNotification.  It should have
its own function, something like RouteNotificationToAlternateProvider() or
something.  I don't think that this patch forwards updates correctly, right?

[liyanhou, 2014-08-08 22:40:26]

Addressed comments. Refactored code, and added browser and unit tests.
petewil@ dewittj@ Could you take another look? Thank you so much!

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:59: bool
GfxImageToNotificationBitmapArgs(
On 2014/08/05 22:03:52, Pete Williamson wrote:
> Why return bool when this has no failure case?  Maybe we missed a return
false,
> or maybe this function should be void.

Done.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:72: size_t
data_size = data_area * BYTES_PER_PIXEL;
On 2014/08/05 22:03:52, Pete Williamson wrote:
> Do we know for sure that all input bitmaps will be 4 bytes per pixel?

Yes. All input images are of gfx::Image type.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:77: for
(size_t t = 0; t < data_area; ++t) {
On 2014/08/05 22:03:52, Pete Williamson wrote:
> nit - "i" is normally used as an iterator variable in our code base, not "t".

Done.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:81:
data[rgba_index + 0] = (((bitmap_pixels[t]) >> 16) & 0xFF);
On 2014/08/05 22:03:52, Pete Williamson wrote:
> Have you checked elsewhere in the code to see if there is a utility function
to
> do this?  It seems likely.

Done.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:115: //
Extract required fileds: type, title, message, and icon.
On 2014/08/05 22:03:52, Pete Williamson wrote:
> nit: fileds: fields

Done.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:120:
"title", new base::StringValue(notification.title()));
On 2014/08/05 22:03:52, Pete Williamson wrote:
> can notification.title() (or any of the other methods we call on it) return
> null?  If so, we can have a crash.  The notification may be coming from an
> untrusted source, we don't want it to be able to crash chrome, which might be
a
> security hole.

id, title, message, and icon url are the required fields for creating
notifications through notifications API. Since Notifications is converted from
that, all notifications should have those four fields.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:145: if
(!base::UTF16ToUTF8(rich_data->context_message).empty())
On 2014/08/05 22:03:52, Pete Williamson wrote:
> Can we check emptyness without doing a conversion?  It would be simpler and
> faster.  (or maybe this function has a side effect of converting in place?  It
> seems to be throwing away any return value)

Done.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:168: if
(type == "image" && !rich_data->image.IsEmpty()) {
On 2014/08/05 22:03:52, Pete Williamson wrote:
> maybe log if we find one that violates this assumption (same for progress)

Done.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:184: for
(size_t i = 0; i < rich_data->items.size(); i++) {
On 2014/08/05 22:03:52, Pete Williamson wrote:
> use "j" instead of "i" here to keep it distinct from the one using "i" above. 
> It can also help in the debugger in some circumstances.

Done.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:197: //
change choosing a random app with this permission to a user selected app.
On 2014/08/05 22:03:53, Pete Williamson wrote:
> nit:  We're not actually getting a random app here, we're getting the last one
> to be returned from the search (which might actually be stable instead of
> random, so it would return the same one every time).

Done.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:199:
extensions::ExtensionRegistry* registry =
On 2014/08/05 22:03:52, Pete Williamson wrote:
> Can this return null (maybe early in setup before the extension registry is
set
> up).

I don't really know. I read cases where they check, and cases where they didn't
check. I'll just add a check in case.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:660: //
notifications will go to both Chrome Notification Center and that app.
On 2014/08/07 22:49:08, dewittj wrote:
> I don't believe this code should live in AddProfileNotification.  It should
have
> its own function, something like RouteNotificationToAlternateProvider() or
> something.  I don't think that this patch forwards updates correctly, right?

Done. No, this patch doesn't forward updates.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:661: 
On 2014/08/05 22:03:52, Pete Williamson wrote:
> We should not check this in turned on.  It is OK to check it in behind a flag,
> or in trunk or canary only (not stable, beta, or dev).  We don't want to turn
> the feature on for everybody until the settings story is figured out and
turned
> on.

Yes it is in trunk.

[dewittj, 2014-08-09 00:29:53]

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:140: }
Do you want to modify the Update function too?

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
File chrome/browser/notifications/notification_conversion_helper.cc (right):

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:119: void
NotificationConversionHelper::GfxImageToNotificationBitmap(
can we also move the conversion from NotificationBitmap to gfx::Image into this
file?

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:123:
sk_bitmap.lockPixels();
hmm, do you know why this line is here?  Why isn't there a corresponding
unlockPixels?

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:153: return "";
NOTREACHED() here before the return since it's an error to ever hit this line.

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
File chrome/browser/notifications/notification_conversion_helper.h (right):

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.h:8: #include
"base/values.h"
We don't need all these include files.  You can remove line 8, 10, 11, and 12,
provided that you forward declare the NotificationOptions and NotificationBitmap
type.  Something like:

namespace extensions {
namespace api {
namespace notifications {
class NotificationOptions;
class NotificationBitmap;
}  // namespace notifications
}  // namespace api
}  // namespace extensions

namespace gfx {
class Image;
}

This will improve build times since the entire headers are not needed to compile
this header.  Note that you still need to include c/b/n/notification.h since the
type is referred to as a const ref, not just a pointer (in line 22.)

Removing these includes here may mean that you will now need to include them in
the cc file, which is better.

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.h:15: class
NotificationConversionHelper {
Probably this class can just be a container for static functions.

[Pete Williamson, 2014-08-09 00:38:19]

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:120:
"title", new base::StringValue(notification.title()));
On 2014/08/08 22:40:24, liyanhou wrote:
> On 2014/08/05 22:03:52, Pete Williamson wrote:
> > can notification.title() (or any of the other methods we call on it) return
> > null?  If so, we can have a crash.  The notification may be coming from an
> > untrusted source, we don't want it to be able to crash chrome, which might
be
> a
> > security hole.
> 
> id, title, message, and icon url are the required fields for creating
> notifications through notifications API. Since Notifications is converted from
> that, all notifications should have those four fields.

Thanks!  Just to be sure, can you check that the notification api is actually
enforcing (checking that they are not null) when the notification enters the
system?

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
File chrome/browser/notifications/notification_conversion_helper.cc (right):

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:52:
options->priority = scoped_ptr<int>(new int(rich_data->priority));
Is options->priority really a scoped_ptr<int>?  It seems like an int would
likely be more appropriate.

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:127: int
pixel_width = sk_bitmap.width() * sk_bitmap.height();
This isn't really a with per say, more of a size.  A better name might be
pixel_count.

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:132: unsigned
char* bitmap_data(new unsigned char[pixel_width * 4]);
Let's put the 4 back to BYTES_PER_PIXEL, both here and below.  Generally
speaking, all numbers other than 0 and 1 in the code should be symbolic
constants instead of direct literals.

[liyanhou, 2014-08-11 17:21:10]

Addressed comments.
Could you take another look? Thank you so much!

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:140: }
On 2014/08/09 00:29:52, dewittj wrote:
> Do you want to modify the Update function too?

Yes. I was going it in the next CL.

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
File chrome/browser/notifications/notification_conversion_helper.cc (right):

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:52:
options->priority = scoped_ptr<int>(new int(rich_data->priority));
On 2014/08/09 00:38:19, Pete Williamson wrote:
> Is options->priority really a scoped_ptr<int>?  It seems like an int would
> likely be more appropriate.

Yes, it's a scoped_ptr<int> in the generated notifications.h file.

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:119: void
NotificationConversionHelper::GfxImageToNotificationBitmap(
On 2014/08/09 00:29:52, dewittj wrote:
> can we also move the conversion from NotificationBitmap to gfx::Image into
this
> file?

Yes. Can that be in a separate CL, since this CL is getting big?

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:123:
sk_bitmap.lockPixels();
On 2014/08/09 00:29:52, dewittj wrote:
> hmm, do you know why this line is here?  Why isn't there a corresponding
> unlockPixels?

Done.

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:127: int
pixel_width = sk_bitmap.width() * sk_bitmap.height();
On 2014/08/09 00:38:19, Pete Williamson wrote:
> This isn't really a with per say, more of a size.  A better name might be
> pixel_count.

Done.

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:132: unsigned
char* bitmap_data(new unsigned char[pixel_width * 4]);
On 2014/08/09 00:38:19, Pete Williamson wrote:
> Let's put the 4 back to BYTES_PER_PIXEL, both here and below.  Generally
> speaking, all numbers other than 0 and 1 in the code should be symbolic
> constants instead of direct literals.

Done.

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:153: return "";
On 2014/08/09 00:29:52, dewittj wrote:
> NOTREACHED() here before the return since it's an error to ever hit this line.

Done.

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
File chrome/browser/notifications/notification_conversion_helper.h (right):

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.h:8: #include
"base/values.h"
On 2014/08/09 00:29:52, dewittj wrote:
> We don't need all these include files.  You can remove line 8, 10, 11, and 12,
> provided that you forward declare the NotificationOptions and
NotificationBitmap
> type.  Something like:
> 
> namespace extensions {
> namespace api {
> namespace notifications {
> class NotificationOptions;
> class NotificationBitmap;
> }  // namespace notifications
> }  // namespace api
> }  // namespace extensions
> 
> namespace gfx {
> class Image;
> }
> 
> This will improve build times since the entire headers are not needed to
compile
> this header.  Note that you still need to include c/b/n/notification.h since
the
> type is referred to as a const ref, not just a pointer (in line 22.)
> 
> Removing these includes here may mean that you will now need to include them
in
> the cc file, which is better.

Done. I didn't know that. That's great. Thank you so much!

https://codereview.chromium.org/441753002/diff/110001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.h:15: class
NotificationConversionHelper {
On 2014/08/09 00:29:52, dewittj wrote:
> Probably this class can just be a container for static functions.

Done.

[Pete Williamson, 2014-08-11 17:42:40]

Loogs good, but my comment at line 120 of message_center_notification_manager
has not yet been addressed.

[liyanhou, 2014-08-11 17:59:23]

petewil@ Sorry that I forgot to reply to the comment in
message_center_notification_manager file.

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/441753002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/message_center_notification_manager.cc:120:
"title", new base::StringValue(notification.title()));
On 2014/08/09 00:38:19, Pete Williamson wrote:
> On 2014/08/08 22:40:24, liyanhou wrote:
> > On 2014/08/05 22:03:52, Pete Williamson wrote:
> > > can notification.title() (or any of the other methods we call on it)
return
> > > null?  If so, we can have a crash.  The notification may be coming from an
> > > untrusted source, we don't want it to be able to crash chrome, which might
> be
> > a
> > > security hole.
> > 
> > id, title, message, and icon url are the required fields for creating
> > notifications through notifications API. Since Notifications is converted
from
> > that, all notifications should have those four fields.
> 
> Thanks!  Just to be sure, can you check that the notification api is actually
> enforcing (checking that they are not null) when the notification enters the
> system?

Yes. Notifications API enforces it when creating a notification.

[Pete Williamson, 2014-08-11 18:07:42]

lgtm

[dewittj, 2014-08-12 18:47:39]

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:502:
notification, profile_notification->profile(), extension_id);
This is still in AddProfileNotification, meaning there is no way for updates to
be sent to the alternate provider; they will all be interpreted as create.

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
File chrome/browser/notifications/notification_conversion_helper.cc (right):

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:10: #include
"chrome/browser/extensions/api/notification_provider/notification_provider_api.h"
I don't think this include is used.

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:83: LOG(WARNING)
<< "Only image type notifications should have images.";
I am not sure that this deserves a LOG, possibly move to DVLOG(WARNING) so it
only logs in verbose debug builds.  Same for all the LOG(WARNING) statements.

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:111: void
NotificationConversionHelper::GfxImageToNotificationBitmap(
Please bring the reverse function from notifications_api into this file as well.

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:130:
notification_bitmap->data = scoped_ptr<std::string>(new std::string(
nit: notification_bitmap->data.reset(new std::string(...))

[dewittj, 2014-08-12 21:04:15]

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:502:
notification, profile_notification->profile(), extension_id);
On 2014/08/12 18:47:39, dewittj wrote:
> This is still in AddProfileNotification, meaning there is no way for updates
to
> be sent to the alternate provider; they will all be interpreted as create.

OK after talking I propose a few changes:
* Update commit comment to make it clearer that updates are not yet handled
* Add a method to the ProfileNotification class called "AddToMessageCenter"
* Add a method to the ProfileNotification class called
"RouteToAlternateProvider"
* Move these changes to the Add function so that it's clearer that the
notificaiton can be redirected, and call AddToMessageCenter and
RouteToAlternateProvider there.

WDYT about that?

[liyanhou, 2014-08-13 17:30:37]

dewittj@ Addressed comments. Could you take another look? Thank you so much!

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:502:
notification, profile_notification->profile(), extension_id);
On 2014/08/12 21:04:15, dewittj wrote:
> On 2014/08/12 18:47:39, dewittj wrote:
> > This is still in AddProfileNotification, meaning there is no way for updates
> to
> > be sent to the alternate provider; they will all be interpreted as create.
> 
> OK after talking I propose a few changes:
> * Update commit comment to make it clearer that updates are not yet handled
> * Add a method to the ProfileNotification class called "AddToMessageCenter"
> * Add a method to the ProfileNotification class called
> "RouteToAlternateProvider"
> * Move these changes to the Add function so that it's clearer that the
> notificaiton can be redirected, and call AddToMessageCenter and
> RouteToAlternateProvider there.
> 
> WDYT about that?

Done. That sound good. I moved RouteToAlternateProvider to ProfileNotification,
and moved these changes to the Add function. Since adding to message center is
just one line, I think it should be pretty clear by itself, so I just kept that
as it is.

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
File chrome/browser/notifications/notification_conversion_helper.cc (right):

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:10: #include
"chrome/browser/extensions/api/notification_provider/notification_provider_api.h"
On 2014/08/12 18:47:39, dewittj wrote:
> I don't think this include is used.

Done.

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:83: LOG(WARNING)
<< "Only image type notifications should have images.";
On 2014/08/12 18:47:39, dewittj wrote:
> I am not sure that this deserves a LOG, possibly move to DVLOG(WARNING) so it
> only logs in verbose debug builds.  Same for all the LOG(WARNING) statements.

Done.

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:111: void
NotificationConversionHelper::GfxImageToNotificationBitmap(
On 2014/08/12 18:47:39, dewittj wrote:
> Please bring the reverse function from notifications_api into this file as
well.

Done.

https://codereview.chromium.org/441753002/diff/210001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.cc:130:
notification_bitmap->data = scoped_ptr<std::string>(new std::string(
On 2014/08/12 18:47:39, dewittj wrote:
> nit: notification_bitmap->data.reset(new std::string(...))

Done. Also changed all other occurrences in this file. Thanks.

[dewittj, 2014-08-13 18:30:18]

lgtm with nits

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

https://codereview.chromium.org/441753002/diff/210001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/notification_provider/events/test.js:8: 
nit: no blank line

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:141: if
(!extension_id.empty()) {
nit: no braces for 1-line if statement

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:508:
std::string extension_id) {
nit: const ref

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:510:
scoped_ptr<extensions::api::notifications::NotificationOptions> options(
nit: just stack allocate this.

extensions::api::notifications::NotificationOptions options;
NotificationConversionHelper::NotificationToNotificationOptions(notification_,
&options);

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:517: new
extensions::NotificationProviderEventRouter(profile_));
same, stack allocate this too

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:561: for
(extensions::ExtensionSet::const_iterator it =
nit: rewrite this loop condition so that it fits on fewer lines.

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:567:
extension_id = (*it->get()).id();
nit: return from here so you don't always iterate all the extensions

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_notification_manager.h (right):

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.h:203: //
Helpers that add/remove the notification from local map and MessageCenter.
please update comment

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
File chrome/browser/notifications/notification_conversion_helper.h (right):

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.h:24: class
NotificationConversionHelper {
please add a class comment

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
File chrome/browser/notifications/notification_conversion_helper_unittest.cc
(right):

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper_unittest.cc:37:
optional_fields.clickable = false;
I think this line is unnecessary

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper_unittest.cc:38:
optional_fields.small_image = gfx::Image();
ditto

[liyanhou, 2014-08-13 20:28:57]

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:141: if
(!extension_id.empty()) {
On 2014/08/13 18:30:17, dewittj wrote:
> nit: no braces for 1-line if statement

Done.

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:508:
std::string extension_id) {
On 2014/08/13 18:30:17, dewittj wrote:
> nit: const ref

Done.

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:510:
scoped_ptr<extensions::api::notifications::NotificationOptions> options(
On 2014/08/13 18:30:17, dewittj wrote:
> nit: just stack allocate this.
> 
> extensions::api::notifications::NotificationOptions options;
> NotificationConversionHelper::NotificationToNotificationOptions(notification_,
> &options);
> 

Done.

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:517: new
extensions::NotificationProviderEventRouter(profile_));
On 2014/08/13 18:30:17, dewittj wrote:
> same, stack allocate this too

Done.

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:561: for
(extensions::ExtensionSet::const_iterator it =
On 2014/08/13 18:30:17, dewittj wrote:
> nit: rewrite this loop condition so that it fits on fewer lines.

Done.

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.cc:567:
extension_id = (*it->get()).id();
On 2014/08/13 18:30:17, dewittj wrote:
> nit: return from here so you don't always iterate all the extensions

Done.

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_notification_manager.h (right):

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/message_center_notification_manager.h:203: //
Helpers that add/remove the notification from local map and MessageCenter.
On 2014/08/13 18:30:17, dewittj wrote:
> please update comment

Done.

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
File chrome/browser/notifications/notification_conversion_helper.h (right):

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper.h:24: class
NotificationConversionHelper {
On 2014/08/13 18:30:18, dewittj wrote:
> please add a class comment

Done.

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
File chrome/browser/notifications/notification_conversion_helper_unittest.cc
(right):

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper_unittest.cc:37:
optional_fields.clickable = false;
On 2014/08/13 18:30:18, dewittj wrote:
> I think this line is unnecessary

I think the default set in the notification constructor is true, so this would
test the changed value.

https://codereview.chromium.org/441753002/diff/310001/chrome/browser/notifica...
chrome/browser/notifications/notification_conversion_helper_unittest.cc:38:
optional_fields.small_image = gfx::Image();
On 2014/08/13 18:30:18, dewittj wrote:
> ditto

Done.

[liyanhou, 2014-08-13 22:18:06]

The CQ bit was checked by liyanhou@chromium.org

[commit-bot: I haz the power, 2014-08-13 22:20:44]

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

[commit-bot: I haz the power, 2014-08-14 10:30:56]

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

[commit-bot: I haz the power, 2014-08-14 12:17:16]

Committed patchset #13 (330001) as 289532