When an extension is uninstalled, close all desktop notifications from that e...

When an extension is uninstalled, close all desktop notifications from that extension.

This change also refactors the balloon collection code to remove duplication between chrome and chromeos.

Removes some gross removal code which was using fake notifications just to get the right ID.

BUG=58266
TEST=open notifications from extension, uninstall extensions


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

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

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

[John Gregg, 2010-11-10 21:18:54]

for review:
 oshima for chromeos
 atwilson for everything else

Thanks!

[Andrew T Wilson (Slow), 2010-11-11 01:07:44]

Looks like a really solid refactoring - I had a few concerns which I noted
below. Also, I notice that notification_types.h declares two notifications:
EXTENSION_UNLOADED and EXTENSION_UNLOADED_DISABLED. I think that
EXTENSION_UNLOADED (which you are listening to) is sent even when an extension
is disabled, but I'm double-checking with erikkay/aa.

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/chromeos/not...
File chrome/browser/chromeos/notifications/balloon_collection_impl.cc (right):

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/chromeos/not...
chrome/browser/chromeos/notifications/balloon_collection_impl.cc:126:
base_.Remove(source);
Previously this routine was freeing |source| - who frees it now? If it's freed
by base_.Remove(), it seems bad to then pass it to notification_ui_->Remove().

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
File chrome/browser/notifications/balloon_collection.h (right):

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/balloon_collection.h:49: // Removes any balloons
that have this notification id.
Should we document what the return values of RemoveById/RemoveBySourceOrigin()
mean?

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
File chrome/browser/notifications/balloon_collection_base.cc (right):

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.cc:71: return *iter;
I'm not certain whether { should be omitted in this case (since the if statement
itself is multi-line). You seem to have a better grasp of the intricacies of the
style guide than I do, but figured I'd point this out in case it's an oversight.

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
File chrome/browser/notifications/balloon_collection_base.h (right):

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.h:38: // calls
CloseByScript on it.
Does this remove it from the array (I'm guessing so) - just curious about why we
explicitly state this below in the documentation for CloseAllBySourceOrigin()
but not here? Also, should document the return value.

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.h:43: virtual bool
CloseAllBySourceOrigin(const GURL& source_origin);
Should document the return value.

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.h:47: // Returns the
balloon matching the given notification.
Should we document that FindBalloon() returns NULL if not found?

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.h:55: // must be managed by
the caller of this class.
It's not clear to me how to reconcile this comment with the comment above at
Add(). Who owns these pointers? And what's the "caller of this class"?

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service.cc:310: } else if
(NotificationType::EXTENSION_UNLOADED) {
Should this be "type == NotificationType::PREF_CHANGED"?

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service.cc:315:
ui_manager_->CancelAllBySourceOrigin(extension->url());
Does this work for applications, which have web_extents? Maybe we don't want to
close notifications when you uninstall apps, but uninstalling an app may remove
permission for notifications for that domain which is something to keep in mind.

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager.cc (right):

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager.cc:88: searching = false;
This seems like an odd way to implement this. I thought that deque.erase()
returned a valid iterator pointing to the next element - I'm wondering if you
could replace the entire while(searching) loop with:

for (iter = begin(); iter != end(); ) {
  if ((*iter)->notification().origin_url() == source) {
    iter = show_queue_.erase(iter);
    removed = true;
  } else {
    ++iter;
  }
}

That way you don't have to start back at the beginning of the loop again and the
logic is simpler?

[tfarina, 2010-11-11 12:56:57]

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/chromeos/not...
File chrome/browser/chromeos/notifications/balloon_view.cc (right):

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/chromeos/not...
chrome/browser/chromeos/notifications/balloon_view.cc:303: return
balloon_->notification().notification_id() == notification.notification_id();
nit: line > 80 characters

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/notification...
File chrome/browser/notifications/balloon_collection_base.h (right):

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.h:14: #include
"base/stl_util-inl.h"
nit: do you need this include here? Can't it be included only in the .cc files?

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.h:21: // still pure virtual
which need to be implemented by the particular
That is not true. Pure virtual means "virtual ... = 0;" but there is not such
method like this in this class.

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.h:51: int count() const {
return balloons_.size(); }
nit: static_cast<int>

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager.h (right):

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager.h:47: // Removes any
notifications matching the supplied ID.
nit: maybe: the given |notification_id|?

[John Gregg, 2010-11-11 18:13:45]

Comments addressed, PTAL

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/chromeos/not...
File chrome/browser/chromeos/notifications/balloon_collection_impl.cc (right):

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/chromeos/not...
chrome/browser/chromeos/notifications/balloon_collection_impl.cc:126:
base_.Remove(source);
On 2010/11/11 01:07:44, Andrew T Wilson wrote:
> Previously this routine was freeing |source| - who frees it now? If it's freed
> by base_.Remove(), it seems bad to then pass it to notification_ui_->Remove().

It is freed by base_.Remove(), so I reordered them.

Turns out notification_ui_->Remove() doesn't deference the pointer, just compare
it, but yeah still dangerous.

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
File chrome/browser/notifications/balloon_collection.h (right):

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/balloon_collection.h:49: // Removes any balloons
that have this notification id.
On 2010/11/11 01:07:44, Andrew T Wilson wrote:
> Should we document what the return values of RemoveById/RemoveBySourceOrigin()
> mean?

Done.

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
File chrome/browser/notifications/balloon_collection_base.cc (right):

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.cc:71: return *iter;
On 2010/11/11 01:07:44, Andrew T Wilson wrote:
> I'm not certain whether { should be omitted in this case (since the if
statement
> itself is multi-line). You seem to have a better grasp of the intricacies of
the
> style guide than I do, but figured I'd point this out in case it's an
oversight.

Strangely the style guide doesn't actually spell out the rules, so I go with the
convention to use braces if the if condition spans multiple lines, cause it just
looks weird without them.

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
File chrome/browser/notifications/balloon_collection_base.h (right):

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.h:38: // calls
CloseByScript on it.
On 2010/11/11 01:07:44, Andrew T Wilson wrote:
> Does this remove it from the array (I'm guessing so) - just curious about why
we
> explicitly state this below in the documentation for CloseAllBySourceOrigin()
> but not here? Also, should document the return value.

No, this comment is precise (just calls CloseByScript), the other one is wrong
and I fixed it.

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.h:43: virtual bool
CloseAllBySourceOrigin(const GURL& source_origin);
On 2010/11/11 01:07:44, Andrew T Wilson wrote:
> Should document the return value.

Done.

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.h:47: // Returns the
balloon matching the given notification.
On 2010/11/11 01:07:44, Andrew T Wilson wrote:
> Should we document that FindBalloon() returns NULL if not found?

Done.

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.h:55: // must be managed by
the caller of this class.
On 2010/11/11 01:07:44, Andrew T Wilson wrote:
> It's not clear to me how to reconcile this comment with the comment above at
> Add(). Who owns these pointers? And what's the "caller of this class"?

Yeah, stale comment.  Fixed.

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service.cc:310: } else if
(NotificationType::EXTENSION_UNLOADED) {
On 2010/11/11 01:07:44, Andrew T Wilson wrote:
> Should this be "type == NotificationType::PREF_CHANGED"?

yikes, good catch.  kinda funny that when there are only two types being
observed this actually works :)

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/desktop_notification_service.cc:315:
ui_manager_->CancelAllBySourceOrigin(extension->url());
On 2010/11/11 01:07:44, Andrew T Wilson wrote:
> Does this work for applications, which have web_extents? Maybe we don't want
to
> close notifications when you uninstall apps, but uninstalling an app may
remove
> permission for notifications for that domain which is something to keep in
mind.

I don't think that would be necessary to fix the bug in question, but I can
follow up on it.

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager.cc (right):

http://codereview.chromium.org/4635007/diff/69001/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager.cc:88: searching = false;
On 2010/11/11 01:07:44, Andrew T Wilson wrote:
> This seems like an odd way to implement this. I thought that deque.erase()
> returned a valid iterator pointing to the next element - I'm wondering if you
> could replace the entire while(searching) loop with:
> 
> for (iter = begin(); iter != end(); ) {
>   if ((*iter)->notification().origin_url() == source) {
>     iter = show_queue_.erase(iter);
>     removed = true;
>   } else {
>     ++iter;
>   }
> }
> 
> That way you don't have to start back at the beginning of the loop again and
the
> logic is simpler?

thanks, i tend to play it safe when modifying iterators but your code works and
is much tighter.

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/chromeos/not...
File chrome/browser/chromeos/notifications/balloon_view.cc (right):

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/chromeos/not...
chrome/browser/chromeos/notifications/balloon_view.cc:303: return
balloon_->notification().notification_id() == notification.notification_id();
On 2010/11/11 12:56:58, tfarina wrote:
> nit: line > 80 characters 

Done.

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/notification...
File chrome/browser/notifications/balloon_collection_base.h (right):

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.h:14: #include
"base/stl_util-inl.h"
On 2010/11/11 12:56:58, tfarina wrote:
> nit: do you need this include here? Can't it be included only in the .cc
files?

Done.

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.h:21: // still pure virtual
which need to be implemented by the particular
On 2010/11/11 12:56:58, tfarina wrote:
> That is not true. Pure virtual means "virtual ... = 0;" but there is not such
> method like this in this class.

yep, stale comment all around from when i was inheriting instead of composing.
fixed.

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_base.h:51: int count() const {
return balloons_.size(); }
On 2010/11/11 12:56:58, tfarina wrote:
> nit: static_cast<int>

Done.

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager.h (right):

http://codereview.chromium.org/4635007/diff/76002/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager.h:47: // Removes any
notifications matching the supplied ID.
On 2010/11/11 12:56:58, tfarina wrote:
> nit: maybe: the given |notification_id|?

Thanks, but I think it's clear as-is.

[John Gregg, 2010-11-11 18:14:47]

> Also, I notice that notification_types.h declares two notifications:
> EXTENSION_UNLOADED and EXTENSION_UNLOADED_DISABLED. I think that
> EXTENSION_UNLOADED (which you are listening to) is sent even when an >
extension is disabled, but I'm double-checking with erikkay/aa.

On the bug, aa suggested listening for EXTENSION_UNLOADED as the fix and didn't
mention the other one, so that's what I went with.

[oshima, 2010-11-11 19:00:16]

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/chromeos/no...
File chrome/browser/chromeos/notifications/desktop_notifications_unittest.cc
(right):

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/chromeos/no...
chrome/browser/chromeos/notifications/desktop_notifications_unittest.cc:41: new
LoggingNotificationProxy(notification.notification_id()));
who deletes this object now?

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
File chrome/browser/notifications/balloon_collection_base.h (right):

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
chrome/browser/notifications/balloon_collection_base.h:14: #include
"chrome/browser/notifications/balloon.h"
looks like forward decl is enough?

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
File chrome/browser/notifications/desktop_notification_service.h (right):

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
chrome/browser/notifications/desktop_notification_service.h:14: #include
"chrome/browser/notifications/notification.h"
do you need this?

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
chrome/browser/notifications/desktop_notification_service.h:19: #include
"chrome/common/notification_service.h"
do you need this?

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
File chrome/browser/notifications/notification_test_util.h (right):

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
chrome/browser/notifications/notification_test_util.h:67: };
DISALLOW_COPY_AND_ASSIGN

[Andrew T Wilson (Slow), 2010-11-11 21:09:49]

LGTM with minor nits.

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
File chrome/browser/notifications/balloon_collection.h (right):

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
chrome/browser/notifications/balloon_collection.h:54: // true if anything was
remove.
nit: was remove -> was removed

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
File chrome/browser/notifications/notification_ui_manager.h (right):

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
chrome/browser/notifications/notification_ui_manager.h:47: // Removes any
notifications matching the supplied ID.
document the return value.

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
chrome/browser/notifications/notification_ui_manager.h:52: virtual bool
CancelAllBySourceOrigin(const GURL& source_origin);
document the return value. BTW, I don't think anyone actually cares about the
return value, but it's fine to have it regardless if you think it might be
useful some day.

[John Gregg, 2010-11-11 23:17:46]

all changes made.  the header file inclusions required me to bring in a few
other files that were depending on transitive inclusions.

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/chromeos/no...
File chrome/browser/chromeos/notifications/desktop_notifications_unittest.cc
(right):

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/chromeos/no...
chrome/browser/chromeos/notifications/desktop_notifications_unittest.cc:41: new
LoggingNotificationProxy(notification.notification_id()));
On 2010/11/11 19:00:17, oshima wrote:
> who deletes this object now?

It's still a scoped_refptr in Notification.  Now it has no other references, so
when Notification is destroyed it is destroyed.

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
File chrome/browser/notifications/balloon_collection.h (right):

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
chrome/browser/notifications/balloon_collection.h:54: // true if anything was
remove.
On 2010/11/11 21:09:50, Andrew T Wilson wrote:
> nit: was remove -> was removed

Done.

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
File chrome/browser/notifications/balloon_collection_base.h (right):

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
chrome/browser/notifications/balloon_collection_base.h:14: #include
"chrome/browser/notifications/balloon.h"
On 2010/11/11 19:00:17, oshima wrote:
> looks like forward decl is enough?

Done.

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
File chrome/browser/notifications/notification_test_util.h (right):

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
chrome/browser/notifications/notification_test_util.h:67: };
On 2010/11/11 19:00:17, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
File chrome/browser/notifications/notification_ui_manager.h (right):

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
chrome/browser/notifications/notification_ui_manager.h:47: // Removes any
notifications matching the supplied ID.
On 2010/11/11 21:09:50, Andrew T Wilson wrote:
> document the return value.

Done.

http://codereview.chromium.org/4635007/diff/112002/chrome/browser/notificatio...
chrome/browser/notifications/notification_ui_manager.h:52: virtual bool
CancelAllBySourceOrigin(const GURL& source_origin);
On 2010/11/11 21:09:50, Andrew T Wilson wrote:
> document the return value. BTW, I don't think anyone actually cares about the
> return value, but it's fine to have it regardless if you think it might be
> useful some day.

Done.

[oshima, 2010-11-12 00:01:25]

LGTM

[evanm, 2010-11-12 03:02:19]

http://build.chromium.org/p/chromium/builders/Interactive%20Tests%20(dbg)/bui...

TestInfoBarsCloseOnNewTheme started failing on this change and has failed ever
since

[tfarina, 2010-11-12 11:27:49]

My nits LGTM