arc: Use v2/auth LSO page.

arc: Use v2/auth LSO page.

This currently shows Android TV LSO page.

BUG=598962
TEST=Manually tested on devices, Auth code is fetched from the
     new LSO page and Arc started. Simulated different error
     scenarios. Error page is shown and retry works as expected.
TEST=unit_tests passes.

Committed: https://crrev.com/25fcad1a0c1ce94fdc46431b32f4be6b27357ace
Cr-Commit-Position: refs/heads/master@{#384932}

[khmel, 2016-03-30 15:24:54]

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

[khmel, 2016-03-30 15:24:55]

Hi Xiyuan,

PTAL

https://codereview.chromium.org/1847463002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/arc_support/background.js (right):

https://codereview.chromium.org/1847463002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:193: var
attemptCount = 10;
This LSO page returns auth code in title.

I know, this code looks bit ugly.
However it is done because probably bug in Chrome:

webview.executeScript is not executed each time, sometimes works, but in most
cases (Desktop Debug config) it fails with a error:
"Extension manifest must request permission to access the respective host"

After investigation I found that this error comes from:
https://chromium.googlesource.com/experimental/chromium/src/+/refs/wip/bajone...

Problem is following. webview.src is not always updated on onCompleted event.

Probably you suggest better way.

[xiyuan, 2016-03-30 17:22:16]

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

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:57: message =
{'action': code};
nit: var, missed this in previous reviews.

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:173:
'https://accounts.google.com/o/oauth2/approval?';
nit: 4-space indent

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:197: },
function(results) {
nit: Can this fit in the previous line?

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:200: authCodePrefix)
{
nit: indent 2 more spaces

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:202:
console.log('Auth code:' + authCode);
debugging log?

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:207:
port.postMessage(message);
I wonder whether we can extend sendNativeMessage to support this,

e.g.

/**
 * Sends a native message to ArcSupportHost.
 * @param {string} code The action code in message.
 * @param {Object=} opt_Props Extra properties for the message.
 */
function sendNativeMessage(code, opt_Props) {
  var message = Object.assign({'action': code}, opt_Props);
  port.postMessage(message);
}

then we can do 

sendNativeMessage('setAuthCode', {code: authCode});

here.

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:209:
setErrorMessage('Authorization failed.');
The string should be put in grd file along with other arc messages for i18n.
Since SetLocalization should be called at this point, we can do

appWindow.contentWindow.loadTimeData.getString(<stringName>);

where stringName is what we set in arc_support_host.cc.

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:215: attemptCount -=
1;
nit: attemptCount--

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:232:
webview.request.onCompleted.addListener(onWebviewRequestCompleted,
We probably should use 'contentload' event on the webview since we do not need
to manipulate http header etc. We probably can get rid of the retry with
'contentload'.

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:234:
webview.request.onResponseStarted.addListener(
Suggest to use 'loadstart' event.

[khmel, 2016-03-30 21:30:18]

Hi Xiyuan,

Thanks for comments, now code looks better.

PTAL

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

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:57: message =
{'action': code};
On 2016/03/30 17:22:16, xiyuan wrote:
> nit: var, missed this in previous reviews.

Done.

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:173:
'https://accounts.google.com/o/oauth2/approval?';
On 2016/03/30 17:22:16, xiyuan wrote:
> nit: 4-space indent

Done.

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:197: },
function(results) {
On 2016/03/30 17:22:16, xiyuan wrote:
> nit: Can this fit in the previous line?

Done.

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:200: authCodePrefix)
{
On 2016/03/30 17:22:16, xiyuan wrote:
> nit: indent 2 more spaces

Done.

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:202:
console.log('Auth code:' + authCode);
On 2016/03/30 17:22:16, xiyuan wrote:
> debugging log?

Done.

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:207:
port.postMessage(message);
On 2016/03/30 17:22:16, xiyuan wrote:
> I wonder whether we can extend sendNativeMessage to support this,
> 
> e.g.
> 
> /**
>  * Sends a native message to ArcSupportHost.
>  * @param {string} code The action code in message.
>  * @param {Object=} opt_Props Extra properties for the message.
>  */
> function sendNativeMessage(code, opt_Props) {
>   var message = Object.assign({'action': code}, opt_Props);
>   port.postMessage(message);
> }
> 
> then we can do 
> 
> sendNativeMessage('setAuthCode', {code: authCode});
> 
> here.

Thanks for idea! I was going to implement something similar.

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:209:
setErrorMessage('Authorization failed.');
On 2016/03/30 17:22:16, xiyuan wrote:
> The string should be put in grd file along with other arc messages for i18n.
> Since SetLocalization should be called at this point, we can do
> 
> appWindow.contentWindow.loadTimeData.getString(<stringName>);
> 
> where stringName is what we set in arc_support_host.cc.

Done.

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:215: attemptCount -=
1;
On 2016/03/30 17:22:16, xiyuan wrote:
> nit: attemptCount--

Done.

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:232:
webview.request.onCompleted.addListener(onWebviewRequestCompleted,
On 2016/03/30 17:22:15, xiyuan wrote:
> We probably should use 'contentload' event on the webview since we do not need
> to manipulate http header etc. We probably can get rid of the retry with
> 'contentload'.

Thanks, it works stable.

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:234:
webview.request.onResponseStarted.addListener(
On 2016/03/30 17:22:16, xiyuan wrote:
> Suggest to use 'loadstart' event.

onResponseStarted is executed on earlier stage.
When I use loadstart event I can see short flickering of the page with an auth
code.

[xiyuan, 2016-03-30 21:32:42]

lgtm

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

https://codereview.chromium.org/1847463002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/arc_support/background.js:234:
webview.request.onResponseStarted.addListener(
On 2016/03/30 21:30:18, khmel wrote:
> On 2016/03/30 17:22:16, xiyuan wrote:
> > Suggest to use 'loadstart' event.
> 
> onResponseStarted is executed on earlier stage.
> When I use loadstart event I can see short flickering of the page with an auth
> code.

Acknowledged.

[khmel, 2016-03-30 21:55:39]

khmel@chromium.org changed reviewers:
+ lhchavez@chromium.org, sergeyu@chromium.org

[khmel, 2016-03-30 21:55:39]

Hi,

Luis, could you please take a look: components/arc/
Sergey, could you please take a look:
	chrome/browser/extensions/api/messaging/arc_support_host.cc

PTAL

[Luis Héctor Chávez, 2016-03-30 23:57:25]

Can you run trybots? It's only removals, so it looks reasonable, but I want to
make sure.

[Sergey Ulanov, 2016-03-31 17:24:24]

LGTM, but please see my comments.
There is a bug ArcSupportHost::OnMessage() when converting Value to
DictionaryValue, but it existed before this CL.

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

https://codereview.chromium.org/1847463002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_support_host.cc:5: #include
"chrome/browser/extensions/api/messaging/arc_support_host.h"
Does this code need to be in this directory? Can it be moved to components/arc?
This directory is intended for messaging API implementation, but not for
implementation of native messaging hosts.

https://codereview.chromium.org/1847463002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_support_host.cc:24: const char
kActionStartLSO[] = "startLSO";
nit: Suggest calling these startLso and kActionStartLso, with lower-case "so".
Style guide is not prescriptive about abbreviations in names, but it gives
"UrlTable" as a good example of proper name, not "URLTable". This approach would
also allow to avoid unreadable names like EnsureNSSSSLInit.

https://codereview.chromium.org/1847463002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_support_host.cc:134:
static_cast<base::DictionaryValue*>(request_value.release()));
You should to use Value::GetAsDictionary() to convert value to DictionaryValue.
This static_cast<> will succeed even if request_value is not a DictionaryValue
and then GetString() call below is likely to crash.

https://codereview.chromium.org/1847463002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_support_host.cc:142: NOTREACHED();
I think you want to log this error, but it shouldn't be marked as NOTREACHED().
You don't want to crash chrome when an extension sends invalid message. The only
exception is when the messages come only from built-in extension. Is that the
case here?

https://codereview.chromium.org/1847463002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_support_host.cc:147:
arc::ArcAuthService::Get()->StartLSO();
nit: StartLso()

[khmel, 2016-03-31 18:02:31]

Thanks Sergey for review.

PTAL

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

https://codereview.chromium.org/1847463002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_support_host.cc:5: #include
"chrome/browser/extensions/api/messaging/arc_support_host.h"
On 2016/03/31 17:24:23, Sergey Ulanov wrote:
> Does this code need to be in this directory? Can it be moved to
components/arc?
> This directory is intended for messaging API implementation, but not for
> implementation of native messaging hosts.

I would say this is hardly achievable. Based on previous reviews we don't want
to add chrome/browser, ash dependencies to components/arc.

As alternative, I can try to move this to c/b/chromeos/arc/
(however arc_support_host.h will be referenced from
c/b/extensions/api/messaging/native_message_host_chromeos.cc)

WDYT?

https://codereview.chromium.org/1847463002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_support_host.cc:24: const char
kActionStartLSO[] = "startLSO";
On 2016/03/31 17:24:23, Sergey Ulanov wrote:
> nit: Suggest calling these startLso and kActionStartLso, with lower-case "so".
> Style guide is not prescriptive about abbreviations in names, but it gives
> "UrlTable" as a good example of proper name, not "URLTable". This approach
would
> also allow to avoid unreadable names like EnsureNSSSSLInit.

Thanks for sharing this, this makes sense. Noted for the future.

https://codereview.chromium.org/1847463002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_support_host.cc:134:
static_cast<base::DictionaryValue*>(request_value.release()));
On 2016/03/31 17:24:23, Sergey Ulanov wrote:
> You should to use Value::GetAsDictionary() to convert value to
DictionaryValue.
> This static_cast<> will succeed even if request_value is not a DictionaryValue
> and then GetString() call below is likely to crash.

https://codereview.chromium.org/1847463002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_support_host.cc:142: NOTREACHED();
On 2016/03/31 17:24:24, Sergey Ulanov wrote:
> I think you want to log this error, but it shouldn't be marked as
NOTREACHED().
> You don't want to crash chrome when an extension sends invalid message. The
only
> exception is when the messages come only from built-in extension. Is that the
> case here?

This is our built_in extension. Seems NOTREACHED  is acceptable here.

https://codereview.chromium.org/1847463002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_support_host.cc:147:
arc::ArcAuthService::Get()->StartLSO();
On 2016/03/31 17:24:23, Sergey Ulanov wrote:
> nit: StartLso()

Done.

[Sergey Ulanov, 2016-03-31 18:47:54]

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

https://codereview.chromium.org/1847463002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_support_host.cc:5: #include
"chrome/browser/extensions/api/messaging/arc_support_host.h"
On 2016/03/31 18:02:31, khmel wrote:
> On 2016/03/31 17:24:23, Sergey Ulanov wrote:
> > Does this code need to be in this directory? Can it be moved to
> components/arc?
> > This directory is intended for messaging API implementation, but not for
> > implementation of native messaging hosts.
> 
> I would say this is hardly achievable. Based on previous reviews we don't want
> to add chrome/browser, ash dependencies to components/arc.
> 
> As alternative, I can try to move this to c/b/chromeos/arc/
> (however arc_support_host.h will be referenced from
> c/b/extensions/api/messaging/native_message_host_chromeos.cc)
> 
> WDYT?

I think c/b/chromeos/arc/ would be a better place for this code.

[khmel, 2016-03-31 19:33:33]

Moved arc_support_host to c/b/chromeos/arc

PTAL

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

https://codereview.chromium.org/1847463002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/arc_support_host.cc:5: #include
"chrome/browser/extensions/api/messaging/arc_support_host.h"
On 2016/03/31 18:47:54, Sergey Ulanov wrote:
> On 2016/03/31 18:02:31, khmel wrote:
> > On 2016/03/31 17:24:23, Sergey Ulanov wrote:
> > > Does this code need to be in this directory? Can it be moved to
> > components/arc?
> > > This directory is intended for messaging API implementation, but not for
> > > implementation of native messaging hosts.
> > 
> > I would say this is hardly achievable. Based on previous reviews we don't
want
> > to add chrome/browser, ash dependencies to components/arc.
> > 
> > As alternative, I can try to move this to c/b/chromeos/arc/
> > (however arc_support_host.h will be referenced from
> > c/b/extensions/api/messaging/native_message_host_chromeos.cc)
> > 
> > WDYT?
> 
> I think c/b/chromeos/arc/ would be a better place for this code.

Done.

[Luis Héctor Chávez, 2016-03-31 19:37:33]

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

[commit-bot: I haz the power, 2016-03-31 19:38:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847463002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847463002/100001

[commit-bot: I haz the power, 2016-03-31 19:41:00]

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

[commit-bot: I haz the power, 2016-03-31 19:41:01]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Luis Héctor Chávez, 2016-03-31 19:49:36]

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

[commit-bot: I haz the power, 2016-03-31 19:50:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1847463002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1847463002/120001

[Sergey Ulanov, 2016-03-31 20:23:20]

lgtm

[Luis Héctor Chávez, 2016-03-31 20:24:27]

components/arc lgtm

[commit-bot: I haz the power, 2016-03-31 21:10:59]

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

[commit-bot: I haz the power, 2016-03-31 21:11:01]

Dry run: This issue passed the CQ dry run.

[khmel, 2016-04-04 15:55:25]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-04-04 15:55:26]

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/1847463002/#ps120001
(title: "rebased")

[commit-bot: I haz the power, 2016-04-04 15:55:50]

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

[commit-bot: I haz the power, 2016-04-04 17:03:52]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-04 17:05:01]

Description was changed from

==========
arc: Use v2/auth LSO page.

This currently shows Android TV LSO page.

BUG=598962
TEST=Manually tested on devices, Auth code is fetched from the
     new LSO page and Arc started. Simulated different error
     scenarios. Error page is shown and retry works as expected.
TEST=unit_tests passes.
==========

to

==========
arc: Use v2/auth LSO page.

This currently shows Android TV LSO page.

BUG=598962
TEST=Manually tested on devices, Auth code is fetched from the
     new LSO page and Arc started. Simulated different error
     scenarios. Error page is shown and retry works as expected.
TEST=unit_tests passes.

Committed: https://crrev.com/25fcad1a0c1ce94fdc46431b32f4be6b27357ace
Cr-Commit-Position: refs/heads/master@{#384932}
==========

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

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