arc: Use AndroidKioskAppInfoProto.display_name() as ARC++ Kiosk app name.

arc: Use AndroidKioskAppInfoProto.display_name() as ARC++ Kiosk app name.

As Chrome can't get proper user-friendly Android app name by package name
on sign in screen, this name is provided from management servers through
device policies as AndroidKioskAppInfoProto.display_name field,
that should be parsed and used as app name in Chrome UI.

If the field is not present, package_name is used as before.

BUG=679928
Review-Url: https://codereview.chromium.org/2691063002
Cr-Commit-Position: refs/heads/master@{#450966}
Committed: https://chromium.googlesource.com/chromium/src/+/cafece6e34023641054a86d9d145cab53ef0b5b3

[Sergey Poromov, 2017-02-13 17:13:27]

poromov@chromium.org changed reviewers:
+ bartfab@chromium.org, oshima@chromium.org, peletskyi@chromium.org

[Sergey Poromov, 2017-02-13 17:13:28]

Adding reviewers similar to http://crrev.com/2451793002 where most of the
modified code was introduced

bartfab@chromium.org: Please review changes in 
chrome/browser/chromeos/policy/
chrome/browser/chromeos/settings/

oshima@chromium.org: Please review changes in 
chrome/browser/chromeos/app_mode/arc/
chromeos/settings/

peletskyi@chromium.org: Please review as owner of the previous similar CL
(http://crrev.com/2451793002)

[Sergey Poromov, 2017-02-13 17:20:21]

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

[commit-bot: I haz the power, 2017-02-13 17:20:46]

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, 2017-02-13 18:14:38]

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

[commit-bot: I haz the power, 2017-02-13 18:14:39]

Dry run: This issue passed the CQ dry run.

[peletskyi2, 2017-02-14 14:11:54]

On 2017/02/13 17:13:28, Sergey Poromov wrote:
> Adding reviewers similar to http://crrev.com/2451793002 where most of the
> modified code was introduced
> 
> mailto:bartfab@chromium.org: Please review changes in 
> chrome/browser/chromeos/policy/
> chrome/browser/chromeos/settings/
> 
> mailto:oshima@chromium.org: Please review changes in 
> chrome/browser/chromeos/app_mode/arc/
> chromeos/settings/
> 
> mailto:peletskyi@chromium.org: Please review as owner of the previous similar
CL
> (http://crrev.com/2451793002)

LGTM

[bartfab (slow), 2017-02-14 17:17:56]

lgtm

[Sergey Poromov, 2017-02-15 10:36:25]

poromov@chromium.org changed reviewers:
+ achuith@chromium.org
- oshima@chromium.org

[achuithb, 2017-02-16 13:41:46]

lgtm

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc (right):

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc:194: for
(std::vector<policy::DeviceLocalAccount>::const_iterator it =
Might be worthwhile switching this to range-based for loop

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc:212: auto name =
!it->arc_kiosk_app_info.display_name().empty()
The type of name isn't obvious here - I don't think auto should be used.

I think this is easier to read and takes the same 3 lines:
std::string display_name = it->arc_kiosk_app_info.display_name();
if (display_name.empty)
  display_name = it->arc_kiosk_app_info.package_name();

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager_browsertest.cc
(right):

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager_browsertest.cc:148:
policy::ArcKioskAppBasicInfo app1("com.package.name1", std::string(),
I think ("com.package.name1", "", "", "") would be better here

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager_browsertest.cc:194:
policy::ArcKioskAppBasicInfo app3("com.package.name3", std::string(),
ditto

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_local_account.h (right):

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account.h:22: const std::string&
display_name);
Should display_name be the first arg?

[Sergey Poromov, 2017-02-16 14:27:12]

The CQ bit was checked by poromov@chromium.org

[Sergey Poromov, 2017-02-16 14:27:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, achuith@chromium.org, peletskyi@managedchrome.com
Link to the patchset: https://codereview.chromium.org/2691063002/#ps40001
(title: "achuith@ comments")

[commit-bot: I haz the power, 2017-02-16 14:27:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sergey Poromov, 2017-02-16 14:28:28]

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc (right):

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc:194: for
(std::vector<policy::DeviceLocalAccount>::const_iterator it =
On 2017/02/16 13:41:45, achuithb wrote:
> Might be worthwhile switching this to range-based for loop

Done.

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager.cc:212: auto name =
!it->arc_kiosk_app_info.display_name().empty()
On 2017/02/16 13:41:45, achuithb wrote:
> The type of name isn't obvious here - I don't think auto should be used.
> 
> I think this is easier to read and takes the same 3 lines:
> std::string display_name = it->arc_kiosk_app_info.display_name();
> if (display_name.empty)
>   display_name = it->arc_kiosk_app_info.package_name();

Done.

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager_browsertest.cc
(right):

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager_browsertest.cc:148:
policy::ArcKioskAppBasicInfo app1("com.package.name1", std::string(),
On 2017/02/16 13:41:45, achuithb wrote:
> I think ("com.package.name1", "", "", "") would be better here

Done.

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_manager_browsertest.cc:194:
policy::ArcKioskAppBasicInfo app3("com.package.name3", std::string(),
On 2017/02/16 13:41:45, achuithb wrote:
> ditto

Done.

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_local_account.h (right):

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account.h:22: const std::string&
display_name);
On 2017/02/16 13:41:46, achuithb wrote:
> Should display_name be the first arg?

I think not. It's the latest added value, optional on so on - everywhere it's
the last one.

[achuithb, 2017-02-16 14:30:51]

lgtm

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_local_account.h (right):

https://codereview.chromium.org/2691063002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account.h:22: const std::string&
display_name);
On 2017/02/16 14:28:28, Sergey Poromov wrote:
> On 2017/02/16 13:41:46, achuithb wrote:
> > Should display_name be the first arg?
> 
> I think not. It's the latest added value, optional on so on - everywhere it's
> the last one.

Acknowledged.

[commit-bot: I haz the power, 2017-02-16 15:11:55]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487255232744480,
"parent_rev": "eb46d6c8c032cbc0a22a5c0ab0af3cf92a9fad97", "commit_rev":
"cafece6e34023641054a86d9d145cab53ef0b5b3"}

[commit-bot: I haz the power, 2017-02-16 15:12:40]

Description was changed from

==========
arc: Use AndroidKioskAppInfoProto.display_name() as ARC++ Kiosk app name.

As Chrome can't get proper user-friendly Android app name by package name
on sign in screen, this name is provided from management servers through
device policies as AndroidKioskAppInfoProto.display_name field,
that should be parsed and used as app name in Chrome UI.

If the field is not present, package_name is used as before.

BUG=679928
==========

to

==========
arc: Use AndroidKioskAppInfoProto.display_name() as ARC++ Kiosk app name.

As Chrome can't get proper user-friendly Android app name by package name
on sign in screen, this name is provided from management servers through
device policies as AndroidKioskAppInfoProto.display_name field,
that should be parsed and used as app name in Chrome UI.

If the field is not present, package_name is used as before.

BUG=679928

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

[commit-bot: I haz the power, 2017-02-16 15:12:42]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/cafece6e34023641054a86d9d145...