exo: Add notification surface to manager after commit

exo: Add notification surface to manager after commit

Add notification surface to manager after commit so that it has
the correct content size when creating the hosting view.

BUG=629854
Committed: https://crrev.com/e55d9d3bf6082a636271efda6ece4cd04dc94109
Cr-Commit-Position: refs/heads/master@{#406851}

[xiyuan, 2016-07-20 15:38:05]

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

[xiyuan, 2016-07-20 15:38:13]

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

[commit-bot: I haz the power, 2016-07-20 15:38:31]

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-07-20 16:27:25]

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

[commit-bot: I haz the power, 2016-07-20 16:27:26]

Dry run: This issue passed the CQ dry run.

[reveman, 2016-07-20 18:58:02]

https://codereview.chromium.org/2169513003/diff/1/components/exo/notification...
File components/exo/notification_surface.cc (right):

https://codereview.chromium.org/2169513003/diff/1/components/exo/notification...
components/exo/notification_surface.cc:88: // TODO(xiyuan): Fix after Surface no
longer has an auar::Window.
nit: aura::Window

https://codereview.chromium.org/2169513003/diff/1/components/exo/notification...
components/exo/notification_surface.cc:112: window_->SetBounds(bounds);
as the bounds can change here, doesn't the manager code already have to handle
changes to size by observing the window size? and empty size -> non-empty size
is just a special case of that..

[xiyuan, 2016-07-20 19:52:41]

https://codereview.chromium.org/2169513003/diff/1/components/exo/notification...
File components/exo/notification_surface.cc (right):

https://codereview.chromium.org/2169513003/diff/1/components/exo/notification...
components/exo/notification_surface.cc:112: window_->SetBounds(bounds);
On 2016/07/20 18:58:02, reveman wrote:
> as the bounds can change here, doesn't the manager code already have to handle
> changes to size by observing the window size? and empty size -> non-empty size
> is just a special case of that..

The hosting ArcCustomNotificationView is based on NativeViewHost, which does not
change its size (preferred size or bounds) in views system based on the hosted
NativeView. Even if we add bounds change handling to ArcCustomNotificationView,
it is still bad UX that a notification shows then immediately resizes.

We should not be showing the notification until the underlying surface is
committed at least once. Otherwise, I am not sure what user would see on screen.
If deferring AddSurface here sounds unreasonable, maybe we should add an
explicit method to notify manager about the commit?

[reveman, 2016-07-20 20:57:30]

https://codereview.chromium.org/2169513003/diff/1/components/exo/notification...
File components/exo/notification_surface.cc (right):

https://codereview.chromium.org/2169513003/diff/1/components/exo/notification...
components/exo/notification_surface.cc:112: window_->SetBounds(bounds);
On 2016/07/20 at 19:52:41, xiyuan wrote:
> On 2016/07/20 18:58:02, reveman wrote:
> > as the bounds can change here, doesn't the manager code already have to
handle
> > changes to size by observing the window size? and empty size -> non-empty
size
> > is just a special case of that..
> 
> The hosting ArcCustomNotificationView is based on NativeViewHost, which does
not change its size (preferred size or bounds) in views system based on the
hosted NativeView. Even if we add bounds change handling to
ArcCustomNotificationView, it is still bad UX that a notification shows then
immediately resizes.
> 
> We should not be showing the notification until the underlying surface is
committed at least once. Otherwise, I am not sure what user would see on screen.
If deferring AddSurface here sounds unreasonable, maybe we should add an
explicit method to notify manager about the commit?

I'm not suggesting we should show it unless bounds are non-empty. This code is
explicitly handling size changes so I assume that this can happen and is handled
correctly? If bounds changes are NOT handled correctly by the manager today then
maybe something like this would be appropriate:

gfx::Size old_size = window_->bounds().size();
gfx::Size new_size = surface_->content_size();
if (old_size != new_size()) {
  if (!old_size.IsEmpty())
    manager_->RemoveSurface(this);
  if (!new_size.IsEmpty())
    manager_->AddSurface(this);
}

and dtor would be updated accordingly.

[xiyuan, 2016-07-20 22:01:20]

https://codereview.chromium.org/2169513003/diff/1/components/exo/notification...
File components/exo/notification_surface.cc (right):

https://codereview.chromium.org/2169513003/diff/1/components/exo/notification...
components/exo/notification_surface.cc:112: window_->SetBounds(bounds);
On 2016/07/20 20:57:30, reveman wrote:
> I'm not suggesting we should show it unless bounds are non-empty. This code is
> explicitly handling size changes so I assume that this can happen and is
handled
> correctly? If bounds changes are NOT handled correctly by the manager today
then
> maybe something like this would be appropriate:
> 
> gfx::Size old_size = window_->bounds().size();
> gfx::Size new_size = surface_->content_size();
> if (old_size != new_size()) {
>   if (!old_size.IsEmpty())
>     manager_->RemoveSurface(this);
>   if (!new_size.IsEmpty())
>     manager_->AddSurface(this);
> }
> 
> and dtor would be updated accordingly.

I prefer not doing remove-then-add on bounds change (even though it works today
because the bounds is only changed once on the first commit). My concern is that
when the manager sees a surface is removed, it would not know the surface will
be added back later and have to update the UI to deal with the removed surface.
This could cause flashing later if we really needs to handle bounds change.

Think the main problem here is that hosting UI is created before surface is
committed. If the commit happens very late (or never happens), we are showing an
empty UI (or undefined contents).

[reveman, 2016-07-20 22:43:09]

On 2016/07/20 at 22:01:20, xiyuan wrote:
>
https://codereview.chromium.org/2169513003/diff/1/components/exo/notification...
> File components/exo/notification_surface.cc (right):
> 
>
https://codereview.chromium.org/2169513003/diff/1/components/exo/notification...
> components/exo/notification_surface.cc:112: window_->SetBounds(bounds);
> On 2016/07/20 20:57:30, reveman wrote:
> > I'm not suggesting we should show it unless bounds are non-empty. This code
is
> > explicitly handling size changes so I assume that this can happen and is
handled
> > correctly? If bounds changes are NOT handled correctly by the manager today
then
> > maybe something like this would be appropriate:
> > 
> > gfx::Size old_size = window_->bounds().size();
> > gfx::Size new_size = surface_->content_size();
> > if (old_size != new_size()) {
> >   if (!old_size.IsEmpty())
> >     manager_->RemoveSurface(this);
> >   if (!new_size.IsEmpty())
> >     manager_->AddSurface(this);
> > }
> > 
> > and dtor would be updated accordingly.
> 
> I prefer not doing remove-then-add on bounds change (even though it works
today because the bounds is only changed once on the first commit). My concern
is that when the manager sees a surface is removed, it would not know the
surface will be added back later and have to update the UI to deal with the
removed surface. This could cause flashing later if we really needs to handle
bounds change.
> 
> Think the main problem here is that hosting UI is created before surface is
committed. If the commit happens very late (or never happens), we are showing an
empty UI (or undefined contents).

Ok, current "added_to_manager" approach is fine then or you could use empty
contents to keep track of if it's been registered or not. Either way, you need
to update the dtor to only remove it if it's been added.

[xiyuan, 2016-07-20 23:19:04]

dtor updated and also include content check before AddSurface.

PTAL. Thanks.

https://codereview.chromium.org/2169513003/diff/1/components/exo/notification...
File components/exo/notification_surface.cc (right):

https://codereview.chromium.org/2169513003/diff/1/components/exo/notification...
components/exo/notification_surface.cc:88: // TODO(xiyuan): Fix after Surface no
longer has an auar::Window.
On 2016/07/20 18:58:02, reveman wrote:
> nit: aura::Window

Done.

[reveman, 2016-07-20 23:29:33]

lgtm

[xiyuan, 2016-07-21 00:20:16]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2016-07-21 00:20:42]

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-07-21 00:56:34]

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

[commit-bot: I haz the power, 2016-07-21 00:56:35]

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

[xiyuan, 2016-07-21 14:20:57]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2016-07-21 14:21:18]

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-07-21 14:54:58]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-21 14:56:19]

Description was changed from

==========
exo: Add notification surface to manager after commit

Add notification surface to manager after commit so that it has
the correct content size when creating the hosting view.

BUG=629854
==========

to

==========
exo: Add notification surface to manager after commit

Add notification surface to manager after commit so that it has
the correct content size when creating the hosting view.

BUG=629854

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

[commit-bot: I haz the power, 2016-07-21 14:56:20]

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