Adding an experimental app notification API.

Adding an experimental app notification API.

BUG=86958
TEST=none for now


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

[asargent_no_longer_on_chrome, 2011-06-16 21:47:43]

Finnur - While not all pieces of this are complete (the image-related ones for
instance don't do anything), I'd like to get this part reviewed and checked in
so we can start iterating on it.

This CL contains everything needed for notification data to get to the NTP, but
doesn't have any of your code for actually displaying them.

[Finnur, 2011-06-20 12:21:46]

Sorry for not replying earlier, June 17th (last Friday) was a national holiday
in Iceland.

General comments: BUG= and TEST= are missing. I just have a few nits and some
questions on the changelist (see below).

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_app_api.cc (right):

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_app_api.cc:33: NotificationMap::iterator
found = items_.find(item->extension_id);
It would probably help readability a bit to rename |items_| to
|app_notifications_| (or something). |items_| is a bit generic.

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_app_api.cc:35: items_[item->extension_id] =
AppNotificationList();
AppNotificationList()? Where does that come from? I can't find it elsewhere in
this patch. Could there be a file missing since the CL didn't go through a try?

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_app_api.cc:65: 
nit: extra line break

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_app_api.cc:92: &item->linkText));
I would move this block into line 88 (within the block).
Also, if details->HasKey(kLinkTextKey) == false then set the linkText to be =
link_url.

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_app_api.h (right):

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_app_api.h:51: NotificationRegistrar
registrar_;
Maybe have this member at the bottom, since technically the typedef is supposed
to be first within the private section.

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_app_api.h:56: };
No DISALLOW_DR_EVIL_CONSTRUCTORS(AppNotificationManager)? :)

http://codereview.chromium.org/7187023/diff/1/chrome/browser/ui/webui/ntp/app...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7187023/diff/1/chrome/browser/ui/webui/ntp/app...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:269: app_info);
Should we be showing notifications for disabled apps?

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:6196: "description": "The title
of the notification. The first several characters of this will appear underneath
the icon on the new tab page."
nit: Try to phrase it without design details such as where it appears, as it
sounds like people are still not 100% on the final design.

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:6206: "description": "The URL
for an optional link to show at the bottom of the notification. See also
linkText."
nit: same here.

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:6211: "description": "If a
linkURL is provided, this will be used as the linkified text. It should be very
short, and will be displayed truncated if too long (details TBD)."
"details TBD"? What details?

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:6211: "description": "If a
linkURL is provided, this will be used as the linkified text. It should be very
short, and will be displayed truncated if too long (details TBD)."
nit: URL is not uppercase above (linkUrl).

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:6233: "name":
"clearAllNotifications",
This function got me thinking... We have passive and active notifications. Do we
foresee that we want to clear both types at the same time and always everything?
Will people want to use, for example, passive notifications in such a way that
they maybe expire their last notification or a notification by id (id provided
in a callback). 

For example, if it is a promotion with a price, then you could have the price
change by clearing just the notification that has the old price and setup a new
one with the updated price. That way you don't have to clear out all previous
active and passive notifications just to update the price?

Just thinking out loud... :)

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.h:1033: };
This struct doesn't seem to be used anywhere and there is another struct
AppNotificationInfo with more params in extension_app_api.h. Is this struct
required still?

http://codereview.chromium.org/7187023/diff/1/content/common/notification_type.h
File content/common/notification_type.h (right):

http://codereview.chromium.org/7187023/diff/1/content/common/notification_typ...
content/common/notification_type.h:980: // An installed app changed state (added
or removed notifications).
Maybe fill in the data that is expected to be provided with this notification...

[asargent_no_longer_on_chrome, 2011-06-21 18:10:04]

Addressed review comments with new version. Please take another look.

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_app_api.cc (right):

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_app_api.cc:33: NotificationMap::iterator
found = items_.find(item->extension_id);
On 2011/06/20 12:21:46, Finnur wrote:
> It would probably help readability a bit to rename |items_| to
> |app_notifications_| (or something). |items_| is a bit generic.

Good point. Changed to "notifications_".

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_app_api.cc:35: items_[item->extension_id] =
AppNotificationList();
On 2011/06/20 12:21:46, Finnur wrote:
> AppNotificationList()? Where does that come from? I can't find it elsewhere in
> this patch. Could there be a file missing since the CL didn't go through a
try?

It's in extension_app_api.h, just before the start of the declaration of the
AppNotificationManager class. I added a comment to the top of it.

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_app_api.cc:65: 
On 2011/06/20 12:21:46, Finnur wrote:
> nit: extra line break

Done.

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_app_api.cc:92: &item->linkText));
On 2011/06/20 12:21:46, Finnur wrote:
> I would move this block into line 88 (within the block).
> Also, if details->HasKey(kLinkTextKey) == false then set the linkText to be =
> link_url.
Good point on moving it into the block above - I've done that.

As for defaulting the link text to the URL if unspecified, I wonder: would it be
better to just do that in the NTP code if the link text is the empty string?
That way we could have some sort of default text like "link" or something which
we might decide works better than potentially long urls.

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_app_api.h (right):

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_app_api.h:51: NotificationRegistrar
registrar_;
On 2011/06/20 12:21:46, Finnur wrote:
> Maybe have this member at the bottom, since technically the typedef is
supposed
> to be first within the private section.

Done.

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_app_api.h:56: };
On 2011/06/20 12:21:46, Finnur wrote:
> No DISALLOW_DR_EVIL_CONSTRUCTORS(AppNotificationManager)? :)

Done.

http://codereview.chromium.org/7187023/diff/1/chrome/browser/ui/webui/ntp/app...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7187023/diff/1/chrome/browser/ui/webui/ntp/app...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:269: app_info);
On 2011/06/20 12:21:46, Finnur wrote:
> Should we be showing notifications for disabled apps?

No, we should not. Good catch.

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:6196: "description": "The title
of the notification. The first several characters of this will appear underneath
the icon on the new tab page."
On 2011/06/20 12:21:46, Finnur wrote:
> nit: Try to phrase it without design details such as where it appears, as it
> sounds like people are still not 100% on the final design.

Done.

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:6206: "description": "The URL
for an optional link to show at the bottom of the notification. See also
linkText."
On 2011/06/20 12:21:46, Finnur wrote:
> nit: same here.

Done.

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:6211: "description": "If a
linkURL is provided, this will be used as the linkified text. It should be very
short, and will be displayed truncated if too long (details TBD)."
On 2011/06/20 12:21:46, Finnur wrote:
> "details TBD"? What details?

Done.

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:6211: "description": "If a
linkURL is provided, this will be used as the linkified text. It should be very
short, and will be displayed truncated if too long (details TBD)."
On 2011/06/20 12:21:46, Finnur wrote:
> "details TBD"? What details?
I just removed this for now. Eventually when we settle on how this will work we
can include more details either here or in the overview documentation at the top
of the page for the API.

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:6233: "name":
"clearAllNotifications",
On 2011/06/20 12:21:46, Finnur wrote:
> This function got me thinking... We have passive and active notifications. Do
we
> foresee that we want to clear both types at the same time and always
everything?
> Will people want to use, for example, passive notifications in such a way that
> they maybe expire their last notification or a notification by id (id provided
> in a callback). 
> 
> For example, if it is a promotion with a price, then you could have the price
> change by clearing just the notification that has the old price and setup a
new
> one with the updated price. That way you don't have to clear out all previous
> active and passive notifications just to update the price?
> 
> Just thinking out loud... :)

Interesting. Perhaps we'd want to add a "clearOthers" flag or something to the
create method?

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.h:1033: };
On 2011/06/20 12:21:46, Finnur wrote:
> This struct doesn't seem to be used anywhere and there is another struct
> AppNotificationInfo with more params in extension_app_api.h. Is this struct
> required still?
Good catch, I forgot to remove this from the CL.

http://codereview.chromium.org/7187023/diff/1/content/common/notification_type.h
File content/common/notification_type.h (right):

http://codereview.chromium.org/7187023/diff/1/content/common/notification_typ...
content/common/notification_type.h:980: // An installed app changed state (added
or removed notifications).
On 2011/06/20 12:21:46, Finnur wrote:
> Maybe fill in the data that is expected to be provided with this
notification...

Done.

[Finnur, 2011-06-22 10:40:23]

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_app_api.cc (right):

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_app_api.cc:92: &item->linkText));
I think we should address it at this funnel rather than rely on consumers to do
the right thing. I see two possibilities why a link text is empty:

1) Left out by mistake
2) On purpose because the link is short and self-describing.

I suspect a default of 'link' is probably no better than showing the URL for
case 1 and annoying for case 2. But how about simply aborting if URL is provided
but the description is blank?



On 2011/06/21 18:10:11, Antony Sargent wrote:
> On 2011/06/20 12:21:46, Finnur wrote:
> > I would move this block into line 88 (within the block).
> > Also, if details->HasKey(kLinkTextKey) == false then set the linkText to be
=
> > link_url.
> Good point on moving it into the block above - I've done that.
> 
> As for defaulting the link text to the URL if unspecified, I wonder: would it
be
> better to just do that in the NTP code if the link text is the empty string?
> That way we could have some sort of default text like "link" or something
which
> we might decide works better than potentially long urls.

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:6233: "name":
"clearAllNotifications",
Well, that's kind of the inverse of what I was talking about. Or, at least that
depends on what 'Others' means in this context.

But here's what I'm thinking of... App adds two notifications: 
A: Special prices all summer long. Check the store now!
B: Clearance sale. Winter clothes: 50% off.

If they just want to update B (because the discount is now 75%) they now need to
recreate A after they clearAll to get the effect they are after. Maybe this is
not an issue, certainly not for this changelist, just something to think about
whether we want to support.

On 2011/06/21 18:10:11, Antony Sargent wrote:
> On 2011/06/20 12:21:46, Finnur wrote:
> > This function got me thinking... We have passive and active notifications.
Do
> we
> > foresee that we want to clear both types at the same time and always
> everything?
> > Will people want to use, for example, passive notifications in such a way
that
> > they maybe expire their last notification or a notification by id (id
provided
> > in a callback). 
> > 
> > For example, if it is a promotion with a price, then you could have the
price
> > change by clearing just the notification that has the old price and setup a
> new
> > one with the updated price. That way you don't have to clear out all
previous
> > active and passive notifications just to update the price?
> > 
> > Just thinking out loud... :)
> 
> Interesting. Perhaps we'd want to add a "clearOthers" flag or something to the
> create method?

[asargent_no_longer_on_chrome, 2011-06-22 17:36:36]

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_app_api.cc (right):

http://codereview.chromium.org/7187023/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_app_api.cc:92: &item->linkText));
On 2011/06/22 10:40:23, Finnur wrote:
> I think we should address it at this funnel rather than rely on consumers to
do
> the right thing. I see two possibilities why a link text is empty:
> 
> 1) Left out by mistake
> 2) On purpose because the link is short and self-describing.
> 
> I suspect a default of 'link' is probably no better than showing the URL for
> case 1 and annoying for case 2. But how about simply aborting if URL is
provided
> but the description is blank?

Ok, that seems reasonable. We can certainly change this later if we decide it's
annoying from a developer perspective.

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7187023/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:6233: "name":
"clearAllNotifications",
On 2011/06/22 10:40:23, Finnur wrote:
> Well, that's kind of the inverse of what I was talking about. Or, at least
that
> depends on what 'Others' means in this context.
> 
> But here's what I'm thinking of... App adds two notifications: 
> A: Special prices all summer long. Check the store now!
> B: Clearance sale. Winter clothes: 50% off.
> 
> If they just want to update B (because the discount is now 75%) they now need
to
> recreate A after they clearAll to get the effect they are after. Maybe this is
> not an issue, certainly not for this changelist, just something to think about
> whether we want to support.
> 
> On 2011/06/21 18:10:11, Antony Sargent wrote:
> > On 2011/06/20 12:21:46, Finnur wrote:
> > > This function got me thinking... We have passive and active notifications.
> Do
> > we
> > > foresee that we want to clear both types at the same time and always
> > everything?
> > > Will people want to use, for example, passive notifications in such a way
> that
> > > they maybe expire their last notification or a notification by id (id
> provided
> > > in a callback). 
> > > 
> > > For example, if it is a promotion with a price, then you could have the
> price
> > > change by clearing just the notification that has the old price and setup
a
> > new
> > > one with the updated price. That way you don't have to clear out all
> previous
> > > active and passive notifications just to update the price?
> > > 
> > > Just thinking out loud... :)
> > 
> > Interesting. Perhaps we'd want to add a "clearOthers" flag or something to
the
> > create method?
> 
Ok, I'll keep thinking about this but not make any changes to the CL for now.

[Evan Stade, 2011-06-22 19:36:22]

sticking my head in due to relationship with NTP...

http://codereview.chromium.org/7187023/diff/20003/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7187023/diff/20003/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:114: AppNotification* n =
(*notifications->rbegin()).get();
is this not the same as notifications->rbegin()->get()

http://codereview.chromium.org/7187023/diff/20003/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:212: case
NotificationType::APP_NOTIFICATION_STATE_CHANGED:
I don't think you want to nuke the entirety of the apps section every time a
notification state changes

[Finnur, 2011-06-22 22:45:31]

My comments have been addressed, so LGTM.

I'll let you and Evan work out the rest of the comments.

On 2011/06/22 19:36:22, Evan Stade wrote:
> sticking my head in due to relationship with NTP...
> 
>
http://codereview.chromium.org/7187023/diff/20003/chrome/browser/ui/webui/ntp...
> File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):
> 
>
http://codereview.chromium.org/7187023/diff/20003/chrome/browser/ui/webui/ntp...
> chrome/browser/ui/webui/ntp/app_launcher_handler.cc:114: AppNotification* n =
> (*notifications->rbegin()).get();
> is this not the same as notifications->rbegin()->get()
> 
>
http://codereview.chromium.org/7187023/diff/20003/chrome/browser/ui/webui/ntp...
> chrome/browser/ui/webui/ntp/app_launcher_handler.cc:212: case
> NotificationType::APP_NOTIFICATION_STATE_CHANGED:
> I don't think you want to nuke the entirety of the apps section every time a
> notification state changes

[asargent_no_longer_on_chrome, 2011-06-23 22:36:24]

estade - Addressed your comments. Please take another look.

http://codereview.chromium.org/7187023/diff/20003/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7187023/diff/20003/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:114: AppNotification* n =
(*notifications->rbegin()).get();
On 2011/06/22 19:36:23, Evan Stade wrote:
> is this not the same as notifications->rbegin()->get()
Yes, good point. (I reworked the code - this moved into
AppNotificationManager::GetLast)

http://codereview.chromium.org/7187023/diff/20003/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:212: case
NotificationType::APP_NOTIFICATION_STATE_CHANGED:
On 2011/06/22 19:36:23, Evan Stade wrote:
> I don't think you want to nuke the entirety of the apps section every time a
> notification state changes
Good catch - I had thrown this in there as a quick hack while we were
prototyping, but it would have been easy to forget to come back and clean this
up later to be more efficient. I've rewritten it to call a JS function passing
the extension id and last notification detail (if any).

[Evan Stade, 2011-06-24 17:29:20]

thanks. parts I commented on LGTM

http://codereview.chromium.org/7187023/diff/23001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp/apps.js (right):

http://codereview.chromium.org/7187023/diff/23001/chrome/browser/resources/nt...
chrome/browser/resources/ntp/apps.js:152: function
appsNotificationChangeCallback(id, last_notification) {
nits:

lastNotification instead of last_notification

also, this fn should be called appNotificationChanged (I think the plural is
wrong, and the Callback is unnecessary even though appsPrefChangeCallbackoddly 
uses it)

[asargent_no_longer_on_chrome, 2011-06-24 20:02:54]

Brett- Can you review the content/ change?

http://codereview.chromium.org/7187023/diff/23001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp/apps.js (right):

http://codereview.chromium.org/7187023/diff/23001/chrome/browser/resources/nt...
chrome/browser/resources/ntp/apps.js:152: function
appsNotificationChangeCallback(id, last_notification) {
On 2011/06/24 17:29:20, Evan Stade wrote:
> nits:
> 
> lastNotification instead of last_notification
> 
> also, this fn should be called appNotificationChanged (I think the plural is
> wrong, and the Callback is unnecessary even though appsPrefChangeCallbackoddly

> uses it)

Done.

[brettw, 2011-06-24 23:29:06]

content LGTM

[commit-bot: I haz the power, 2011-06-27 18:03:34]

Try job failure for 7187023-28001 (retry) on linux for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-06-28 02:07:32]

Change committed as 90705