arc: Show custom notification via notification surface

arc: Show custom notification via notification surface

- ArcCustomotificationItem to use notification surface to create Chrome
  notification;
- ArcCustomNotificationView to host and show the notification surface;
- Add a use_custom_notification property to ArcNotificationData;
- ArcNotificationManager choose ArcNotificationItem
  or ArcCustomNotificationItem based on that property;

BUG=b/25816173

Committed: https://crrev.com/241469472d4bef36af0641d428312fd3ab73377c
Cr-Commit-Position: refs/heads/master@{#401802}

[xiyuan, 2016-06-14 17:01:01]

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

[xiyuan, 2016-06-14 17:01:01]

The dependent CLs on Android side has not landed but I would like to start the
review earlier. :)

[yoshiki, 2016-06-15 11:47:20]

Thank you for doing this!

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_item.cc (right):

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_item.cc:287: return;
Please call AddToMessageCenter(). At least, we need to process |newer_data_|.

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_item.cc:337: return;
Ditto for calling AddTestMessageCenter().

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_item.h (right):

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_item.h:23: class ArcNotificationItem :
public ArcNotificationSurfaceCollection::Observer {
Most of ArcNotificationItem code is not shared with custom notification. Could
you create a new class like ArcCurstomNotificationItem which inherits
ArcNotificationItem (or new base class like ArcNotificationItemBase)?

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_surface_collection.cc (right):

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_surface_collection.cc:15:
ArcNotificationSurfaceCollection::ArcNotificationSurfaceCollection() {
Can we ensure that no notification surface is added before this constructor?

[xiyuan, 2016-06-15 19:32:44]

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_item.cc (right):

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_item.cc:287: return;
On 2016/06/15 11:47:20, yoshiki wrote:
> Please call AddToMessageCenter(). At least, we need to process |newer_data_|.

AddToMessageCenter() is called once the surface is available. The call is
deferred because there is nothing to show before surface is created.

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_item.cc:337: return;
On 2016/06/15 11:47:20, yoshiki wrote:
> Ditto for calling AddTestMessageCenter().

AddToMessageCenter() is called at the end. The condition above basically says:
if there is not pending |notification_| to add, or the created surface is not
ours. And we should not AddToMessageCenter() when that is true.

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_item.h (right):

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_item.h:23: class ArcNotificationItem :
public ArcNotificationSurfaceCollection::Observer {
On 2016/06/15 11:47:20, yoshiki wrote:
> Most of ArcNotificationItem code is not shared with custom notification. Could
> you create a new class like ArcCurstomNotificationItem which inherits
> ArcNotificationItem (or new base class like ArcNotificationItemBase)?

Done.

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_surface_collection.cc (right):

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_surface_collection.cc:15:
ArcNotificationSurfaceCollection::ArcNotificationSurfaceCollection() {
On 2016/06/15 11:47:20, yoshiki wrote:
> Can we ensure that no notification surface is added before this constructor?

This is created with ArcNotificationManager when we start ARC. There should be
no notification surfaces created before it.

But I get your point. I will make it more explicit by moving this class into exo
and make its lifetime the same as the exo system to ease the concern.

[yoshiki, 2016-06-16 02:44:50]

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_item.cc (right):

https://codereview.chromium.org/2066853002/diff/20001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_item.cc:287: return;
On 2016/06/15 19:32:44, xiyuan wrote:
> On 2016/06/15 11:47:20, yoshiki wrote:
> > Please call AddToMessageCenter(). At least, we need to process
|newer_data_|.
> 
> AddToMessageCenter() is called once the surface is available. The call is
> deferred because there is nothing to show before surface is created.

I got it. Thank you for explanation.

https://codereview.chromium.org/2066853002/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_manager.cc (right):

https://codereview.chromium.org/2066853002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_manager.cc:17: constexpr bool
use_custom_notification = true;
We use custom notification only for non-standard notification (+ standard music
controller notification). We need to pass the flag from Android via mojo IPC,
instead of static flag.

Or, perhaps, has the UX design plan been changed?

[xiyuan, 2016-06-20 23:02:54]

Description was changed from

==========
arc: Show custom notification via notification surface

- ArcNotificationSurfaceCollection to keep track of notification
  surfaces;
- ArcNotificationItem to use notification surface to create Chrome
  notification;
- ArcCustomNotificationView to host and show the notification surface;

BUG=b/25816173
==========

to

==========
arc: Show custom notification via notification surface

- ArcCustomotificationItem to use notification surface to create Chrome
  notification;
- ArcCustomNotificationView to host and show the notification surface;
- Add a use_custom_notification property to ArcNotificationData;
- ArcNotificationManager choose ArcNotificationItem
  or ArcCustomNotificationItem based on that property;

BUG=b/25816173
==========

[xiyuan, 2016-06-20 23:04:58]

https://codereview.chromium.org/2066853002/diff/40001/ui/arc/notification/arc...
File ui/arc/notification/arc_notification_manager.cc (right):

https://codereview.chromium.org/2066853002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_notification_manager.cc:17: constexpr bool
use_custom_notification = true;
On 2016/06/16 02:44:50, yoshiki wrote:
> We use custom notification only for non-standard notification (+ standard
music
> controller notification). We need to pass the flag from Android via mojo IPC,
> instead of static flag.
> 
> Or, perhaps, has the UX design plan been changed?

Added a use_custom_notification field to notifications.mojom and use it to
choose whether to use custom notifications.

We can revisit this of whether we want to use it for all after the initial CL is
in.

[yoshiki, 2016-06-21 04:01:07]

LGTM with nit. Thanks!

https://codereview.chromium.org/2066853002/diff/60001/components/arc/common/n...
File components/arc/common/notifications.mojom (right):

https://codereview.chromium.org/2066853002/diff/60001/components/arc/common/n...
components/arc/common/notifications.mojom:74: bool use_custom_notification;
nit: please add a comment that this flag is valid only on creation (this flag on
update is ignored).

[xiyuan, 2016-06-21 14:38:43]

https://codereview.chromium.org/2066853002/diff/60001/components/arc/common/n...
File components/arc/common/notifications.mojom (right):

https://codereview.chromium.org/2066853002/diff/60001/components/arc/common/n...
components/arc/common/notifications.mojom:74: bool use_custom_notification;
On 2016/06/21 04:01:07, yoshiki wrote:
> nit: please add a comment that this flag is valid only on creation (this flag
on
> update is ignored).

Done.

[xiyuan, 2016-06-21 14:48:17]

xiyuan@chromium.org changed reviewers:
+ dcheng@chromium.org, elijahtaylor@chromium.org, reveman@chromium.org,
thakis@chromium.org

[xiyuan, 2016-06-21 14:48:17]

reveman@, fyi and review how notification surface is used:
  chrome/browser/chrome_browser_main_extra_parts_exo.cc
  ui/arc/notification/arc_custom_notification_item.cc
  ui/arc/notification/arc_custom_notification_view.cc

elijahtaylor@ for owner approval of:
  ui/arc/BUILD.gn 
  ui/arc/arc.gyp

thakis@, for owner approval of:
  chrome/browser/chrome_browser_main_extra_parts_exo.cc

dcheng@, for owner approval of:
  components/arc/common/notifications.mojom

Thanks.

[Luis Héctor Chávez, 2016-06-21 16:30:46]

lhchavez@chromium.org changed reviewers:
+ lhchavez@chromium.org

[Luis Héctor Chávez, 2016-06-21 16:30:47]

ui/arc/ lgtm with a nit.

Thanks for sorting the file names, btw :)

https://codereview.chromium.org/2066853002/diff/80001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_view.h (right):

https://codereview.chromium.org/2066853002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.h:29: exo::NotificationSurface*
surface_;
nit: can this be |exo::NotificationSurface* const surface_|? Or even better,
|const exo::NotificationSurface* const surface_|? The second would require you
changing window() to be const, but seems doable.

[Nico, 2016-06-21 18:00:29]

https://codereview.chromium.org/2066853002/diff/80001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main_extra_parts_exo.cc (right):

https://codereview.chromium.org/2066853002/diff/80001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main_extra_parts_exo.cc:129:
display_->notification_surface_registry());
Does this do any interesting work? This runs very early here, so this could slow
down startup.

[xiyuan, 2016-06-21 18:03:57]

https://codereview.chromium.org/2066853002/diff/80001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main_extra_parts_exo.cc (right):

https://codereview.chromium.org/2066853002/diff/80001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main_extra_parts_exo.cc:129:
display_->notification_surface_registry());
On 2016/06/21 18:00:29, Nico wrote:
> Does this do any interesting work? This runs very early here, so this could
slow
> down startup.

Nope. It just passes down the registry pointer to the custom notification code
and stores it there for later use.

[Nico, 2016-06-21 18:05:17]

lgtm

(that still causes that code to be swapped in, but ok)

[xiyuan, 2016-06-21 19:39:33]

https://codereview.chromium.org/2066853002/diff/80001/ui/arc/notification/arc...
File ui/arc/notification/arc_custom_notification_view.h (right):

https://codereview.chromium.org/2066853002/diff/80001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_view.h:29: exo::NotificationSurface*
surface_;
On 2016/06/21 16:30:47, Luis Héctor Chávez wrote:
> nit: can this be |exo::NotificationSurface* const surface_|? Or even better,
> |const exo::NotificationSurface* const surface_|? The second would require you
> changing window() to be const, but seems doable.

Changed to a const ptr. However, did not use the second format because if we
make window() a const method then it needs to return a const aura::Window. And
NativeHostView needs a non const one since it needs to change the window's
state.

[dcheng, 2016-06-22 02:23:56]

LGTM with a few nits

https://codereview.chromium.org/2066853002/diff/100001/ui/arc/notification/ar...
File ui/arc/notification/arc_custom_notification_item.cc (right):

https://codereview.chromium.org/2066853002/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_custom_notification_item.cc:29: return
std::unique_ptr<views::View>();
return nullptr

https://codereview.chromium.org/2066853002/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_custom_notification_item.cc:31: return
base::WrapUnique(new ArcCustomNotificationView(surface_));
base::MakeUnique<ArcCustomNotificationView>(surface_)

https://codereview.chromium.org/2066853002/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_custom_notification_item.cc:73:
DCHECK(notification_key() == data.key);
Nit: DCHECK_EQ?

https://codereview.chromium.org/2066853002/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_custom_notification_item.cc:88:
SetNotification(base::WrapUnique(new message_center::Notification(
Nit: base::MakeUnique<message_center::Notification>(...)

[xiyuan, 2016-06-22 14:36:00]

https://codereview.chromium.org/2066853002/diff/100001/ui/arc/notification/ar...
File ui/arc/notification/arc_custom_notification_item.cc (right):

https://codereview.chromium.org/2066853002/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_custom_notification_item.cc:29: return
std::unique_ptr<views::View>();
On 2016/06/22 02:23:56, dcheng wrote:
> return nullptr

Done.

https://codereview.chromium.org/2066853002/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_custom_notification_item.cc:31: return
base::WrapUnique(new ArcCustomNotificationView(surface_));
On 2016/06/22 02:23:56, dcheng wrote:
> base::MakeUnique<ArcCustomNotificationView>(surface_)

Done.

https://codereview.chromium.org/2066853002/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_custom_notification_item.cc:73:
DCHECK(notification_key() == data.key);
On 2016/06/22 02:23:56, dcheng wrote:
> Nit: DCHECK_EQ?

Done.

https://codereview.chromium.org/2066853002/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_custom_notification_item.cc:88:
SetNotification(base::WrapUnique(new message_center::Notification(
On 2016/06/22 02:23:56, dcheng wrote:
> Nit: base::MakeUnique<message_center::Notification>(...)

Done.

[xiyuan, 2016-06-23 18:16:30]

reveman@, just found that this CL needs your blessing for adding
"+components/exo" to ui/arc/notification/DEPS.

Please help. Thanks.

[reveman, 2016-06-23 18:18:33]

ui/arc/notification/DEPS lgtm

[xiyuan, 2016-06-23 21:49:39]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2016-06-23 21:49:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
yoshiki@chromium.org, lhchavez@chromium.org, thakis@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2066853002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-23 21:50:17]

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

[commit-bot: I haz the power, 2016-06-23 23:58:09]

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

[commit-bot: I haz the power, 2016-06-23 23:58:10]

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

[xiyuan, 2016-06-24 00:33:56]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2016-06-24 00:34:33]

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

[commit-bot: I haz the power, 2016-06-24 03:06:30]

Description was changed from

==========
arc: Show custom notification via notification surface

- ArcCustomotificationItem to use notification surface to create Chrome
  notification;
- ArcCustomNotificationView to host and show the notification surface;
- Add a use_custom_notification property to ArcNotificationData;
- ArcNotificationManager choose ArcNotificationItem
  or ArcCustomNotificationItem based on that property;

BUG=b/25816173
==========

to

==========
arc: Show custom notification via notification surface

- ArcCustomotificationItem to use notification surface to create Chrome
  notification;
- ArcCustomNotificationView to host and show the notification surface;
- Add a use_custom_notification property to ArcNotificationData;
- ArcNotificationManager choose ArcNotificationItem
  or ArcCustomNotificationItem based on that property;

BUG=b/25816173
==========

[commit-bot: I haz the power, 2016-06-24 03:06:32]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-24 03:09:01]

Description was changed from

==========
arc: Show custom notification via notification surface

- ArcCustomotificationItem to use notification surface to create Chrome
  notification;
- ArcCustomNotificationView to host and show the notification surface;
- Add a use_custom_notification property to ArcNotificationData;
- ArcNotificationManager choose ArcNotificationItem
  or ArcCustomNotificationItem based on that property;

BUG=b/25816173
==========

to

==========
arc: Show custom notification via notification surface

- ArcCustomotificationItem to use notification surface to create Chrome
  notification;
- ArcCustomNotificationView to host and show the notification surface;
- Add a use_custom_notification property to ArcNotificationData;
- ArcNotificationManager choose ArcNotificationItem
  or ArcCustomNotificationItem based on that property;

BUG=b/25816173

Committed: https://crrev.com/241469472d4bef36af0641d428312fd3ab73377c
Cr-Commit-Position: refs/heads/master@{#401802}
==========

[commit-bot: I haz the power, 2016-06-24 03:09:03]

Patchset 9 (id:??) landed as
https://crrev.com/241469472d4bef36af0641d428312fd3ab73377c
Cr-Commit-Position: refs/heads/master@{#401802}