Extract ArcTermsOfServiceNegotiator implementation.

Extract ArcTermsOfServiceNegotiator implementation.

This extracts Terms-Of-Service agreement handling into
a standalone class.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.

Committed: https://crrev.com/3b196d322a4a334c17879af6b2f7bbe284eedb1c
Cr-Commit-Position: refs/heads/master@{#435910}

[hidehiko, 2016-11-28 12:37:36]

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

[commit-bot: I haz the power, 2016-11-28 12:37:44]

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

[hidehiko, 2016-11-28 12:42:53]

hidehiko@chromium.org changed reviewers:
+ bartfab@chromium.org, lhchavez@chromium.org

[hidehiko, 2016-11-28 12:42:54]

PTAL.

lhchavez@: all.
bartfab@: c/b/policy.

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_support_host.cc (left):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_support_host.cc:466: DCHECK(!device_id.empty());
Relaxed for testing.

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

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_support_host.cc:337: display::Screen* screen =
display::Screen::GetScreen();
Relaxed for testing. Ditto for below.

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_support_host.h (left):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_support_host.h:55: // this is also called when
RETRY on error page is clicked. Fix this.
This is obsolete. Removed.

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.cc (right):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.cc:27: if
(g_browser_process->local_state()) {
Unfortunately, injecting ScopedTestingLocalState does not help to test for our
specific case. Specifically, ChangeMetricsReportingState below crashes even with
test objects. So, relaxed for testing, too.

[hidehiko, 2016-11-28 13:23:26]

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

[hidehiko, 2016-11-28 13:23:26]

R+=peletskyi@ for Kiosk related TODO.

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

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:495: // TODO(peletskyi): Move
to more Kiosk dedicated set-up phase.
peletskyi@, can I assign this to you?

[commit-bot: I haz the power, 2016-11-28 13:42:47]

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

[commit-bot: I haz the power, 2016-11-28 13:42:48]

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

[peletskyi, 2016-11-28 14:35:55]

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

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:495: // TODO(peletskyi): Move
to more Kiosk dedicated set-up phase.
On 2016/11/28 13:23:26, hidehiko wrote:
> peletskyi@, can I assign this to you?

poromov@ can take care about this.

[Luis Héctor Chávez, 2016-11-29 00:15:29]

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

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:521: !IsArcKioskMode()) {
I had a *very* hard time parsing all this. How about we make it:

StartArc();

if (IsOptInVerificationDisabled() || IsArcKioskMode())
  return;

// If sign-in is not yet completed, but terms-of-service have already
// been agreed to, then (the agreement?? <- didn't you just mention it has
already been agreed to?) should have been done in
// the OOBE phase.
if (!profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn))
  return;

if (g_disable_ui_for_testing &&
    !g_enable_check_android_management_for_testing) {
  return;
}

android_management_checker_ = ...

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:677: CancelAuthCode();
Briefly mention that the only way to not agree is by closing the window.
(Pressing cancel effectively just closes the window).

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

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:206: void
StartTermsOfServiceNegotiate();
nit: StartTermsOfServiceNegotiation

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_support_host.h (right):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_support_host.h:128: void
SetRequestOpenAppCallbackForTest(const RequestOpenAppCallback& callback);
nit: ...ForTesting

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/extensions/fake_arc_support.cc (right):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/extensions/fake_arc_support.cc:58:
base::JSONWriter::Write(message, &message_string);
nit: DCHECK? NOTREACHED?

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/extensions/fake_arc_support.cc:95: NOTREACHED();
nit: NOTREACHED() << page;

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/extensions/fake_arc_support.cc:116: NOTREACHED();
nit: NOTREACHED() << action;

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.cc (right):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.cc:27: if
(g_browser_process->local_state()) {
On 2016/11/28 12:42:54, hidehiko wrote:
> Unfortunately, injecting ScopedTestingLocalState does not help to test for our
> specific case. Specifically, ChangeMetricsReportingState below crashes even
with
> test objects. So, relaxed for testing, too.

nit:

if (!g_browser_process->local_state()) {
  // is there a way to log when we're not testing?
  return;
}

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.cc
(right):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.cc:43:
DCHECK(!callback_.is_null());
nit: for consistency with OnTermsAgreed:

DCHECK(preference_handler_);
support_host_->RemoveObserver(this);
preference_handler_.reset();

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h
(right):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:37: void
StartNegotiate(const AgreementCallback& callback);
nit: StartNegotiation

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:54:
PrefService* pref_service_;
nit: PrefService* const pref_service_;

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:56:
ArcSupportHost* support_host_;
nit: ArcSupportHost* const support_host_;

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc
(right):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc:84:
auto negotiator = base::MakeUnique<ArcTermsOfServiceNegotiator>(
consider moving L84-L101 to a helper function so all three tests can just call
that instead of having repeated boilerplate.

[hidehiko, 2016-11-29 17:42:08]

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

[commit-bot: I haz the power, 2016-11-29 17:42:23]

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

[hidehiko, 2016-11-29 17:43:29]

+cc: poromov@.

Thank you for review. PTAL.

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

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:495: // TODO(peletskyi): Move
to more Kiosk dedicated set-up phase.
On 2016/11/28 14:35:55, peletskyi wrote:
> On 2016/11/28 13:23:26, hidehiko wrote:
> > peletskyi@, can I assign this to you?
> 
> poromov@ can take care about this.

Done.

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:521: !IsArcKioskMode()) {
On 2016/11/29 00:15:28, Luis Héctor Chávez wrote:
> I had a *very* hard time parsing all this. How about we make it:
> 
> StartArc();
> 
> if (IsOptInVerificationDisabled() || IsArcKioskMode())
>   return;
> 
> // If sign-in is not yet completed, but terms-of-service have already
> // been agreed to, then (the agreement?? <- didn't you just mention it has
> already been agreed to?) should have been done in
> // the OOBE phase.
> if (!profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn))
>   return;
> 
> if (g_disable_ui_for_testing &&
>     !g_enable_check_android_management_for_testing) {
>   return;
> }
> 
> android_management_checker_ = ...

Done, but in a slightly different form with comments. How about this?

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:677: CancelAuthCode();
On 2016/11/29 00:15:28, Luis Héctor Chávez wrote:
> Briefly mention that the only way to not agree is by closing the window.
> (Pressing cancel effectively just closes the window).

Done... But the cancelling should be abstracted here?

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

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:206: void
StartTermsOfServiceNegotiate();
On 2016/11/29 00:15:28, Luis Héctor Chávez wrote:
> nit: StartTermsOfServiceNegotiation

Done.

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_support_host.h (right):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_support_host.h:128: void
SetRequestOpenAppCallbackForTest(const RequestOpenAppCallback& callback);
On 2016/11/29 00:15:28, Luis Héctor Chávez wrote:
> nit: ...ForTesting

Done.

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/extensions/fake_arc_support.cc (right):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/extensions/fake_arc_support.cc:58:
base::JSONWriter::Write(message, &message_string);
On 2016/11/29 00:15:28, Luis Héctor Chávez wrote:
> nit: DCHECK? NOTREACHED?

Sorry, DCHECK for ... what?

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/extensions/fake_arc_support.cc:95: NOTREACHED();
On 2016/11/29 00:15:28, Luis Héctor Chávez wrote:
> nit: NOTREACHED() << page;

Instead, let's output entire message.

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/extensions/fake_arc_support.cc:116: NOTREACHED();
On 2016/11/29 00:15:28, Luis Héctor Chávez wrote:
> nit: NOTREACHED() << action;

Acknowledged.

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.cc (right):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.cc:27: if
(g_browser_process->local_state()) {
On 2016/11/29 00:15:28, Luis Héctor Chávez wrote:
> On 2016/11/28 12:42:54, hidehiko wrote:
> > Unfortunately, injecting ScopedTestingLocalState does not help to test for
our
> > specific case. Specifically, ChangeMetricsReportingState below crashes even
> with
> > test objects. So, relaxed for testing, too.
> 
> nit:
> 
> if (!g_browser_process->local_state()) {
>   // is there a way to log when we're not testing?
>   return;
> }

We still want to initialize pref_change_registrar_ below even in test.
Also, there seems no good way to see if this is testing or not...

An alternative is to have another abstraction layer here to inject Fake/Stub,
but IMHO it looks overkill. WDYT?

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.cc
(right):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.cc:43:
DCHECK(!callback_.is_null());
On 2016/11/29 00:15:28, Luis Héctor Chávez wrote:
> nit: for consistency with OnTermsAgreed:
> 
> DCHECK(preference_handler_);
> support_host_->RemoveObserver(this);
> preference_handler_.reset();

Done.

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h
(right):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:37: void
StartNegotiate(const AgreementCallback& callback);
On 2016/11/29 00:15:28, Luis Héctor Chávez wrote:
> nit: StartNegotiation

Done.

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:54:
PrefService* pref_service_;
On 2016/11/29 00:15:28, Luis Héctor Chávez wrote:
> nit: PrefService* const pref_service_;

Done.

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:56:
ArcSupportHost* support_host_;
On 2016/11/29 00:15:28, Luis Héctor Chávez wrote:
> nit: ArcSupportHost* const support_host_;

Done.

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc
(right):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc:84:
auto negotiator = base::MakeUnique<ArcTermsOfServiceNegotiator>(
On 2016/11/29 00:15:28, Luis Héctor Chávez wrote:
> consider moving L84-L101 to a helper function so all three tests can just call
> that instead of having repeated boilerplate.

Done.

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right):

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:291: // Emulate to
accept the terms of service.
Note: I seem to overlooked the real test failure for some reason in PS1...
Fixed.

[Sergey Poromov, 2016-11-29 17:50:59]

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

[Sergey Poromov, 2016-11-29 17:51:00]

lgtm for Kiosk part. I'll take care of the TODO later.

[Luis Héctor Chávez, 2016-11-29 18:18:12]

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

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:521: !IsArcKioskMode()) {
On 2016/11/29 17:43:28, hidehiko wrote:
> On 2016/11/29 00:15:28, Luis Héctor Chávez wrote:
> > I had a *very* hard time parsing all this. How about we make it:
> > 
> > StartArc();
> > 
> > if (IsOptInVerificationDisabled() || IsArcKioskMode())
> >   return;
> > 
> > // If sign-in is not yet completed, but terms-of-service have already
> > // been agreed to, then (the agreement?? <- didn't you just mention it has
> > already been agreed to?) should have been done in
> > // the OOBE phase.
> > if (!profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn))
> >   return;
> > 
> > if (g_disable_ui_for_testing &&
> >     !g_enable_check_android_management_for_testing) {
> >   return;
> > }
> > 
> > android_management_checker_ = ...
> 
> Done, but in a slightly different form with comments. How about this?

much better :)

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/extensions/fake_arc_support.cc (right):

https://codereview.chromium.org/2534883002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/extensions/fake_arc_support.cc:58:
base::JSONWriter::Write(message, &message_string);
On 2016/11/29 17:43:28, hidehiko wrote:
> On 2016/11/29 00:15:28, Luis Héctor Chávez wrote:
> > nit: DCHECK? NOTREACHED?
> 
> Sorry, DCHECK for ... what?

base::JSONWriter::Write returns bool. We should stop the test if it failed to
serialize.

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

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:520: // then it should have
been done in OOBE phase.
ah! now I understand this part.

"If Sign-in is not yet completed, then management checking will be performed in
the OOBE phase."

IIUC, we never perform management checking (from the Chrome side) on first
sign-in, which means that management checking will be done later, either in
Android or next boot on Chrome.

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:679: void
ArcSessionManager::OnTermsOfServiceNegotiated(bool agreed) {
nit: let's standardize on "accepted" since you're then storing this in a pref
called prefs::kArcTermsAccepted.

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/extensions/fake_arc_support.h (right):

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/extensions/fake_arc_support.h:53: ArcSupportHost*
support_host_;
nit: ArcSupportHost* const support_host_;

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h
(right):

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:36: using
AgreementCallback = base::Callback<void(bool agreed)>;
same here, s/agreed/accepted/. Maybe also rename AgreementCallback to
NegotiationCallback or something like that.

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc
(right):

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc:89:
// Initialize the preference value.
IIUC L89-L99 can be safely moved to SetUp() to avoid duplicated boilerplate. You
can also expose |status| through a method.

[commit-bot: I haz the power, 2016-11-29 18:58:29]

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

[commit-bot: I haz the power, 2016-11-29 18:58:30]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-12-01 14:18:50]

Patchset #5 (id:80001) has been deleted

[hidehiko, 2016-12-01 14:19:53]

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

[commit-bot: I haz the power, 2016-12-01 14:20:04]

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

[hidehiko, 2016-12-01 14:20:09]

Thank you for review. PTAL.

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

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:520: // then it should have
been done in OOBE phase.
On 2016/11/29 18:18:12, Luis Héctor Chávez wrote:
> ah! now I understand this part.
> 
> "If Sign-in is not yet completed, then management checking will be performed
in
> the OOBE phase."
> 
> IIUC, we never perform management checking (from the Chrome side) on first
> sign-in, which means that management checking will be done later, either in
> Android or next boot on Chrome.

Chatted with khmel@ offline.
It was not, but it was planned.

However, this check hit an edge case (2) of the comment in new code), and there
were no good way to split them.

So, instead, we agreed to check Android management status even OOBE case here.
Also, I found that it is necessary to check kArcSignIn flag first, for backward
compatibility.

Revised the code.

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:679: void
ArcSessionManager::OnTermsOfServiceNegotiated(bool agreed) {
On 2016/11/29 18:18:12, Luis Héctor Chávez wrote:
> nit: let's standardize on "accepted" since you're then storing this in a pref
> called prefs::kArcTermsAccepted.

Done for ArcTermsOfServiceNegotiator and above layers.

I left OnTermsAgreed as is, because the button is "AGREE" for now.
I'm happy to change them, too (either changing ArcSupportHost /
arc_support/background.js and/or UI itself) if necessary, but then let me work
on in a separate CL.

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/extensions/fake_arc_support.h (right):

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/extensions/fake_arc_support.h:53: ArcSupportHost*
support_host_;
On 2016/11/29 18:18:12, Luis Héctor Chávez wrote:
> nit: ArcSupportHost* const support_host_;

Done.

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h
(right):

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h:36: using
AgreementCallback = base::Callback<void(bool agreed)>;
On 2016/11/29 18:18:12, Luis Héctor Chávez wrote:
> same here, s/agreed/accepted/. Maybe also rename AgreementCallback to
> NegotiationCallback or something like that.

Done.

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc
(right):

https://codereview.chromium.org/2534883002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc:89:
// Initialize the preference value.
On 2016/11/29 18:18:12, Luis Héctor Chávez wrote:
> IIUC L89-L99 can be safely moved to SetUp() to avoid duplicated boilerplate.
You
> can also expose |status| through a method.

Moved L89 - L92, but let me keep L93-, because;
1) these invocation is a part of the main test scenario, rather than a part of
testing env set up.
2) We bind |status| local variable. Although, it can be a field of the
ArcTermsOfServiceNegotiatorTest class, but IMHO, this style looks less
boilerplate. WDYT?

[bartfab (slow), 2016-12-01 14:32:42]

chrome/browser/policy/policy_browsertest.cc LGTM

[commit-bot: I haz the power, 2016-12-01 15:14:20]

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

[commit-bot: I haz the power, 2016-12-01 15:14:21]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2016-12-01 17:24:14]

lgtm with one final nit.

https://codereview.chromium.org/2534883002/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc
(right):

https://codereview.chromium.org/2534883002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc:95:
negotiator()->StartNegotiation(base::Bind(
last attempt :P

if you really don't want to move |status| as a member, can you at least move the
boilerplate base::Bind() to a function and instead do:

namespace {
void UpdateStatusCallback(Status* status) {
  return base::Bind(
      [](Status* status, bool agreed) {
        *status = agreed ? Status::AGREED : Status::CANCELLED;
      }, &status));
}
}  //namespace

...

Status status = Status::PENDING;
negotiator()->StartNegotiation(UpdateStatusCallback(&status));

[hidehiko, 2016-12-02 08:47:22]

Thank you for review. Submitting.

https://codereview.chromium.org/2534883002/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc
(right):

https://codereview.chromium.org/2534883002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc:95:
negotiator()->StartNegotiation(base::Bind(
On 2016/12/01 17:24:14, Luis Héctor Chávez wrote:
> last attempt :P
> 
> if you really don't want to move |status| as a member, can you at least move
the
> boilerplate base::Bind() to a function and instead do:
> 
> namespace {
> void UpdateStatusCallback(Status* status) {
>   return base::Bind(
>       [](Status* status, bool agreed) {
>         *status = agreed ? Status::AGREED : Status::CANCELLED;
>       }, &status));
> }
> }  //namespace
> 
> ...
> 
> Status status = Status::PENDING;
> negotiator()->StartNegotiation(UpdateStatusCallback(&status));

SGTM. Done. Thank you for suggestion.

https://codereview.chromium.org/2534883002/diff/140001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc
(right):

https://codereview.chromium.org/2534883002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator_unittest.cc:76:
ACCEPTED,
Note: one more forgotten AGREED. Fixed.

[hidehiko, 2016-12-02 08:47:33]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2016-12-02 08:47:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
poromov@chromium.org, bartfab@chromium.org, lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2534883002/#ps140001
(title: "Address comments")

[commit-bot: I haz the power, 2016-12-02 08:47:46]

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-12-02 11:05:27]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480668453121590,
"parent_rev": "2309805427d619f87dde2ba5872adc7926e3e80e", "commit_rev":
"773a153c25c478eaa1e04013320704dc891ecbbd"}

[commit-bot: I haz the power, 2016-12-02 11:06:01]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-12-02 11:08:05]

Description was changed from

==========
Extract ArcTermsOfServiceNegotiator implementation.

This extracts Terms-Of-Service agreement handling into
a standalone class.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.
==========

to

==========
Extract ArcTermsOfServiceNegotiator implementation.

This extracts Terms-Of-Service agreement handling into
a standalone class.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.

Committed: https://crrev.com/3b196d322a4a334c17879af6b2f7bbe284eedb1c
Cr-Commit-Position: refs/heads/master@{#435910}
==========

[commit-bot: I haz the power, 2016-12-02 11:08:06]

Patchset 7 (id:??) landed as
https://crrev.com/3b196d322a4a334c17879af6b2f7bbe284eedb1c
Cr-Commit-Position: refs/heads/master@{#435910}