arc: Enable silent PlayStore Sign-In.

arc: Enable silent PlayStore Sign-In.

This CL implements experimental feature to skip 'Sign In' button
in OptIn flow. Currently it is implemented as 'sandbox' on server
side and this is available only for internal IP addresses. It is
optional and can be activated in chrome://flags.

BUG=b/28987931
TEST=Manually on device in both modes.

Committed: https://crrev.com/76e7847f8735fb2d0903aa779865a3728a3da8c8
Cr-Commit-Position: refs/heads/master@{#417180}

[khmel, 2016-09-07 19:30:27]

khmel@chromium.org changed reviewers:
+ xiyuan@chromium.org

[khmel, 2016-09-07 19:30:27]

Hi Xiyuan,

PTAL

[khmel, 2016-09-07 19:30:48]

Description was changed from

==========
arc: Enable silient PlayStore Sign-In.

This CL implements experemental feature to skip 'Sign In' button
in OptIn flow. Currently it is implemented as 'sandbox' on server
side and this is available only for internal IP addresses. It is
optional and can be activated in chrome://flags.

BUG=b/28987931
TEST=Manually on device in both modes.
==========

to

==========
arc: Enable silent PlayStore Sign-In.

This CL implements experimental feature to skip 'Sign In' button
in OptIn flow. Currently it is implemented as 'sandbox' on server
side and this is available only for internal IP addresses. It is
optional and can be activated in chrome://flags.

BUG=b/28987931
TEST=Manually on device in both modes.
==========

[xiyuan, 2016-09-07 21:07:56]

https://codereview.chromium.org/2320813002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2320813002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13584: +      <message
name="IDS_FLAGS_ARC_USE_AUTH_ENDPOINT_NAME"
Can we put these under a <if expr="chromeos"> ?

https://codereview.chromium.org/2320813002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_context.cc (right):

https://codereview.chromium.org/2320813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_context.cc:114:
auth_code_fetcher_->SetLoadFlags(net::LOAD_NORMAL);
We might want to use LOAD_DISABLE_CACHE | LOAD_BYPASS_CACHE instead of
LOAD_NORMAL so that we don't cache the response and don't use the cached
response.

https://codereview.chromium.org/2320813002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_context.h (right):

https://codereview.chromium.org/2320813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_context.h:32: public
net::URLFetcherDelegate {
Can we create a separate class and make it a member of ArcAuthContext instead of
using inheritance? That way, it would be easier to read the code and move it
around (e.g. moving into /google_apis in the future).

https://codereview.chromium.org/2320813002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_context_delegate.h (right):

https://codereview.chromium.org/2320813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_context_delegate.h:14: virtual void
OnAuthCodeRequested(const std::string& auth_code) = 0;
nit: OnAuthCodeRequested -> OnAuthCodeSuccess, Requested sounds like the auth
code is being requested instead of it is retrieved successfully.

[khmel, 2016-09-07 23:08:53]

Thanks Xiyuan for comments,

PTAL

https://codereview.chromium.org/2320813002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2320813002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13584: +      <message
name="IDS_FLAGS_ARC_USE_AUTH_ENDPOINT_NAME"
On 2016/09/07 21:07:55, xiyuan wrote:
> Can we put these under a <if expr="chromeos"> ?

Missed this, thanks

https://codereview.chromium.org/2320813002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_context.cc (right):

https://codereview.chromium.org/2320813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_context.cc:114:
auth_code_fetcher_->SetLoadFlags(net::LOAD_NORMAL);
On 2016/09/07 21:07:55, xiyuan wrote:
> We might want to use LOAD_DISABLE_CACHE | LOAD_BYPASS_CACHE instead of
> LOAD_NORMAL so that we don't cache the response and don't use the cached
> response.
Done

https://codereview.chromium.org/2320813002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_context.h (right):

https://codereview.chromium.org/2320813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_context.h:32: public
net::URLFetcherDelegate {
On 2016/09/07 21:07:55, xiyuan wrote:
> Can we create a separate class and make it a member of ArcAuthContext instead
of
> using inheritance? That way, it would be easier to read the code and move it
> around (e.g. moving into /google_apis in the future).

+1

[xiyuan, 2016-09-07 23:20:10]

lgtm

https://codereview.chromium.org/2320813002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_code_fetcher.h (right):

https://codereview.chromium.org/2320813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_code_fetcher.h:18: class
URLFetcherDelegate;
nit: not needed since we need to include the header.

https://codereview.chromium.org/2320813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_code_fetcher.h:54: std::string
auth_endpoint_;
nit: const since it should not change.

[khmel, 2016-09-07 23:30:11]

khmel@chromium.org changed reviewers:
+ isherman@chromium.org

[khmel, 2016-09-07 23:30:12]

Thanks Xiyuan for review.

Adding isherman@ (	tools/metrics/histograms/histograms.xml)

PTAL

https://codereview.chromium.org/2320813002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_code_fetcher.h (right):

https://codereview.chromium.org/2320813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_code_fetcher.h:18: class
URLFetcherDelegate;
On 2016/09/07 23:20:09, xiyuan wrote:
> nit: not needed since we need to include the header.

Good catch, I meant URLFetcher actually.

https://codereview.chromium.org/2320813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_code_fetcher.h:54: std::string
auth_endpoint_;
On 2016/09/07 23:20:09, xiyuan wrote:
> nit: const since it should not change.

Done.

[Ilya Sherman, 2016-09-08 01:53:20]

histograms.xml lgtm

[khmel, 2016-09-08 01:53:46]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-09-08 01:53:46]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2320813002/#ps60001
(title: "nits")

[commit-bot: I haz the power, 2016-09-08 01:54:10]

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-09-08 03:48:17]

Description was changed from

==========
arc: Enable silent PlayStore Sign-In.

This CL implements experimental feature to skip 'Sign In' button
in OptIn flow. Currently it is implemented as 'sandbox' on server
side and this is available only for internal IP addresses. It is
optional and can be activated in chrome://flags.

BUG=b/28987931
TEST=Manually on device in both modes.
==========

to

==========
arc: Enable silent PlayStore Sign-In.

This CL implements experimental feature to skip 'Sign In' button
in OptIn flow. Currently it is implemented as 'sandbox' on server
side and this is available only for internal IP addresses. It is
optional and can be activated in chrome://flags.

BUG=b/28987931
TEST=Manually on device in both modes.
==========

[commit-bot: I haz the power, 2016-09-08 03:48:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-08 03:50:31]

Description was changed from

==========
arc: Enable silent PlayStore Sign-In.

This CL implements experimental feature to skip 'Sign In' button
in OptIn flow. Currently it is implemented as 'sandbox' on server
side and this is available only for internal IP addresses. It is
optional and can be activated in chrome://flags.

BUG=b/28987931
TEST=Manually on device in both modes.
==========

to

==========
arc: Enable silent PlayStore Sign-In.

This CL implements experimental feature to skip 'Sign In' button
in OptIn flow. Currently it is implemented as 'sandbox' on server
side and this is available only for internal IP addresses. It is
optional and can be activated in chrome://flags.

BUG=b/28987931
TEST=Manually on device in both modes.

Committed: https://crrev.com/76e7847f8735fb2d0903aa779865a3728a3da8c8
Cr-Commit-Position: refs/heads/master@{#417180}
==========

[commit-bot: I haz the power, 2016-09-08 03:50:33]

Patchset 4 (id:??) landed as
https://crrev.com/76e7847f8735fb2d0903aa779865a3728a3da8c8
Cr-Commit-Position: refs/heads/master@{#417180}