exo: Implement notification surface support

exo: Implement notification surface support

BUG=b/25816173

Committed: https://crrev.com/4a599b40e8a90cd2940444c8c446b2b5d13b43af
Cr-Commit-Position: refs/heads/master@{#401735}

[xiyuan, 2016-06-14 16:50:00]

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

[yoshiki, 2016-06-16 02:46:25]

lgtm, although I'm not an owner.

[reveman, 2016-06-16 03:35:20]

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
File components/exo/notification_surface.cc (right):

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
components/exo/notification_surface.cc:26: return
surface_->GetHitTestBounds().size();
Why hit test bounds and not the contents size?

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
File components/exo/notification_surface.h (right):

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
components/exo/notification_surface.h:18: class NotificationSurface : public
SurfaceObserver {
I think this should be a SurfaceDelegate so we can handle commits properly and
we prevent the surface from being used for something else.

Also, should we be creating an aura::Window for this? Or how is this otherwise
being embedded in another part of the UI? Note that Surface is or has an
aura::Window but is wrong and will soon change.

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
components/exo/notification_surface.h:25: Surface* surface() { return surface_;
}
Do we need to expose the Surface? What part of it is needed to draw the
notifications and handle input?

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
components/exo/notification_surface.h:29: // SurfaceObserver
nit: "Overridden from SurfaceObserver:"

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
File components/exo/notification_surface_registry.cc (right):

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
components/exo/notification_surface_registry.cc:37:
DCHECK(notification_surface_map_.find(surface->notification_id()) ==
Can a client cause this to fail? That would be a security issue as clients
should not be able to crash the browser. In that case, this would need to be
validated earlier and handled as an error condition when creating the
notifiction surface.

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
File components/exo/notification_surface_registry.h (right):

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
components/exo/notification_surface_registry.h:20: class
NotificationSurfaceRegistry {
I'd like to avoid this class if possible. So far components/exo is not creating
a single global variable and it would be sad to see this change as part of this
change. Some options below:

1. If NotificationSurface class provided an aura::Window then we could just set
some unique notication property on the window and we get the same functionality
as this registry.

2. Have the NotificationSurface call into some notification manager API to
register itself.

https://codereview.chromium.org/2065133002/diff/20001/components/exo/wayland/...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/2065133002/diff/20001/components/exo/wayland/...
components/exo/wayland/server.cc:1718: DCHECK(notification_surface);
what can cause this to fail? fyi, if the client can cause this to fail then
that's a security issue.

[xiyuan, 2016-06-20 22:38:37]

Patchset #3 (id:40001) has been deleted

[xiyuan, 2016-06-20 22:40:29]

CL updated. PTAL. Thanks.

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
File components/exo/notification_surface.cc (right):

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
components/exo/notification_surface.cc:26: return
surface_->GetHitTestBounds().size();
On 2016/06/16 03:35:20, reveman wrote:
> Why hit test bounds and not the contents size?

Did not notice we added a content_size(). Use it now.

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
File components/exo/notification_surface.h (right):

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
components/exo/notification_surface.h:18: class NotificationSurface : public
SurfaceObserver {
On 2016/06/16 03:35:20, reveman wrote:
> I think this should be a SurfaceDelegate so we can handle commits properly and
> we prevent the surface from being used for something else.
> 
> Also, should we be creating an aura::Window for this? Or how is this otherwise
> being embedded in another part of the UI? Note that Surface is or has an
> aura::Window but is wrong and will soon change.

Done. Made it a SurfaceDelegate and create its own aura::Window instead of using
the one from Surface. We would need to revisit this after Surface becomes a
layer though.

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
components/exo/notification_surface.h:25: Surface* surface() { return surface_;
}
On 2016/06/16 03:35:20, reveman wrote:
> Do we need to expose the Surface? What part of it is needed to draw the
> notifications and handle input?

Right now, the surface is handled via NativeViewHost. See
arc_custom_notification_view.cc in https://codereview.chromium.org/2066853002/.

We need an aura::Window for this to work. I will refactor this class to have its
own aura::Window instead of using the one from Surface.

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
components/exo/notification_surface.h:29: // SurfaceObserver
On 2016/06/16 03:35:20, reveman wrote:
> nit: "Overridden from SurfaceObserver:"

Done.

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
File components/exo/notification_surface_registry.cc (right):

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
components/exo/notification_surface_registry.cc:37:
DCHECK(notification_surface_map_.find(surface->notification_id()) ==
On 2016/06/16 03:35:20, reveman wrote:
> Can a client cause this to fail? That would be a security issue as clients
> should not be able to crash the browser. In that case, this would need to be
> validated earlier and handled as an error condition when creating the
> notifiction surface.

Do the check when processing the request. Client should never cause this to
trigger and changed this to NOTREACHED and bail.

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
File components/exo/notification_surface_registry.h (right):

https://codereview.chromium.org/2065133002/diff/20001/components/exo/notifica...
components/exo/notification_surface_registry.h:20: class
NotificationSurfaceRegistry {
On 2016/06/16 03:35:20, reveman wrote:
> I'd like to avoid this class if possible. So far components/exo is not
creating
> a single global variable and it would be sad to see this change as part of
this
> change. Some options below:
> 
> 1. If NotificationSurface class provided an aura::Window then we could just
set
> some unique notication property on the window and we get the same
functionality
> as this registry.
> 
> 2. Have the NotificationSurface call into some notification manager API to
> register itself.

Unfortunately, we cannot avoid this class. We can have an aura::Window for
NotificationSurface and use a property. I had a patch that does that. However,
looking at the your comment in notification_surface_registry.cc file that we
should fail when a client tried to create a NotificatoinSurface with bad id.
This would require to have something like this class so that we can validate the
request to create a NotificationSurface.

I have removed the LazyInstance part since that looks like the problematic part.
Now this registry is part of Display and NotificationSurface is created with a
pointer to the registry.

https://codereview.chromium.org/2065133002/diff/20001/components/exo/wayland/...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/2065133002/diff/20001/components/exo/wayland/...
components/exo/wayland/server.cc:1718: DCHECK(notification_surface);
On 2016/06/16 03:35:20, reveman wrote:
> what can cause this to fail? fyi, if the client can cause this to fail then
> that's a security issue.

Replaced with notification id validating and role checking.

[reveman, 2016-06-22 16:44:31]

https://codereview.chromium.org/2065133002/diff/60001/components/exo/notifica...
File components/exo/notification_surface.cc (right):

https://codereview.chromium.org/2065133002/diff/60001/components/exo/notifica...
components/exo/notification_surface.cc:88: // TODO(xiyuan): Fix this after
Surface becomes just a layer.
nit/fyi: surfaces will not even be layers, they will just be backed by a
cc:surface.

https://codereview.chromium.org/2065133002/diff/60001/components/exo/notifica...
File components/exo/notification_surface_registry.h (right):

https://codereview.chromium.org/2065133002/diff/60001/components/exo/notifica...
components/exo/notification_surface_registry.h:19: class
NotificationSurfaceRegistry {
There seems to be multiple levels of indirection here and I find it hard to
follow. Too many manager/registry classes maybe.

In src/ui/ there seems to be some manager/registry that already outlives the
lifetime of these notification surfaces. Can the class here just be an abstract
interface that is instead implemented by the manager class in
ui/arc/notification?

Basically, can we just reduce this to something like the
exo::NotificationSurfaceRegistry::Observer interface, maybe call it
exo::NotificationSurfaceManager and have exo::Display just call into that when
creating/destroying NotificationSurfaces? The benefit would be it avoid multiple
places where we manage these surfaces and reduce the code in exo/ which I think
should know as little as possible about how these notification surfaces are used
and managed. Does that make sense?

https://codereview.chromium.org/2065133002/diff/60001/components/exo/wayland/...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/2065133002/diff/60001/components/exo/wayland/...
components/exo/wayland/server.cc:1532: bool IsValidNotificationId(const
std::string& notification_id) const {
Would be nice if this could instead be validated when inserting the surface so
we can avoid two lookups. Maybe just have a null return value from
CreateNotificationSurface mean generate this error. I think that should work
after changing how ERROR_ROLE messages are generated as suggested below.

https://codereview.chromium.org/2065133002/diff/60001/components/exo/wayland/...
components/exo/wayland/server.cc:1735: if (!notification_surface) {
Instead of using a null return value here. Can you check this earlier by calling
Surface::HasSurfaceDelegate()?

[xiyuan, 2016-06-23 14:46:43]

PTAL. Thanks.

https://codereview.chromium.org/2065133002/diff/60001/components/exo/notifica...
File components/exo/notification_surface.cc (right):

https://codereview.chromium.org/2065133002/diff/60001/components/exo/notifica...
components/exo/notification_surface.cc:88: // TODO(xiyuan): Fix this after
Surface becomes just a layer.
On 2016/06/22 16:44:30, reveman wrote:
> nit/fyi: surfaces will not even be layers, they will just be backed by a
> cc:surface.

Comments updated. It now says:
  // TODO(xiyuan): Fix after Surface no longer has an auar::Window.

https://codereview.chromium.org/2065133002/diff/60001/components/exo/notifica...
File components/exo/notification_surface_registry.h (right):

https://codereview.chromium.org/2065133002/diff/60001/components/exo/notifica...
components/exo/notification_surface_registry.h:19: class
NotificationSurfaceRegistry {
On 2016/06/22 16:44:30, reveman wrote:
> There seems to be multiple levels of indirection here and I find it hard to
> follow. Too many manager/registry classes maybe.
> 
> In src/ui/ there seems to be some manager/registry that already outlives the
> lifetime of these notification surfaces. Can the class here just be an
abstract
> interface that is instead implemented by the manager class in
> ui/arc/notification?
> 
> Basically, can we just reduce this to something like the
> exo::NotificationSurfaceRegistry::Observer interface, maybe call it
> exo::NotificationSurfaceManager and have exo::Display just call into that when
> creating/destroying NotificationSurfaces? The benefit would be it avoid
multiple
> places where we manage these surfaces and reduce the code in exo/ which I
think
> should know as little as possible about how these notification surfaces are
used
> and managed. Does that make sense?

Okay. I moved this into ui/arc. However, I did not merge it into
ArcNotifiationManager because that is an ArcService and created after user
sign-in. It would be tricky to bind with exo::Display at that time point since
there is no global reference of either exo::Display or exo::wayland::Server.

I kept this code into a ArcNotificationSurfaceManager (more like rename of
NotificationSurfaceRegistry) and create and set into exo::Display on Wayland
init time.

https://codereview.chromium.org/2065133002/diff/60001/components/exo/wayland/...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/2065133002/diff/60001/components/exo/wayland/...
components/exo/wayland/server.cc:1532: bool IsValidNotificationId(const
std::string& notification_id) const {
On 2016/06/22 16:44:30, reveman wrote:
> Would be nice if this could instead be validated when inserting the surface so
> we can avoid two lookups. Maybe just have a null return value from
> CreateNotificationSurface mean generate this error. I think that should work
> after changing how ERROR_ROLE messages are generated as suggested below.

Done.

https://codereview.chromium.org/2065133002/diff/60001/components/exo/wayland/...
components/exo/wayland/server.cc:1735: if (!notification_surface) {
On 2016/06/22 16:44:31, reveman wrote:
> Instead of using a null return value here. Can you check this earlier by
calling
> Surface::HasSurfaceDelegate()?

Done.

[reveman, 2016-06-23 16:34:53]

lgtm % nits

https://codereview.chromium.org/2065133002/diff/80001/components/exo/display.cc
File components/exo/display.cc (right):

https://codereview.chromium.org/2065133002/diff/80001/components/exo/display....
components/exo/display.cc:190: if (notification_surface_manager_ &&
nit: please fail if "!notification_surface_manager_" instead of having do deal
with that in the notification surface impl

https://codereview.chromium.org/2065133002/diff/80001/components/exo/display....
components/exo/display.cc:191:
notification_surface_manager_->GetSurface(notification_id) != nullptr) {
nit: s/GetSurface(notification_id) != nullptr/GetSurface(notification_id)/

https://codereview.chromium.org/2065133002/diff/80001/components/exo/display.h
File components/exo/display.h (right):

https://codereview.chromium.org/2065133002/diff/80001/components/exo/display....
components/exo/display.h:91: std::unique_ptr<NotificationSurfaceManager>
notification_surface_manager);
nit: please change this to a raw pointer and have the caller manage the lifetime
of the manager instance

https://codereview.chromium.org/2065133002/diff/80001/components/exo/display....
components/exo/display.h:94: std::unique_ptr<NotificationSurfaceManager>
notification_surface_manager_;
nit: NotificationSurfaceManager* const notification_surface_manager_;

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
File components/exo/notification_surface.cc (right):

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
components/exo/notification_surface.cc:92: if (manager_)
nit: remove if statement after changing Display::CreateNotificationSurface to
fail early if not manager exists

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
components/exo/notification_surface.cc:101: if (manager_)
nit: remove if statement after changing Display::CreateNotificationSurface to
fail early if not manager exists

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
File components/exo/notification_surface_manager.h (right):

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
components/exo/notification_surface_manager.h:5: #ifndef
COMPONENTS_EXO_NOTIFICATION_SURFACE_REGISTRY_H_
nit: s/REGISTRY/MANAGER/

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
components/exo/notification_surface_manager.h:6: #define
COMPONENTS_EXO_NOTIFICATION_SURFACE_REGISTRY_H_
nit: s/REGISTRY/MANAGER/

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
components/exo/notification_surface_manager.h:27: virtual
~NotificationSurfaceManager() {}
nit: please make this protected after changing SetNotificationSurfaceManager to
take a raw pointer

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
components/exo/notification_surface_manager.h:32: #endif  //
COMPONENTS_EXO_NOTIFICATION_SURFACE_REGISTRY_H_
nit: s/REGISTRY/MANAGER/

https://codereview.chromium.org/2065133002/diff/80001/components/exo/wayland/...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/2065133002/diff/80001/components/exo/wayland/...
components/exo/wayland/server.cc:1746: const uint32_t remote_shell_version = 4;
Note: I think this will be 6 after rebase

[xiyuan, 2016-06-23 17:46:10]

https://codereview.chromium.org/2065133002/diff/80001/components/exo/display.cc
File components/exo/display.cc (right):

https://codereview.chromium.org/2065133002/diff/80001/components/exo/display....
components/exo/display.cc:190: if (notification_surface_manager_ &&
On 2016/06/23 16:34:52, reveman wrote:
> nit: please fail if "!notification_surface_manager_" instead of having do deal
> with that in the notification surface impl

Done.

https://codereview.chromium.org/2065133002/diff/80001/components/exo/display....
components/exo/display.cc:191:
notification_surface_manager_->GetSurface(notification_id) != nullptr) {
On 2016/06/23 16:34:52, reveman wrote:
> nit: s/GetSurface(notification_id) != nullptr/GetSurface(notification_id)/

Done.

https://codereview.chromium.org/2065133002/diff/80001/components/exo/display.h
File components/exo/display.h (right):

https://codereview.chromium.org/2065133002/diff/80001/components/exo/display....
components/exo/display.h:91: std::unique_ptr<NotificationSurfaceManager>
notification_surface_manager);
On 2016/06/23 16:34:53, reveman wrote:
> nit: please change this to a raw pointer and have the caller manage the
lifetime
> of the manager instance

Sure. Since we would use a raw const pointer, it could need to be passed from
ctor. Thus removed this setter function.

https://codereview.chromium.org/2065133002/diff/80001/components/exo/display....
components/exo/display.h:94: std::unique_ptr<NotificationSurfaceManager>
notification_surface_manager_;
On 2016/06/23 16:34:53, reveman wrote:
> nit: NotificationSurfaceManager* const notification_surface_manager_;

Done.

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
File components/exo/notification_surface.cc (right):

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
components/exo/notification_surface.cc:92: if (manager_)
On 2016/06/23 16:34:53, reveman wrote:
> nit: remove if statement after changing Display::CreateNotificationSurface to
> fail early if not manager exists

Done.

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
components/exo/notification_surface.cc:101: if (manager_)
On 2016/06/23 16:34:53, reveman wrote:
> nit: remove if statement after changing Display::CreateNotificationSurface to
> fail early if not manager exists

Done.

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
File components/exo/notification_surface_manager.h (right):

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
components/exo/notification_surface_manager.h:5: #ifndef
COMPONENTS_EXO_NOTIFICATION_SURFACE_REGISTRY_H_
On 2016/06/23 16:34:53, reveman wrote:
> nit: s/REGISTRY/MANAGER/

Done.

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
components/exo/notification_surface_manager.h:6: #define
COMPONENTS_EXO_NOTIFICATION_SURFACE_REGISTRY_H_
On 2016/06/23 16:34:53, reveman wrote:
> nit: s/REGISTRY/MANAGER/

Done.

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
components/exo/notification_surface_manager.h:27: virtual
~NotificationSurfaceManager() {}
On 2016/06/23 16:34:53, reveman wrote:
> nit: please make this protected after changing SetNotificationSurfaceManager
to
> take a raw pointer

Done.

https://codereview.chromium.org/2065133002/diff/80001/components/exo/notifica...
components/exo/notification_surface_manager.h:32: #endif  //
COMPONENTS_EXO_NOTIFICATION_SURFACE_REGISTRY_H_
On 2016/06/23 16:34:53, reveman wrote:
> nit: s/REGISTRY/MANAGER/

Done.

https://codereview.chromium.org/2065133002/diff/80001/components/exo/wayland/...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/2065133002/diff/80001/components/exo/wayland/...
components/exo/wayland/server.cc:1746: const uint32_t remote_shell_version = 4;
On 2016/06/23 16:34:53, reveman wrote:
> Note: I think this will be 6 after rebase

will rebase and fix it in next PS.

[xiyuan, 2016-06-23 19:34:40]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2016-06-23 19:34:40]

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

[commit-bot: I haz the power, 2016-06-23 19:35:19]

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

[commit-bot: I haz the power, 2016-06-23 19:52:20]

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

[commit-bot: I haz the power, 2016-06-23 19:52:21]

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

[xiyuan, 2016-06-23 20:09:48]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2016-06-23 20:09:48]

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

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

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

[commit-bot: I haz the power, 2016-06-23 21:48:40]

Committed patchset #7 (id:130001)

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

Description was changed from

==========
exo: Implement notification surface support

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

to

==========
exo: Implement notification surface support

BUG=b/25816173

Committed: https://crrev.com/4a599b40e8a90cd2940444c8c446b2b5d13b43af
Cr-Commit-Position: refs/heads/master@{#401735}
==========

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

Patchset 7 (id:??) landed as
https://crrev.com/4a599b40e8a90cd2940444c8c446b2b5d13b43af
Cr-Commit-Position: refs/heads/master@{#401735}