arc: Logging ARC data removal reasons

arc: Logging ARC data removal reasons

There have been cases where the ARC user data has been requested to be
removed without having a paper trail about the reason why it happened.
This change should provide the reason in the logs so feedback reports at
least can tell what happened.

BUG=None
TEST=cq

Review-Url: https://codereview.chromium.org/2678273005
Cr-Commit-Position: refs/heads/master@{#449047}
Committed: https://chromium.googlesource.com/chromium/src/+/2512542164d696c21c6a62a0f103e1af9c37ca45

[Luis Héctor Chávez, 2017-02-07 23:16:09]

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

[Luis Héctor Chávez, 2017-02-07 23:16:48]

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

[Luis Héctor Chávez, 2017-02-07 23:16:49]

PTAL

This is in response to a feedback report where ARC data was removed after a
Chrome crash, but we're not really sure why.

[commit-bot: I haz the power, 2017-02-07 23:17:18]

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-08 00:05:53]

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

[commit-bot: I haz the power, 2017-02-08 00:05:54]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-02-08 04:19:32]

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

[hidehiko, 2017-02-08 04:19:33]

FYI:

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:370: VLOG(1) << "ARC
provisioning failed: " << result << ". Removing user data";
How about logging in L320 or so? After the line, we enter error handling code.

[Yusuke Sato, 2017-02-08 07:48:55]

lgtm

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_optin_uma.cc (right):

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_optin_uma.cc:59: case
ProvisioningResult::SUCCESS:
How about using a macro to avoid repeating the same string twice? This seems a
bit error-prone.

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_optin_uma.cc:104: return os;
os << "Invalid value " << static_cast<...>(result);

maybe slightly more useful here? (to catch uninitialized or corrupted |result|
etc.)

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:976: case
ArcSessionManager::State::NOT_INITIALIZED:
same

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:993: return os;
same

[hidehiko, 2017-02-08 14:16:30]

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_optin_uma.cc (right):

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_optin_uma.cc:59: case
ProvisioningResult::SUCCESS:
On 2017/02/08 07:48:55, Yusuke Sato wrote:
> How about using a macro to avoid repeating the same string twice? This seems a
> bit error-prone.

FYI: then maybe it looks better to use something similar pattern with
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_auth_ser...
?

[Luis Héctor Chávez, 2017-02-08 18:11:16]

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_optin_uma.cc (right):

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_optin_uma.cc:59: case
ProvisioningResult::SUCCESS:
On 2017/02/08 14:16:30, hidehiko wrote:
> On 2017/02/08 07:48:55, Yusuke Sato wrote:
> > How about using a macro to avoid repeating the same string twice? This seems
a
> > bit error-prone.
> 
> FYI: then maybe it looks better to use something similar pattern with
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_auth_ser...
> ?

sounds good. I'll also convert the one in arc_session_manager now that I'm at
it.

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_optin_uma.cc:104: return os;
On 2017/02/08 07:48:55, Yusuke Sato wrote:
> os << "Invalid value " << static_cast<...>(result);
> 
> maybe slightly more useful here? (to catch uninitialized or corrupted |result|
> etc.) 

Done.

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:370: VLOG(1) << "ARC
provisioning failed: " << result << ". Removing user data";
On 2017/02/08 04:19:33, hidehiko wrote:
> How about logging in L320 or so? After the line, we enter error handling code.

I want to comply with the comment I'm adding to RemoveArcData() to log just
before calling. But I'll add an additional log there.

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:976: case
ArcSessionManager::State::NOT_INITIALIZED:
On 2017/02/08 07:48:55, Yusuke Sato wrote:
> same

Done.

https://codereview.chromium.org/2678273005/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:993: return os;
On 2017/02/08 07:48:55, Yusuke Sato wrote:
> same

Done.

[Luis Héctor Chávez, 2017-02-08 18:15:30]

The CQ bit was checked by lhchavez@chromium.org

[Luis Héctor Chávez, 2017-02-08 18:15:30]

The patchset sent to the CQ was uploaded after l-g-t-m from yusukes@chromium.org
Link to the patchset: https://codereview.chromium.org/2678273005/#ps10006
(title: "Addressed feedback")

[commit-bot: I haz the power, 2017-02-08 18:16:03]

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-08 18:51:04]

CQ is committing da patch.

Bot data: {"patchset_id": 10006, "attempt_start_ts": 1486577730420820,
"parent_rev": "59dc1fe4df1db90553f572d20cf2b732438063b2", "commit_rev":
"2512542164d696c21c6a62a0f103e1af9c37ca45"}

[commit-bot: I haz the power, 2017-02-08 18:51:53]

Description was changed from

==========
arc: Logging ARC data removal reasons

There have been cases where the ARC user data has been requested to be
removed without having a paper trail about the reason why it happened.
This change should provide the reason in the logs so feedback reports at
least can tell what happened.

BUG=None
TEST=cq
==========

to

==========
arc: Logging ARC data removal reasons

There have been cases where the ARC user data has been requested to be
removed without having a paper trail about the reason why it happened.
This change should provide the reason in the logs so feedback reports at
least can tell what happened.

BUG=None
TEST=cq

Review-Url: https://codereview.chromium.org/2678273005
Cr-Commit-Position: refs/heads/master@{#449047}
Committed:
https://chromium.googlesource.com/chromium/src/+/2512542164d696c21c6a62a0f103...
==========

[commit-bot: I haz the power, 2017-02-08 18:51:55]

Committed patchset #3 (id:10006) as
https://chromium.googlesource.com/chromium/src/+/2512542164d696c21c6a62a0f103...