Hide popup custom notifications on non-primary displays

Hide popup custom notifications on non-primary displays

Currently custom notifications are supported to be shown only
on one display at the same time. So this CL disables popup custom notification on non-primary displays.

This CL affects only on Chrome OS.

BUG=b/37456756
BUG=715362
TEST=manual

Review-Url: https://codereview.chromium.org/2834773002
Cr-Commit-Position: refs/heads/master@{#467377}
Committed: https://chromium.googlesource.com/chromium/src/+/7bb91538766f932d182801724d0c0d1826e6dde6

[yoshiki, 2017-04-21 03:51:27]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 03:51:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 04:02:35]

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

[commit-bot: I haz the power, 2017-04-21 04:02:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2017-04-21 06:04:19]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 06:04:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 06:28:16]

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

[commit-bot: I haz the power, 2017-04-21 06:28:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2017-04-21 07:39:44]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 07:39:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 07:51:37]

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

[commit-bot: I haz the power, 2017-04-21 07:51:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2017-04-21 08:10:56]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 08:11:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoshiki, 2017-04-21 08:28:20]

Patchset #3 (id:40001) has been deleted

[yoshiki, 2017-04-21 08:28:23]

Patchset #2 (id:20001) has been deleted

[yoshiki, 2017-04-21 08:28:26]

Patchset #1 (id:1) has been deleted

[yoshiki, 2017-04-21 08:41:21]

Description was changed from

==========
wip

.


.

BUG=
==========

to

==========
Hide popup custom notifications on non-primary displays

Currently custom notifications are supported to be shown only
on one display at the same time. So this CL disables popup custom notification
on non-primary displays.

This CL affects only on Chrome OS.

BUG=b/37456756
TEST=manual
==========

[yoshiki, 2017-04-21 08:41:21]

yoshiki@chromium.org changed reviewers:
+ yhanada@chromium.org

[yoshiki, 2017-04-21 08:41:35]

Hanada-san, PTAL. Thanks.

[commit-bot: I haz the power, 2017-04-21 08:53:48]

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

[commit-bot: I haz the power, 2017-04-21 08:53:49]

Dry run: This issue passed the CQ dry run.

[yhanada, 2017-04-21 09:02:23]

lgtm with nit

https://codereview.chromium.org/2834773002/diff/60001/ui/message_center/views...
File ui/message_center/views/message_popup_collection.cc (right):

https://codereview.chromium.org/2834773002/diff/60001/ui/message_center/views...
ui/message_center/views/message_popup_collection.cc:254: LOG(ERROR) << "POPUP: "
<< (*iter)->id();
remove?

[yoshiki, 2017-04-21 09:49:40]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 09:49:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoshiki, 2017-04-21 09:58:05]

Thanks!

https://codereview.chromium.org/2834773002/diff/60001/ui/message_center/views...
File ui/message_center/views/message_popup_collection.cc (right):

https://codereview.chromium.org/2834773002/diff/60001/ui/message_center/views...
ui/message_center/views/message_popup_collection.cc:254: LOG(ERROR) << "POPUP: "
<< (*iter)->id();
On 2017/04/21 09:02:23, yhanada wrote:
> remove?

Done.

[commit-bot: I haz the power, 2017-04-21 10:34:32]

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

[commit-bot: I haz the power, 2017-04-21 10:34:33]

Dry run: This issue passed the CQ dry run.

[yoshiki, 2017-04-21 11:21:43]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2017-04-21 11:21:43]

The patchset sent to the CQ was uploaded after l-g-t-m from yhanada@chromium.org
Link to the patchset: https://codereview.chromium.org/2834773002/#ps80001
(title: "Remove debug output")

[commit-bot: I haz the power, 2017-04-21 11:21:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 11:30:10]

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

[commit-bot: I haz the power, 2017-04-21 11:30:10]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yoshiki, 2017-04-21 15:21:59]

yoshiki@chromium.org changed reviewers:
+ jennyz@chromium.org

[yoshiki, 2017-04-21 15:22:27]

Jenny, could you take a look at the change in ash/system? Thanks.

[yoshiki, 2017-04-24 19:40:13]

yoshiki@chromium.org changed reviewers:
+ stevenjb@chromium.org

[yoshiki, 2017-04-24 19:40:14]

Steven, could you take a look at ash/system/*?

[stevenjb, 2017-04-24 20:40:02]

Please create a chromium bug to track this.

This behavior seems very odd to me. 'NotificationType' seems to determine the
layout of the notification. Why does that affect which display(s) it should be
shown on?

https://codereview.chromium.org/2834773002/diff/80001/ui/message_center/views...
File ui/message_center/views/message_popup_collection.cc (right):

https://codereview.chromium.org/2834773002/diff/80001/ui/message_center/views...
ui/message_center/views/message_popup_collection.cc:177: bool isPrimaryDisplay =
is_primary_display

[yoshiki, 2017-04-26 01:12:08]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 01:12:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoshiki, 2017-04-26 01:20:44]

The CQ bit was checked by yoshiki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 01:21:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yoshiki, 2017-04-26 01:25:40]

Description was changed from

==========
Hide popup custom notifications on non-primary displays

Currently custom notifications are supported to be shown only
on one display at the same time. So this CL disables popup custom notification
on non-primary displays.

This CL affects only on Chrome OS.

BUG=b/37456756
TEST=manual
==========

to

==========
Hide popup custom notifications on non-primary displays

Currently custom notifications are supported to be shown only
on one display at the same time. So this CL disables popup custom notification
on non-primary displays.

This CL affects only on Chrome OS.

BUG=b/37456756
BUG=715362
TEST=manual
==========

[yoshiki, 2017-04-26 01:29:19]

On 2017/04/24 20:40:02, stevenjb wrote:
> Please create a chromium bug to track this.
Done

> This behavior seems very odd to me. 'NotificationType' seems to determine the
> layout of the notification. Why does that affect which display(s) it should be
> shown on?

Sorry for lacking words. This is a tentative fix. Currently, this is just
because custom notification doesn't support multiple displays. I modified the
comment and added TODO comment to fix it in near feature.

[yoshiki, 2017-04-26 01:29:29]

https://codereview.chromium.org/2834773002/diff/80001/ui/message_center/views...
File ui/message_center/views/message_popup_collection.cc (right):

https://codereview.chromium.org/2834773002/diff/80001/ui/message_center/views...
ui/message_center/views/message_popup_collection.cc:177: bool isPrimaryDisplay =
On 2017/04/24 20:40:02, stevenjb wrote:
> is_primary_display

Done.

[commit-bot: I haz the power, 2017-04-26 02:16:43]

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

[commit-bot: I haz the power, 2017-04-26 02:16:44]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-04-26 15:59:57]

stevenjb@chromium.org changed reviewers:
+ xiyuan@chromium.org

[stevenjb, 2017-04-26 15:59:58]

I'm still unclear why NOTIFICATION_TYPE_CUSTOM has issues on multiple displays.
The issue only mentions that ARC notifications are broken. Is that the only use
of NOTIFICATION_TYPE_CUSTOM? (And if so, should we rename that? If not, what
else will be impacted?).

+xiyuan@

[yoshiki, 2017-04-26 17:02:39]

On 2017/04/26 15:59:58, stevenjb wrote:
> I'm still unclear why NOTIFICATION_TYPE_CUSTOM has issues on multiple
displays.
> The issue only mentions that ARC notifications are broken. Is that the only
use
> of NOTIFICATION_TYPE_CUSTOM? (And if so, should we rename that? If not, what
> else will be impacted?).
> 
> +xiyuan@

NOTIFICATION_TYPE_CUSTOM is a notification which hosts a native view inside.
This is only used by ARC notifications as for now. In Chrome OS, native view can
have only one parent, in other words, multiple notifications can't share one
native view. But now, on multi-display environment, Chrome OS tries to show one
native view on all the popup on all the display. This is the cause of the issue.

In near feature, we have to add some logic to duplicate or share native view.
Until then, I want to disable popup notification on non-primary display.

[Peter Beverloo, 2017-04-26 17:07:43]

On 2017/04/26 17:02:39, yoshiki wrote:
> On 2017/04/26 15:59:58, stevenjb wrote:
> > I'm still unclear why NOTIFICATION_TYPE_CUSTOM has issues on multiple
> displays.
> > The issue only mentions that ARC notifications are broken. Is that the only
> use
> > of NOTIFICATION_TYPE_CUSTOM? (And if so, should we rename that? If not, what
> > else will be impacted?).
> > 
> > +xiyuan@
> 
> NOTIFICATION_TYPE_CUSTOM is a notification which hosts a native view inside.
> This is only used by ARC notifications as for now. In Chrome OS, native view
can
> have only one parent, in other words, multiple notifications can't share one
> native view. But now, on multi-display environment, Chrome OS tries to show
one
> native view on all the popup on all the display. This is the cause of the
issue.
> 
> In near feature, we have to add some logic to duplicate or share native view.
> Until then, I want to disable popup notification on non-primary display.

Are custom views used for *all* Android-originating notifications, or only those
specifying a custom layout?

[stevenjb, 2017-04-26 17:09:39]

Thanks for adding the issue and the explanation.
owner lgtm.

[xiyuan, 2017-04-26 17:11:42]

On 2017/04/26 17:02:39, yoshiki wrote:
> On 2017/04/26 15:59:58, stevenjb wrote:
> > I'm still unclear why NOTIFICATION_TYPE_CUSTOM has issues on multiple
> displays.
> > The issue only mentions that ARC notifications are broken. Is that the only
> use
> > of NOTIFICATION_TYPE_CUSTOM? (And if so, should we rename that? If not, what
> > else will be impacted?).
> > 
> > +xiyuan@
> 
> NOTIFICATION_TYPE_CUSTOM is a notification which hosts a native view inside.
> This is only used by ARC notifications as for now. In Chrome OS, native view
can
> have only one parent, in other words, multiple notifications can't share one
> native view. But now, on multi-display environment, Chrome OS tries to show
one
> native view on all the popup on all the display. This is the cause of the
issue.
> 
> In near feature, we have to add some logic to duplicate or share native view.
> Until then, I want to disable popup notification on non-primary display.

I've duped http://crbug.com/661723 there. Currently NOTIFICATION_TYPE_CUSTOM is
only used by ARC notifications. An ARC notification is backed by a notification
surface. Currently, we only create one such surface per notification. A surface
is an aura::Window (aka native view ATM) and could not be used on multiple
display. When there are multiple displays, only one display would get the real
surface that responses to mouse event etc. Other displays get a static snapshot,
which are confusing to users. The intention of the CL is to eliminate the
confusion.

[xiyuan, 2017-04-26 17:26:46]

On 2017/04/26 17:07:43, Peter Beverloo wrote:
> On 2017/04/26 17:02:39, yoshiki wrote:
> > On 2017/04/26 15:59:58, stevenjb wrote:
> > > I'm still unclear why NOTIFICATION_TYPE_CUSTOM has issues on multiple
> > displays.
> > > The issue only mentions that ARC notifications are broken. Is that the
only
> > use
> > > of NOTIFICATION_TYPE_CUSTOM? (And if so, should we rename that? If not,
what
> > > else will be impacted?).
> > > 
> > > +xiyuan@
> > 
> > NOTIFICATION_TYPE_CUSTOM is a notification which hosts a native view inside.
> > This is only used by ARC notifications as for now. In Chrome OS, native view
> can
> > have only one parent, in other words, multiple notifications can't share one
> > native view. But now, on multi-display environment, Chrome OS tries to show
> one
> > native view on all the popup on all the display. This is the cause of the
> issue.
> > 
> > In near feature, we have to add some logic to duplicate or share native
view.
> > Until then, I want to disable popup notification on non-primary display.
> 
> Are custom views used for *all* Android-originating notifications, or only
those
> specifying a custom layout?

Yes, all Android-originating notifications are using NOTIFICATION_TYPE_CUSTOM
now.

[yoshiki, 2017-04-26 17:48:49]

Thanks!

[yoshiki, 2017-04-26 17:48:54]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2017-04-26 17:48:55]

The patchset sent to the CQ was uploaded after l-g-t-m from yhanada@chromium.org
Link to the patchset: https://codereview.chromium.org/2834773002/#ps120001
(title: "Rewrote comment")

[commit-bot: I haz the power, 2017-04-26 17:49:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 17:54:46]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493228934770030,
"parent_rev": "bc9e95817ed4033644e3556920805ff14088c3e1", "commit_rev":
"7bb91538766f932d182801724d0c0d1826e6dde6"}

[commit-bot: I haz the power, 2017-04-26 17:56:16]

Description was changed from

==========
Hide popup custom notifications on non-primary displays

Currently custom notifications are supported to be shown only
on one display at the same time. So this CL disables popup custom notification
on non-primary displays.

This CL affects only on Chrome OS.

BUG=b/37456756
BUG=715362
TEST=manual
==========

to

==========
Hide popup custom notifications on non-primary displays

Currently custom notifications are supported to be shown only
on one display at the same time. So this CL disables popup custom notification
on non-primary displays.

This CL affects only on Chrome OS.

BUG=b/37456756
BUG=715362
TEST=manual

Review-Url: https://codereview.chromium.org/2834773002
Cr-Commit-Position: refs/heads/master@{#467377}
Committed:
https://chromium.googlesource.com/chromium/src/+/7bb91538766f932d182801724d0c...
==========

[commit-bot: I haz the power, 2017-04-26 17:56:17]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/7bb91538766f932d182801724d0c...