arc: add enterprise_reporting.mojom interface.

arc: add enterprise_reporting.mojom interface.

This allows ARC to clear user data.

BUG=629574
TEST=manual

Committed: https://crrev.com/9282f59bec8693034cdf390927f41cfd2ea4c06c
Cr-Commit-Position: refs/heads/master@{#407771}

[Polina Bondarenko, 2016-07-19 18:39:01]

Description was changed from

==========
arc: add user_data.mojom interface.

This allowes ARC to clear user data.

BUG=629574
==========

to

==========
arc: add user_data.mojom interface.

This allows ARC to clear user data.

BUG=629574
==========

[Polina Bondarenko, 2016-07-19 18:39:56]

Patchset #1 (id:1) has been deleted

[Polina Bondarenko, 2016-07-19 18:40:19]

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

[commit-bot: I haz the power, 2016-07-19 18:40:55]

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

[Polina Bondarenko, 2016-07-19 18:41:28]

pbond@chromium.org changed reviewers:
+ bartfab@chromium.org

[commit-bot: I haz the power, 2016-07-19 19:43:56]

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

[commit-bot: I haz the power, 2016-07-19 19:43:57]

Dry run: This issue passed the CQ dry run.

[Polina Bondarenko, 2016-07-20 14:49:54]

Patchset #2 (id:40001) has been deleted

[Polina Bondarenko, 2016-07-20 14:50:32]

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

[commit-bot: I haz the power, 2016-07-20 14:50:53]

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-07-20 15:57:17]

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

[commit-bot: I haz the power, 2016-07-20 15:57:17]

Dry run: This issue passed the CQ dry run.

[uekawa-, 2016-07-21 11:55:37]

uekawa@google.com changed reviewers:
+ dspaid@chromium.org, uekawa@google.com

[uekawa-, 2016-07-21 11:55:38]

drive-by question

https://codereview.chromium.org/2165643004/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_watchdog_service.cc (right):

https://codereview.chromium.org/2165643004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_watchdog_service.cc:36:
weak_factory_.GetWeakPtr()));
is it safe to immediately restart ?
I recall there was a race condition that we wanted to fix somewhere.

[Luis Héctor Chávez, 2016-07-21 16:19:53]

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

[Luis Héctor Chávez, 2016-07-21 16:19:54]

https://codereview.chromium.org/2165643004/diff/100001/components/arc/common/...
File components/arc/common/watchdog.mojom (right):

https://codereview.chromium.org/2165643004/diff/100001/components/arc/common/...
components/arc/common/watchdog.mojom:12: interface WatchdogInstance {
Hmmm the name of this interface does not match what it does (I thought it would
detect the lifetime of ARC or something).

Can you rename it to UserData{Host,Instance}?

Also, maybe discuss this with dspaid@, but is it possible to merge it with
https://cs.chromium.org/chromium/src/components/arc/user_data/arc_user_data_s...
? They both seem to be doing mostly the same thing.

[Polina Bondarenko, 2016-07-21 16:58:49]

On 2016/07/21 16:19:54, Luis Héctor Chávez wrote:
>
https://codereview.chromium.org/2165643004/diff/100001/components/arc/common/...
> File components/arc/common/watchdog.mojom (right):
> 
>
https://codereview.chromium.org/2165643004/diff/100001/components/arc/common/...
> components/arc/common/watchdog.mojom:12: interface WatchdogInstance {
> Hmmm the name of this interface does not match what it does (I thought it
would
> detect the lifetime of ARC or something).
> 
> Can you rename it to UserData{Host,Instance}?
> 
> Also, maybe discuss this with dspaid@, but is it possible to merge it with
>
https://cs.chromium.org/chromium/src/components/arc/user_data/arc_user_data_s...
> ? They both seem to be doing mostly the same thing.

Agree, it is a better place for this, but it can't call ArcAuthService methods
and shutdown bridge safely. BTW when we started tasting it, this implementation
crashes:(

[Polina Bondarenko, 2016-07-21 18:05:26]

Patchset #5 (id:120001) has been deleted

[Polina Bondarenko, 2016-07-21 18:07:25]

Renamed: watchdog.mojom --> user_data.mojom,
         ArcWatchdogService --> ArcUserDataHost.

Works after rebase.

[Polina Bondarenko, 2016-07-21 18:08:47]

Description was changed from

==========
arc: add user_data.mojom interface.

This allows ARC to clear user data.

BUG=629574
==========

to

==========
arc: add user_data.mojom interface.

This allows ARC to clear user data.

BUG=629574
TEST=manual
==========

[Polina Bondarenko, 2016-07-21 18:17:41]

pbond@chromium.org changed reviewers:
+ mdempsky@chromium.org

[Polina Bondarenko, 2016-07-21 18:17:42]

Hi,

mdempsky@ please review changes in
components/arc/common/user_data.mojom
components/arc/common/arc_bridge.mojom

lhchavez@, dspaid@ please review changes in
components/arc/* files
chrome/browser/chromeos/arc/* files

[Luis Héctor Chávez, 2016-07-21 19:44:17]

On 2016/07/21 16:58:49, Polina Bondarenko wrote:
> On 2016/07/21 16:19:54, Luis Héctor Chávez wrote:
> >
>
https://codereview.chromium.org/2165643004/diff/100001/components/arc/common/...
> > File components/arc/common/watchdog.mojom (right):
> > 
> >
>
https://codereview.chromium.org/2165643004/diff/100001/components/arc/common/...
> > components/arc/common/watchdog.mojom:12: interface WatchdogInstance {
> > Hmmm the name of this interface does not match what it does (I thought it
> would
> > detect the lifetime of ARC or something).
> > 
> > Can you rename it to UserData{Host,Instance}?
> > 
> > Also, maybe discuss this with dspaid@, but is it possible to merge it with
> >
>
https://cs.chromium.org/chromium/src/components/arc/user_data/arc_user_data_s...
> > ? They both seem to be doing mostly the same thing.
> 
> Agree, it is a better place for this, but it can't call ArcAuthService methods
> and shutdown bridge safely. BTW when we started tasting it, this
implementation
> crashes:(

Can you elaborate why it cannot call them? If it's just a problem of user_data
being in components/arc, then it's better to move the current ArcUserDataService
to the location you are adding the new file to and then you should be able to
merge them cleanly.

[Luis Héctor Chávez, 2016-07-21 19:44:27]

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
File components/arc/common/arc_bridge.mojom (right):

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
components/arc/common/arc_bridge.mojom:28: // Next MinVersion: 15
Next MinVersion: 16

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
File components/arc/common/user_data.mojom (right):

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
components/arc/common/user_data.mojom:3: // found in the LICENSE file.
// Next MinVersion: 1

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
components/arc/common/user_data.mojom:7: interface UserDataHost {
// Next Method ID: 1

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
components/arc/common/user_data.mojom:9: ClearArcData();
ClearArcData@0

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
components/arc/common/user_data.mojom:12: interface UserDataInstance {
// Next Method ID: 1

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
components/arc/common/user_data.mojom:14: Init(UserDataHost host_ptr);
Init@0

[Polina Bondarenko, 2016-07-22 08:06:57]

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

[commit-bot: I haz the power, 2016-07-22 08:07:15]

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-07-22 08:08:58]

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

[commit-bot: I haz the power, 2016-07-22 08:08:59]

Dry run: Try jobs failed on following builders:
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Polina Bondarenko, 2016-07-22 08:11:42]

> 
> Can you elaborate why it cannot call them? If it's just a problem of user_data
> being in components/arc, then it's better to move the current
ArcUserDataService
> to the location you are adding the new file to and then you should be able to
> merge them cleanly.

It could not call ArcAuthService because of the location.

Fixed. Got rid of calling to ArcAuthService. As it was suggested, put
ClearArcData() to ArcUserDataService, restarted bridge directly.

[Polina Bondarenko, 2016-07-22 08:12:10]

Fixed mojom comments.

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
File components/arc/common/arc_bridge.mojom (right):

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
components/arc/common/arc_bridge.mojom:28: // Next MinVersion: 15
On 2016/07/21 19:44:27, Luis Héctor Chávez wrote:
> Next MinVersion: 16

Done.

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
File components/arc/common/user_data.mojom (right):

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
components/arc/common/user_data.mojom:3: // found in the LICENSE file.
On 2016/07/21 19:44:27, Luis Héctor Chávez wrote:
> // Next MinVersion: 1

Done.

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
components/arc/common/user_data.mojom:7: interface UserDataHost {
On 2016/07/21 19:44:27, Luis Héctor Chávez wrote:
> // Next Method ID: 1

Done.

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
components/arc/common/user_data.mojom:9: ClearArcData();
On 2016/07/21 19:44:27, Luis Héctor Chávez wrote:
> ClearArcData@0

Done.

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
components/arc/common/user_data.mojom:12: interface UserDataInstance {
On 2016/07/21 19:44:27, Luis Héctor Chávez wrote:
> // Next Method ID: 1

Done.

https://codereview.chromium.org/2165643004/diff/140001/components/arc/common/...
components/arc/common/user_data.mojom:14: Init(UserDataHost host_ptr);
On 2016/07/21 19:44:27, Luis Héctor Chávez wrote:
> Init@0

Done.

[Polina Bondarenko, 2016-07-22 08:16:55]

https://codereview.chromium.org/2165643004/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_watchdog_service.cc (right):

https://codereview.chromium.org/2165643004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_watchdog_service.cc:36:
weak_factory_.GetWeakPtr()));
On 2016/07/21 11:55:38, uekawa- wrote:
> is it safe to immediately restart ?
> I recall there was a race condition that we wanted to fix somewhere.

Fixed a CL, but the call, that you're talking about, is still in place. I think,
it is safe to restart a bridge. ArcBridgeService::reconnect_ is set to false
when Shutdown() was called explicitly, thus it doesn't try to reconnect itself.

[Polina Bondarenko, 2016-07-22 08:22:36]

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

[commit-bot: I haz the power, 2016-07-22 08:22: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-07-22 09:11:26]

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

[commit-bot: I haz the power, 2016-07-22 09:11:27]

Dry run: This issue passed the CQ dry run.

[Polina Bondarenko, 2016-07-22 14:15:18]

Patchset #9 (id:220001) has been deleted

[Polina Bondarenko, 2016-07-22 14:18:50]

Patchset #9 (id:240001) has been deleted

[Polina Bondarenko, 2016-07-22 14:18:56]

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

[commit-bot: I haz the power, 2016-07-22 14:19:09]

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

[Polina Bondarenko, 2016-07-22 14:24:23]

Patchset #9 (id:260001) has been deleted

[Polina Bondarenko, 2016-07-22 14:24:34]

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

[commit-bot: I haz the power, 2016-07-22 14:24:45]

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

[Polina Bondarenko, 2016-07-22 14:43:09]

Renamed mojom to enterprise_reporting.

[Polina Bondarenko, 2016-07-22 14:43:41]

Description was changed from

==========
arc: add user_data.mojom interface.

This allows ARC to clear user data.

BUG=629574
TEST=manual
==========

to

==========
arc: add enterprise_reporting.mojom interface.

This allows ARC to clear user data.

BUG=629574
TEST=manual
==========

[commit-bot: I haz the power, 2016-07-22 15:20:55]

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

[commit-bot: I haz the power, 2016-07-22 15:20:56]

Dry run: This issue passed the CQ dry run.

[Polina Bondarenko, 2016-07-22 15:36:23]

Patchset #10 (id:300001) has been deleted

[Luis Héctor Chávez, 2016-07-22 15:51:42]

https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_auth_service.h (right):

https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_auth_service.h:175: void StopArc();
nit: add a comment

https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_user_data_service.h (right):

https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_user_data_service.h:28: public
InstanceHolder<mojom::EnterpriseReportingInstance>::Observer,
I am a bit worried about the mismatch between the mojo interface and the service
it lives in. I guess EnterpriseDataManagementService  or
EnterpriseManagementService can be better names, but I'm not sure if they are
the best.

So, pick a name that accurately describes both the purpose of this class and the
mojo interface and rename both the .mojom and this class.

https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_user_data_service.h:74: bool clean_required_ =
false;
We use a lot of names to refer to this operation: clear, clean, wipe,
removeuserdata. Let's standardize on the name of the commandline flag:
|arc_user_data_wipe_required_|. Also update all comments/log statements.

https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/login/session/user_session_manager.cc:1172:
base::WrapUnique(new arc::ArcUserDataService(
nit: prefer base::MakeUnique<T>(...) over base::WrapUnique(new T(...))

[Polina Bondarenko, 2016-07-22 16:41:19]

https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_user_data_service.h (right):

https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_user_data_service.h:28: public
InstanceHolder<mojom::EnterpriseReportingInstance>::Observer,
On 2016/07/22 15:51:42, Luis Héctor Chávez wrote:
> I am a bit worried about the mismatch between the mojo interface and the
service
> it lives in. I guess EnterpriseDataManagementService  or
> EnterpriseManagementService can be better names, but I'm not sure if they are
> the best.
> 
> So, pick a name that accurately describes both the purpose of this class and
the
> mojo interface and rename both the .mojom and this class.

ArcEnterpriseReportingService?

[Luis Héctor Chávez, 2016-07-22 16:46:37]

On 2016/07/22 16:41:19, Polina Bondarenko wrote:
>
https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
> File chrome/browser/chromeos/arc/arc_user_data_service.h (right):
> 
>
https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
> chrome/browser/chromeos/arc/arc_user_data_service.h:28: public
> InstanceHolder<mojom::EnterpriseReportingInstance>::Observer,
> On 2016/07/22 15:51:42, Luis Héctor Chávez wrote:
> > I am a bit worried about the mismatch between the mojo interface and the
> service
> > it lives in. I guess EnterpriseDataManagementService  or
> > EnterpriseManagementService can be better names, but I'm not sure if they
are
> > the best.
> > 
> > So, pick a name that accurately describes both the purpose of this class and
> the
> > mojo interface and rename both the .mojom and this class.
> 
> ArcEnterpriseReportingService?

The thing is that it doesn't _just_ report stuff, it also takes action based on
what is reported, and also on external inputs (if the user opts-out). Naming is
hard :(

Anyways, whatever name is chosen, make sure bartfab@ and alexchau@ agree with
that to avoid another bikeshed in the Android-side review.

[bartfab (slow), 2016-07-22 16:53:39]

On 2016/07/22 16:46:37, Luis Héctor Chávez wrote:
> On 2016/07/22 16:41:19, Polina Bondarenko wrote:
> >
>
https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
> > File chrome/browser/chromeos/arc/arc_user_data_service.h (right):
> > 
> >
>
https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
> > chrome/browser/chromeos/arc/arc_user_data_service.h:28: public
> > InstanceHolder<mojom::EnterpriseReportingInstance>::Observer,
> > On 2016/07/22 15:51:42, Luis Héctor Chávez wrote:
> > > I am a bit worried about the mismatch between the mojo interface and the
> > service
> > > it lives in. I guess EnterpriseDataManagementService  or
> > > EnterpriseManagementService can be better names, but I'm not sure if they
> are
> > > the best.
> > > 
> > > So, pick a name that accurately describes both the purpose of this class
and
> > the
> > > mojo interface and rename both the .mojom and this class.
> > 
> > ArcEnterpriseReportingService?
> 
> The thing is that it doesn't _just_ report stuff, it also takes action based
on
> what is reported, and also on external inputs (if the user opts-out). Naming
is
> hard :(
> 
> Anyways, whatever name is chosen, make sure bartfab@ and alexchau@ agree with
> that to avoid another bikeshed in the Android-side review.

EnterpriseReporting is something Alex and I arrived at based on what this
interface will be used for in the future. Right now, we report a status constant
only (and yes, we do take action based on that... it is not entirely pure). We
want to add more comprehensive reporting next.

[Luis Héctor Chávez, 2016-07-22 17:00:03]

On 2016/07/22 16:53:39, bartfab (slow) wrote:
> On 2016/07/22 16:46:37, Luis Héctor Chávez wrote:
> > On 2016/07/22 16:41:19, Polina Bondarenko wrote:
> > >
> >
>
https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
> > > File chrome/browser/chromeos/arc/arc_user_data_service.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
> > > chrome/browser/chromeos/arc/arc_user_data_service.h:28: public
> > > InstanceHolder<mojom::EnterpriseReportingInstance>::Observer,
> > > On 2016/07/22 15:51:42, Luis Héctor Chávez wrote:
> > > > I am a bit worried about the mismatch between the mojo interface and the
> > > service
> > > > it lives in. I guess EnterpriseDataManagementService  or
> > > > EnterpriseManagementService can be better names, but I'm not sure if
they
> > are
> > > > the best.
> > > > 
> > > > So, pick a name that accurately describes both the purpose of this class
> and
> > > the
> > > > mojo interface and rename both the .mojom and this class.
> > > 
> > > ArcEnterpriseReportingService?
> > 
> > The thing is that it doesn't _just_ report stuff, it also takes action based
> on
> > what is reported, and also on external inputs (if the user opts-out). Naming
> is
> > hard :(
> > 
> > Anyways, whatever name is chosen, make sure bartfab@ and alexchau@ agree
with
> > that to avoid another bikeshed in the Android-side review.
> 
> EnterpriseReporting is something Alex and I arrived at based on what this
> interface will be used for in the future. Right now, we report a status
constant
> only (and yes, we do take action based on that... it is not entirely pure). We
> want to add more comprehensive reporting next.

All right, given this information, the right thing to do is to keep
ArcUserDataService as being a standalone service (more or less as it is right
now, but with the restart logic you added), and make
ArcEnterpriseReportingService another class that can consume
ArcUserDataService's data-wiping functionality. Sorry for the churn, but at
least it looks like we're all on the same page now.

[Luis Héctor Chávez, 2016-07-22 17:00:16]

https://codereview.chromium.org/2165643004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_user_data_service.cc (right):

https://codereview.chromium.org/2165643004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_user_data_service.cc:65: FROM_HERE,
base::Bind(&ArcUserDataService::RestartArc,
IIUC this is racy. What needs to happen is that the
session_manager_client->RemoveArcData should have a callback that gets invoked
once it finishes removing the data. Otherwise you can get into a state where the
invocation to restart arc times out, and it wouldn't attempt to restart since
the initial connection was not ready.

https://codereview.chromium.org/2165643004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_user_data_service.cc:108: void
ArcUserDataService::RestartArc() {
This method does not access any members, so it can be in the anonymous
namespace.

[Polina Bondarenko, 2016-07-25 15:42:01]

Patchset #12 (id:360001) has been deleted

[Polina Bondarenko, 2016-07-25 15:45:29]

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

[commit-bot: I haz the power, 2016-07-25 15:45:43]

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-07-25 16:00:16]

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

[commit-bot: I haz the power, 2016-07-25 16:00:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Polina Bondarenko, 2016-07-25 16:17:59]

Fixed comments:
- Added separated ArcEnterpriseReportingService.
- Added callback to RemoveArcData.

https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_auth_service.h (right):

https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_auth_service.h:175: void StopArc();
On 2016/07/22 15:51:42, Luis Héctor Chávez wrote:
> nit: add a comment

Done.

https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_user_data_service.h (right):

https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_user_data_service.h:74: bool clean_required_ =
false;
On 2016/07/22 15:51:42, Luis Héctor Chávez wrote:
> We use a lot of names to refer to this operation: clear, clean, wipe,
> removeuserdata. Let's standardize on the name of the commandline flag:
> |arc_user_data_wipe_required_|. Also update all comments/log statements.

Done.

https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/2165643004/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/login/session/user_session_manager.cc:1172:
base::WrapUnique(new arc::ArcUserDataService(
On 2016/07/22 15:51:42, Luis Héctor Chávez wrote:
> nit: prefer base::MakeUnique<T>(...) over base::WrapUnique(new T(...))

Removed.

https://codereview.chromium.org/2165643004/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_user_data_service.cc (right):

https://codereview.chromium.org/2165643004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_user_data_service.cc:65: FROM_HERE,
base::Bind(&ArcUserDataService::RestartArc,
On 2016/07/22 17:00:15, Luis Héctor Chávez wrote:
> IIUC this is racy. What needs to happen is that the
> session_manager_client->RemoveArcData should have a callback that gets invoked
> once it finishes removing the data. Otherwise you can get into a state where
the
> invocation to restart arc times out, and it wouldn't attempt to restart since
> the initial connection was not ready.

Done.

https://codereview.chromium.org/2165643004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_user_data_service.cc:108: void
ArcUserDataService::RestartArc() {
On 2016/07/22 17:00:15, Luis Héctor Chávez wrote:
> This method does not access any members, so it can be in the anonymous
> namespace.

Moved this method to ArcEnterpriseReportingService and changed.

[Polina Bondarenko, 2016-07-25 16:22:28]

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

[commit-bot: I haz the power, 2016-07-25 16:22:38]

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

[Luis Héctor Chávez, 2016-07-25 16:47:57]

https://codereview.chromium.org/2165643004/diff/400001/components/arc/arc_ser...
File components/arc/arc_service_manager.cc (right):

https://codereview.chromium.org/2165643004/diff/400001/components/arc/arc_ser...
components/arc/arc_service_manager.cc:113: services_.clear();
You need to also clear |arc_user_data_service_| here, since you are not adding
it to |services_|.

https://codereview.chromium.org/2165643004/diff/400001/components/arc/user_da...
File components/arc/user_data/arc_user_data_service.cc (right):

https://codereview.chromium.org/2165643004/diff/400001/components/arc/user_da...
components/arc/user_data/arc_user_data_service.cc:64: LOG(ERROR) << "ARC
instance not stopped, user data can't be cleared";
nit: s/cleared/wiped/

https://codereview.chromium.org/2165643004/diff/400001/components/arc/user_da...
File components/arc/user_data/arc_user_data_service.h (right):

https://codereview.chromium.org/2165643004/diff/400001/components/arc/user_da...
components/arc/user_data/arc_user_data_service.h:67: // again after user data
has been wiped or user data wipe is required.
"or user data wipe is required". Is this part correct? You only set it to false
again after user data has been wiped.

[Polina Bondarenko, 2016-07-25 17:19:44]

Fixed comments.

https://codereview.chromium.org/2165643004/diff/400001/components/arc/arc_ser...
File components/arc/arc_service_manager.cc (right):

https://codereview.chromium.org/2165643004/diff/400001/components/arc/arc_ser...
components/arc/arc_service_manager.cc:113: services_.clear();
On 2016/07/25 16:47:57, Luis Héctor Chávez wrote:
> You need to also clear |arc_user_data_service_| here, since you are not adding
> it to |services_|.

Done.

https://codereview.chromium.org/2165643004/diff/400001/components/arc/user_da...
File components/arc/user_data/arc_user_data_service.cc (right):

https://codereview.chromium.org/2165643004/diff/400001/components/arc/user_da...
components/arc/user_data/arc_user_data_service.cc:64: LOG(ERROR) << "ARC
instance not stopped, user data can't be cleared";
On 2016/07/25 16:47:57, Luis Héctor Chávez wrote:
> nit: s/cleared/wiped/

Done.

https://codereview.chromium.org/2165643004/diff/400001/components/arc/user_da...
File components/arc/user_data/arc_user_data_service.h (right):

https://codereview.chromium.org/2165643004/diff/400001/components/arc/user_da...
components/arc/user_data/arc_user_data_service.h:67: // again after user data
has been wiped or user data wipe is required.
On 2016/07/25 16:47:57, Luis Héctor Chávez wrote:
> "or user data wipe is required". Is this part correct? You only set it to
false
> again after user data has been wiped.

Fixed, thanks for catch!

[Polina Bondarenko, 2016-07-25 17:23:31]

pbond@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Polina Bondarenko, 2016-07-25 17:23:32]

Hi Steven,

stevenjb@chromium.org: Please review changes in 

chromeos/dbus/* files.

I've added a callback to RemoveArcData() to be able to be notified when the ARC
user data is removed.

[Polina Bondarenko, 2016-07-25 17:35:46]

pbond@chromium.org changed reviewers:
+ tsepez@chromium.org
- mdempsky@chromium.org

[Polina Bondarenko, 2016-07-25 17:35:47]

Hi Thomas,

tsepez@chromium.org: Please review changes in
components/arc/common/*.mojom files.

Added new enterprise_reporting mojom interface.

[stevenjb, 2016-07-25 17:37:03]

chromeos/dbus RS LGTM

[Luis Héctor Chávez, 2016-07-25 17:40:34]

*/arc/* lgtm

Thanks!

https://codereview.chromium.org/2165643004/diff/440001/components/arc/arc_ser...
File components/arc/arc_service_manager.cc (right):

https://codereview.chromium.org/2165643004/diff/440001/components/arc/arc_ser...
components/arc/arc_service_manager.cc:114: arc_user_data_service_ = nullptr;
nit: move this one line above (and remove the extra leading whitespace).

[Polina Bondarenko, 2016-07-25 17:45:33]

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

[commit-bot: I haz the power, 2016-07-25 17:46:03]

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

[Tom Sepez, 2016-07-25 18:17:25]

mojom LGTM

[commit-bot: I haz the power, 2016-07-25 18:53:58]

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

[commit-bot: I haz the power, 2016-07-25 18:53:59]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Polina Bondarenko, 2016-07-25 20:01:22]

Patchset #17 (id:480001) has been deleted

[Polina Bondarenko, 2016-07-25 20:01:45]

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

[commit-bot: I haz the power, 2016-07-25 20:02:57]

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

[Polina Bondarenko, 2016-07-25 20:03:31]

Thanks everyone for a review!

Also fixed FakeSessionManagerClient::RemoveArcData method according to
OnArcMethod, that checks callback.is_null() before running a |callback|. Browser
tests should pass now.

https://codereview.chromium.org/2165643004/diff/440001/components/arc/arc_ser...
File components/arc/arc_service_manager.cc (right):

https://codereview.chromium.org/2165643004/diff/440001/components/arc/arc_ser...
components/arc/arc_service_manager.cc:114: arc_user_data_service_ = nullptr;
On 2016/07/25 17:40:33, Luis Héctor Chávez wrote:
> nit: move this one line above (and remove the extra leading whitespace).

Done.

[commit-bot: I haz the power, 2016-07-25 21:27:00]

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

[commit-bot: I haz the power, 2016-07-25 21:27:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Polina Bondarenko, 2016-07-26 10:43:17]

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

[commit-bot: I haz the power, 2016-07-26 10:43:48]

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-07-26 11:33:15]

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

[commit-bot: I haz the power, 2016-07-26 11:33:16]

Dry run: This issue passed the CQ dry run.

[Polina Bondarenko, 2016-07-26 11:51:30]

The CQ bit was checked by pbond@chromium.org

[Polina Bondarenko, 2016-07-26 11:51:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, lhchavez@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2165643004/#ps520001
(title: "Fixed SessionManagerClient stub RemoveArcData method.")

[commit-bot: I haz the power, 2016-07-26 11:51:42]

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-07-26 11:55:36]

Description was changed from

==========
arc: add enterprise_reporting.mojom interface.

This allows ARC to clear user data.

BUG=629574
TEST=manual
==========

to

==========
arc: add enterprise_reporting.mojom interface.

This allows ARC to clear user data.

BUG=629574
TEST=manual
==========

[commit-bot: I haz the power, 2016-07-26 11:55:38]

Committed patchset #18 (id:520001)

[commit-bot: I haz the power, 2016-07-26 11:57:18]

Description was changed from

==========
arc: add enterprise_reporting.mojom interface.

This allows ARC to clear user data.

BUG=629574
TEST=manual
==========

to

==========
arc: add enterprise_reporting.mojom interface.

This allows ARC to clear user data.

BUG=629574
TEST=manual

Committed: https://crrev.com/9282f59bec8693034cdf390927f41cfd2ea4c06c
Cr-Commit-Position: refs/heads/master@{#407771}
==========

[commit-bot: I haz the power, 2016-07-26 11:57:20]

Patchset 18 (id:??) landed as
https://crrev.com/9282f59bec8693034cdf390927f41cfd2ea4c06c
Cr-Commit-Position: refs/heads/master@{#407771}