Add sessionType and playStoreStatus to chromeosInfoPrivate.

Add sessionType and playStoreStatus to chromeosInfoPrivate.

Add properties for getting the session type and the playstore status to
chromeosInfoPrivate. This CL also whitelists the GetHelp and the Demo
apps to be able to use the API.

R=rdevlin.cronin@chromium.org, xiyuan@chromium.org
BUG=644432, 644446
Committed: https://crrev.com/dd8446dec9f27e7d46dbc607f6d16e8b7fbab8ab
Cr-Commit-Position: refs/heads/master@{#417182}

[Devlin, 2016-09-06 23:04:54]

I'm guessing you forgot to git add a few files?

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/info_private_api.cc (right):

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_api.cc:119: const char
kSessionTypePublicSession[] = "public session";
Why not use the generated conversion functions?

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_api.cc:247: } else if
(ExtensionsBrowserClient::Get()->IsLoggedInAsPublicAccount()) {
no else after return.  Ideally, that'd go for this whole function.  If you don't
want to do that in this CL, that's fine, but please add a TODO and format these
correctly.

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_apitest.cc:22: // This class
contains chrome.chromeosInfoPrivate API tests.
nit: I don't think this comment adds much that isn't apparent from the class
name; I'd remove it.

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_apitest.cc:90:
ASSERT_TRUE(RunPlatformAppTestWithArg("chromeos_info_private/extended",
This file doesn't exist

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/chromeos_info_private.json (right):

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/chromeos_info_private.json:35: "enum": ["normal",
nit: we don't need to line wrap these

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/chromeos_info_private.json:38: "description":
"Current session type"
Can we describe the different values?

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/chromeos_info_private.json:43: "enum": ["not
available",
ditto re line wrapping

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/chromeos_info_private.json:46: "description":
"Status of the play store"
ditto - esp given "available" and "enabled" sound very similar to me.

https://codereview.chromium.org/2314313002/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/chromeos_info_private/background.js
(left):

https://codereview.chromium.org/2314313002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/chromeos_info_private/background.js:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
Where did these tests go?

[Rahul Chaturvedi, 2016-09-06 23:19:36]

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/info_private_api.cc (right):

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_api.cc:119: const char
kSessionTypePublicSession[] = "public session";
On 2016/09/06 23:04:53, Devlin wrote:
> Why not use the generated conversion functions?

Which generated conversion functions?

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_api.cc:247: } else if
(ExtensionsBrowserClient::Get()->IsLoggedInAsPublicAccount()) {
On 2016/09/06 23:04:53, Devlin wrote:
> no else after return.  Ideally, that'd go for this whole function.  If you
don't
> want to do that in this CL, that's fine, but please add a TODO and format
these
> correctly.

I was trying to stick to the style in the file. If we're going to change it
here, I would rather we change it everywhere.
Done.

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_apitest.cc:22: // This class
contains chrome.chromeosInfoPrivate API tests.
On 2016/09/06 23:04:53, Devlin wrote:
> nit: I don't think this comment adds much that isn't apparent from the class
> name; I'd remove it.

Done.

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_apitest.cc:90:
ASSERT_TRUE(RunPlatformAppTestWithArg("chromeos_info_private/extended",
On 2016/09/06 23:04:53, Devlin wrote:
> This file doesn't exist

Done.

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/chromeos_info_private.json (right):

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/chromeos_info_private.json:35: "enum": ["normal",
On 2016/09/06 23:04:54, Devlin wrote:
> nit: we don't need to line wrap these

Done.

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/chromeos_info_private.json:38: "description":
"Current session type"
On 2016/09/06 23:04:53, Devlin wrote:
> Can we describe the different values?

I am not sure if adding a description here will convey much more than what the
values themselves convey.

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/chromeos_info_private.json:43: "enum": ["not
available",
On 2016/09/06 23:04:53, Devlin wrote:
> ditto re line wrapping

Done.

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/chromeos_info_private.json:46: "description":
"Status of the play store"
On 2016/09/06 23:04:53, Devlin wrote:
> ditto - esp given "available" and "enabled" sound very similar to me.

Clarified the difference between available and enabled. The rest of the names
are pretty self-descriptive.

https://codereview.chromium.org/2314313002/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/chromeos_info_private/background.js
(left):

https://codereview.chromium.org/2314313002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/chromeos_info_private/background.js:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
On 2016/09/06 23:04:53, Devlin wrote:
> Where did these tests go?

Uploaded the missing test files.

[Devlin, 2016-09-07 16:12:29]

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/info_private_api.cc (right):

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_api.cc:119: const char
kSessionTypePublicSession[] = "public session";
On 2016/09/06 23:19:35, Rahul Chaturvedi wrote:
> On 2016/09/06 23:04:53, Devlin wrote:
> > Why not use the generated conversion functions?
> 
> Which generated conversion functions?
> 

We generate conversions from enums to strings, e.g.
https://cs.chromium.org/chromium/src/out/Debug/gen/extensions/common/api/runt....
 This helps prevent typos in the string value.

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/chromeos_info_private.json (right):

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/chromeos_info_private.json:38: "description":
"Current session type"
On 2016/09/06 23:19:35, Rahul Chaturvedi wrote:
> On 2016/09/06 23:04:53, Devlin wrote:
> > Can we describe the different values?
> 
> I am not sure if adding a description here will convey much more than what the
> values themselves convey.

What is a "normal" session?  Signed-in?  Not signed in?  Do we document the
meaning and implication of public sessions?  Can we link to that documentation
here?

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/chromeos_info_private.json:46: "description":
"Status of the play store"
On 2016/09/06 23:19:35, Rahul Chaturvedi wrote:
> On 2016/09/06 23:04:53, Devlin wrote:
> > ditto - esp given "available" and "enabled" sound very similar to me.
> 
> Clarified the difference between available and enabled. The rest of the names
> are pretty self-descriptive.

What is the play store in this context?  Why wouldn't it be available?  When
would it be enabled but not available?

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:37:
base::CommandLine::ForCurrentProcess()->AppendSwitch(
Does it matter that we're appending all these switches after the command line is
set up and we initialize everything?

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:54:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, Basic) {
"Basic" doesn't really describe what this tests.

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:87:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, Kiosk) {
What about public session?

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:87:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, Kiosk) {
Browser tests are pretty slow.  If we can adjust the session type on the fly
like this (see also comment at l37), can we run all these tests in a single
test?

https://codereview.chromium.org/2314313002/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js
(right):

https://codereview.chromium.org/2314313002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js:1:
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
not 2011 :)

https://codereview.chromium.org/2314313002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js:7:
var testArgs = JSON.parse(config.customArg);
Why not just pass the test name?

[Rahul Chaturvedi, 2016-09-07 20:13:24]

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/info_private_api.cc (right):

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_api.cc:119: const char
kSessionTypePublicSession[] = "public session";
On 2016/09/07 16:12:28, Devlin wrote:
> On 2016/09/06 23:19:35, Rahul Chaturvedi wrote:
> > On 2016/09/06 23:04:53, Devlin wrote:
> > > Why not use the generated conversion functions?
> > 
> > Which generated conversion functions?
> > 
> 
> We generate conversions from enums to strings, e.g.
>
https://cs.chromium.org/chromium/src/out/Debug/gen/extensions/common/api/runt....
>  This helps prevent typos in the string value.

I understand that - but where do I find the conversions for this API. AFAICT, we
don't actually generate a gen/extensions/common/api/info_private.[h|cc]

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/chromeos_info_private.json (right):

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/chromeos_info_private.json:38: "description":
"Current session type"
On 2016/09/07 16:12:28, Devlin wrote:
> On 2016/09/06 23:19:35, Rahul Chaturvedi wrote:
> > On 2016/09/06 23:04:53, Devlin wrote:
> > > Can we describe the different values?
> > 
> > I am not sure if adding a description here will convey much more than what
the
> > values themselves convey.
> 
> What is a "normal" session?  Signed-in?  Not signed in?  Do we document the
> meaning and implication of public sessions?  Can we link to that documentation
> here?

Aren't these things that we'd typically put in API documentation? It seems a bit
excessive to give someone that much context with just this description.

Considering this is currently a private API and hence we don't have any
documentation for it, if someone does not actually understand these concepts,
they really should not be using this API.

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/chromeos_info_private.json:46: "description":
"Status of the play store"
On 2016/09/07 16:12:28, Devlin wrote:
> On 2016/09/06 23:19:35, Rahul Chaturvedi wrote:
> > On 2016/09/06 23:04:53, Devlin wrote:
> > > ditto - esp given "available" and "enabled" sound very similar to me.
> > 
> > Clarified the difference between available and enabled. The rest of the
names
> > are pretty self-descriptive.
> 
> What is the play store in this context?  Why wouldn't it be available?  When
> would it be enabled but not available?

Same explanation as above.

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:37:
base::CommandLine::ForCurrentProcess()->AppendSwitch(
On 2016/09/07 16:12:28, Devlin wrote:
> Does it matter that we're appending all these switches after the command line
is
> set up and we initialize everything?

Nope. These aren't checked at startup, they are checked during the call to the
API.

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:54:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, Basic) {
On 2016/09/07 16:12:28, Devlin wrote:
> "Basic" doesn't really describe what this tests.

It is better than "ChromeOSInfoPrivateTest", which it was before.
When I read "Basic", to me it usually means basic functionality, which is
exactly what this test does.

If you can suggest a better name, I'll gladly change it to that.

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:87:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, Kiosk) {
On 2016/09/07 16:12:28, Devlin wrote:
> What about public session?

I have no good way to test that now. I will add tests for it later, once this is
merged in to M54.

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:87:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, Kiosk) {
On 2016/09/07 16:12:28, Devlin wrote:
> Browser tests are pretty slow.  If we can adjust the session type on the fly
> like this (see also comment at l37), can we run all these tests in a single
> test?

Calling RunPlatformAppTestWithArg multiple times on the same app from within the
same test seems to crash.

https://codereview.chromium.org/2314313002/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js
(right):

https://codereview.chromium.org/2314313002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js:1:
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
On 2016/09/07 16:12:28, Devlin wrote:
> not 2011 :)

Done.

https://codereview.chromium.org/2314313002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js:7:
var testArgs = JSON.parse(config.customArg);
On 2016/09/07 16:12:28, Devlin wrote:
> Why not just pass the test name?

Done.

[Devlin, 2016-09-07 20:23:27]

Looks like the latest patch upload was missing?

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/info_private_api.cc (right):

https://codereview.chromium.org/2314313002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/info_private_api.cc:119: const char
kSessionTypePublicSession[] = "public session";
On 2016/09/07 20:13:23, Rahul Chaturvedi wrote:
> On 2016/09/07 16:12:28, Devlin wrote:
> > On 2016/09/06 23:19:35, Rahul Chaturvedi wrote:
> > > On 2016/09/06 23:04:53, Devlin wrote:
> > > > Why not use the generated conversion functions?
> > > 
> > > Which generated conversion functions?
> > > 
> > 
> > We generate conversions from enums to strings, e.g.
> >
>
https://cs.chromium.org/chromium/src/out/Debug/gen/extensions/common/api/runt....
> >  This helps prevent typos in the string value.
> 
> I understand that - but where do I find the conversions for this API. AFAICT,
we
> don't actually generate a gen/extensions/common/api/info_private.[h|cc]

/sigh...it looks like we aren't compiling this API (it's not included in the
schemas.gypi).  That, I suppose, also explains why we're hand-constructing the
return value.

Changing it to compile and use the generated conversions is probably out of
scope for this change.  Mind adding a TODO?

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/chromeos_info_private.json (right):

https://codereview.chromium.org/2314313002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/chromeos_info_private.json:38: "description":
"Current session type"
On 2016/09/07 20:13:23, Rahul Chaturvedi wrote:
> On 2016/09/07 16:12:28, Devlin wrote:
> > On 2016/09/06 23:19:35, Rahul Chaturvedi wrote:
> > > On 2016/09/06 23:04:53, Devlin wrote:
> > > > Can we describe the different values?
> > > 
> > > I am not sure if adding a description here will convey much more than what
> the
> > > values themselves convey.
> > 
> > What is a "normal" session?  Signed-in?  Not signed in?  Do we document the
> > meaning and implication of public sessions?  Can we link to that
documentation
> > here?
> 
> Aren't these things that we'd typically put in API documentation? It seems a
bit
> excessive to give someone that much context with just this description.
> 
> Considering this is currently a private API and hence we don't have any
> documentation for it, if someone does not actually understand these concepts,
> they really should not be using this API.

This is the API documentation. :)  If we want to hold off on more documentation
for this for now, that's fine, but we'll probably need to add it at some point.

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:87:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, Kiosk) {
On 2016/09/07 20:13:23, Rahul Chaturvedi wrote:
> On 2016/09/07 16:12:28, Devlin wrote:
> > Browser tests are pretty slow.  If we can adjust the session type on the fly
> > like this (see also comment at l37), can we run all these tests in a single
> > test?
> 
> Calling RunPlatformAppTestWithArg multiple times on the same app from within
the
> same test seems to crash.

That's because RunPlatformAppTest loads the extension, which is already loaded
here.  All you want to do is actually run the test (i.e. navigate to the page
and wait for a result).

[Devlin, 2016-09-07 20:38:50]

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:87:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, Kiosk) {
On 2016/09/07 20:23:27, Devlin wrote:
> On 2016/09/07 20:13:23, Rahul Chaturvedi wrote:
> > On 2016/09/07 16:12:28, Devlin wrote:
> > > Browser tests are pretty slow.  If we can adjust the session type on the
fly
> > > like this (see also comment at l37), can we run all these tests in a
single
> > > test?
> > 
> > Calling RunPlatformAppTestWithArg multiple times on the same app from within
> the
> > same test seems to crash.
> 
> That's because RunPlatformAppTest loads the extension, which is already loaded
> here.  All you want to do is actually run the test (i.e. navigate to the page
> and wait for a result).

As discussed offline, we can leave this as-is for now.  Please add a TODO to
investigate merging these tests.

[Devlin, 2016-09-07 20:40:29]

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:54:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, Basic) {
On 2016/09/07 20:13:23, Rahul Chaturvedi wrote:
> On 2016/09/07 16:12:28, Devlin wrote:
> > "Basic" doesn't really describe what this tests.
> 
> It is better than "ChromeOSInfoPrivateTest", which it was before.
> When I read "Basic", to me it usually means basic functionality, which is
> exactly what this test does.
> 
> If you can suggest a better name, I'll gladly change it to that.

Looks like we just test get() and set() in that test, so how about
TestGetAndSet?

[Rahul Chaturvedi, 2016-09-07 20:45:02]

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:54:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, Basic) {
On 2016/09/07 20:40:29, Devlin wrote:
> On 2016/09/07 20:13:23, Rahul Chaturvedi wrote:
> > On 2016/09/07 16:12:28, Devlin wrote:
> > > "Basic" doesn't really describe what this tests.
> > 
> > It is better than "ChromeOSInfoPrivateTest", which it was before.
> > When I read "Basic", to me it usually means basic functionality, which is
> > exactly what this test does.
> > 
> > If you can suggest a better name, I'll gladly change it to that.
> 
> Looks like we just test get() and set() in that test, so how about
> TestGetAndSet?

Done.

https://codereview.chromium.org/2314313002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:87:
IN_PROC_BROWSER_TEST_F(ChromeOSInfoPrivateTest, Kiosk) {
On 2016/09/07 20:38:50, Devlin wrote:
> On 2016/09/07 20:23:27, Devlin wrote:
> > On 2016/09/07 20:13:23, Rahul Chaturvedi wrote:
> > > On 2016/09/07 16:12:28, Devlin wrote:
> > > > Browser tests are pretty slow.  If we can adjust the session type on the
> fly
> > > > like this (see also comment at l37), can we run all these tests in a
> single
> > > > test?
> > > 
> > > Calling RunPlatformAppTestWithArg multiple times on the same app from
within
> > the
> > > same test seems to crash.
> > 
> > That's because RunPlatformAppTest loads the extension, which is already
loaded
> > here.  All you want to do is actually run the test (i.e. navigate to the
page
> > and wait for a result).
> 
> As discussed offline, we can leave this as-is for now.  Please add a TODO to
> investigate merging these tests.

Done.

[xiyuan, 2016-09-07 22:25:00]

lgtm

[Devlin, 2016-09-07 23:08:43]

lgtm with nits

https://codereview.chromium.org/2314313002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2314313002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:89:
GenerateTestName("kiosk").c_str()))
Why not just "kiosk"?

https://codereview.chromium.org/2314313002/diff/80001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js
(right):

https://codereview.chromium.org/2314313002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js:7:
var testName = JSON.parse(config.customArg);
why not just testname = config.customArg?

[Rahul Chaturvedi, 2016-09-07 23:40:55]

https://codereview.chromium.org/2314313002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/info_private_apitest.cc (right):

https://codereview.chromium.org/2314313002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/info_private_apitest.cc:89:
GenerateTestName("kiosk").c_str()))
On 2016/09/07 23:08:43, Devlin wrote:
> Why not just "kiosk"?

Done.

https://codereview.chromium.org/2314313002/diff/80001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js
(right):

https://codereview.chromium.org/2314313002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js:7:
var testName = JSON.parse(config.customArg);
On 2016/09/07 23:08:43, Devlin wrote:
> why not just testname = config.customArg?

Done.

[Rahul Chaturvedi, 2016-09-07 23:41:02]

The CQ bit was checked by steel@chromium.org

[Rahul Chaturvedi, 2016-09-07 23:41:02]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2314313002/#ps100001
(title: "test changes")

[commit-bot: I haz the power, 2016-09-07 23:41:57]

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-07 23:45:02]

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

[commit-bot: I haz the power, 2016-09-07 23:45:03]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Rahul Chaturvedi, 2016-09-08 03:38:46]

The CQ bit was checked by steel@chromium.org

[commit-bot: I haz the power, 2016-09-08 03:39:00]

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:54:35]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-08 03:56:16]

Description was changed from

==========
Add sessionType and playStoreStatus to chromeosInfoPrivate.

Add properties for getting the session type and the playstore status to
chromeosInfoPrivate. This CL also whitelists the GetHelp and the Demo
apps to be able to use the API.

R=rdevlin.cronin@chromium.org, xiyuan@chromium.org
BUG=644432,644446
==========

to

==========
Add sessionType and playStoreStatus to chromeosInfoPrivate.

Add properties for getting the session type and the playstore status to
chromeosInfoPrivate. This CL also whitelists the GetHelp and the Demo
apps to be able to use the API.

R=rdevlin.cronin@chromium.org, xiyuan@chromium.org
BUG=644432,644446

Committed: https://crrev.com/dd8446dec9f27e7d46dbc607f6d16e8b7fbab8ab
Cr-Commit-Position: refs/heads/master@{#417182}
==========

[commit-bot: I haz the power, 2016-09-08 03:56:17]

Patchset 6 (id:??) landed as
https://crrev.com/dd8446dec9f27e7d46dbc607f6d16e8b7fbab8ab
Cr-Commit-Position: refs/heads/master@{#417182}