Add a floating close button for arc custom notification

Add a floating close button for arc custom notification

Needed because default notification close button is part of views
hierarchy and covered by the notification surface. This CL adds
a floating version of close button that sits on top of the notification
surface.

BUG=b/25816173
TBR=brettw@chromium.org

Committed: https://crrev.com/369cf0cdf40fd31940504f01785b1e7a6c80ef13
Cr-Commit-Position: refs/heads/master@{#403196}

[xiyuan, 2016-06-24 23:06:17]

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

[yoshiki, 2016-06-27 12:55:01]

lgtm

[xiyuan, 2016-06-27 15:46:31]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2016-06-27 15:46:52]

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-06-27 15:54:21]

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

[commit-bot: I haz the power, 2016-06-27 15:54:22]

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-06-27 16:27:53]

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

[xiyuan, 2016-06-27 16:27:54]

sky@, could you help with owner approval for adding
  ui/base
  ui/resources
to ui/arc/notification/DEPS?

Context in case you are curious: Those are used to access the notification close
button resources. We create a floating close button rather than use the existing
one because arc custom notification is an aura::Window added via NativeViewHost
and sits on top of the existing close button.

Thanks.

[sky, 2016-06-27 17:38:40]

The DEPS is fine, but don't you need to update build files too? I strongly
encourage you to add ui/arc to .gn so that gn check is run.

[xiyuan, 2016-06-27 19:30:56]

On 2016/06/27 17:38:40, sky wrote:
> The DEPS is fine, but don't you need to update build files too? I strongly
> encourage you to add ui/arc to .gn so that gn check is run.

Build files updated. However, I could not add it to .gn yet because there is a
circular dependency between components/arc and ui/arc.

  ArcNotificationManager is an ArcService, which makes ui/arc -> components/arc
  ArcBridgetServiceImp in components/arc instantiates ArcNotificationManager,
thus components/arc -> ui/arc

We need to break this before adding ui/arc to .gn. How about do it in a follow
up since it is not relevant to this CL?

[sky, 2016-06-27 20:43:03]

Won't that lead to build problems/flake? If the two targets really
need a circule dependency then allow_circular_includes_from might be
right, although I wouldn't expect ui/arc and components/arc to be so
entwined.

On Mon, Jun 27, 2016 at 12:30 PM,  <xiyuan@chromium.org> wrote:
> On 2016/06/27 17:38:40, sky wrote:
>> The DEPS is fine, but don't you need to update build files too? I strongly
>> encourage you to add ui/arc to .gn so that gn check is run.
>
> Build files updated. However, I could not add it to .gn yet because there is
> a
> circular dependency between components/arc and ui/arc.
>
> ArcNotificationManager is an ArcService, which makes ui/arc ->
> components/arc
> ArcBridgetServiceImp in components/arc instantiates ArcNotificationManager,
> thus components/arc -> ui/arc
>
> We need to break this before adding ui/arc to .gn. How about do it in a
> follow
> up since it is not relevant to this CL?
>
> https://codereview.chromium.org/2093563007/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[xiyuan, 2016-06-27 21:35:51]

Patchset #3 (id:40001) has been deleted

[xiyuan, 2016-06-27 21:51:06]

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

[xiyuan, 2016-06-27 21:51:07]

+lhchavez

Okay. I moved files that //ui/arc depends on into a new arc_base target in
//components/arc. This breaks the circular deps between the two.

Now we have
//ui/arc -> //components/arc:arc_base
// components/arc -> //components/arc:arc_base

I added //ui/arc to .gn and gn check is now happy.

lhchavez@, adding you for owner approval since this CL now touches
//components/arc.

[Luis Héctor Chávez, 2016-06-27 22:07:11]

lgtm with a small request

https://codereview.chromium.org/2093563007/diff/60001/components/arc/BUILD.gn
File components/arc/BUILD.gn (right):

https://codereview.chromium.org/2093563007/diff/60001/components/arc/BUILD.gn...
components/arc/BUILD.gn:102: "bitmap/bitmap_type_converters.h",
These two files don't seem to be very base-y. Can you split them off into
arc_bitmap or arc_ui?

[Luis Héctor Chávez, 2016-06-27 22:07:50]

On 2016/06/27 22:07:11, Luis Héctor Chávez wrote:
> lgtm with a small request
> 
> https://codereview.chromium.org/2093563007/diff/60001/components/arc/BUILD.gn
> File components/arc/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2093563007/diff/60001/components/arc/BUILD.gn...
> components/arc/BUILD.gn:102: "bitmap/bitmap_type_converters.h",
> These two files don't seem to be very base-y. Can you split them off into
> arc_bitmap or arc_ui?

also, can you s/notificaiton/notification/ in the commit message?

[xiyuan, 2016-06-27 22:08:47]

Description was changed from

==========
Add a floating close button for arc custom notificaiton

Needed because default notification close button is part of views
hierarchy and covered by the notification surface. This CL adds
a floating version of close button that sits on top of the notification
surface.

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

to

==========
Add a floating close button for arc custom notification

Needed because default notification close button is part of views
hierarchy and covered by the notification surface. This CL adds
a floating version of close button that sits on top of the notification
surface.

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

[xiyuan, 2016-06-27 22:24:07]

CL description typo fixed too.

https://codereview.chromium.org/2093563007/diff/60001/components/arc/BUILD.gn
File components/arc/BUILD.gn (right):

https://codereview.chromium.org/2093563007/diff/60001/components/arc/BUILD.gn...
components/arc/BUILD.gn:102: "bitmap/bitmap_type_converters.h",
On 2016/06/27 22:07:11, Luis Héctor Chávez wrote:
> These two files don't seem to be very base-y. Can you split them off into
> arc_bitmap or arc_ui?

Added an arc_bitmap target for them.

[sky, 2016-06-27 22:57:07]

sky@chromium.org changed reviewers:
+ brettw@chromium.org

[sky, 2016-06-27 22:57:08]

Thanks, DEPS LGTM
+brettw for //.gn

[xiyuan, 2016-06-30 15:41:23]

Description was changed from

==========
Add a floating close button for arc custom notification

Needed because default notification close button is part of views
hierarchy and covered by the notification surface. This CL adds
a floating version of close button that sits on top of the notification
surface.

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

to

==========
Add a floating close button for arc custom notification

Needed because default notification close button is part of views
hierarchy and covered by the notification surface. This CL adds
a floating version of close button that sits on top of the notification
surface.

BUG=b/25816173
TBR=brettw@chromium.org
==========

[xiyuan, 2016-06-30 15:42:01]

TBR=brettw since .gn change is a one-liner and it is adding //ui/arc to gn check
coverage.

[xiyuan, 2016-06-30 15:42:11]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2016-06-30 15:42:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
yoshiki@chromium.org, lhchavez@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2093563007/#ps100001
(title: "fix typo")

[commit-bot: I haz the power, 2016-06-30 15:42:31]

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-06-30 16:34:28]

Description was changed from

==========
Add a floating close button for arc custom notification

Needed because default notification close button is part of views
hierarchy and covered by the notification surface. This CL adds
a floating version of close button that sits on top of the notification
surface.

BUG=b/25816173
TBR=brettw@chromium.org
==========

to

==========
Add a floating close button for arc custom notification

Needed because default notification close button is part of views
hierarchy and covered by the notification surface. This CL adds
a floating version of close button that sits on top of the notification
surface.

BUG=b/25816173
TBR=brettw@chromium.org
==========

[commit-bot: I haz the power, 2016-06-30 16:34:29]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-06-30 16:34:32]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 16:37:38]

Description was changed from

==========
Add a floating close button for arc custom notification

Needed because default notification close button is part of views
hierarchy and covered by the notification surface. This CL adds
a floating version of close button that sits on top of the notification
surface.

BUG=b/25816173
TBR=brettw@chromium.org
==========

to

==========
Add a floating close button for arc custom notification

Needed because default notification close button is part of views
hierarchy and covered by the notification surface. This CL adds
a floating version of close button that sits on top of the notification
surface.

BUG=b/25816173
TBR=brettw@chromium.org

Committed: https://crrev.com/369cf0cdf40fd31940504f01785b1e7a6c80ef13
Cr-Commit-Position: refs/heads/master@{#403196}
==========

[commit-bot: I haz the power, 2016-06-30 16:37:40]

Patchset 5 (id:??) landed as
https://crrev.com/369cf0cdf40fd31940504f01785b1e7a6c80ef13
Cr-Commit-Position: refs/heads/master@{#403196}