arc: Make ChromeVox read an ARC custom notification

arc: Make notifications from Android read by ChromeVox.

BUG=644154
TEST=Go through notification list in message center with ChromeVox,
     then ChromeVox read ARC custom notifications

Committed: https://crrev.com/02f8ca650a8976e67ec89f0a9c87da70fc34ea2e
Cr-Commit-Position: refs/heads/master@{#417202}

[yhanada, 2016-09-06 06:05:34]

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

[commit-bot: I haz the power, 2016-09-06 06:05:49]

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-09-06 06:41:09]

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

[commit-bot: I haz the power, 2016-09-06 06:41:10]

Dry run: This issue passed the CQ dry run.

[yhanada, 2016-09-06 07:10:06]

yhanada@chromium.org changed reviewers:
+ xiyuan@chromium.org, yawano@chromium.org, yoshiki@chromium.org

[yhanada, 2016-09-06 07:10:06]

PTAL. Thanks.

[yawano, 2016-09-06 08:07:17]

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

https://codereview.chromium.org/2318463002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_item.cc:85: accessible_name_ =
base::UTF8ToUTF16(data.accessible_name.get());
You will need to set empty string to accessible_name_ if data.accessible_name is
null. If this has been updated with new data with accessible_name==null, this
can have stale accessible name.

[yhanada, 2016-09-06 10:15:01]

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

https://codereview.chromium.org/2318463002/diff/40001/ui/arc/notification/arc...
ui/arc/notification/arc_custom_notification_item.cc:85: accessible_name_ =
base::UTF8ToUTF16(data.accessible_name.get());
On 2016/09/06 08:07:16, yawano wrote:
> You will need to set empty string to accessible_name_ if data.accessible_name
is
> null. If this has been updated with new data with accessible_name==null, this
> can have stale accessible name.

Done.

[yawano, 2016-09-06 10:16:47]

lgtm

[xiyuan, 2016-09-06 16:07:27]

lgtm

[yoshiki, 2016-09-06 18:47:20]

I think NotificationView::SetAccessibleName() may override the accessible name
you've set.

How about adding CustomNotificationView::SetAccessibleName() and overriding
NotificationView::SetAccessibleName()? And I think it's ok to add a new field
named "custom_accessible_name" to message_center::Notification class, if you
need to pass the name.

[xiyuan, 2016-09-06 21:06:31]

CustomNotificationView derives from MessageView and is a sibling of
NotificationView in the class hierarchy. So NotificationView::SetAccessibleName
should not affect CustomNotificationView's |accessible_name_| member.

[yoshiki, 2016-09-07 03:07:06]

On 2016/09/06 21:06:31, xiyuan wrote:
> CustomNotificationView derives from MessageView and is a sibling of
> NotificationView in the class hierarchy. So
NotificationView::SetAccessibleName
> should not affect CustomNotificationView's |accessible_name_| member.

Good catch. 

Then, how about just moving NotificationView::SetAccessibleName() method to
MessageView class? I think doing the same way as NotificationView and sharing
the code are good for code readability.

What I meant is:
1. Add a method Notification::set_accessible_name()
2. Add a method Notification::GetAccessibleName(), which returns the accessible
name set by the above method or the fallen-back string (title + message + ...,
see NotificationView::SetAccessibleName) if the string is not set.
3. Move NotificationView::SetAccessibleName() to
MessageView::SetAccessibleName(), and use Notification::GetAccessibleName() in
the method.
4. Call MessageView::SetAccessibleName() from CustomNotificationView ctor and
CustomNotificationView::UpdateWithNotification (as same as NotificationView).

[yhanada, 2016-09-07 08:05:02]

On 2016/09/07 03:07:06, yoshiki wrote:
> On 2016/09/06 21:06:31, xiyuan wrote:
> > CustomNotificationView derives from MessageView and is a sibling of
> > NotificationView in the class hierarchy. So
> NotificationView::SetAccessibleName
> > should not affect CustomNotificationView's |accessible_name_| member.
> 
> Good catch. 
> 
> Then, how about just moving NotificationView::SetAccessibleName() method to
> MessageView class? I think doing the same way as NotificationView and sharing
> the code are good for code readability.
> 
> What I meant is:
> 1. Add a method Notification::set_accessible_name()
> 2. Add a method Notification::GetAccessibleName(), which returns the
accessible
> name set by the above method or the fallen-back string (title + message + ...,
> see NotificationView::SetAccessibleName) if the string is not set.
> 3. Move NotificationView::SetAccessibleName() to
> MessageView::SetAccessibleName(), and use Notification::GetAccessibleName() in
> the method.
> 4. Call MessageView::SetAccessibleName() from CustomNotificationView ctor and
> CustomNotificationView::UpdateWithNotification (as same as NotificationView).

I agree with you. I moved NotificationView::SetAccessibleName() to MessageView,
so
now NotificationView and CustomNotificationView shares the implementation about
accessible_name.

PTAL. Thanks!

[yoshiki, 2016-09-07 08:29:34]

Thanks! Almost there.

https://codereview.chromium.org/2318463002/diff/100001/ui/arc/notification/ar...
File ui/arc/notification/arc_custom_notification_item.cc (right):

https://codereview.chromium.org/2318463002/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_custom_notification_item.cc:68: if
(!data.accessible_name.is_null()) {
nit: no need of braces for one line body.

https://codereview.chromium.org/2318463002/diff/100001/ui/message_center/noti...
File ui/message_center/notification.cc (right):

https://codereview.chromium.org/2318463002/diff/100001/ui/message_center/noti...
ui/message_center/notification.cc:42: accessible_name(base::string16()) {}
nit: no need to initialize explicitly.

https://codereview.chromium.org/2318463002/diff/100001/ui/message_center/view...
File ui/message_center/views/message_view.cc (right):

https://codereview.chromium.org/2318463002/diff/100001/ui/message_center/view...
ui/message_center/views/message_view.cc:267: base::string16
MessageView::CreateAccessibleName(
Could you make this method non-member of the MessageView and move it to the
anonymous namespace block (around l.35), since this doesn't touch any members?

https://codereview.chromium.org/2318463002/diff/100001/ui/message_center/view...
ui/message_center/views/message_view.cc:269: if
(!notification.accessible_name().empty()) {
nit: no need of the braces for one line body.

[yhanada, 2016-09-07 08:52:26]

https://codereview.chromium.org/2318463002/diff/100001/ui/arc/notification/ar...
File ui/arc/notification/arc_custom_notification_item.cc (right):

https://codereview.chromium.org/2318463002/diff/100001/ui/arc/notification/ar...
ui/arc/notification/arc_custom_notification_item.cc:68: if
(!data.accessible_name.is_null()) {
On 2016/09/07 08:29:34, yoshiki wrote:
> nit: no need of braces for one line body.

Done.

https://codereview.chromium.org/2318463002/diff/100001/ui/message_center/noti...
File ui/message_center/notification.cc (right):

https://codereview.chromium.org/2318463002/diff/100001/ui/message_center/noti...
ui/message_center/notification.cc:42: accessible_name(base::string16()) {}
On 2016/09/07 08:29:34, yoshiki wrote:
> nit: no need to initialize explicitly.

Done.

https://codereview.chromium.org/2318463002/diff/100001/ui/message_center/view...
File ui/message_center/views/message_view.cc (right):

https://codereview.chromium.org/2318463002/diff/100001/ui/message_center/view...
ui/message_center/views/message_view.cc:267: base::string16
MessageView::CreateAccessibleName(
On 2016/09/07 08:29:34, yoshiki wrote:
> Could you make this method non-member of the MessageView and move it to the
> anonymous namespace block (around l.35), since this doesn't touch any members?
> 

Done.

https://codereview.chromium.org/2318463002/diff/100001/ui/message_center/view...
ui/message_center/views/message_view.cc:269: if
(!notification.accessible_name().empty()) {
On 2016/09/07 08:29:34, yoshiki wrote:
> nit: no need of the braces for one line body.

Done.

[yoshiki, 2016-09-07 08:57:46]

lgtm with nits

https://codereview.chromium.org/2318463002/diff/120001/ui/message_center/noti...
File ui/message_center/notification.cc (right):

https://codereview.chromium.org/2318463002/diff/120001/ui/message_center/noti...
ui/message_center/notification.cc:42: accessible_name() {}
nit: you can remove this change completely. We don't need to do anything at all
in the ctor.

https://codereview.chromium.org/2318463002/diff/120001/ui/message_center/view...
File ui/message_center/views/message_view.cc (right):

https://codereview.chromium.org/2318463002/diff/120001/ui/message_center/view...
ui/message_center/views/message_view.cc:37: // in the notification.
nit: could you move "in the" to the first line in the comment?

[yhanada, 2016-09-07 09:08:04]

Thank you for reviewing!

https://codereview.chromium.org/2318463002/diff/120001/ui/message_center/noti...
File ui/message_center/notification.cc (right):

https://codereview.chromium.org/2318463002/diff/120001/ui/message_center/noti...
ui/message_center/notification.cc:42: accessible_name() {}
On 2016/09/07 08:57:46, yoshiki wrote:
> nit: you can remove this change completely. We don't need to do anything at
all
> in the ctor.

Done.

https://codereview.chromium.org/2318463002/diff/120001/ui/message_center/view...
File ui/message_center/views/message_view.cc (right):

https://codereview.chromium.org/2318463002/diff/120001/ui/message_center/view...
ui/message_center/views/message_view.cc:37: // in the notification.
On 2016/09/07 08:57:46, yoshiki wrote:
> nit: could you move "in the" to the first line in the comment?

Done.

[yhanada, 2016-09-07 09:08:40]

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

[yhanada, 2016-09-07 09:08:40]

dcheng@: could you review notifications.mojom change? Thanks!

[yhanada, 2016-09-07 09:08:41]

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

[yhanada, 2016-09-07 09:08:41]

dcheng@: could you review notifications.mojom change? Thanks!

[yhanada, 2016-09-07 09:08:43]

dcheng@: could you review notifications.mojom change? Thanks!

[yhanada, 2016-09-07 09:11:14]

oops... sorry for annoying duplicate comments

[xiyuan, 2016-09-07 16:26:08]

https://codereview.chromium.org/2318463002/diff/140001/ui/message_center/noti...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/2318463002/diff/140001/ui/message_center/noti...
ui/message_center/notification.h:228: base::string16 accessible_name() const {
nit: const base::string16& to avoid copying ?

[dcheng, 2016-09-07 22:15:02]

mojo lgtm

https://codereview.chromium.org/2318463002/diff/140001/ui/message_center/view...
File ui/message_center/views/message_view.cc (right):

https://codereview.chromium.org/2318463002/diff/140001/ui/message_center/view...
ui/message_center/views/message_view.cc:44: std::vector<base::string16>
accessible_lines;
It might be slightly more readable to use an initializer list here:

std::vector<base::string16> accessible_lines = {
  notification.title(),
  notification.message(),
  notification.context_message(),
};

[yhanada, 2016-09-08 03:37:24]

Thank you for reviewing.

https://codereview.chromium.org/2318463002/diff/140001/ui/message_center/noti...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/2318463002/diff/140001/ui/message_center/noti...
ui/message_center/notification.h:228: base::string16 accessible_name() const {
On 2016/09/07 16:26:08, xiyuan wrote:
> nit: const base::string16& to avoid copying ?

Done.

https://codereview.chromium.org/2318463002/diff/140001/ui/message_center/view...
File ui/message_center/views/message_view.cc (right):

https://codereview.chromium.org/2318463002/diff/140001/ui/message_center/view...
ui/message_center/views/message_view.cc:44: std::vector<base::string16>
accessible_lines;
On 2016/09/07 22:15:02, dcheng wrote:
> It might be slightly more readable to use an initializer list here:
> 
> std::vector<base::string16> accessible_lines = {
>   notification.title(),
>   notification.message(),
>   notification.context_message(),
> };

Done.

[yhanada, 2016-09-08 03:43:20]

The CQ bit was checked by yhanada@chromium.org

[yhanada, 2016-09-08 03:43:21]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
yawano@chromium.org, yoshiki@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2318463002/#ps160001
(title: "rebase and cleanup")

[commit-bot: I haz the power, 2016-09-08 03:43:40]

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-08 05:51:11]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-08 05:53:51]

Description was changed from

==========
arc: Make notifications from Android read by ChromeVox.

BUG=644154
TEST=Go through notification list in message center with ChromeVox,
     then ChromeVox read ARC custom notifications
==========

to

==========
arc: Make notifications from Android read by ChromeVox.

BUG=644154
TEST=Go through notification list in message center with ChromeVox,
     then ChromeVox read ARC custom notifications

Committed: https://crrev.com/02f8ca650a8976e67ec89f0a9c87da70fc34ea2e
Cr-Commit-Position: refs/heads/master@{#417202}
==========

[commit-bot: I haz the power, 2016-09-08 05:53:52]

Patchset 9 (id:??) landed as
https://crrev.com/02f8ca650a8976e67ec89f0a9c87da70fc34ea2e
Cr-Commit-Position: refs/heads/master@{#417202}