arc: Use incognito profile for OptIn and cookie fetcher

arc: Use platform app for OptIn dialog.

BUG=585324
TEST=Manually tested with different combination of signed
     profiles and ARC optin state. OptIn dialog does not
     show multiply profile and auth code can be fetched.
     Once fetched authority code remains in cookies and
     can be used on next signin.

Committed: https://crrev.com/14d72dba39bed3c2d5fb6a90e798855013d37534
Cr-Commit-Position: refs/heads/master@{#375775}

[khmel, 2016-02-09 03:56:22]

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

[khmel, 2016-02-09 03:56:22]

Hi Xiyuan,

Could you please take a look to this CL where I try to solve problem we
discussed offline when user signed in with multiple profiles.

1. Use SignIn profile for OptIn dialog. This profile is prepared by ubertoken
fetcher and merge session afterword.
2. Use SignIn profile to fetch auth code (in ArcAuthFetcher)

Besides this following was done:
Signin profile by nature cannot store any cookies. That means when user signs in
again we have a problem to automatically fetch an auth code and need to show
OptIn dialog again. However when OptIn finishes SignIn profile contains cookies
that can be used for automatically fetching of auth code. To implement this, I
stores several cookies from SignIn profile on default user profile (SID, SSID,
LSID, HSID). They are stored as ARC_SID and so on. When user activates ARC next
time, I try to use these cookies by coping them from default user profile to
signin profile and try to fetch auth code from it. 
There is yet another problem I found, Reproduction rate is rare but it still
occurs time to time. The [problem is following, when user is opted in and signs
in again, we immidiately start our fetcher. But Signin profile can be 'in-use'
that time and there is  outside call possible that cleans SignIn profile during
our work. Best way to solve this is to implement retry scheme.

This work is not done and I am currently testing it on different devices but
your feedback on current stage is very appreciated.

PTAL

[xiyuan, 2016-02-09 22:39:35]

Good point to reuse the cookies so that we can auto fetch auth code in the
future. In this regard, signin profile is probably not the best choice to make
things work.

I wonder maybe we should get an isolated storage partition instead of using the
signin profile. One way is to get it via a platform app. We can create a
component platform app and use it for the web flow. After that, we should be
able to get the app's storage partition and use it with ArcAuthFetcher. This
way, we don't need to copy the cookies back and forth and the cookies can stay
there as long as it takes.

WDYT?

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

https://codereview.chromium.org/1681813003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:354:
scopes.insert(GaiaConstants::kOAuth1LoginScope);
not used.

[khmel, 2016-02-09 22:41:32]

On 2016/02/09 22:39:35, xiyuan wrote:
> Good point to reuse the cookies so that we can auto fetch auth code in the
> future. In this regard, signin profile is probably not the best choice to make
> things work.
> 
> I wonder maybe we should get an isolated storage partition instead of using
the
> signin profile. One way is to get it via a platform app. We can create a
> component platform app and use it for the web flow. After that, we should be
> able to get the app's storage partition and use it with ArcAuthFetcher. This
> way, we don't need to copy the cookies back and forth and the cookies can stay
> there as long as it takes.
> 
> WDYT?

Yes, this sounds reasonable and would simplify whole process. I will try. 

> 
>
https://codereview.chromium.org/1681813003/diff/1/chrome/browser/chromeos/arc...
> File chrome/browser/chromeos/arc/arc_auth_service.cc (right):
> 
>
https://codereview.chromium.org/1681813003/diff/1/chrome/browser/chromeos/arc...
> chrome/browser/chromeos/arc/arc_auth_service.cc:354:
> scopes.insert(GaiaConstants::kOAuth1LoginScope);
> not used.

[khmel, 2016-02-11 02:15:03]

Description was changed from

==========
arc: Use incognito profile for OptIn and cookie fetcher

BUG=585324
TEST=Manually tested with different combination of signed
     profiles and ARC optin state. OptIn dialog does not
     show multiply profile and auth code can be fetched.
     Once fetched authority code remains in cookies and
     can be used on next signin.
==========

to

==========
arc: Use platform app for OptIn dialog.

BUG=585324
TEST=Manually tested with different combination of signed
     profiles and ARC optin state. OptIn dialog does not
     show multiply profile and auth code can be fetched.
     Once fetched authority code remains in cookies and
     can be used on next signin.
==========

[khmel, 2016-02-11 02:46:16]

Hi,

This is implementation based on platform app as we discussed.

1. Implement OptIn Platform App itself.
2. Add ArcOptInHost to support communication ArcAuthService <-> Arc OptIn UI.
3. Removed deprecated ArcAuthUI
4. Session merge for UI is the same as in previous revision.

PS. I plan to use this Platform app in the future to implement additional UI for
OptIn process (show user info about Arc, Ask confirmation Yes/No, current LSO
dialog, progress bar).

PTAL

[xiyuan, 2016-02-11 17:57:21]

This looks awesome.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:392: <include
name="IDR_ARC_OPT_IN_MANIFEST"
file="resources\chromeos\arc_opt_in\manifest.json" type="BINDATA" />
nit: arc_opt_in is very specific. Since we want to extends the app to do more
stuff in the future, how about give the app/dir a more general name, e.g.
arc_support?

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

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:11: #include
"chrome/browser/chromeos/profiles/profile_helper.h"
remove?

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:148: auth_code_ = std::string();
nit: auth_code_.clear();

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:171:
controller->ActivateApp(profile_, extension,
Think we can just call OpenApplication() instead of doing this.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:255: void
ArcAuthService::CheckAuthCode() {
Can we merge this with FetchAuthCode? It seems to be that this can be done if we
move SetState(State::FETCHING_CODE); to OnOptInPreferenceChanged() where
FetchAuthCode() is called. And this this would make the state change more
explicit.

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

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.h:23: class PrefService;
nit: not used?

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/arc_opt_in_host.cc (right):

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_opt_in_host.cc:15: }
nit: }  // namespace

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/arc_opt_in_host.h (right):

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_opt_in_host.h:9: #include
"base/thread_task_runner_handle.h"
move to cc file?

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_opt_in_host.h:21: return
scoped_ptr<NativeMessageHost>(new ArcOptInHost());
Move impl to cc file.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/arc_opt_in/background.js (right):

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/background.js:5: var appWindow =
null;
Please document global variables and functions.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/background.js:31:
port.onDisconnect.addListener(onDisconnected);
onDisconnected is not defined.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/background.js:35: var options = {
nit: move this down to chrome.app.window.create call where it is used.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/background.js:37: 'singleton':
true,
'singleton' is deprecated. We should have only one instance of the window as
long as 'id' is specified.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/background.js:49:
webview.executeScript({
Would "webview.src" give us the URL?

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/background.js:68:
chrome.app.window.create('main.html', options, onWindowCreated);
What happens if user closes the window without going through the web flow?
Should we handle that?

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/arc_opt_in/main.html (right):

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/main.html:2: <html
style="width:100%; height:100%">
Inline css is not recommended. Please move them into a <style> tag or a .css
flie

<head>
  <style>
    html,
    body,
    webview {
      height: 100%;
      margin: 0;
      padding: 0;
      width: 100%;
    }
  </style>
</head>

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/main.html:10:
src="https://accounts.google.com/o/oauth2/programmatic_auth?scope=https://www.google.com/accounts/OAuthLogin&client_id=1070009224336-sdh77n7uot3oc99ais00jmuft6sk2fg9.apps.googleusercontent.com">
Do we need to have "hl" URL param so that the page is displayed in the correct
language?

[khmel, 2016-02-12 02:45:24]

Thank you for review, as usual it is very useful!

Some points:
1. arc_opt_in renamed to arc_support.
2. Support close window by user - opt arc out.
3. Tests extended.
4. CloseUI is not closed from OnAuthCodeNeedUI. (Befor ArcAuthUI did this, but
now platform app sends request to to check auth code and potentially
OnAuthCodeNeedUI may be called. This may cause window Show/Hide... It is because
loadstop event for webview works quite tricky and in some cases it may be called
for initial request)

PTAL

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:392: <include
name="IDR_ARC_OPT_IN_MANIFEST"
file="resources\chromeos\arc_opt_in\manifest.json" type="BINDATA" />
On 2016/02/11 17:57:20, xiyuan wrote:
> nit: arc_opt_in is very specific. Since we want to extends the app to do more
> stuff in the future, how about give the app/dir a more general name, e.g.
> arc_support?

Agree!

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

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:11: #include
"chrome/browser/chromeos/profiles/profile_helper.h"
On 2016/02/11 17:57:20, xiyuan wrote:
> remove?

Done.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:148: auth_code_ = std::string();
On 2016/02/11 17:57:20, xiyuan wrote:
> nit: auth_code_.clear();

Done.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:171:
controller->ActivateApp(profile_, extension,
On 2016/02/11 17:57:20, xiyuan wrote:
> Think we can just call OpenApplication() instead of doing this.

Yes, more convenient

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:255: void
ArcAuthService::CheckAuthCode() {
On 2016/02/11 17:57:20, xiyuan wrote:
> Can we merge this with FetchAuthCode? It seems to be that this can be done if
we
> move SetState(State::FETCHING_CODE); to OnOptInPreferenceChanged() where
> FetchAuthCode() is called. And this this would make the state change more
> explicit.

Was also thinking about this, Thanks for confirmation.

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

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.h:23: class PrefService;
On 2016/02/11 17:57:20, xiyuan wrote:
> nit: not used?

Done.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/arc_opt_in_host.cc (right):

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_opt_in_host.cc:15: }
On 2016/02/11 17:57:20, xiyuan wrote:
> nit: }  // namespace

Done.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/arc_opt_in_host.h (right):

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_opt_in_host.h:9: #include
"base/thread_task_runner_handle.h"
On 2016/02/11 17:57:20, xiyuan wrote:
> move to cc file?

Done.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_opt_in_host.h:21: return
scoped_ptr<NativeMessageHost>(new ArcOptInHost());
On 2016/02/11 17:57:20, xiyuan wrote:
> Move impl to cc file.

Done.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/arc_opt_in/background.js (right):

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/background.js:5: var appWindow =
null;
On 2016/02/11 17:57:21, xiyuan wrote:
> Please document global variables and functions.

Done.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/background.js:31:
port.onDisconnect.addListener(onDisconnected);
On 2016/02/11 17:57:21, xiyuan wrote:
> onDisconnected is not defined.

Done.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/background.js:35: var options = {
On 2016/02/11 17:57:20, xiyuan wrote:
> nit: move this down to chrome.app.window.create call where it is used.

Done.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/background.js:37: 'singleton':
true,
On 2016/02/11 17:57:21, xiyuan wrote:
> 'singleton' is deprecated. We should have only one instance of the window as
> long as 'id' is specified.

Yes, is see that it is redundant.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/background.js:49:
webview.executeScript({
On 2016/02/11 17:57:21, xiyuan wrote:
> Would "webview.src" give us the URL?

:) This simplifies code.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/background.js:68:
chrome.app.window.create('main.html', options, onWindowCreated);
On 2016/02/11 17:57:21, xiyuan wrote:
> What happens if user closes the window without going through the web flow?
> Should we handle that?

Good point, need to opt out arc. Thanks!

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/arc_opt_in/main.html (right):

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/main.html:2: <html
style="width:100%; height:100%">
On 2016/02/11 17:57:21, xiyuan wrote:
> Inline css is not recommended. Please move them into a <style> tag or a .css
> flie
> 
> <head>
>   <style>
>     html,
>     body,
>     webview {
>       height: 100%;
>       margin: 0;
>       padding: 0;
>       width: 100%;
>     }
>   </style>
> </head>

Nice code. My html is not so good as wanted.

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/main.html:10:
src="https://accounts.google.com/o/oauth2/programmatic_auth?scope=https://www.google.com/accounts/OAuthLogin&client_id=1070009224336-sdh77n7uot3oc99ais00jmuft6sk2fg9.apps.googleusercontent.com">
On 2016/02/11 17:57:21, xiyuan wrote:
> Do we need to have "hl" URL param so that the page is displayed in the correct
> language?

It seems, it can resolve lang automatically. I see different languages on diff
devices. Particular in my recent tests it was shown in Russian lang .

[xiyuan, 2016-02-12 17:06:59]

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/arc_opt_in/main.html (right):

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/main.html:10:
src="https://accounts.google.com/o/oauth2/programmatic_auth?scope=https://www.google.com/accounts/OAuthLogin&client_id=1070009224336-sdh77n7uot3oc99ais00jmuft6sk2fg9.apps.googleusercontent.com">
On 2016/02/12 02:45:24, khmel wrote:
> On 2016/02/11 17:57:21, xiyuan wrote:
> > Do we need to have "hl" URL param so that the page is displayed in the
correct
> > language?
> 
> It seems, it can resolve lang automatically. I see different languages on diff
> devices. Particular in my recent tests it was shown in Russian lang .

Okay. We can leave it for now but I suspect we might need to revisit this later.

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

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:167: AppLaunchParams
params(profile_, extension, SINGLETON_TAB,
nit: SINGLETON_TAB -> NEW_WINDOW

WindowOpenDisposition does not really matter for platform apps. Prefer to use
NEW_WINDOW because TAB sounds a bit strange.

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/arc_support/background.js (right):

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:5: // Chrome window
that hosts UI. Only one window is allowed.
nit: Use JsDoc style and also doc type

/**
 * Chrome window that hosts UI. Only one window is allowed.
 * @type {chrome.app.window.AppWindow}
 */

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:8: // Contains Web
content provided by Google authorization server.
/**
 * Contains Web content provided by Google authorization server.
 * @type {WebView}
 */

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:11: // Used for
bidirectional communication with native code.
/**
 * Used for bidirectional communication with native code.
 * @type {chrome.runtime.Port}
 */

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:15: // closure
notification.
/**
 * Indicates that window was closed internally and it is not required to send
 * closure notification.
 * @type {boolean}
 */

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:18: function
sendNativeMessage(code) {
/**
 * Sends a native message to ArcSupportHost.
 * @param {string} code The action code in message.
 */

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:23: function
onNativeMessage(message) {
/**
 * Handles native messages received from ArcSupportHost.
 * @param {!Object} message The message received.
 */

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:31:
appWindow.contentWindow.close();
nit: appWindow.close();

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:37: function
connectPort() {
/**
 * Connects to ArcSupportHost.
 */

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:44: var
onContentLoad = function() {
nit: onContentLoad -> onAppContentLoad to make it clear that this is the load
event handler for the app window.

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:55:
webview.addEventListener('loadstop', onLoadStop);
loadstop is indeed tricky and triggering unnecessary FetchAuthCode calls is not
good. I wonder whether we should switch to some other event. Maybe use the
webrequest event as we did for the login?

Look for webview_.request.onCompleted.addListener in authenticator.js

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/arc_support/manifest.json (right):

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/manifest.json:12: "name": "ARC
OptIn",
nit: OptIn -> Support

[khmel, 2016-02-12 18:15:15]

Comments addressed + use webRequest.onCompleted instead loadstop event.

PTAL

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/arc_opt_in/main.html (right):

https://codereview.chromium.org/1681813003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_opt_in/main.html:10:
src="https://accounts.google.com/o/oauth2/programmatic_auth?scope=https://www.google.com/accounts/OAuthLogin&client_id=1070009224336-sdh77n7uot3oc99ais00jmuft6sk2fg9.apps.googleusercontent.com">
On 2016/02/12 17:06:59, xiyuan wrote:
> On 2016/02/12 02:45:24, khmel wrote:
> > On 2016/02/11 17:57:21, xiyuan wrote:
> > > Do we need to have "hl" URL param so that the page is displayed in the
> correct
> > > language?
> > 
> > It seems, it can resolve lang automatically. I see different languages on
diff
> > devices. Particular in my recent tests it was shown in Russian lang .
> 
> Okay. We can leave it for now but I suspect we might need to revisit this
later.

Acknowledged.

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

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:167: AppLaunchParams
params(profile_, extension, SINGLETON_TAB,
On 2016/02/12 17:06:59, xiyuan wrote:
> nit: SINGLETON_TAB -> NEW_WINDOW
> 
> WindowOpenDisposition does not really matter for platform apps. Prefer to use
> NEW_WINDOW because TAB sounds a bit strange.

Thanks for explanation. I could not understand the difference for this case :)

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/arc_support/background.js (right):

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:5: // Chrome window
that hosts UI. Only one window is allowed.
On 2016/02/12 17:06:59, xiyuan wrote:
> nit: Use JsDoc style and also doc type
> 
> /**
>  * Chrome window that hosts UI. Only one window is allowed.
>  * @type {chrome.app.window.AppWindow}
>  */

Done.

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:8: // Contains Web
content provided by Google authorization server.
On 2016/02/12 17:06:59, xiyuan wrote:
> /**
>  * Contains Web content provided by Google authorization server.
>  * @type {WebView}
>  */

Done.

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:11: // Used for
bidirectional communication with native code.
On 2016/02/12 17:06:59, xiyuan wrote:
> /**
>  * Used for bidirectional communication with native code.
>  * @type {chrome.runtime.Port}
>  */

Done.

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:15: // closure
notification.
On 2016/02/12 17:06:59, xiyuan wrote:
> /**
>  * Indicates that window was closed internally and it is not required to send
>  * closure notification.
>  * @type {boolean}
>  */ 

Done.

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:18: function
sendNativeMessage(code) {
On 2016/02/12 17:06:59, xiyuan wrote:
> /**
>  * Sends a native message to ArcSupportHost.
>  * @param {string} code The action code in message.
>  */

Done.

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:23: function
onNativeMessage(message) {
On 2016/02/12 17:06:59, xiyuan wrote:
> /**
>  * Handles native messages received from ArcSupportHost.
>  * @param {!Object} message The message received.
>  */

Done.

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:31:
appWindow.contentWindow.close();
On 2016/02/12 17:06:59, xiyuan wrote:
> nit: appWindow.close();

Done.

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:37: function
connectPort() {
On 2016/02/12 17:06:59, xiyuan wrote:
> /**
>  * Connects to ArcSupportHost.
>  */

Done.

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:44: var
onContentLoad = function() {
On 2016/02/12 17:06:59, xiyuan wrote:
> nit: onContentLoad -> onAppContentLoad to make it clear that this is the load
> event handler for the app window.

Done.

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:55:
webview.addEventListener('loadstop', onLoadStop);
On 2016/02/12 17:06:59, xiyuan wrote:
> loadstop is indeed tricky and triggering unnecessary FetchAuthCode calls is
not
> good. I wonder whether we should switch to some other event. Maybe use the
> webrequest event as we did for the login?
> 
> Look for webview_.request.onCompleted.addListener in authenticator.js
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

Thanks for hint! It works much better and predictable.

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/arc_support/manifest.json (right):

https://codereview.chromium.org/1681813003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/manifest.json:12: "name": "ARC
OptIn",
On 2016/02/12 17:06:59, xiyuan wrote:
> nit: OptIn -> Support

Done.

[xiyuan, 2016-02-12 18:25:43]

LGTM! Thank you for being patient with me and making all the changes.

You still need owner's blessings for some dirs, c/b/extensions/*.

https://codereview.chromium.org/1681813003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/arc_support/background.js (right):

https://codereview.chromium.org/1681813003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/arc_support/background.js:86:
requestFilterOptions);
nit: since we don't parsing http header, we can get rid of
|requestFilterOptions|.

[khmel, 2016-02-12 19:30:36]

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

[khmel, 2016-02-12 19:30:36]

Hi Ken,

I need your attention for browser/extensions part.

PTAL

[Ken Rockot(use gerrit already), 2016-02-12 19:35:14]

lgtm

[xiyuan, 2016-02-13 13:57:30]

https://codereview.chromium.org/1681813003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/arc_support/background.js (right):

https://codereview.chromium.org/1681813003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/arc_support/background.js:99: 
We should reset |windowClosedInternally| here just in case.

[jabdelmalek, 2016-02-17 01:47:36]

lgtm

[khmel, 2016-02-17 02:02:30]

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

[khmel, 2016-02-17 02:02:30]

Hi John,

You put l-g-t-m but presubmit shows that I still need approval for 
chrome/browser/extensions/component_extensions_whitelist/whitelist.cc. Probably
you need to use jam@chromium.org account?

https://codereview.chromium.org/1681813003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/arc_support/background.js (right):

https://codereview.chromium.org/1681813003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/chromeos/arc_support/background.js:99: 
On 2016/02/13 13:57:30, xiyuan wrote:
> We should reset |windowClosedInternally| here just in case.

Done.

[jam, 2016-02-17 03:14:40]

lgtm

[khmel, 2016-02-17 03:18:27]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-02-17 03:18:28]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1681813003/#ps140001
(title: "rebased, added reseting windowClosedInternally before window creation")

[commit-bot: I haz the power, 2016-02-17 03:20:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681813003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681813003/140001

[commit-bot: I haz the power, 2016-02-17 04:08:42]

Description was changed from

==========
arc: Use platform app for OptIn dialog.

BUG=585324
TEST=Manually tested with different combination of signed
     profiles and ARC optin state. OptIn dialog does not
     show multiply profile and auth code can be fetched.
     Once fetched authority code remains in cookies and
     can be used on next signin.
==========

to

==========
arc: Use platform app for OptIn dialog.

BUG=585324
TEST=Manually tested with different combination of signed
     profiles and ARC optin state. OptIn dialog does not
     show multiply profile and auth code can be fetched.
     Once fetched authority code remains in cookies and
     can be used on next signin.
==========

[commit-bot: I haz the power, 2016-02-17 04:08:42]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-17 04:10:09]

Description was changed from

==========
arc: Use platform app for OptIn dialog.

BUG=585324
TEST=Manually tested with different combination of signed
     profiles and ARC optin state. OptIn dialog does not
     show multiply profile and auth code can be fetched.
     Once fetched authority code remains in cookies and
     can be used on next signin.
==========

to

==========
arc: Use platform app for OptIn dialog.

BUG=585324
TEST=Manually tested with different combination of signed
     profiles and ARC optin state. OptIn dialog does not
     show multiply profile and auth code can be fetched.
     Once fetched authority code remains in cookies and
     can be used on next signin.

Committed: https://crrev.com/14d72dba39bed3c2d5fb6a90e798855013d37534
Cr-Commit-Position: refs/heads/master@{#375775}
==========

[commit-bot: I haz the power, 2016-02-17 04:10:10]

Patchset 8 (id:??) landed as
https://crrev.com/14d72dba39bed3c2d5fb6a90e798855013d37534
Cr-Commit-Position: refs/heads/master@{#375775}

[Luis Héctor Chávez, 2016-04-06 20:23:05]

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

[Luis Héctor Chávez, 2016-04-06 20:23:06]

https://codereview.chromium.org/1681813003/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/1681813003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_auth_service.cc:34: ArcAuthService*
arc_auth_service = nullptr;
|g_arc_auth_service|

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

https://codereview.chromium.org/1681813003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_auth_service.h:142: scoped_ptr<UbertokenFetcher>
ubertoken_fethcher_;
|ubertoken_fetcher_|.

[xiyuan, 2016-04-06 20:36:23]

https://codereview.chromium.org/1681813003/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/1681813003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_auth_service.cc:34: ArcAuthService*
arc_auth_service = nullptr;
On 2016/04/06 20:23:06, Luis Héctor Chávez wrote:
> |g_arc_auth_service|

g_ prefix is only needed when the var is globally accessible. Here it is in an
anonymous namespace. So no need to have the prefix.