arc: Defer notification surface creation

arc: Defer notification surface creation

- Add Create/CloseNotificationWindow to NotificationsInstance;
- ArcCustomNotificationItem tracks a window count for the views and
  sends Create/CloseNotificationWindow based on the count;
- ArcCustomNotificationItem initializes the view with snapshot image;
- ArcCustomNotificationView observes surface manager and dynamically
  switch from snapshot to live surface;

BUG=640305, 640356
BUG=b/30864565
TEST=Manual. Custom notification works as before and CTS passes.

Committed: https://crrev.com/a21cae6cb172afe362f8a6d12f6e64ce56347b65
Cr-Commit-Position: refs/heads/master@{#415887}

[xiyuan, 2016-08-24 22:52:43]

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

[xiyuan, 2016-08-25 15:41:20]

Description was changed from

==========
arc: Defer notification surface creation

- Add Create/CloseNotificationWindow to NotificationsInstance;
- ArcCustomNotificationItem tracks a window count for the views and
  sends Create/CloseNotificationWindow based on the count;
- ArcCustomNotificationItem initializes the view with snapshot image;
- ArcCustomNotificationView observes surface manager and dynamically
  switch from snapshot to live surface;

BUG=640305
BUG=b/30864565
TEST=Manual. Custom notification works as before and CTS passes.
==========

to

==========
arc: Defer notification surface creation

- Add Create/CloseNotificationWindow to NotificationsInstance;
- ArcCustomNotificationItem tracks a window count for the views and
  sends Create/CloseNotificationWindow based on the count;
- ArcCustomNotificationItem initializes the view with snapshot image;
- ArcCustomNotificationView observes surface manager and dynamically
  switch from snapshot to live surface;

BUG=640305,640356
BUG=b/30864565
TEST=Manual. Custom notification works as before and CTS passes.
==========

[xiyuan, 2016-08-26 05:08:49]

ping?

[yoshiki, 2016-08-26 15:36:24]

lgtm. sorry for late.

[xiyuan, 2016-08-26 16:15:32]

On 2016/08/26 15:36:24, yoshiki wrote:
> lgtm. sorry for late.

No problem. Thanks for the review.

[xiyuan, 2016-08-26 16:15:42]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2016-08-26 16:16:12]

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, 2016-08-26 16:23:25]

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

[commit-bot: I haz the power, 2016-08-26 16:23:26]

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...)

[xiyuan, 2016-08-26 16:46:42]

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

[xiyuan, 2016-08-26 16:46:43]

dcheng@, could you review notifications.mojom change? Thanks.

[xiyuan, 2016-08-26 16:46:50]

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

[commit-bot: I haz the power, 2016-08-26 16:47:20]

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, 2016-08-26 17:42:38]

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

[commit-bot: I haz the power, 2016-08-26 17:42:38]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2016-08-29 19:16:16]

On 2016/08/26 16:46:43, xiyuan wrote:
> dcheng@, could you review notifications.mojom change? Thanks.

ping?

[kinaba, 2016-08-30 01:25:55]

drive-by. It looks components/arc/ OWNER as a reviewer looks needed, too

On 2016/08/29 19:16:16, xiyuan wrote:
> On 2016/08/26 16:46:43, xiyuan wrote:
> > dcheng@, could you review notifications.mojom change? Thanks.
> 
> ping?

[xiyuan, 2016-08-30 15:38:57]

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

[xiyuan, 2016-08-30 15:38:57]

+lhchavez, could you help with owner's review for

components/arc/test/fake_notifications_instance.*

?

Thanks.

[Luis Héctor Chávez, 2016-08-30 15:48:31]

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

[Luis Héctor Chávez, 2016-08-30 15:48:32]

components/arc/test lgtm

[Luis Héctor Chávez, 2016-08-30 15:51:13]

lhchavez@chromium.org changed reviewers:
- lhchavez@lhchavez.com

[Luis Héctor Chávez, 2016-08-30 15:51:13]

(-the wrong lhchavez as well)

[dcheng, 2016-08-31 05:57:54]

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

https://codereview.chromium.org/2269403004/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_item.cc:89:
data.snapshot_image.To<SkBitmap>(), data.snapshot_image_scale));
Do we have a bug filed to migrate ARC++ over to use typemapping?

https://codereview.chromium.org/2269403004/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_item.cc:122:
manager()->CloseNotificationWindow(notification_key());
So I don't completely understand the design here, since I don't know how the
classes relate.

I assume there's only one ArcNotificationManager? But what about
ArcCustomNotificationItem/ArcCustomNotificationView? Are those always a 1:1
relationship? Why do we need this window "ref count"?

1) As far as I can tell, we construct the view with an item, and always
increment the refcount then, and then always decrement it when the view is
destroyed.

2) Why does ArcCustomNotificationItem have an ObserverList? It seems like
there's ever only one observer? SHould it just be a delegate instead, and then
we can key this on the delegate being set / unset?

[xiyuan, 2016-08-31 14:01:55]

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

https://codereview.chromium.org/2269403004/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_item.cc:122:
manager()->CloseNotificationWindow(notification_key());
On 2016/08/31 05:57:54, dcheng wrote:
> So I don't completely understand the design here, since I don't know how the
> classes relate.
> 
> I assume there's only one ArcNotificationManager? But what about
> ArcCustomNotificationItem/ArcCustomNotificationView? Are those always a 1:1
> relationship? Why do we need this window "ref count"?
> 
> 1) As far as I can tell, we construct the view with an item, and always
> increment the refcount then, and then always decrement it when the view is
> destroyed.
> 
> 2) Why does ArcCustomNotificationItem have an ObserverList? It seems like
> there's ever only one observer? SHould it just be a delegate instead, and then
> we can key this on the delegate being set / unset?

ArcCustomNotificationItem contains the data and it could have two
ArcCustomNotificationView when we go from floating notification bubble to the
notification list. At that transition, there are two views, one is going away
and there is one being created. If we don't do ref count, the window could be
wrongly closed.

Same reason to have an ObserverList vs delegate here.

[dcheng, 2016-08-31 23:52:04]

OK, I don't think I can honestly follow the logic for managing the notifications
UI. I hope we can try to think of a way to simplify it.

mojom LGTM, but please file a bug (if there isn't already one) to remove ARC++'s
type converters.

[xiyuan, 2016-09-01 04:29:15]

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

https://codereview.chromium.org/2269403004/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_item.cc:89:
data.snapshot_image.To<SkBitmap>(), data.snapshot_image_scale));
On 2016/08/31 05:57:54, dcheng wrote:
> Do we have a bug filed to migrate ARC++ over to use typemapping?

Filed http://crbug.com/643050

[xiyuan, 2016-09-01 04:29:30]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2016-09-01 04:29:30]

The patchset sent to the CQ was uploaded after l-g-t-m from yoshiki@chromium.org
Link to the patchset: https://codereview.chromium.org/2269403004/#ps40001
(title: "rebase and fix compile")

[commit-bot: I haz the power, 2016-09-01 04:29: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, 2016-09-01 04:58:03]

Description was changed from

==========
arc: Defer notification surface creation

- Add Create/CloseNotificationWindow to NotificationsInstance;
- ArcCustomNotificationItem tracks a window count for the views and
  sends Create/CloseNotificationWindow based on the count;
- ArcCustomNotificationItem initializes the view with snapshot image;
- ArcCustomNotificationView observes surface manager and dynamically
  switch from snapshot to live surface;

BUG=640305,640356
BUG=b/30864565
TEST=Manual. Custom notification works as before and CTS passes.
==========

to

==========
arc: Defer notification surface creation

- Add Create/CloseNotificationWindow to NotificationsInstance;
- ArcCustomNotificationItem tracks a window count for the views and
  sends Create/CloseNotificationWindow based on the count;
- ArcCustomNotificationItem initializes the view with snapshot image;
- ArcCustomNotificationView observes surface manager and dynamically
  switch from snapshot to live surface;

BUG=640305,640356
BUG=b/30864565
TEST=Manual. Custom notification works as before and CTS passes.
==========

[commit-bot: I haz the power, 2016-09-01 04:58:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-01 05:01:00]

Description was changed from

==========
arc: Defer notification surface creation

- Add Create/CloseNotificationWindow to NotificationsInstance;
- ArcCustomNotificationItem tracks a window count for the views and
  sends Create/CloseNotificationWindow based on the count;
- ArcCustomNotificationItem initializes the view with snapshot image;
- ArcCustomNotificationView observes surface manager and dynamically
  switch from snapshot to live surface;

BUG=640305,640356
BUG=b/30864565
TEST=Manual. Custom notification works as before and CTS passes.
==========

to

==========
arc: Defer notification surface creation

- Add Create/CloseNotificationWindow to NotificationsInstance;
- ArcCustomNotificationItem tracks a window count for the views and
  sends Create/CloseNotificationWindow based on the count;
- ArcCustomNotificationItem initializes the view with snapshot image;
- ArcCustomNotificationView observes surface manager and dynamically
  switch from snapshot to live surface;

BUG=640305,640356
BUG=b/30864565
TEST=Manual. Custom notification works as before and CTS passes.

Committed: https://crrev.com/a21cae6cb172afe362f8a6d12f6e64ce56347b65
Cr-Commit-Position: refs/heads/master@{#415887}
==========

[commit-bot: I haz the power, 2016-09-01 05:01:02]

Patchset 3 (id:??) landed as
https://crrev.com/a21cae6cb172afe362f8a6d12f6e64ce56347b65
Cr-Commit-Position: refs/heads/master@{#415887}