Limit number of notifications that can be received by the client.

Persist notification creation time.
Ensure notification displayed is latest
Limit number of notifications that can be received by the client. (Client side Garbage collection)

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

[Munjal (Google), 2011-11-17 19:45:32]

I think Antony should also take a look.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification.cc (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.cc:13: const char* kCreationKey=
"creation_timestamp_ms";
Nit: name nit. Here and in the rest of the file.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.cc:77: if (value.HasKey(kCreationKey)
&& !value.GetDouble(
Nit: take !value.GetDouble in the next line.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification.h (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.h:24: double creation_timestamp_ms,
Is double the right data type for ms since epoch?

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.h:24: double creation_timestamp_ms,
Nit: may be just name it creation_time_ms

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.h:41: double creation_timestamp_ms()
const { return creation_timestamp_ms_; }
Nit: name creation_time_ms()

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.h:64: double creation_timestamp_ms_;
Nit: same name nit here.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification_manager.cc (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.cc:119: }
Actually, I am wondering if we should add in a sorted manner into the list. That
way we can always return the head in GetLast and delete the last one here (or
vice versa depending on whether sort is asc or desc).

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.cc:430: bool
shouldProcessSyncerChanges) {
Actually seems like you call this with true for second parameter. Remove second
parameter then?

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.cc:436: &&
!shouldProcessSyncerChanges))
Don't we want this to happen even when we are processing syncer changes?

Would the following suffice?
if (notif.is_local() || !modles_associated_)
  reutnr;

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.cc:437: return;
If your if is multiple lines you need braces even for single line in the body.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.cc:439: // TODO(munjal):
crbug.com/10059. Work with Lingesh/Antony to resolve.
This TODO is not relevant here I think.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification_manager.h (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.h:23: namespace
notification_manager_constants {
Add one empty line before namespace.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.h:24: const unsigned int
kMaxNotificationPerApp = 5;
Actually this constant is not used in header, and should go in an anonymous
namespace in the .cc file.
also, make it a static const int.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.h:138: bool
shouldProcessSyncerChanges);
This method is an instance method and it can access the member
processing_syncer_changes_. So you probably don't need the second parameter?

FYI, naming convention is to name it should_process_sync_changes.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_app_api.cc (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_app_api.cc:53: &creation_timestamp_ms));
Shouldn't we just fill in creation time ourselves when an extension calls the JS
API to create a notification?

[elvin, 2011-11-17 22:28:17]

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification.cc (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.cc:13: const char* kCreationKey=
"creation_timestamp_ms";
On 2011/11/17 19:45:32, munjal wrote:
> Nit: name nit. Here and in the rest of the file.

Done.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.cc:77: if (value.HasKey(kCreationKey)
&& !value.GetDouble(
On 2011/11/17 19:45:32, munjal wrote:
> Nit: take !value.GetDouble in the next line.

Done.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification_manager.cc (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.cc:430: bool
shouldProcessSyncerChanges) {
On 2011/11/17 19:45:32, munjal wrote:
> Actually seems like you call this with true for second parameter. Remove
second
> parameter then?

Done.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.cc:436: &&
!shouldProcessSyncerChanges))
On 2011/11/17 19:45:32, munjal wrote:
> Don't we want this to happen even when we are processing syncer changes?
> 
> Would the following suffice?
> if (notif.is_local() || !modles_associated_)
>   reutnr;

Done.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.cc:437: return;
On 2011/11/17 19:45:32, munjal wrote:
> If your if is multiple lines you need braces even for single line in the body.

Done.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.cc:439: // TODO(munjal):
crbug.com/10059. Work with Lingesh/Antony to resolve.
On 2011/11/17 19:45:32, munjal wrote:
> This TODO is not relevant here I think.

Done.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification_manager.h (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.h:23: namespace
notification_manager_constants {
On 2011/11/17 19:45:32, munjal wrote:
> Add one empty line before namespace.

Done.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.h:24: const unsigned int
kMaxNotificationPerApp = 5;
It's used in the test file?
On 2011/11/17 19:45:32, munjal wrote:
> Actually this constant is not used in header, and should go in an anonymous
> namespace in the .cc file.
> also, make it a static const int.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.h:138: bool
shouldProcessSyncerChanges);
REmoved
On 2011/11/17 19:45:32, munjal wrote:
> This method is an instance method and it can access the member
> processing_syncer_changes_. So you probably don't need the second parameter?
> 
> FYI, naming convention is to name it should_process_sync_changes.
>

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_app_api.cc (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_app_api.cc:53: &creation_timestamp_ms));
On 2011/11/17 19:45:32, munjal wrote:
> Shouldn't we just fill in creation time ourselves when an extension calls the
JS
> API to create a notification?

Done.

[asargent_no_longer_on_chrome, 2011-11-17 22:36:24]

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification.cc (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.cc:52:
result->SetDouble(kCreationKey, creation_timestamp_ms_);
for a Time, you can serialize it as a string like this:

base::Int64ToString(create_time.ToInternalValue())

and to de-serialize, you can do:


Time::FromInternalValue(base::StringToInt64(string_value))

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification.h (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.h:24: double creation_timestamp_ms,
On 2011/11/17 19:45:32, munjal wrote:
> Is double the right data type for ms since epoch?

Actually it would probably be better to use our Time class (from base/time.h)

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification_manager.cc (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.cc:119: }
On 2011/11/17 19:45:32, munjal wrote:
> Actually, I am wondering if we should add in a sorted manner into the list.
That
> way we can always return the head in GetLast and delete the last one here (or
> vice versa depending on whether sort is asc or desc).
+1 - keeping them sorted is a good idea

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_app_api.cc (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_app_api.cc:53: &creation_timestamp_ms));
On 2011/11/17 19:45:32, munjal wrote:
> Shouldn't we just fill in creation time ourselves when an extension calls the
JS
> API to create a notification?

Yeah, we should just use Time::Now()

[elvin, 2011-11-17 22:52:58]

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification.cc (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.cc:52:
result->SetDouble(kCreationKey, creation_timestamp_ms_);
Done
On 2011/11/17 22:36:24, Antony Sargent wrote:
> for a Time, you can serialize it as a string like this:
> 
> base::Int64ToString(create_time.ToInternalValue())
> 
> and to de-serialize, you can do:
> 
> 
> Time::FromInternalValue(base::StringToInt64(string_value))

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification.h (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.h:24: double creation_timestamp_ms,
On 2011/11/17 22:36:24, Antony Sargent wrote:
> On 2011/11/17 19:45:32, munjal wrote:
> > Is double the right data type for ms since epoch?
> 
> Actually it would probably be better to use our Time class (from base/time.h)

Done.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.h:64: double creation_timestamp_ms_;
On 2011/11/17 19:45:32, munjal wrote:
> Nit: same name nit here.

Done.

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_app_api.cc (right):

http://codereview.chromium.org/8567018/diff/9004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_app_api.cc:53: &creation_timestamp_ms));
On 2011/11/17 22:36:24, Antony Sargent wrote:
> On 2011/11/17 19:45:32, munjal wrote:
> > Shouldn't we just fill in creation time ourselves when an extension calls
the
> JS
> > API to create a notification?
> 
> Yeah, we should just use Time::Now()
> 
> 

Done.

[asargent_no_longer_on_chrome, 2011-11-17 23:40:14]

LGTM w/ some nits you should address before checking in

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification.cc (right):

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.cc:53: if (creation_time_.is_null())
did you mean "if (!creation_time_.is_null())" ?

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.cc:127:
other.creation_time_.ToInternalValue() &&
nit: the Time class has an operator== method, so you can just use
"creation_time_ == other.creation_time_" here.

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification.h (right):

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.h:25: const base::Time creation_time,
nit: make this "const base::Time&"

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification_manager.cc (right):

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.cc:99:
a2.get()->creation_time().ToInternalValue();
nit: Time has operator< and operator> methods, so you can skip the call to
ToInternalValue here and just compare the creation_time's directly.

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.cc:169:
AppNotificationSortPredicate)->get();
I guess it's a little strange that GetLast returns the one with the highest
timestamp, but GetAll above can return the items in a potentially unsorted
order. No need to hold up this CL, but I'll think about whether we should always
sort or switch to storing items in a std::set or something instead of a list.

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification_manager.h (right):

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.h:25: static const unsigned
int kMaxNotificationPerApp = 5;
nit: We typically use int's instead of unsigned int's for constants, and we
often just put constants inside the class instead of a namespace, unless there
are a huge pile of them that deserve to be in their own separate file. 

So in the .h:

class AppNotificationManager ... {
 public:
  static const int kMaxNotificationPerApp;

  ...
};

and then define it inside the .cc file

const int AppNotificationManager::kMaxNotificationPerApp = 5;

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification_test_util.cc (right):

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_test_util.cc:32: true, count, guid,
extension_id, title, body);
does this still compile with the switch to Time? Maybe this should just be
switched to Time::Now() ?

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_app_api.cc (right):

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_app_api.cc:21: const char kCreationKey[] =
"creation_timestamp_ms";
nit: remove this

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_app_api.cc:54: &creation_timestamp_ms));
nit: remove this block since we're just passing Time::Now() below.

[elvin, 2011-11-18 00:10:18]

also added the sorting on Add

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification.cc (right):

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.cc:53: if (creation_time_.is_null())
On 2011/11/17 23:40:14, Antony Sargent wrote:
> did you mean "if (!creation_time_.is_null())" ?

Done.

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.cc:127:
other.creation_time_.ToInternalValue() &&
On 2011/11/17 23:40:14, Antony Sargent wrote:
> nit: the Time class has an operator== method, so you can just use
> "creation_time_ == other.creation_time_" here.

Done.

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification.h (right):

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification.h:25: const base::Time creation_time,
On 2011/11/17 23:40:14, Antony Sargent wrote:
> nit: make this "const base::Time&"

Done.

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification_manager.cc (right):

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.cc:99:
a2.get()->creation_time().ToInternalValue();
On 2011/11/17 23:40:14, Antony Sargent wrote:
> nit: Time has operator< and operator> methods, so you can skip the call to
> ToInternalValue here and just compare the creation_time's directly.

Done.

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification_manager.h (right):

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_manager.h:25: static const unsigned
int kMaxNotificationPerApp = 5;
Done, kept it unsigned as size() is unsigned?
On 2011/11/17 23:40:14, Antony Sargent wrote:
> nit: We typically use int's instead of unsigned int's for constants, and we
> often just put constants inside the class instead of a namespace, unless there
> are a huge pile of them that deserve to be in their own separate file. 
> 
> So in the .h:
> 
> class AppNotificationManager ... {
>  public:
>   static const int kMaxNotificationPerApp;
> 
>   ...
> };
> 
> and then define it inside the .cc file
> 
> const int AppNotificationManager::kMaxNotificationPerApp = 5;

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
File chrome/browser/extensions/app_notification_test_util.cc (right):

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ap...
chrome/browser/extensions/app_notification_test_util.cc:32: true, count, guid,
extension_id, title, body);
On 2011/11/17 23:40:14, Antony Sargent wrote:
> does this still compile with the switch to Time? Maybe this should just be
> switched to Time::Now() ?

Done.

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_app_api.cc (right):

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_app_api.cc:21: const char kCreationKey[] =
"creation_timestamp_ms";
On 2011/11/17 23:40:14, Antony Sargent wrote:
> nit: remove this

Done.

http://codereview.chromium.org/8567018/diff/5005/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_app_api.cc:54: &creation_timestamp_ms));
On 2011/11/17 23:40:14, Antony Sargent wrote:
> nit: remove this block since we're just passing Time::Now() below. 

Done.

[asargent_no_longer_on_chrome, 2011-11-18 00:19:58]

> Done, kept it unsigned as size() is unsigned?

Ok, no problem.

[commit-bot: I haz the power, 2011-11-18 06:29:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/elvin@google.com/8567018/14005

[commit-bot: I haz the power, 2011-11-18 07:50:15]

Change committed as 110669