Remove close_on_deactivate=false for tray bubbles (2nd)

Remove close_on_deactivate=false for tray bubbles (2nd)

Actually we want to close those bubbles on deactivation (like, opening
settings page or opening the chat window for a notification). To deal
with the race condition of close-on-deactivate and button action,
a static delay is introduced.

The first attempt (r184572) has been reverted due to a crash bug.
It seems that the crash happens because of missing cleanup of system
notification bubbles.

BUG=179992, 177075
TEST=on goobuntu, open the systme tray, wait for the power notifications, then click 'settings' and see no crash.


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

[stevenjb, 2013-03-06 02:35:26]

https://codereview.chromium.org/12510005/diff/1/ash/system/tray/tray_bubble_w...
File ash/system/tray/tray_bubble_wrapper.cc (right):

https://codereview.chromium.org/12510005/diff/1/ash/system/tray/tray_bubble_w...
ash/system/tray/tray_bubble_wrapper.cc:54: base::Unretained(tray_),
base::Unretained(bubble_view_)));
Is there any possibility that tray_ or bubble_view_ could get destroyed before
HideBubbleWithView gets called? We may need to add a method in this class
instead that calls HideBubbleWithView(tray_, bubble_view), and add a
WeakPtrFactory to this class?

[Jun Mukai, 2013-03-06 04:36:57]

https://codereview.chromium.org/12510005/diff/1/ash/system/tray/tray_bubble_w...
File ash/system/tray/tray_bubble_wrapper.cc (right):

https://codereview.chromium.org/12510005/diff/1/ash/system/tray/tray_bubble_w...
ash/system/tray/tray_bubble_wrapper.cc:54: base::Unretained(tray_),
base::Unretained(bubble_view_)));
On 2013/03/06 02:35:26, stevenjb (chromium) wrote:
> Is there any possibility that tray_ or bubble_view_ could get destroyed before
> HideBubbleWithView gets called? We may need to add a method in this class
> instead that calls HideBubbleWithView(tray_, bubble_view), and add a
> WeakPtrFactory to this class?

Good catch. It's really unlikely but it may happen. I think it's still safe to
use bubble_view_ because it's an argument (and it is just only used for
comparison).  I took notes of this as the comment of tray_background_view.h

tray_ owns this and outlives, but probably it could also happen. Thus I put
WeakPtr to this class and added HideBubbleWithView method too.

[stevenjb, 2013-03-06 17:40:31]

LGTM

[jennyz, 2013-03-06 18:04:04]

lgtm with nits.

https://codereview.chromium.org/12510005/diff/5001/ash/system/tray/tray_backg...
File ash/system/tray/tray_background_view.h (right):

https://codereview.chromium.org/12510005/diff/5001/ash/system/tray/tray_backg...
ash/system/tray/tray_background_view.h:91: // any methods are called to
|bubble_view| without any checks.
Can you be clearer about "any methods are called to |bubble_view| without any
checks"? Do you mean |bubble_view| may be already destroyed, so any methods
called to |bubble_view| should be made with check?

[Jun Mukai, 2013-03-07 00:26:42]

https://codereview.chromium.org/12510005/diff/5001/ash/system/tray/tray_backg...
File ash/system/tray/tray_background_view.h (right):

https://codereview.chromium.org/12510005/diff/5001/ash/system/tray/tray_backg...
ash/system/tray/tray_background_view.h:91: // any methods are called to
|bubble_view| without any checks.
On 2013/03/06 18:04:04, jennyz wrote:
> Can you be clearer about "any methods are called to |bubble_view| without any
> checks"? Do you mean |bubble_view| may be already destroyed, so any methods
> called to |bubble_view| should be made with check?

removed the sentence of 'any methods...', I don't have enough literal sense to
write it, and 'may be already destroyed' would be good enough.

[commit-bot: I haz the power, 2013-03-07 01:24:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/12510005/12001

[commit-bot: I haz the power, 2013-03-07 04:13:05]

Change committed as 186612