Create org.chromium.KioskAppService.

Create org.chromium.KioskAppService.

This creates a D-Bus service that extracts a "Kiosk app" related method
from LibCrosService. Clients have to be migrated to this before the
corresponding method may be removed from LibCrosService.

BUG=703229
TEST=deploy_chrome, manually sent a message to new method, got response.

Review-Url: https://codereview.chromium.org/2798703002
Cr-Commit-Position: refs/heads/master@{#462653}
Committed: https://chromium.googlesource.com/chromium/src/+/6e2e787479f591d24386ba91d8bbf3d1cfbe1fd3

[teravest, 2017-04-04 21:03:12]

teravest@chromium.org changed reviewers:
+ derat@chromium.org

[teravest, 2017-04-04 21:26:16]

Description was changed from

==========
Create org.chromium.KioskAppService.

This creates a D-Bus service that extracts a "Kiosk app" related method
from LibCrosService. Clients have to be migrated to this before the
corresponding method may be removed from LibCrosService.

BUG=703229
TESTED=deploy_chrome, manually sent a message to new method, got response.
==========

to

==========
Create org.chromium.KioskAppService.

This creates a D-Bus service that extracts a "Kiosk app" related method
from LibCrosService. Clients have to be migrated to this before the
corresponding method may be removed from LibCrosService.

BUG=703229
TEST=deploy_chrome, manually sent a message to new method, got response.
==========

[Daniel Erat, 2017-04-05 02:55:41]

lgtm with nits

https://codereview.chromium.org/2798703002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2798703002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:264:
std::move(kiosk_service_providers));
nit: no idea how clang-format will indent it, but how do things look if you
instead create the ServiceProviderList inline in the CrosDBusService::Create()
call? it feels like most/all of these service will just have a single provider,
so it might be nice to get rid of the local list vars.

https://codereview.chromium.org/2798703002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:332:
std::unique_ptr<CrosDBusService> kiosk_info_service_;
nit: add a blank line above this line to make it clear that the comments above
aren't referring to it

https://codereview.chromium.org/2798703002/diff/20001/chromeos/dbus/services/...
File chromeos/dbus/services/org.chromium.KioskAppService.conf (right):

https://codereview.chromium.org/2798703002/diff/20001/chromeos/dbus/services/...
chromeos/dbus/services/org.chromium.KioskAppService.conf:4: Copyright (c) 2017
The Chromium Authors. All rights reserved.
nit: drop the "(c)"

https://codereview.chromium.org/2798703002/diff/20001/chromeos/dbus/services/...
chromeos/dbus/services/org.chromium.KioskAppService.conf:8: This file will be
installed at /opt/google/chrome/dbus on Chromium OS.
nit: "Chrome OS" (since it's referring to the product rather than the project)

https://codereview.chromium.org/2798703002/diff/20001/chromeos/dbus/services/...
chromeos/dbus/services/org.chromium.KioskAppService.conf:14: 
nit: might be worthwhile adding a comment here describing what it's called by
(session_manager, right?)

[teravest, 2017-04-05 15:58:21]

https://codereview.chromium.org/2798703002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2798703002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:264:
std::move(kiosk_service_providers));
On 2017/04/05 02:55:41, Daniel Erat wrote:
> nit: no idea how clang-format will indent it, but how do things look if you
> instead create the ServiceProviderList inline in the CrosDBusService::Create()
> call? it feels like most/all of these service will just have a single
provider,
> so it might be nice to get rid of the local list vars.

I wrestled with this, but can't figure out how to create the vector inline.
push_back() seems to behave well with unique_ptr; list initialization always
tries to make a copy of the
unique_ptr<CrosDBusService::ServiceProviderInterface>, even when I wrap it in
std::move().

https://codereview.chromium.org/2798703002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:332:
std::unique_ptr<CrosDBusService> kiosk_info_service_;
On 2017/04/05 02:55:41, Daniel Erat wrote:
> nit: add a blank line above this line to make it clear that the comments above
> aren't referring to it

Done.

https://codereview.chromium.org/2798703002/diff/20001/chromeos/dbus/services/...
File chromeos/dbus/services/org.chromium.KioskAppService.conf (right):

https://codereview.chromium.org/2798703002/diff/20001/chromeos/dbus/services/...
chromeos/dbus/services/org.chromium.KioskAppService.conf:4: Copyright (c) 2017
The Chromium Authors. All rights reserved.
On 2017/04/05 02:55:41, Daniel Erat wrote:
> nit: drop the "(c)"

Done.

https://codereview.chromium.org/2798703002/diff/20001/chromeos/dbus/services/...
chromeos/dbus/services/org.chromium.KioskAppService.conf:8: This file will be
installed at /opt/google/chrome/dbus on Chromium OS.
On 2017/04/05 02:55:41, Daniel Erat wrote:
> nit: "Chrome OS" (since it's referring to the product rather than the project)

Done.

https://codereview.chromium.org/2798703002/diff/20001/chromeos/dbus/services/...
chromeos/dbus/services/org.chromium.KioskAppService.conf:14: 
On 2017/04/05 02:55:41, Daniel Erat wrote:
> nit: might be worthwhile adding a comment here describing what it's called by
> (session_manager, right?)

Done.

[Daniel Erat, 2017-04-05 18:18:00]

https://codereview.chromium.org/2798703002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/2798703002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:264:
std::move(kiosk_service_providers));
On 2017/04/05 15:58:20, teravest wrote:
> On 2017/04/05 02:55:41, Daniel Erat wrote:
> > nit: no idea how clang-format will indent it, but how do things look if you
> > instead create the ServiceProviderList inline in the
CrosDBusService::Create()
> > call? it feels like most/all of these service will just have a single
> provider,
> > so it might be nice to get rid of the local list vars.
> 
> I wrestled with this, but can't figure out how to create the vector inline.
> push_back() seems to behave well with unique_ptr; list initialization always
> tries to make a copy of the
> unique_ptr<CrosDBusService::ServiceProviderInterface>, even when I wrap it in
> std::move().

hmm. i take it that just passing e.g.

  {base::MakeUnique<KioskInfoService>(...)}

doesn't work either? i don't really understand the rules for when a
std::unique_ptr can be upcast, so

  {base::WrapUnique<ServiceProviderInterface>(
      new KioskInfoService(...))}

might be worth trying too if you haven't already. sigh.

[teravest, 2017-04-06 15:55:07]

On 2017/04/05 18:18:00, Daniel Erat wrote:
>
https://codereview.chromium.org/2798703002/diff/20001/chrome/browser/chromeos...
> File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):
> 
>
https://codereview.chromium.org/2798703002/diff/20001/chrome/browser/chromeos...
> chrome/browser/chromeos/chrome_browser_main_chromeos.cc:264:
> std::move(kiosk_service_providers));
> On 2017/04/05 15:58:20, teravest wrote:
> > On 2017/04/05 02:55:41, Daniel Erat wrote:
> > > nit: no idea how clang-format will indent it, but how do things look if
you
> > > instead create the ServiceProviderList inline in the
> CrosDBusService::Create()
> > > call? it feels like most/all of these service will just have a single
> > provider,
> > > so it might be nice to get rid of the local list vars.
> > 
> > I wrestled with this, but can't figure out how to create the vector inline.
> > push_back() seems to behave well with unique_ptr; list initialization always
> > tries to make a copy of the
> > unique_ptr<CrosDBusService::ServiceProviderInterface>, even when I wrap it
in
> > std::move().
> 
> hmm. i take it that just passing e.g.
> 
>   {base::MakeUnique<KioskInfoService>(...)}
> 
> doesn't work either? i don't really understand the rules for when a
> std::unique_ptr can be upcast, so

That's what I tried first, and then wrapping the element and the vector with
std::move, to no avail.

> 
>   {base::WrapUnique<ServiceProviderInterface>(
>       new KioskInfoService(...))}
> 
> might be worth trying too if you haven't already. sigh.

I feel like it's not a casting issue, but rather that the {} initialization
syntax is always incurring a copy, which we can't do with unique_ptr?

Reading here:
http://devdocs.io/cpp/utility/initializer_list

"The underlying array is a temporary array (until C++17) materialized array
prvalue (since C++17) of type const T[N], in which each element is
copy-initialized (except that narrowing conversions are invalid) from the
corresponding element of the original initializer list."

[Daniel Erat, 2017-04-06 16:27:19]

On 2017/04/06 15:55:07, teravest wrote:
> On 2017/04/05 18:18:00, Daniel Erat wrote:
> >
>
https://codereview.chromium.org/2798703002/diff/20001/chrome/browser/chromeos...
> > File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):
> > 
> >
>
https://codereview.chromium.org/2798703002/diff/20001/chrome/browser/chromeos...
> > chrome/browser/chromeos/chrome_browser_main_chromeos.cc:264:
> > std::move(kiosk_service_providers));
> > On 2017/04/05 15:58:20, teravest wrote:
> > > On 2017/04/05 02:55:41, Daniel Erat wrote:
> > > > nit: no idea how clang-format will indent it, but how do things look if
> you
> > > > instead create the ServiceProviderList inline in the
> > CrosDBusService::Create()
> > > > call? it feels like most/all of these service will just have a single
> > > provider,
> > > > so it might be nice to get rid of the local list vars.
> > > 
> > > I wrestled with this, but can't figure out how to create the vector
inline.
> > > push_back() seems to behave well with unique_ptr; list initialization
always
> > > tries to make a copy of the
> > > unique_ptr<CrosDBusService::ServiceProviderInterface>, even when I wrap it
> in
> > > std::move().
> > 
> > hmm. i take it that just passing e.g.
> > 
> >   {base::MakeUnique<KioskInfoService>(...)}
> > 
> > doesn't work either? i don't really understand the rules for when a
> > std::unique_ptr can be upcast, so
> 
> That's what I tried first, and then wrapping the element and the vector with
> std::move, to no avail.
> 
> > 
> >   {base::WrapUnique<ServiceProviderInterface>(
> >       new KioskInfoService(...))}
> > 
> > might be worth trying too if you haven't already. sigh.
> 
> I feel like it's not a casting issue, but rather that the {} initialization
> syntax is always incurring a copy, which we can't do with unique_ptr?
> 
> Reading here:
> http://devdocs.io/cpp/utility/initializer_list
> 
> "The underlying array is a temporary array (until C++17) materialized array
> prvalue (since C++17) of type const T[N], in which each element is
> copy-initialized (except that narrowing conversions are invalid) from the
> corresponding element of the original initializer list."

ah, that's a bummer. if you're okay with adding a tiny helper function that
takes a unique_ptr and moves it into and returns a provider list, that's great
(since i think we're going to be using this pattern a lot), but lgtm as-is too.

[teravest, 2017-04-06 17:29:30]

On 2017/04/06 16:27:19, Daniel Erat wrote:
> On 2017/04/06 15:55:07, teravest wrote:
> > On 2017/04/05 18:18:00, Daniel Erat wrote:
> > >
> >
>
https://codereview.chromium.org/2798703002/diff/20001/chrome/browser/chromeos...
> > > File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2798703002/diff/20001/chrome/browser/chromeos...
> > > chrome/browser/chromeos/chrome_browser_main_chromeos.cc:264:
> > > std::move(kiosk_service_providers));
> > > On 2017/04/05 15:58:20, teravest wrote:
> > > > On 2017/04/05 02:55:41, Daniel Erat wrote:
> > > > > nit: no idea how clang-format will indent it, but how do things look
if
> > you
> > > > > instead create the ServiceProviderList inline in the
> > > CrosDBusService::Create()
> > > > > call? it feels like most/all of these service will just have a single
> > > > provider,
> > > > > so it might be nice to get rid of the local list vars.
> > > > 
> > > > I wrestled with this, but can't figure out how to create the vector
> inline.
> > > > push_back() seems to behave well with unique_ptr; list initialization
> always
> > > > tries to make a copy of the
> > > > unique_ptr<CrosDBusService::ServiceProviderInterface>, even when I wrap
it
> > in
> > > > std::move().
> > > 
> > > hmm. i take it that just passing e.g.
> > > 
> > >   {base::MakeUnique<KioskInfoService>(...)}
> > > 
> > > doesn't work either? i don't really understand the rules for when a
> > > std::unique_ptr can be upcast, so
> > 
> > That's what I tried first, and then wrapping the element and the vector with
> > std::move, to no avail.
> > 
> > > 
> > >   {base::WrapUnique<ServiceProviderInterface>(
> > >       new KioskInfoService(...))}
> > > 
> > > might be worth trying too if you haven't already. sigh.
> > 
> > I feel like it's not a casting issue, but rather that the {} initialization
> > syntax is always incurring a copy, which we can't do with unique_ptr?
> > 
> > Reading here:
> > http://devdocs.io/cpp/utility/initializer_list
> > 
> > "The underlying array is a temporary array (until C++17) materialized array
> > prvalue (since C++17) of type const T[N], in which each element is
> > copy-initialized (except that narrowing conversions are invalid) from the
> > corresponding element of the original initializer list."
> 
> ah, that's a bummer. if you're okay with adding a tiny helper function that
> takes a unique_ptr and moves it into and returns a provider list, that's great
> (since i think we're going to be using this pattern a lot), but lgtm as-is
too.

No problem, done. Let me know if you'd like a different name.

[Daniel Erat, 2017-04-06 18:13:34]

lgtm

thanks!

https://codereview.chromium.org/2798703002/diff/60001/chromeos/dbus/services/...
File chromeos/dbus/services/cros_dbus_service.cc (right):

https://codereview.chromium.org/2798703002/diff/60001/chromeos/dbus/services/...
chromeos/dbus/services/cros_dbus_service.cc:118: ServiceProviderList l;
nit: should maybe be 'list' per style guide (but i sympathize) :-P

https://codereview.chromium.org/2798703002/diff/60001/chromeos/dbus/services/...
File chromeos/dbus/services/cros_dbus_service.h (right):

https://codereview.chromium.org/2798703002/diff/60001/chromeos/dbus/services/...
chromeos/dbus/services/cros_dbus_service.h:53: static ServiceProviderList
SingleProviderList(
nit: how about CreateSingleProviderList (or even just
CreateServiceProviderList)? it's hard to tell that it isn't a class name
otherwise.

[teravest, 2017-04-06 20:18:33]

https://codereview.chromium.org/2798703002/diff/60001/chromeos/dbus/services/...
File chromeos/dbus/services/cros_dbus_service.cc (right):

https://codereview.chromium.org/2798703002/diff/60001/chromeos/dbus/services/...
chromeos/dbus/services/cros_dbus_service.cc:118: ServiceProviderList l;
On 2017/04/06 18:13:34, Daniel Erat wrote:
> nit: should maybe be 'list' per style guide (but i sympathize) :-P

Done.

https://codereview.chromium.org/2798703002/diff/60001/chromeos/dbus/services/...
File chromeos/dbus/services/cros_dbus_service.h (right):

https://codereview.chromium.org/2798703002/diff/60001/chromeos/dbus/services/...
chromeos/dbus/services/cros_dbus_service.h:53: static ServiceProviderList
SingleProviderList(
On 2017/04/06 18:13:34, Daniel Erat wrote:
> nit: how about CreateSingleProviderList (or even just
> CreateServiceProviderList)? it's hard to tell that it isn't a class name
> otherwise.

Done.

[Daniel Erat, 2017-04-06 20:23:38]

lgtm

[teravest, 2017-04-06 20:31:12]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2017-04-06 20:33:22]

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-04-06 22:30:30]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491510672060830,
"parent_rev": "fd6c74121796da3012155a89c16ada1332736a06", "commit_rev":
"6e2e787479f591d24386ba91d8bbf3d1cfbe1fd3"}

[commit-bot: I haz the power, 2017-04-06 22:31:28]

Description was changed from

==========
Create org.chromium.KioskAppService.

This creates a D-Bus service that extracts a "Kiosk app" related method
from LibCrosService. Clients have to be migrated to this before the
corresponding method may be removed from LibCrosService.

BUG=703229
TEST=deploy_chrome, manually sent a message to new method, got response.
==========

to

==========
Create org.chromium.KioskAppService.

This creates a D-Bus service that extracts a "Kiosk app" related method
from LibCrosService. Clients have to be migrated to this before the
corresponding method may be removed from LibCrosService.

BUG=703229
TEST=deploy_chrome, manually sent a message to new method, got response.

Review-Url: https://codereview.chromium.org/2798703002
Cr-Commit-Position: refs/heads/master@{#462653}
Committed:
https://chromium.googlesource.com/chromium/src/+/6e2e787479f591d24386ba91d8bb...
==========

[commit-bot: I haz the power, 2017-04-06 22:31:29]

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

[teravest, 2017-04-07 17:00:44]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2799963004/ by teravest@chromium.org.

The reason for reverting is: Appears to have broken the simplechrome workflow,
maybe all of the dependent changes aren't in yet?.

[James Cook, 2017-04-07 17:11:03]

On 2017/04/07 17:00:44, teravest wrote:
> A revert of this CL (patchset #5 id:80001) has been created in
> https://codereview.chromium.org/2799963004/ by mailto:teravest@chromium.org.
> 
> The reason for reverting is: Appears to have broken the simplechrome workflow,
> maybe all of the dependent changes aren't in yet?.

FYI - chrome starts up properly with this CL in place if I use the latest canary
OS image. The breakage might be that people are using OS images that are too
old.

[teravest, 2017-04-07 18:54:30]

On 2017/04/07 17:11:03, James Cook wrote:
> On 2017/04/07 17:00:44, teravest wrote:
> > A revert of this CL (patchset #5 id:80001) has been created in
> > https://codereview.chromium.org/2799963004/ by mailto:teravest@chromium.org.
> > 
> > The reason for reverting is: Appears to have broken the simplechrome
workflow,
> > maybe all of the dependent changes aren't in yet?.
> 
> FYI - chrome starts up properly with this CL in place if I use the latest
canary
> OS image. The breakage might be that people are using OS images that are too
> old.

Thanks for checking. Do you know, generally, how old we allow OS images to be
for using simple chrome?

[Daniel Erat, 2017-04-07 20:18:46]

On 2017/04/07 18:54:30, teravest wrote:
> On 2017/04/07 17:11:03, James Cook wrote:
> > On 2017/04/07 17:00:44, teravest wrote:
> > > A revert of this CL (patchset #5 id:80001) has been created in
> > > https://codereview.chromium.org/2799963004/ by
mailto:teravest@chromium.org.
> > > 
> > > The reason for reverting is: Appears to have broken the simplechrome
> workflow,
> > > maybe all of the dependent changes aren't in yet?.
> > 
> > FYI - chrome starts up properly with this CL in place if I use the latest
> canary
> > OS image. The breakage might be that people are using OS images that are too
> > old.
> 
> Thanks for checking. Do you know, generally, how old we allow OS images to be
> for using simple chrome?

(to answer the question, stevenjb@ and i concluded that a week is probably
reasonable.)