Add CloseAllByProfile to NotificationUIManager and call before destroyng a Profile

Add CloseAllByProfile to NotificationUIManager and call before destroyng a Profile


BUG=160657
For chrome/browser/ui/views/notifications/
TBR=sky@chromium.org

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

[stevenjb, 2012-11-28 23:38:30]

This fixes a crash bug (that's probably been around for a while, just not a
common use case).

[oshima, 2012-11-29 02:21:50]

lgtm with nits.

https://codereview.chromium.org/11414215/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/notification_ui_manager_impl.cc (right):

https://codereview.chromium.org/11414215/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/notification_ui_manager_impl.cc:129: bool removed =
false;
is it ok to say
bool removed = balloon_collection_->RemoveByProfile(profile);

and just do
return removed?

https://codereview.chromium.org/11414215/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/notification_ui_manager_impl.cc:131: for (iter =
show_queue_.begin(); iter != show_queue_.end();) {
Move the iter def into for, or if you want to keep it outside, then the
following would be a little bit better.

NotificationDeque::iterator iter = show_queue.begin();
while (iter != show_queue_.end()) {
}

[stevenjb, 2012-11-29 02:52:24]

PTAL

https://codereview.chromium.org/11414215/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/notification_ui_manager_impl.cc (right):

https://codereview.chromium.org/11414215/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/notification_ui_manager_impl.cc:129: bool removed =
false;
On 2012/11/29 02:21:50, oshima wrote:
> is it ok to say
> bool removed = balloon_collection_->RemoveByProfile(profile);
> 
> and just do
> return removed?

No, we set it in the for loop also. Should be clearer now.

https://codereview.chromium.org/11414215/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/notification_ui_manager_impl.cc:131: for (iter =
show_queue_.begin(); iter != show_queue_.end();) {
On 2012/11/29 02:21:50, oshima wrote:
> Move the iter def into for, or if you want to keep it outside, then the
> following would be a little bit better.
> 
> NotificationDeque::iterator iter = show_queue.begin();
> while (iter != show_queue_.end()) {
> }
I didn't write these iterators, I just copy/pasted them.
Now you've convinced me to fix them all :P
(But I found an pointless copy in the process, so maybe worth it)

https://codereview.chromium.org/11414215/diff/5001/chrome/browser/notificatio...
File chrome/browser/notifications/notification_ui_manager_impl.cc (right):

https://codereview.chromium.org/11414215/diff/5001/chrome/browser/notificatio...
chrome/browser/notifications/notification_ui_manager_impl.cc:209: const
BalloonCollection::Balloons& balloons =
Note: now a reference

https://codereview.chromium.org/11414215/diff/5001/chrome/browser/notificatio...
chrome/browser/notifications/notification_ui_manager_impl.cc:211: for
(BalloonCollection::Balloons::const_iterator iter = balloons.begin();
Note: now a const_iterator. This worked before only because were were copying
pointers and not modifying |balloons|.

[oshima, 2012-11-29 16:57:47]

still lgtm

https://codereview.chromium.org/11414215/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/notification_ui_manager_impl.cc (right):

https://codereview.chromium.org/11414215/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/notification_ui_manager_impl.cc:129: bool removed =
false;
On 2012/11/29 02:52:25, stevenjb (chromium) wrote:
> On 2012/11/29 02:21:50, oshima wrote:
> > is it ok to say
> > bool removed = balloon_collection_->RemoveByProfile(profile);
> > 
> > and just do
> > return removed?
> 
> No, we set it in the for loop also. Should be clearer now.
> 

I think you can because the loop changes the value only from
false to true, unless there is order dependency. Not big deal and
I'll leave it to you.

[stevenjb, 2012-11-29 17:10:15]

On 2012/11/29 16:57:47, oshima wrote:
> still lgtm
> 
>
https://codereview.chromium.org/11414215/diff/1/chrome/browser/notifications/...
> File chrome/browser/notifications/notification_ui_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/11414215/diff/1/chrome/browser/notifications/...
> chrome/browser/notifications/notification_ui_manager_impl.cc:129: bool removed
=
> false;
> On 2012/11/29 02:52:25, stevenjb (chromium) wrote:
> > On 2012/11/29 02:21:50, oshima wrote:
> > > is it ok to say
> > > bool removed = balloon_collection_->RemoveByProfile(profile);
> > > 
> > > and just do
> > > return removed?
> > 
> > No, we set it in the for loop also. Should be clearer now.
> > 
> 
> I think you can because the loop changes the value only from
> false to true, unless there is order dependency. Not big deal and
> I'll leave it to you.

Oh, I misunderstood your suggestion. Yeah, that'd be a little cleaner, I don't
think there are any ordering dependencies, but I'll keep the existing pattern
for now.

[commit-bot: I haz the power, 2012-11-29 17:10:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11414215/5001

[commit-bot: I haz the power, 2012-11-29 17:10:59]

Presubmit check for 11414215-5001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/notifications/notification_ui_manager_mac.mm:237
    return builtin_manager_->GetQueuedNotificationsForTesting(notifications);

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/views

Presubmit checks took 1.6s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2012-11-29 18:01:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11414215/5001

[commit-bot: I haz the power, 2012-11-29 18:39:54]

Retried try job too often on linux_chromeos for step(s) unit_tests

[commit-bot: I haz the power, 2012-11-29 19:29:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11414215/15001

[commit-bot: I haz the power, 2012-11-29 20:52:26]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-11-30 00:09:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11414215/2006

[commit-bot: I haz the power, 2012-11-30 01:54:04]

Retried try job too often on linux_rel for step(s) nacl_integration