[EasyUnlock] Introduce EasyUnlockNotificationController

[EasyUnlock] Introduce EasyUnlockNotificationController

This class is responsible for displaying and handling all EasyUnlock notifications.

Originally, EasyUnlock notifications were shown by a component app, but this app is now deprecated and we are migrating EasyUnlock natively for v2.

BUG=726791
Review-Url: https://codereview.chromium.org/2968323002
Cr-Commit-Position: refs/heads/master@{#488522}
Committed: https://chromium.googlesource.com/chromium/src/+/b8c14e087eb455c3e712753d44eb8a2c62cba9f8

[Tim Song, 2017-07-07 00:12:36]

Description was changed from

==========
Notification test.
==========

to

==========
[EasyUnlock] Introduce EasyUnlockNotificationController

This class is responsible for displaying and handling all EasyUnlock
notifications.

Originally, EasyUnlock notifications were shown by a component app, but this app
is now deprecated and we are migrating EasyUnlock natively for v2.

BUG=726791
==========

[Tim Song, 2017-07-07 00:13:19]

tengs@chromium.org changed reviewers:
+ oshima@chromium.org, sacomoto@chromium.org

[Tim Song, 2017-07-07 00:13:20]

+sacomoto@ for general review
+oshima@ for assets

[oshima, 2017-07-07 00:45:00]

can you use vector icon instead?

https://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui

[Tim Song, 2017-07-07 18:31:57]

These assets are pretty old, but I'll sync up with UX to see if it's feasible to
vectorize this icon. 

Is it okay to land this CL in case we can't get the assets from UX in time for
the branch point?

[Peter Beverloo, 2017-07-07 18:36:57]

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

[Peter Beverloo, 2017-07-07 18:36:58]

https://codereview.chromium.org/2968323002/diff/20001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_notification_controller.cc (right):

https://codereview.chromium.org/2968323002/diff/20001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_notification_controller.cc:40: profile,
message_center::MessageCenter::Get());
Will this functionality stay restricted to Chrome OS? Note that you cannot rely
on there being a message center on the other platforms.

https://codereview.chromium.org/2968323002/diff/20001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_notification_controller_chromeos.cc
(right):

https://codereview.chromium.org/2968323002/diff/20001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_notification_controller_chromeos.cc:38: const
message_center::RichNotificationData rich_notification_data) {
nit: const& (saves the unnecessary copy)

https://codereview.chromium.org/2968323002/diff/20001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_notification_controller_chromeos.cc:55:
message_center_->AddObserver(this);
Please provide a delegate to the Notification object as opposed to observing the
MC.

[oshima, 2017-07-07 19:20:36]

On 2017/07/07 18:31:57, Tim Song wrote:
> These assets are pretty old, but I'll sync up with UX to see if it's feasible
to
> vectorize this icon. 

UX should be able to provide vector icons. Please let me know you you have any
trouble.

> 
> Is it okay to land this CL in case we can't get the assets from UX in time for
> the branch point?

Branch point is 2 weeks from now, so you should be able to get them, but if you
couldn't yes.

[Tim Song, 2017-07-07 19:54:04]

https://codereview.chromium.org/2968323002/diff/20001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_notification_controller.cc (right):

https://codereview.chromium.org/2968323002/diff/20001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_notification_controller.cc:40: profile,
message_center::MessageCenter::Get());
On 2017/07/07 18:36:58, Peter Beverloo wrote:
> Will this functionality stay restricted to Chrome OS? Note that you cannot
rely
> on there being a message center on the other platforms.

Yes. At the moment these notifications are only intended for ChromeOS.

https://codereview.chromium.org/2968323002/diff/20001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_notification_controller_chromeos.cc
(right):

https://codereview.chromium.org/2968323002/diff/20001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_notification_controller_chromeos.cc:38: const
message_center::RichNotificationData rich_notification_data) {
On 2017/07/07 18:36:58, Peter Beverloo wrote:
> nit: const& (saves the unnecessary copy)

Done.

https://codereview.chromium.org/2968323002/diff/20001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_notification_controller_chromeos.cc:55:
message_center_->AddObserver(this);
On 2017/07/07 18:36:58, Peter Beverloo wrote:
> Please provide a delegate to the Notification object as opposed to observing
the
> MC.

Done.

[sacomoto, 2017-07-11 18:01:53]

lgtm

[sacomoto, 2017-07-12 15:59:04]

https://codereview.chromium.org/2968323002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_notification_controller.h (right):

https://codereview.chromium.org/2968323002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_notification_controller.h:34: const
std::string& phone_name) = 0;
I'm working on the promotions, and I'll need a promotion notification. Can you
add it in this CL?

[Tim Song, 2017-07-13 20:46:33]

I added another notification for promoting EasyUnlock. Please take another look.

https://codereview.chromium.org/2968323002/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_notification_controller.h (right):

https://codereview.chromium.org/2968323002/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_notification_controller.h:34: const
std::string& phone_name) = 0;
On 2017/07/12 15:59:04, sacomoto wrote:
> I'm working on the promotions, and I'll need a promotion notification. Can you
> add it in this CL?

Done.

[sacomoto, 2017-07-17 19:53:44]

lgtm++

[Tim Song, 2017-07-18 00:43:05]

Ping on this code review. We would like to get it in by branch point :)

[sacomoto, 2017-07-19 10:01:06]

On 2017/07/18 00:43:05, Tim Song wrote:
> Ping on this code review. We would like to get it in by branch point :)

BTW, can you also add an notification controller interface in the proximity_auth
component?

[oshima, 2017-07-20 18:21:56]

On 2017/07/19 10:01:06, sacomoto wrote:
> On 2017/07/18 00:43:05, Tim Song wrote:
> > Ping on this code review. We would like to get it in by branch point :)
> 
> BTW, can you also add an notification controller interface in the
proximity_auth
> component?

lgtm this time, but please ask UX to provide svg. We're switching to svg for
good reason,
and I really want to avoid adding another png files, even for short term
solution.

[Tim Song, 2017-07-20 18:35:29]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2017-07-20 18:35:53]

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, 2017-07-20 19:24:47]

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

[commit-bot: I haz the power, 2017-07-20 19:24:48]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Tim Song, 2017-07-20 23:43:19]

The CQ bit was checked by tengs@chromium.org

[Tim Song, 2017-07-20 23:43:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
sacomoto@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2968323002/#ps80001
(title: "fix unittests")

[commit-bot: I haz the power, 2017-07-20 23:43:35]

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, 2017-07-21 00:55:58]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1500594199984430,
"parent_rev": "56fdb075eb79bb939d3541bc70225fe8441f7a7a", "commit_rev":
"321d5feb7b08e0e9b2b697de21438f10c7c004c3"}

[commit-bot: I haz the power, 2017-07-21 00:56:18]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1500594199984430,
"parent_rev": "bfc6ec6735cf439c2971eb2129ae357e5b57e0de", "commit_rev":
"b8c14e087eb455c3e712753d44eb8a2c62cba9f8"}

[commit-bot: I haz the power, 2017-07-21 00:56:37]

Description was changed from

==========
[EasyUnlock] Introduce EasyUnlockNotificationController

This class is responsible for displaying and handling all EasyUnlock
notifications.

Originally, EasyUnlock notifications were shown by a component app, but this app
is now deprecated and we are migrating EasyUnlock natively for v2.

BUG=726791
==========

to

==========
[EasyUnlock] Introduce EasyUnlockNotificationController

This class is responsible for displaying and handling all EasyUnlock
notifications.

Originally, EasyUnlock notifications were shown by a component app, but this app
is now deprecated and we are migrating EasyUnlock natively for v2.

BUG=726791

Review-Url: https://codereview.chromium.org/2968323002
Cr-Commit-Position: refs/heads/master@{#488522}
Committed:
https://chromium.googlesource.com/chromium/src/+/b8c14e087eb455c3e712753d44eb...
==========

[commit-bot: I haz the power, 2017-07-21 00:56:39]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b8c14e087eb455c3e712753d44eb...