Implement kiosk multiple apps feature.

Implement kiosk multiple apps feature. This includes:
1. Add new manifest keys in the manifest file of the primary kiosk app to configure secondary kiosk apps.
2. Implement the work flow for primary kiosk to download and install secondary apps before its first time launch.
3. Expand FakeCWS to support multiple kiosk apps' update check.
4. Move the hard coded local extension cache director to chromeos path service so that it can be created in testing environment.

BUG=510217
TBR=stevenjb

Committed: https://crrev.com/26b7f4356b905831f152cebb28c08a16ad72d80c
Cr-Commit-Position: refs/heads/master@{#347237}

[jennyz, 2015-08-26 00:19:48]

jennyz@chromium.org changed reviewers:
+ kalman@chromium.org, xiyuan@chromium.org

[jennyz, 2015-08-26 00:37:09]

xiyuan: Please review all. Sorry this is pretty big.
kalman: Please review the files under chrome/browser/extensions/ and extensions

Thanks,

Jenny

[not at google - send to devlin, 2015-08-26 17:53:59]

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/fake_cws.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.cc:69: FakeCWS::FakeCWS() :
update_check_count_(0) {
Is somebody reviewing this file for correctness?

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.cc:76: has_update_template_ =
std::string(kAppHasUpdateTemplate);
std::string not needed, it will implicitly convert. This comment applies
throughout this file.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/fake_cws.h (right):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.h:63: // map keyed by app_id to
app_update_content.
"map keyed..." --> "Map keyed..."

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:509: for (size_t i = 0; i
< ids.size(); ++i) {
for (const std::string& it : ids) { ... }

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:207:
!extensions::ExtensionsBrowserClient::Get()->IsRunningInForcedAppMode()) {
Can you write this comment in such a way that doesn't only draw attention to the
kiosk app? Something like "External extensions can be cached because they're the
same across users, but it's necessary to exclude kiosk apps because...?"

Nobody is really around that understands this code, so I need some help to
understand it.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:494:
DCHECK(kiosk_app_manager);
When will this be null? It's a pain to DCHECK it here then "if
(kiosk_app_manager)" check it everywhere below. It'd be nice to have this be a
CHECK then don't check |kiosk_app_manager| all the time.

https://codereview.chromium.org/1301323005/diff/20001/extensions/browser/upda...
File extensions/browser/updater/extension_downloader.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/extensions/browser/upda...
extensions/browser/updater/extension_downloader.cc:572: }
Why this change?

https://codereview.chromium.org/1301323005/diff/20001/extensions/common/manif...
File extensions/common/manifest_handlers/kiosk_secondary_apps_info.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info.cc:47:
base::string16* error) {
It's easier to use a manifest type than hand-writing your own parsing:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

for example usage of one of these, see:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

It's less error prone, and gives you the opportunity to auto-generate
documentation.

https://codereview.chromium.org/1301323005/diff/20001/extensions/common/manif...
File extensions/common/manifest_handlers/kiosk_secondary_apps_info_unittest.cc
(right):

https://codereview.chromium.org/1301323005/diff/20001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info_unittest.cc:31:
kSecondaryAppId2 == info->ids[1]);
This doesn't test the case where they're the same. I think there is some gtest
helper which compares lists in an arbitrary order.

[xiyuan, 2015-08-26 18:16:17]

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/fake_cws.cc (left):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.cc:43: no_update_template_ =
"chromeos/app_mode/webstore/update_check/no_update.xml";
nit: remove the two files since we have moved the content in this file now?

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.cc:55:
"chromeos/app_mode/webstore/update_check/has_update_private_store.xml";
nit: remove this file too.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/fake_cws.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.cc:35: "<updatecheck
status=\"noupdate\"/></app>";
nit: indent 2 more spaces and split </app> to the next line.

It would be easier to read the indented xml format.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.cc:173:
apps_content.append(id_to_update_check_content_map_[ids[i]]);
nit: use iterate to save a bit

auto it = id_to_update_check_content_map_.find(ids[i]);
if (it == id_to_update_check_content_map_.end()) {
...
}
apps_content.append(*it);

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/fake_cws.h (right):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.h:50: bool GetUpdateCheckContent(const
std::vector<std::string>& ids,
nit: #include <vector> and <string>

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.h:64: std::map<std::string,
std::string> id_to_update_check_content_map_;
nit: #include <map>

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:546: :
ownership_established_(false), external_loader_created_(false) {
Initialize |secondary_app_external_loader_created_| to false here.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1484:
EXPECT_TRUE(base::TouchFile(cache_init_file, now, now));
nit: This is probably not necessary since we just created the file on previous
line.

https://codereview.chromium.org/1301323005/diff/20001/chromeos/chromeos_paths.h
File chromeos/chromeos_paths.h (right):

https://codereview.chromium.org/1301323005/diff/20001/chromeos/chromeos_paths...
chromeos/chromeos_paths.h:48: // calibration files can be found.
nit: The whitespace change from line 22 to here seems to be random. Why do we
need to change that?

https://codereview.chromium.org/1301323005/diff/20001/extensions/common/manif...
File extensions/common/manifest_handlers/kiosk_secondary_apps_info.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info.cc:22: }
nit: insert an empty line before. And append "// namespace".

https://codereview.chromium.org/1301323005/diff/20001/extensions/common/manif...
File extensions/common/manifest_handlers/kiosk_secondary_apps_info.h (right):

https://codereview.chromium.org/1301323005/diff/20001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info.h:33: // Parses
the "kiosk_enabled" and "kiosk_only" manifest keys.
nit: update this?

[jennyz, 2015-08-28 18:24:09]

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/fake_cws.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.cc:35: "<updatecheck
status=\"noupdate\"/></app>";
On 2015/08/26 18:16:16, xiyuan wrote:
> nit: indent 2 more spaces and split </app> to the next line.
> 
> It would be easier to read the indented xml format.

Done.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.cc:69: FakeCWS::FakeCWS() :
update_check_count_(0) {
On 2015/08/26 17:53:58, kalman wrote:
> Is somebody reviewing this file for correctness?

xiyuan is reviewing this one.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.cc:76: has_update_template_ =
std::string(kAppHasUpdateTemplate);
On 2015/08/26 17:53:58, kalman wrote:
> std::string not needed, it will implicitly convert. This comment applies
> throughout this file.

Done.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.cc:173:
apps_content.append(id_to_update_check_content_map_[ids[i]]);
On 2015/08/26 18:16:16, xiyuan wrote:
> nit: use iterate to save a bit
> 
> auto it = id_to_update_check_content_map_.find(ids[i]);
> if (it == id_to_update_check_content_map_.end()) {
> ...
> }
> apps_content.append(*it);

Done.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/fake_cws.h (right):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.h:50: bool GetUpdateCheckContent(const
std::vector<std::string>& ids,
On 2015/08/26 18:16:16, xiyuan wrote:
> nit: #include <vector> and <string>

Done.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.h:63: // map keyed by app_id to
app_update_content.
On 2015/08/26 17:53:58, kalman wrote:
> "map keyed..." --> "Map keyed..."

Done.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/fake_cws.h:64: std::map<std::string,
std::string> id_to_update_check_content_map_;
On 2015/08/26 18:16:16, xiyuan wrote:
> nit: #include <map>

Done.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:509: for (size_t i = 0; i
< ids.size(); ++i) {
On 2015/08/26 17:53:58, kalman wrote:
> for (const std::string& it : ids) { ... }

Done.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:546: :
ownership_established_(false), external_loader_created_(false) {
On 2015/08/26 18:16:16, xiyuan wrote:
> Initialize |secondary_app_external_loader_created_| to false here.

Done.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/kiosk_browsertest.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/kiosk_browsertest.cc:1484:
EXPECT_TRUE(base::TouchFile(cache_init_file, now, now));
On 2015/08/26 18:16:17, xiyuan wrote:
> nit: This is probably not necessary since we just created the file on previous
> line.

Done.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:207:
!extensions::ExtensionsBrowserClient::Get()->IsRunningInForcedAppMode()) {
On 2015/08/26 17:53:58, kalman wrote:
> Can you write this comment in such a way that doesn't only draw attention to
the
> kiosk app? Something like "External extensions can be cached because they're
the
> same across users, but it's necessary to exclude kiosk apps because...?"
> 
> Nobody is really around that understands this code, so I need some help to
> understand it.

Done.

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:494:
DCHECK(kiosk_app_manager);
On 2015/08/26 17:53:58, kalman wrote:
> When will this be null? It's a pain to DCHECK it here then "if
> (kiosk_app_manager)" check it everywhere below. It'd be nice to have this be a
> CHECK then don't check |kiosk_app_manager| all the time.

Done.

https://codereview.chromium.org/1301323005/diff/20001/chromeos/chromeos_paths.h
File chromeos/chromeos_paths.h (right):

https://codereview.chromium.org/1301323005/diff/20001/chromeos/chromeos_paths...
chromeos/chromeos_paths.h:48: // calibration files can be found.
On 2015/08/26 18:16:17, xiyuan wrote:
> nit: The whitespace change from line 22 to here seems to be random. Why do we
> need to change that?

It's done by "git cl format" :) Restored.

https://codereview.chromium.org/1301323005/diff/20001/extensions/browser/upda...
File extensions/browser/updater/extension_downloader.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/extensions/browser/upda...
extensions/browser/updater/extension_downloader.cc:572: }
On 2015/08/26 17:53:58, kalman wrote:
> Why this change?

Woops, removed this one by accident. Restored.

https://codereview.chromium.org/1301323005/diff/20001/extensions/common/manif...
File extensions/common/manifest_handlers/kiosk_secondary_apps_info.cc (right):

https://codereview.chromium.org/1301323005/diff/20001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info.cc:22: }
On 2015/08/26 18:16:17, xiyuan wrote:
> nit: insert an empty line before. And append "// namespace".

Done.

https://codereview.chromium.org/1301323005/diff/20001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info.cc:47:
base::string16* error) {
On 2015/08/26 17:53:59, kalman wrote:
> It's easier to use a manifest type than hand-writing your own parsing:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
> 
> for example usage of one of these, see:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
> 
> It's less error prone, and gives you the opportunity to auto-generate
> documentation.

Yes, I applied the patch you suggested to
tools/json_schema_compiler/h_generator.py, it fixed the issue of header file of
the type missing FromValue function, but still it does not generate the parsing
code to wrap the top level vector type. So I called the
json_schema_compiler::util::PopulateArrayFromList to pass the list here. It
would be nice if this is supported by the utility script.

https://codereview.chromium.org/1301323005/diff/20001/extensions/common/manif...
File extensions/common/manifest_handlers/kiosk_secondary_apps_info.h (right):

https://codereview.chromium.org/1301323005/diff/20001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info.h:33: // Parses
the "kiosk_enabled" and "kiosk_only" manifest keys.
On 2015/08/26 18:16:17, xiyuan wrote:
> nit: update this?

Done.

https://codereview.chromium.org/1301323005/diff/20001/extensions/common/manif...
File extensions/common/manifest_handlers/kiosk_secondary_apps_info_unittest.cc
(right):

https://codereview.chromium.org/1301323005/diff/20001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info_unittest.cc:31:
kSecondaryAppId2 == info->ids[1]);
On 2015/08/26 17:53:59, kalman wrote:
> This doesn't test the case where they're the same. I think there is some gtest
> helper which compares lists in an arbitrary order.

I didn't find the helper function for comparing two lists in an arbitrary order,
so I just use std::sort and std::equal to implement it.

[xiyuan, 2015-08-28 20:22:08]

LGTM

https://codereview.chromium.org/1301323005/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/1301323005/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:497: if (kiosk_app_manager
&& !kiosk_app_manager->external_loader_created()) {
nit: |kiosk_app_manager| check is no longer necessary now.

https://codereview.chromium.org/1301323005/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:510: if (kiosk_app_manager
&&
same here.

https://codereview.chromium.org/1301323005/diff/60001/extensions/common/manif...
File extensions/common/manifest_handlers/kiosk_secondary_apps_info_unittest.cc
(right):

https://codereview.chromium.org/1301323005/diff/60001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info_unittest.cc:21:
std::vector<std::string> expected_ids = {"fiehokkcgaojmbhfhlpiheggjhaedjoc",
nit: const

[jennyz, 2015-08-28 20:49:55]

https://codereview.chromium.org/1301323005/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/1301323005/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:497: if (kiosk_app_manager
&& !kiosk_app_manager->external_loader_created()) {
On 2015/08/28 20:22:08, xiyuan wrote:
> nit: |kiosk_app_manager| check is no longer necessary now.

Done.

https://codereview.chromium.org/1301323005/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/external_provider_impl.cc:510: if (kiosk_app_manager
&&
On 2015/08/28 20:22:08, xiyuan wrote:
> same here.

Done.

https://codereview.chromium.org/1301323005/diff/60001/extensions/common/manif...
File extensions/common/manifest_handlers/kiosk_secondary_apps_info_unittest.cc
(right):

https://codereview.chromium.org/1301323005/diff/60001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info_unittest.cc:21:
std::vector<std::string> expected_ids = {"fiehokkcgaojmbhfhlpiheggjhaedjoc",
On 2015/08/28 20:22:08, xiyuan wrote:
> nit: const

Done.

[not at google - send to devlin, 2015-08-31 18:11:52]

This needs an entry in a manifest_features_ file (I'd look at what kiosk_only
etc do). When do you plan on documenting it?

https://codereview.chromium.org/1301323005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/1301323005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:208: // and should not be shared
across.
This comment isn't a complete sentence.

https://codereview.chromium.org/1301323005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:210:
!extensions::ExtensionsBrowserClient::Get()->IsRunningInForcedAppMode()) {
Without this change, does the test you added fail?

https://codereview.chromium.org/1301323005/diff/80001/extensions/common/manif...
File extensions/common/manifest_handlers/kiosk_secondary_apps_info.cc (right):

https://codereview.chromium.org/1301323005/diff/80001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info.cc:59: if
(!json_schema_compiler::util::PopulateArrayFromList(*list, &id_list,
This shouldn't be needed. It should be possible for just (after grabbing
|list|),

std::vector<std::string> ids;
for (const base::Value* value : list) {
  scoped_ptr<KioskSecondaryAppsType> app =
      KioskSecondaryAppsType::FromValue(value, error);
  if (!app)
    return scoped_ptr<KioskSecondaryAppsInfo>();
  ids.push_back(app->id);
}
return make_scoped_ptr(new KioskSecondaryAppsInfo(ids));

and yeah, it's unfortunate that the schema compiler doesn't properly
generate/use the full KioskSecondaryApps type, it's what I was hoping for, but
this isn't so bad.

https://codereview.chromium.org/1301323005/diff/80001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info.cc:80: }
This isn't actually needed because the code will only be triggered if one of the
keys in |Keys()| matches.

However if you do move this into KioskModeInfo you will need something like it.

https://codereview.chromium.org/1301323005/diff/80001/extensions/common/manif...
File extensions/common/manifest_handlers/kiosk_secondary_apps_info.h (right):

https://codereview.chromium.org/1301323005/diff/80001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info.h:17: struct
KioskSecondaryAppsInfo : public Extension::ManifestData {
Should this all be in the KioskModeInfo manifest handler? Having a single place
for handling kiosk mode configuration would be nice, and KioskModeInfo is
already a bundle of a couple of manifest keys.

https://codereview.chromium.org/1301323005/diff/80001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info.h:23: static bool
HaveSecondaryApps(const Extension* extension);
// Returns true if |extension| declares secondary kiosk apps.
static bool HasSecondaryApps(...);

[jennyz, 2015-08-31 23:57:25]

https://codereview.chromium.org/1301323005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/1301323005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:208: // and should not be shared
across.
On 2015/08/31 18:11:52, kalman wrote:
> This comment isn't a complete sentence.

Done.

https://codereview.chromium.org/1301323005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:210:
!extensions::ExtensionsBrowserClient::Get()->IsRunningInForcedAppMode()) {
On 2015/08/31 18:11:52, kalman wrote:
> Without this change, does the test you added fail?

Yes, the test will fail since the cache write to a directory under /var which is
not accessible under test environment.

https://codereview.chromium.org/1301323005/diff/80001/extensions/common/manif...
File extensions/common/manifest_handlers/kiosk_secondary_apps_info.cc (right):

https://codereview.chromium.org/1301323005/diff/80001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info.cc:59: if
(!json_schema_compiler::util::PopulateArrayFromList(*list, &id_list,
On 2015/08/31 18:11:52, kalman wrote:
> This shouldn't be needed. It should be possible for just (after grabbing
> |list|),
> 
> std::vector<std::string> ids;
> for (const base::Value* value : list) {
>   scoped_ptr<KioskSecondaryAppsType> app =
>       KioskSecondaryAppsType::FromValue(value, error);
>   if (!app)
>     return scoped_ptr<KioskSecondaryAppsInfo>();
>   ids.push_back(app->id);
> }
> return make_scoped_ptr(new KioskSecondaryAppsInfo(ids));
> 
> and yeah, it's unfortunate that the schema compiler doesn't properly
> generate/use the full KioskSecondaryApps type, it's what I was hoping for, but
> this isn't so bad.

Done.

https://codereview.chromium.org/1301323005/diff/80001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info.cc:80: }
On 2015/08/31 18:11:52, kalman wrote:
> This isn't actually needed because the code will only be triggered if one of
the
> keys in |Keys()| matches.
> 
> However if you do move this into KioskModeInfo you will need something like
it.

Acknowledged.

https://codereview.chromium.org/1301323005/diff/80001/extensions/common/manif...
File extensions/common/manifest_handlers/kiosk_secondary_apps_info.h (right):

https://codereview.chromium.org/1301323005/diff/80001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info.h:17: struct
KioskSecondaryAppsInfo : public Extension::ManifestData {
On 2015/08/31 18:11:52, kalman wrote:
> Should this all be in the KioskModeInfo manifest handler? Having a single
place
> for handling kiosk mode configuration would be nice, and KioskModeInfo is
> already a bundle of a couple of manifest keys.

Done.

https://codereview.chromium.org/1301323005/diff/80001/extensions/common/manif...
extensions/common/manifest_handlers/kiosk_secondary_apps_info.h:23: static bool
HaveSecondaryApps(const Extension* extension);
On 2015/08/31 18:11:52, kalman wrote:
> // Returns true if |extension| declares secondary kiosk apps.
> static bool HasSecondaryApps(...);

Done.

[jennyz, 2015-09-01 16:53:09]

https://codereview.chromium.org/1301323005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/1301323005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:210:
!extensions::ExtensionsBrowserClient::Get()->IsRunningInForcedAppMode()) {
On 2015/08/31 18:11:52, kalman wrote:
> Without this change, does the test you added fail?

Actually the tests passed now, since I have moved the hard coded cache path to
chromeos PathProvider. So I removed the special case code for kiosk mode here.

[not at google - send to devlin, 2015-09-01 22:51:11]

Mostly lg, but did you see my comment about the entry in
_manifest_features.json? See how the other kiosk manifest entries are declared:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

- is this starting out as an experimental (dev) feature or do you want to push
it out immediately in the next release? Usually we require staging it for 1
release.

- you also didn't address my question about documentation.

https://codereview.chromium.org/1301323005/diff/120001/extensions/common/mani...
File extensions/common/manifest_handlers/kiosk_mode_info.cc (right):

https://codereview.chromium.org/1301323005/diff/120001/extensions/common/mani...
extensions/common/manifest_handlers/kiosk_mode_info.cc:45:
static_cast<KioskModeInfo*>(extension->GetManifestData(keys::kKioskMode));
More concise, just "KioskModeInfo* info = Get(extension);", and you can do that
for the other occurrences of this pattern.

https://codereview.chromium.org/1301323005/diff/120001/extensions/common/mani...
File extensions/common/manifest_handlers/kiosk_mode_info.h (right):

https://codereview.chromium.org/1301323005/diff/120001/extensions/common/mani...
extensions/common/manifest_handlers/kiosk_mode_info.h:29: KioskStatus
kiosk_status;
Could you move this below the static methods, and above the new
|secondary_app_ids| property?

See
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_...

https://codereview.chromium.org/1301323005/diff/120001/extensions/common/mani...
extensions/common/manifest_handlers/kiosk_mode_info.h:42: static KioskModeInfo*
Get(const Extension* extension);
Nice to put this as the first static method in the list, since it's the most
generic.

https://codereview.chromium.org/1301323005/diff/120001/extensions/common/mani...
File extensions/common/manifest_handlers/kiosk_mode_info_unittest.cc (right):

https://codereview.chromium.org/1301323005/diff/120001/extensions/common/mani...
extensions/common/manifest_handlers/kiosk_mode_info_unittest.cc:1: // Copyright
(c) 2015 The Chromium Authors. All rights reserved.
No (c)

[jennyz, 2015-09-02 21:42:23]

I added the kiosk_secondary_apps manifest key into _manifest_features.json, and
marked it for "stable" channel, since we are going to push it in R47 release.
For the documentation part, what do I need to do for this new key? I add some in
the description section of  extensions_manifest_types.json. What else do I need
to do to get it properly documented? Do I need to write a separate document?

https://codereview.chromium.org/1301323005/diff/120001/extensions/common/mani...
File extensions/common/manifest_handlers/kiosk_mode_info.cc (right):

https://codereview.chromium.org/1301323005/diff/120001/extensions/common/mani...
extensions/common/manifest_handlers/kiosk_mode_info.cc:45:
static_cast<KioskModeInfo*>(extension->GetManifestData(keys::kKioskMode));
On 2015/09/01 22:51:10, kalman wrote:
> More concise, just "KioskModeInfo* info = Get(extension);", and you can do
that
> for the other occurrences of this pattern.

Done.

https://codereview.chromium.org/1301323005/diff/120001/extensions/common/mani...
File extensions/common/manifest_handlers/kiosk_mode_info.h (right):

https://codereview.chromium.org/1301323005/diff/120001/extensions/common/mani...
extensions/common/manifest_handlers/kiosk_mode_info.h:29: KioskStatus
kiosk_status;
On 2015/09/01 22:51:10, kalman wrote:
> Could you move this below the static methods, and above the new
> |secondary_app_ids| property?
> 
> See
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_...

Done.

https://codereview.chromium.org/1301323005/diff/120001/extensions/common/mani...
extensions/common/manifest_handlers/kiosk_mode_info.h:42: static KioskModeInfo*
Get(const Extension* extension);
On 2015/09/01 22:51:11, kalman wrote:
> Nice to put this as the first static method in the list, since it's the most
> generic.

Done.

https://codereview.chromium.org/1301323005/diff/120001/extensions/common/mani...
File extensions/common/manifest_handlers/kiosk_mode_info_unittest.cc (right):

https://codereview.chromium.org/1301323005/diff/120001/extensions/common/mani...
extensions/common/manifest_handlers/kiosk_mode_info_unittest.cc:1: // Copyright
(c) 2015 The Chromium Authors. All rights reserved.
On 2015/09/01 22:51:11, kalman wrote:
> No (c)

Done.

[not at google - send to devlin, 2015-09-02 21:44:39]

On 2015/09/02 21:42:23, jennyz wrote:
> I added the kiosk_secondary_apps manifest key into _manifest_features.json,
and
> marked it for "stable" channel, since we are going to push it in R47 release.
> For the documentation part, what do I need to do for this new key? I add some
in
> the description section of  extensions_manifest_types.json. What else do I
need
> to do to get it properly documented? Do I need to write a separate document?

Like, add it to https://developer.chrome.com/apps/manifest/kiosk_enabled which
is hosted in
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte....
You don't need to update it as part of this CL.

[not at google - send to devlin, 2015-09-02 21:45:36]

lgtm

[jennyz, 2015-09-02 22:19:21]

The CQ bit was checked by jennyz@chromium.org

[jennyz, 2015-09-02 22:19:21]

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/1301323005/#ps140001
(title: "Add new kiosk manifest key into _manifest_features.json,etc.")

[commit-bot: I haz the power, 2015-09-02 22:19:49]

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

[commit-bot: I haz the power, 2015-09-02 22:56:05]

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

[commit-bot: I haz the power, 2015-09-02 22:56:06]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jennyz, 2015-09-02 23:29:50]

The CQ bit was checked by jennyz@chromium.org

[jennyz, 2015-09-02 23:29:51]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1301323005/#ps160001
(title: "Fix the compiler error on mac.")

[commit-bot: I haz the power, 2015-09-02 23:30:05]

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

[commit-bot: I haz the power, 2015-09-03 00:02:45]

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

[commit-bot: I haz the power, 2015-09-03 00:02:46]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jennyz, 2015-09-03 00:10:45]

Patchset #9 (id:160001) has been deleted

[jennyz, 2015-09-03 15:16:22]

The CQ bit was checked by jennyz@chromium.org

[jennyz, 2015-09-03 15:16:22]

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

[commit-bot: I haz the power, 2015-09-03 15:16:45]

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

[commit-bot: I haz the power, 2015-09-03 16:21:40]

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

[commit-bot: I haz the power, 2015-09-03 16:21:41]

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

[jennyz, 2015-09-03 16:50:07]

The CQ bit was checked by jennyz@chromium.org

[commit-bot: I haz the power, 2015-09-03 16:50:29]

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

[commit-bot: I haz the power, 2015-09-03 20:59:34]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2015-09-03 21:00:13]

Patchset 9 (id:??) landed as
https://crrev.com/26b7f4356b905831f152cebb28c08a16ad72d80c
Cr-Commit-Position: refs/heads/master@{#347237}