Allow tray_event_filter to handle popup notifications.

Allow tray_event_filter to handle popup notifications from message center.

BUG=369617
Committed: https://crrev.com/3510f83d9d833bf2bce601421dadbae9bd247278
Cr-Commit-Position: refs/heads/master@{#301145}

[xdai1, 2014-10-20 20:11:36]

xdai@chromium.org changed reviewers:
+ jennyz@chromium.org, mukai@chromium.org, stevenjb@chromium.org

[stevenjb, 2014-10-21 17:14:58]

stevenjb@chromium.org changed reviewers:
- stevenjb@chromium.org

[stevenjb, 2014-10-21 17:14:59]

This looks OK to me, but I am going to defer to mukai@

[jennyz, 2014-10-21 17:21:35]

https://codereview.chromium.org/661423004/diff/1/ash/system/tray/tray_event_f...
File ash/system/tray/tray_event_filter.cc (right):

https://codereview.chromium.org/661423004/diff/1/ash/system/tray/tray_event_f...
ash/system/tray/tray_event_filter.cc:65: // Don't process events that occurred
inside a popup notification.
How about make it clear this is a message center pop up notification, since we
still have the legacy system tray notification live in the code.

[Jun Mukai, 2014-10-21 17:32:31]

looks good, but would be better to clean up the code at the same time.

https://codereview.chromium.org/661423004/diff/1/ash/system/tray/tray_event_f...
File ash/system/tray/tray_event_filter.cc (right):

https://codereview.chromium.org/661423004/diff/1/ash/system/tray/tray_event_f...
ash/system/tray/tray_event_filter.cc:65: // Don't process events that occurred
inside a popup notification.
To be strict, this is not true.  StatusContainer also contains the status area
widget (the button to invoke system tray bubble, and the button for the message
center bubble).

Also, the following loop over |wrappers_| may not be necessary anymore. IIRC the
StatusContainer also contains the system tray bubbles and message center bubble.

[xdai1, 2014-10-21 21:13:45]

Please review the cl.

https://codereview.chromium.org/661423004/diff/1/ash/system/tray/tray_event_f...
File ash/system/tray/tray_event_filter.cc (right):

https://codereview.chromium.org/661423004/diff/1/ash/system/tray/tray_event_f...
ash/system/tray/tray_event_filter.cc:65: // Don't process events that occurred
inside a popup notification.
On 2014/10/21 17:21:34, jennyz wrote:
> How about make it clear this is a message center pop up notification, since we
> still have the legacy system tray notification live in the code.

Done.

https://codereview.chromium.org/661423004/diff/1/ash/system/tray/tray_event_f...
ash/system/tray/tray_event_filter.cc:65: // Don't process events that occurred
inside a popup notification.
On 2014/10/21 17:32:31, Jun Mukai wrote:
> To be strict, this is not true.  StatusContainer also contains the status area
> widget (the button to invoke system tray bubble, and the button for the
message
> center bubble).
Done.
> 
> Also, the following loop over |wrappers_| may not be necessary anymore. IIRC
the
> StatusContainer also contains the system tray bubbles and message center
bubble.
The loop for |wrappers_| is still necessary. StatusContainer doesn't contain the
system tray bubbles and the legacy notification bubbles (e.g. the power
notification bubbles). If the loop is removed, the clicks on the system tray
items and on the legacy notification bubbles will close the system tray, which
may not what we want.

[Jun Mukai, 2014-10-21 22:12:39]

https://codereview.chromium.org/661423004/diff/20001/ash/system/tray/tray_eve...
File ash/system/tray/tray_event_filter.cc (right):

https://codereview.chromium.org/661423004/diff/20001/ash/system/tray/tray_eve...
ash/system/tray/tray_event_filter.cc:77: iter != wrappers_.end(); ++iter) {
I believe you can remove this for loop now, since all of |wrappers_| belong to
StatusContainer.  Could you try that?

[xdai1, 2014-10-21 22:45:23]

On 2014/10/21 22:12:39, Jun Mukai wrote:
>
https://codereview.chromium.org/661423004/diff/20001/ash/system/tray/tray_eve...
> File ash/system/tray/tray_event_filter.cc (right):
> 
>
https://codereview.chromium.org/661423004/diff/20001/ash/system/tray/tray_eve...
> ash/system/tray/tray_event_filter.cc:77: iter != wrappers_.end(); ++iter) {
> I believe you can remove this for loop now, since all of |wrappers_| belong to
> StatusContainer.  Could you try that?

Yes. After removing the loop over |wrappers_|, the clicks on the system tray
items and on the legacy notification bubbles will close the system tray.

[Jun Mukai, 2014-10-21 22:56:48]

lgtm -- but I'm not an owner of ash/system. Please wait for owner's approval :)

On 2014/10/21 22:45:23, xdai1 wrote:
> On 2014/10/21 22:12:39, Jun Mukai wrote:
> >
>
https://codereview.chromium.org/661423004/diff/20001/ash/system/tray/tray_eve...
> > File ash/system/tray/tray_event_filter.cc (right):
> > 
> >
>
https://codereview.chromium.org/661423004/diff/20001/ash/system/tray/tray_eve...
> > ash/system/tray/tray_event_filter.cc:77: iter != wrappers_.end(); ++iter) {
> > I believe you can remove this for loop now, since all of |wrappers_| belong
to
> > StatusContainer.  Could you try that?
> 
> Yes. After removing the loop over |wrappers_|, the clicks on the system tray
> items and on the legacy notification bubbles will close the system tray.

Okay, so I'm wrong.

[jennyz, 2014-10-21 23:12:13]

lgtm

[xdai1, 2014-10-23 21:02:39]

The CQ bit was checked by xdai@chromium.org

[commit-bot: I haz the power, 2014-10-23 21:04:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661423004/20001

[commit-bot: I haz the power, 2014-10-23 21:22:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-23 21:22:35]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[jennyz, 2014-10-24 17:10:40]

The CQ bit was checked by jennyz@chromium.org

[commit-bot: I haz the power, 2014-10-24 17:12:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661423004/20001

[xdai1, 2014-10-24 17:15:20]

The CQ bit was checked by xdai@chromium.org

[commit-bot: I haz the power, 2014-10-24 17:15:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661423004/20001

[commit-bot: I haz the power, 2014-10-24 18:01:08]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-24 18:01:54]

Patchset 2 (id:??) landed as
https://crrev.com/3510f83d9d833bf2bce601421dadbae9bd247278
Cr-Commit-Position: refs/heads/master@{#301145}