Files required by a service now listed in manifest.

Files required by a service now listed in manifest.

The files required in a service process can now be listed in the service's
manifest.
In the manifest the file path is given with a string ID and a platform. The file
is then opened in the parent process and passed to the child process, its ID is
specified with a new command-line switch that associates the manifest specified
key and a GlobalDescriptor ID. There are used to populate the new
FileDescriptorStore which is similar to GlobalDescriptor but uses string as keys
(which makes it easier to prevent ID collisions between multiple services).
The service code can then retrieve the FD from the FileDescriptorStore.
The goal is to eventually switch entirely to only using the FileDescriptorStore,
and getting rid of the GlobalDescriptor.

BUG=690189
Review-Url: https://codereview.chromium.org/2684433003
Cr-Commit-Position: refs/heads/master@{#451019}
Committed: https://chromium.googlesource.com/chromium/src/+/dad0cefaebae56eb5ed984b62ccebee165723a6a

[Jay Civelli, 2017-02-08 22:34:26]

Description was changed from

==========
Mapped files now in manifest.

BUG=
==========

to

==========
The files required in a service process can now be listed in the service's
manifest.

BUG=
==========

[Jay Civelli, 2017-02-08 22:43:15]

Description was changed from

==========
The files required in a service process can now be listed in the service's
manifest.

BUG=
==========

to

==========
The files required in a service process can now be listed in the service's
manifest.
In the manifest the file path is given with a string ID and a platform. The file
is then opened in the parent process and passed to the child process, its ID is
specified with a new command-line switch that associates the manifest specified
key and a GlobalDescriptor ID. There are used to populate the new
FileDescriptorStore, which is similar to GlobalDescriptor but uses string as
keys. The service code can then retrieve the FD from the FileDescriptorStore.
The goal is to eventually switch entirely to only using the FileDescriptorStore,
and getting rid of the GlobalDescriptor.

BUG=
==========

[Jay Civelli, 2017-02-08 22:47:52]

Description was changed from

==========
The files required in a service process can now be listed in the service's
manifest.
In the manifest the file path is given with a string ID and a platform. The file
is then opened in the parent process and passed to the child process, its ID is
specified with a new command-line switch that associates the manifest specified
key and a GlobalDescriptor ID. There are used to populate the new
FileDescriptorStore, which is similar to GlobalDescriptor but uses string as
keys. The service code can then retrieve the FD from the FileDescriptorStore.
The goal is to eventually switch entirely to only using the FileDescriptorStore,
and getting rid of the GlobalDescriptor.

BUG=
==========

to

==========
The files required in a service process can now be listed in the service's
manifest.
In the manifest the file path is given with a string ID and a platform. The file
is then opened in the parent process and passed to the child process, its ID is
specified with a new command-line switch that associates the manifest specified
key and a GlobalDescriptor ID. There are used to populate the new
FileDescriptorStore, which is similar to GlobalDescriptor but uses string as
keys (which makes it easier to prevent ID collisions from different services). 
The service code can then retrieve the FD from the FileDescriptorStore.
The goal is to eventually switch entirely to only using the FileDescriptorStore,
and getting rid of the GlobalDescriptor.

BUG=
==========

[Jay Civelli, 2017-02-08 22:51:50]

Description was changed from

==========
The files required in a service process can now be listed in the service's
manifest.
In the manifest the file path is given with a string ID and a platform. The file
is then opened in the parent process and passed to the child process, its ID is
specified with a new command-line switch that associates the manifest specified
key and a GlobalDescriptor ID. There are used to populate the new
FileDescriptorStore, which is similar to GlobalDescriptor but uses string as
keys (which makes it easier to prevent ID collisions from different services). 
The service code can then retrieve the FD from the FileDescriptorStore.
The goal is to eventually switch entirely to only using the FileDescriptorStore,
and getting rid of the GlobalDescriptor.

BUG=
==========

to

==========
The files required in a service process can now be listed in the service's
manifest.
In the manifest the file path is given with a string ID and a platform. The file
is then opened in the parent process and passed to the child process, its ID is
specified with a new command-line switch that associates the manifest specified
key and a GlobalDescriptor ID. There are used to populate the new
FileDescriptorStore which is similar to GlobalDescriptor but uses string as keys
(which makes it easier to prevent ID collisions between multiple services).
The service code can then retrieve the FD from the FileDescriptorStore.
The goal is to eventually switch entirely to only using the FileDescriptorStore,
and getting rid of the GlobalDescriptor.

BUG=690189
==========

[Jay Civelli, 2017-02-08 22:51:50]

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

[Jay Civelli, 2017-02-08 22:52:03]

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

[Jay Civelli, 2017-02-08 22:52:27]

https://codereview.chromium.org/2684433003/diff/40001/services/catalog/entry.cc
File services/catalog/entry.cc (left):

https://codereview.chromium.org/2684433003/diff/40001/services/catalog/entry....
services/catalog/entry.cc:112: std::unique_ptr<base::DictionaryValue>
Entry::Serialize() const {
Removed as unused.

[commit-bot: I haz the power, 2017-02-08 22:52:46]

Dry run: 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, 2017-02-08 22:58:32]

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

[commit-bot: I haz the power, 2017-02-08 22:58:33]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Jay Civelli, 2017-02-08 23:39:07]

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

[commit-bot: I haz the power, 2017-02-08 23:39:54]

Dry run: 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, 2017-02-09 00:07:03]

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

[commit-bot: I haz the power, 2017-02-09 00:07:04]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Ken Rockot(use gerrit already), 2017-02-09 17:17:28]

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
File base/file_descriptor_store.cc (right):

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.cc:25: typedef Singleton<base::FileDescriptorStore,
Not sure Singleton is necessary, though I honestly don't know what even
separates it from manually using a LazyInstance, so maybe I'm missing something.
Note that we are now allowed to use function-scoped static variables with no
destructor, so it's perfectly legal for a leaky lazy instance to be written as:

FileDescriptorStore* FileDescriptorStore::GetInstance() {
  static FileDescriptorStore* store = new FileDescriptorStore;
  return store;
}

Also please consider making this return a FileDescriptorStore& instead since
that clearly signals that the caller need never expect it to be null.

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
File base/file_descriptor_store.h (right):

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:11: 
include <string>

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:31: int fd;
nit: Use ScopedFD? You will have to define a move ctor and operator=, or use a
unique_ptr<Descriptor> in Mapping, but it either way it's much safer. I'm not so
much worried about leaks here as I am worried about double-closing, which seems
to be a popular mistake to make.

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:35: typedef std::vector<Descriptor> Mapping;
Why not a map<std::string, Descriptor> and remove "key" from Descriptor?

Also please use "using" syntax instead of typedef.

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:41: int Get(const std::string& key) const;
Get = TakeFD, and make it return a ScopedFD?

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:44: int MaybeGet(const std::string& key) const;
MaybeTakeFD?

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:51: void Set(const std::string& key, int fd);
ScopedFD

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:54: void Set(const std::string& key,
ScopedFD

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:58: void Reset(const Mapping& mapping);
If making the above changes, this should just become Mapping and can be moved at
the call site. Alternatively, if as a matter of convenience you're initializing
this from a vector anyway (it should really probably stored as a map internally
in this class), maybe it's better to just iterate over the vector and make Set
calls instead of adding a Reset method?

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:66: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2684433003/diff/80001/content/browser/child_p...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/2684433003/diff/80001/content/browser/child_p...
content/browser/child_process_launcher.cc:152:
ChildProcessLauncherHelper::SetRegisteredFilesForService(
nit: Can we just have ServiceManagerContext call this directly?

https://codereview.chromium.org/2684433003/diff/80001/content/browser/child_p...
File content/browser/child_process_launcher_helper.cc (right):

https://codereview.chromium.org/2684433003/diff/80001/content/browser/child_p...
content/browser/child_process_launcher_helper.cc:239: std::map<std::string,
std::string>& service_name_resolver =
I don't see this being used? Is the |service_name| argument meant to be
|process_type| and used to look up the service name in this map?

https://codereview.chromium.org/2684433003/diff/80001/content/browser/child_p...
content/browser/child_process_launcher_helper.cc:242: // Populate the process
name to service name.
nit: s/Populate/Translate/?

Also maybe add a TODO to eliminate this translation, since we'll eventually just
have the service name on the command line.

https://codereview.chromium.org/2684433003/diff/80001/content/browser/child_p...
File content/browser/child_process_launcher_helper.h (right):

https://codereview.chromium.org/2684433003/diff/80001/content/browser/child_p...
content/browser/child_process_launcher_helper.h:168: static base::File
OpenFile(const base::FilePath& path,
nit: Maybe a more detailed name like OpenFileToShare?

https://codereview.chromium.org/2684433003/diff/80001/content/public/app/mojo...
File content/public/app/mojo/content_renderer_manifest.json (right):

https://codereview.chromium.org/2684433003/diff/80001/content/public/app/mojo...
content/public/app/mojo/content_renderer_manifest.json:47: "required_files" : {
I know we use v8 in the utility process sometimes too (for out of process proxy
resolver, at least). Do you need to add these to content_utility_manifest.json
as well? The duplication should be OK for now.

https://codereview.chromium.org/2684433003/diff/80001/content/public/app/mojo...
content/public/app/mojo/content_renderer_manifest.json:48:
"v8_natives_data_descriptor" : [
nit: maybe as a matter of convention, "descriptor" should be considered
redundant in these keys

https://codereview.chromium.org/2684433003/diff/80001/services/catalog/BUILD.gn
File services/catalog/BUILD.gn (right):

https://codereview.chromium.org/2684433003/diff/80001/services/catalog/BUILD....
services/catalog/BUILD.gn:16: source_set("constants_lib") {
nit: How about just "constants"?  The "lib" pattern is generally just used to
separate service impl code from the actual standalone service output target so
we can link services into other targets.

[Jay Civelli, 2017-02-09 22:07:10]

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

[commit-bot: I haz the power, 2017-02-09 22:08:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jay Civelli, 2017-02-09 22:08:27]

I moved the actual mapping to the Posix specific ChildProcessLauncherHelper for
now as more work is required for Windows (and no file is shared that way on
Windows).

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
File base/file_descriptor_store.cc (right):

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.cc:25: typedef Singleton<base::FileDescriptorStore,
On 2017/02/09 17:17:27, Ken Rockot wrote:
> Not sure Singleton is necessary, though I honestly don't know what even
> separates it from manually using a LazyInstance, so maybe I'm missing
something.
> Note that we are now allowed to use function-scoped static variables with no
> destructor, so it's perfectly legal for a leaky lazy instance to be written
as:
> 
> FileDescriptorStore* FileDescriptorStore::GetInstance() {
>   static FileDescriptorStore* store = new FileDescriptorStore;
>   return store;
> }
> 
> Also please consider making this return a FileDescriptorStore& instead since
> that clearly signals that the caller need never expect it to be null.

Done.

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
File base/file_descriptor_store.h (right):

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:11: 
On 2017/02/09 17:17:27, Ken Rockot wrote:
> include <string>

Done.

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:31: int fd;
On 2017/02/09 17:17:27, Ken Rockot wrote:
> nit: Use ScopedFD? You will have to define a move ctor and operator=, or use a
> unique_ptr<Descriptor> in Mapping, but it either way it's much safer. I'm not
so
> much worried about leaks here as I am worried about double-closing, which
seems
> to be a popular mistake to make.

Great idea, now using a ScopedFD.

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:35: typedef std::vector<Descriptor> Mapping;
On 2017/02/09 17:17:27, Ken Rockot wrote:
> Why not a map<std::string, Descriptor> and remove "key" from Descriptor?
> 
> Also please use "using" syntax instead of typedef.

Done.
I copied this file over from GlobalDescriptor without much thoughts.
It's much cleaner now, thanks!

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:41: int Get(const std::string& key) const;
On 2017/02/09 17:17:27, Ken Rockot wrote:
> Get = TakeFD, and make it return a ScopedFD?

Done.

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:44: int MaybeGet(const std::string& key) const;
On 2017/02/09 17:17:27, Ken Rockot wrote:
> MaybeTakeFD?

Done.

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:51: void Set(const std::string& key, int fd);
On 2017/02/09 17:17:28, Ken Rockot wrote:
> ScopedFD

Done.

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:54: void Set(const std::string& key,
On 2017/02/09 17:17:28, Ken Rockot wrote:
> ScopedFD

Done.

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:58: void Reset(const Mapping& mapping);
On 2017/02/09 17:17:27, Ken Rockot wrote:
> If making the above changes, this should just become Mapping and can be moved
at
> the call site. Alternatively, if as a matter of convenience you're
initializing
> this from a vector anyway (it should really probably stored as a map
internally
> in this class), maybe it's better to just iterate over the vector and make Set
> calls instead of adding a Reset method?

This is actually not used, a left over from me taking inspiration from
GlobalDescriptors.
Removed.

https://codereview.chromium.org/2684433003/diff/80001/base/file_descriptor_st...
base/file_descriptor_store.h:66: };
On 2017/02/09 17:17:27, Ken Rockot wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2684433003/diff/80001/content/browser/child_p...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/2684433003/diff/80001/content/browser/child_p...
content/browser/child_process_launcher.cc:152:
ChildProcessLauncherHelper::SetRegisteredFilesForService(
On 2017/02/09 17:17:28, Ken Rockot wrote:
> nit: Can we just have ServiceManagerContext call this directly?

ChildProcessLauncherHelper is declared in an internal namespace
(content::internal).

https://codereview.chromium.org/2684433003/diff/80001/content/browser/child_p...
File content/browser/child_process_launcher_helper.cc (right):

https://codereview.chromium.org/2684433003/diff/80001/content/browser/child_p...
content/browser/child_process_launcher_helper.cc:239: std::map<std::string,
std::string>& service_name_resolver =
On 2017/02/09 17:17:28, Ken Rockot wrote:
> I don't see this being used? Is the |service_name| argument meant to be
> |process_type| and used to look up the service name in this map?

It's a bit confusing, I am setting a local var that points to the global
g_process_name_to_service_name. (it is to make the code more concise).
We then use that in LaunchOnLauncherThread() above to know which service the
launched process is so we can map the correct files.

https://codereview.chromium.org/2684433003/diff/80001/content/browser/child_p...
content/browser/child_process_launcher_helper.cc:242: // Populate the process
name to service name.
On 2017/02/09 17:17:28, Ken Rockot wrote:
> nit: s/Populate/Translate/?
Changed to populate map.

> Also maybe add a TODO to eliminate this translation, since we'll eventually
just
> have the service name on the command line.
Done

https://codereview.chromium.org/2684433003/diff/80001/content/browser/child_p...
File content/browser/child_process_launcher_helper.h (right):

https://codereview.chromium.org/2684433003/diff/80001/content/browser/child_p...
content/browser/child_process_launcher_helper.h:168: static base::File
OpenFile(const base::FilePath& path,
On 2017/02/09 17:17:28, Ken Rockot wrote:
> nit: Maybe a more detailed name like OpenFileToShare?

Done.

https://codereview.chromium.org/2684433003/diff/80001/content/public/app/mojo...
File content/public/app/mojo/content_renderer_manifest.json (right):

https://codereview.chromium.org/2684433003/diff/80001/content/public/app/mojo...
content/public/app/mojo/content_renderer_manifest.json:47: "required_files" : {
On 2017/02/09 17:17:28, Ken Rockot wrote:
> I know we use v8 in the utility process sometimes too (for out of process
proxy
> resolver, at least). Do you need to add these to content_utility_manifest.json
> as well? The duplication should be OK for now.

Ah, right! Done.

https://codereview.chromium.org/2684433003/diff/80001/content/public/app/mojo...
content/public/app/mojo/content_renderer_manifest.json:48:
"v8_natives_data_descriptor" : [
On 2017/02/09 17:17:28, Ken Rockot wrote:
> nit: maybe as a matter of convention, "descriptor" should be considered
> redundant in these keys

Good point, removed.

https://codereview.chromium.org/2684433003/diff/80001/services/catalog/BUILD.gn
File services/catalog/BUILD.gn (right):

https://codereview.chromium.org/2684433003/diff/80001/services/catalog/BUILD....
services/catalog/BUILD.gn:16: source_set("constants_lib") {
On 2017/02/09 17:17:28, Ken Rockot wrote:
> nit: How about just "constants"?  The "lib" pattern is generally just used to
> separate service impl code from the actual standalone service output target so
> we can link services into other targets.

Done.

[commit-bot: I haz the power, 2017-02-09 22:10:35]

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

[commit-bot: I haz the power, 2017-02-09 22:10:38]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ken Rockot(use gerrit already), 2017-02-09 22:46:13]

lgtm

https://codereview.chromium.org/2684433003/diff/140001/content/browser/child_...
File content/browser/child_process_launcher_helper_posix.cc (right):

https://codereview.chromium.org/2684433003/diff/140001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:123:
std::map<std::string, std::string>& service_name_resolver =
nit: move this lazy initialization into the function which actually uses
g_process_name_to_service_name?

[Jay Civelli, 2017-02-09 23:16:35]

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

[commit-bot: I haz the power, 2017-02-09 23:17:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jay Civelli, 2017-02-09 23:25:06]

jcivelli@chromium.org changed reviewers:
+ boliu@chromium.org, dcheng@chromium.org, jam@chromium.org,
nyquist@chromium.org

[Jay Civelli, 2017-02-09 23:27:00]

Adding OWNERS reviewers:
@boliu, could you look at android_webview/?
@jam,  could you look at content/?
@dcheng, could you look at base/? 
@nyquist, could you look at base/android/?

https://codereview.chromium.org/2684433003/diff/140001/content/browser/child_...
File content/browser/child_process_launcher_helper_posix.cc (right):

https://codereview.chromium.org/2684433003/diff/140001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:123:
std::map<std::string, std::string>& service_name_resolver =
On 2017/02/09 22:46:13, Ken Rockot wrote:
> nit: move this lazy initialization into the function which actually uses
> g_process_name_to_service_name?

Done.

[commit-bot: I haz the power, 2017-02-09 23:38:35]

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

[commit-bot: I haz the power, 2017-02-09 23:38:36]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[boliu, 2017-02-09 23:59:55]

looked at android part of content, since I supposedly own that as well

Properly format CL description pls:
https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...

https://codereview.chromium.org/2684433003/diff/160001/content/app/android/ch...
File content/app/android/child_process_service_impl.cc (right):

https://codereview.chromium.org/2684433003/diff/160001/content/app/android/ch...
content/app/android/child_process_service_impl.cc:131: if (shared_param.obj() ==
nullptr)
nit: shared_param.is_null()

https://codereview.chromium.org/2684433003/diff/160001/content/app/android/ch...
content/app/android/child_process_service_impl.cc:135:
std::unique_ptr<std::map<int, std::string>> values =
nit: auto

https://codereview.chromium.org/2684433003/diff/160001/content/app/android/ch...
content/app/android/child_process_service_impl.cc:160:
reinterpret_cast<std::map<int, std::string>*>(store_context);
really should alias this

https://codereview.chromium.org/2684433003/diff/160001/content/browser/child_...
File content/browser/child_process_launcher_helper_android.cc (left):

https://codereview.chromium.org/2684433003/diff/160001/content/browser/child_...
content/browser/child_process_launcher_helper_android.cc:95: #if
defined(V8_USE_EXTERNAL_STARTUP_DATA)
it's not clear to me where this chunk of code went? it's not needed anymore?

https://codereview.chromium.org/2684433003/diff/160001/content/browser/child_...
File content/browser/child_process_launcher_helper_android.cc (right):

https://codereview.chromium.org/2684433003/diff/160001/content/browser/child_...
content/browser/child_process_launcher_helper_android.cc:175: base::File
file(base::android::OpenApkAsset(path.value(), region));
nit: one line return so RVO works?

https://codereview.chromium.org/2684433003/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2684433003/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:276:
nativeRegisterGlobalFileDescriptor(
that's a lot of jni calls

just put these things into parallel vectors, and send them all down to native in
a single call?

Actually, why not just do all of this in native code, since native code has
access to command line as well?

[Jay Civelli, 2017-02-10 00:03:19]

Description was changed from

==========
The files required in a service process can now be listed in the service's
manifest.
In the manifest the file path is given with a string ID and a platform. The file
is then opened in the parent process and passed to the child process, its ID is
specified with a new command-line switch that associates the manifest specified
key and a GlobalDescriptor ID. There are used to populate the new
FileDescriptorStore which is similar to GlobalDescriptor but uses string as keys
(which makes it easier to prevent ID collisions between multiple services).
The service code can then retrieve the FD from the FileDescriptorStore.
The goal is to eventually switch entirely to only using the FileDescriptorStore,
and getting rid of the GlobalDescriptor.

BUG=690189
==========

to

==========
Files required by a service now listed in manifest.

The files required in a service process can now be listed in the service's
manifest.
In the manifest the file path is given with a string ID and a platform. The file
is then opened in the parent process and passed to the child process, its ID is
specified with a new command-line switch that associates the manifest specified
key and a GlobalDescriptor ID. There are used to populate the new
FileDescriptorStore which is similar to GlobalDescriptor but uses string as keys
(which makes it easier to prevent ID collisions between multiple services).
The service code can then retrieve the FD from the FileDescriptorStore.
The goal is to eventually switch entirely to only using the FileDescriptorStore,
and getting rid of the GlobalDescriptor.

BUG=690189
==========

[jam, 2017-02-10 00:44:52]

I defer to boliu for android files inside content

what about windows?

https://codereview.chromium.org/2684433003/diff/160001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2684433003/diff/160001/content/app/content_ma...
content/app/content_main_runner.cc:615: PopulateFDsFromCommandLine();
nit: it's the same whether the ifdef goes here, and the function above goes into
an existing posix block. that would better match the rest of this function's
style

https://codereview.chromium.org/2684433003/diff/160001/content/browser/child_...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/2684433003/diff/160001/content/browser/child_...
content/browser/child_process_launcher.h:129: static void
SetRegisteredFilesForService(
nit: document

https://codereview.chromium.org/2684433003/diff/160001/content/public/common/...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/2684433003/diff/160001/content/public/common/...
content/public/common/content_switches.cc:799: // <file id string>:<global
descriptor id>,<file id string>:<global descriptor
nit: for someone reading this, "file id string" is ambiguous?

[Jay Civelli, 2017-02-10 06:32:16]

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

[Jay Civelli, 2017-02-10 06:32:34]

https://codereview.chromium.org/2684433003/diff/160001/content/app/android/ch...
File content/app/android/child_process_service_impl.cc (right):

https://codereview.chromium.org/2684433003/diff/160001/content/app/android/ch...
content/app/android/child_process_service_impl.cc:131: if (shared_param.obj() ==
nullptr)
On 2017/02/09 23:59:54, boliu wrote:
> nit: shared_param.is_null()

Done.

https://codereview.chromium.org/2684433003/diff/160001/content/app/android/ch...
content/app/android/child_process_service_impl.cc:135:
std::unique_ptr<std::map<int, std::string>> values =
On 2017/02/09 23:59:54, boliu wrote:
> nit: auto

Done.

https://codereview.chromium.org/2684433003/diff/160001/content/app/android/ch...
content/app/android/child_process_service_impl.cc:160:
reinterpret_cast<std::map<int, std::string>*>(store_context);
On 2017/02/09 23:59:54, boliu wrote:
> really should alias this

The method has changed and the map comes up just once now.

https://codereview.chromium.org/2684433003/diff/160001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2684433003/diff/160001/content/app/content_ma...
content/app/content_main_runner.cc:615: PopulateFDsFromCommandLine();
On 2017/02/10 00:44:52, jam wrote:
> nit: it's the same whether the ifdef goes here, and the function above goes
into
> an existing posix block. that would better match the rest of this function's
> style

Done.

https://codereview.chromium.org/2684433003/diff/160001/content/browser/child_...
File content/browser/child_process_launcher_helper_android.cc (left):

https://codereview.chromium.org/2684433003/diff/160001/content/browser/child_...
content/browser/child_process_launcher_helper_android.cc:95: #if
defined(V8_USE_EXTERNAL_STARTUP_DATA)
On 2017/02/09 23:59:54, boliu wrote:
> it's not clear to me where this chunk of code went? it's not needed anymore?

Yes, it's not needed.
Now these files are specified in the service's manifest.
The goal is to have any non "primordial" file (not common to all/most services)
specified in the manifest.

https://codereview.chromium.org/2684433003/diff/160001/content/browser/child_...
File content/browser/child_process_launcher_helper_android.cc (right):

https://codereview.chromium.org/2684433003/diff/160001/content/browser/child_...
content/browser/child_process_launcher_helper_android.cc:175: base::File
file(base::android::OpenApkAsset(path.value(), region));
On 2017/02/09 23:59:54, boliu wrote:
> nit: one line return so RVO works?

Done.

https://codereview.chromium.org/2684433003/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2684433003/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:276:
nativeRegisterGlobalFileDescriptor(
On 2017/02/09 23:59:55, boliu wrote:
> that's a lot of jni calls
> 
> just put these things into parallel vectors, and send them all down to native
in
> a single call?
> 
> Actually, why not just do all of this in native code, since native code has
> access to command line as well?

Merged it all in one JNI call.

https://codereview.chromium.org/2684433003/diff/160001/content/public/common/...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/2684433003/diff/160001/content/public/common/...
content/public/common/content_switches.cc:799: // <file id string>:<global
descriptor id>,<file id string>:<global descriptor
On 2017/02/10 00:44:52, jam wrote:
> nit: for someone reading this, "file id string" is ambiguous?

Changed to explain it's the ID from the manifest.

[commit-bot: I haz the power, 2017-02-10 06:32:42]

Dry run: 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, 2017-02-10 06:46:23]

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

[commit-bot: I haz the power, 2017-02-10 06:46:24]

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

[dcheng, 2017-02-10 08:44:15]

https://codereview.chromium.org/2684433003/diff/160001/base/android/apk_assets.h
File base/android/apk_assets.h (right):

https://codereview.chromium.org/2684433003/diff/160001/base/android/apk_asset...
base/android/apk_assets.h:39: const std::string& file_path);
This should be a FilePath (unless it's not really a path, in which case this
shouldn't be called |file_path|).

A similar comment applies to all the other arguments named file_path in this
header (which don't need to be fixed this patch, but should be addressed in a
followup)

https://codereview.chromium.org/2684433003/diff/160001/base/file_descriptor_s...
File base/file_descriptor_store.cc (right):

https://codereview.chromium.org/2684433003/diff/160001/base/file_descriptor_s...
base/file_descriptor_store.cc:32: FileDescriptorStore::Descriptor&& other) {
I think this and the move ctor can be explicitly defaulted.

https://codereview.chromium.org/2684433003/diff/160001/base/file_descriptor_s...
base/file_descriptor_store.cc:62: *region = iter->second.region;
Isn't referencing |iter| here undefined behavior? From [associative.reqmts]/9:
The insert and emplace members shall not affect the validity of iterators and
references to the container,
and the erase members shall invalidate only iterators and references to the
erased elements.

https://codereview.chromium.org/2684433003/diff/160001/base/file_descriptor_s...
File base/file_descriptor_store.h (right):

https://codereview.chromium.org/2684433003/diff/160001/base/file_descriptor_s...
base/file_descriptor_store.h:19: // It is used to share file descriptors from a
process to its child.
Can we just change GlobalDescriptors to have the desired interface in a separate
patch, rather than adding a new thing?

[boliu, 2017-02-10 21:33:03]

android_webview and android under content lg2m % comments, but I'll wait until
base reviews gets sorted

https://codereview.chromium.org/2684433003/diff/180001/content/app/android/ch...
File content/app/android/child_process_service_impl.cc (right):

https://codereview.chromium.org/2684433003/diff/180001/content/app/android/ch...
content/app/android/child_process_service_impl.cc:128: jlong
CreateFileDescriptorStoreRegistrationContext(
delete this

https://codereview.chromium.org/2684433003/diff/180001/content/app/android/ch...
content/app/android/child_process_service_impl.cc:142: void
ReleaseFileDescriptorStoreRegistrationContext(
and this

https://codereview.chromium.org/2684433003/diff/180001/content/app/android/ch...
content/app/android/child_process_service_impl.cc:164: DCHECK(ids.size() ==
fds.size() && fds.size() == offsets.size() &&
nit: separate DCHECKs, and use DCHECK_EQ

https://codereview.chromium.org/2684433003/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2684433003/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:264:
String sharedFilesValue = CommandLine.getInstance().getSwitchValue(
delete this, seems like don't need to copy the switch in java code after as well

[boliu, 2017-02-10 21:34:25]

also, all android tests crashed.. so maybe this doesn't even work yet, but
that's cq's job :)

[nyquist, 2017-02-10 23:37:50]

https://codereview.chromium.org/2684433003/diff/180001/base/android/apk_asset...
File base/android/apk_assets.cc (right):

https://codereview.chromium.org/2684433003/diff/180001/base/android/apk_asset...
base/android/apk_assets.cc:42: return false;
Should the function for global descriptors follow the same pattern? I find this
new function easier to read.

[Jay Civelli, 2017-02-12 04:46:43]

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

[commit-bot: I haz the power, 2017-02-12 04:46:49]

Dry run: 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, 2017-02-12 05:12:11]

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

[commit-bot: I haz the power, 2017-02-12 05:12:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jay Civelli, 2017-02-12 18:55:13]

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

[commit-bot: I haz the power, 2017-02-12 18:55:29]

Dry run: 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, 2017-02-12 19:14:13]

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

[commit-bot: I haz the power, 2017-02-12 19:14:14]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jay Civelli, 2017-02-13 18:48:11]

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

[Jay Civelli, 2017-02-13 18:48:26]

https://codereview.chromium.org/2684433003/diff/160001/base/android/apk_assets.h
File base/android/apk_assets.h (right):

https://codereview.chromium.org/2684433003/diff/160001/base/android/apk_asset...
base/android/apk_assets.h:39: const std::string& file_path);
On 2017/02/10 08:44:15, dcheng wrote:
> This should be a FilePath (unless it's not really a path, in which case this
> shouldn't be called |file_path|).
> 
> A similar comment applies to all the other arguments named file_path in this
> header (which don't need to be fixed this patch, but should be addressed in a
> followup)

Done.

https://codereview.chromium.org/2684433003/diff/160001/base/file_descriptor_s...
File base/file_descriptor_store.cc (right):

https://codereview.chromium.org/2684433003/diff/160001/base/file_descriptor_s...
base/file_descriptor_store.cc:32: FileDescriptorStore::Descriptor&& other) {
On 2017/02/10 08:44:15, dcheng wrote:
> I think this and the move ctor can be explicitly defaulted.

If I default the  move constructor, I get a "Complex class/struct needs an
explicit out-of-line copy constructor."
The = operator works though.

https://codereview.chromium.org/2684433003/diff/160001/base/file_descriptor_s...
base/file_descriptor_store.cc:62: *region = iter->second.region;
On 2017/02/10 08:44:15, dcheng wrote:
> Isn't referencing |iter| here undefined behavior? From [associative.reqmts]/9:
> The insert and emplace members shall not affect the validity of iterators and
> references to the container,
> and the erase members shall invalidate only iterators and references to the
> erased elements.

You are right! Fixed.

https://codereview.chromium.org/2684433003/diff/160001/base/file_descriptor_s...
File base/file_descriptor_store.h (right):

https://codereview.chromium.org/2684433003/diff/160001/base/file_descriptor_s...
base/file_descriptor_store.h:19: // It is used to share file descriptors from a
process to its child.
On 2017/02/10 08:44:15, dcheng wrote:
> Can we just change GlobalDescriptors to have the desired interface in a
separate
> patch, rather than adding a new thing?

It's going to be quite a bit of work to change to process start-up to use the
new interface in all cases.
This patch is the first one toward that goal.
When we get there, the GlobalDescriptors class will go away altogether. It's
probably easier during the transition to keep both classes separate.

https://codereview.chromium.org/2684433003/diff/160001/content/browser/child_...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/2684433003/diff/160001/content/browser/child_...
content/browser/child_process_launcher.h:129: static void
SetRegisteredFilesForService(
On 2017/02/10 00:44:52, jam wrote:
> nit: document

Done.

https://codereview.chromium.org/2684433003/diff/180001/base/android/apk_asset...
File base/android/apk_assets.cc (right):

https://codereview.chromium.org/2684433003/diff/180001/base/android/apk_asset...
base/android/apk_assets.cc:42: return false;
On 2017/02/10 23:37:50, nyquist wrote:
> Should the function for global descriptors follow the same pattern? I find
this
> new function easier to read.

Anything to make you happy, Tommy :-)

https://codereview.chromium.org/2684433003/diff/180001/content/app/android/ch...
File content/app/android/child_process_service_impl.cc (right):

https://codereview.chromium.org/2684433003/diff/180001/content/app/android/ch...
content/app/android/child_process_service_impl.cc:128: jlong
CreateFileDescriptorStoreRegistrationContext(
On 2017/02/10 21:33:03, boliu wrote:
> delete this

Oh, yeah.

https://codereview.chromium.org/2684433003/diff/180001/content/app/android/ch...
content/app/android/child_process_service_impl.cc:142: void
ReleaseFileDescriptorStoreRegistrationContext(
On 2017/02/10 21:33:03, boliu wrote:
> and this

Oh yeah (continued).

https://codereview.chromium.org/2684433003/diff/180001/content/app/android/ch...
content/app/android/child_process_service_impl.cc:164: DCHECK(ids.size() ==
fds.size() && fds.size() == offsets.size() &&
On 2017/02/10 21:33:03, boliu wrote:
> nit: separate DCHECKs, and use DCHECK_EQ

Sure.

https://codereview.chromium.org/2684433003/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2684433003/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:264:
String sharedFilesValue = CommandLine.getInstance().getSwitchValue(
On 2017/02/10 21:33:03, boliu wrote:
> delete this, seems like don't need to copy the switch in java code after as
well

Done.

[commit-bot: I haz the power, 2017-02-13 18:48:44]

Dry run: 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, 2017-02-13 18:52:51]

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

[commit-bot: I haz the power, 2017-02-13 18:52:52]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Jay Civelli, 2017-02-13 19:32:22]

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

[commit-bot: I haz the power, 2017-02-13 19:33:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[boliu, 2017-02-13 19:40:24]

https://codereview.chromium.org/2684433003/diff/280001/content/public/android...
File
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java
(right):

https://codereview.chromium.org/2684433003/diff/280001/content/public/android...
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:82:
public static final String SHARED_FILES = "shared-files";
this is no longer needed

[commit-bot: I haz the power, 2017-02-13 21:42:17]

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

[commit-bot: I haz the power, 2017-02-13 21:42:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2017-02-13 23:10:59]

lgtm

https://codereview.chromium.org/2684433003/diff/280001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2684433003/diff/280001/content/app/content_ma...
content/app/content_main_runner.cc:292: void PopulateFDsFromCommandLine() {
nit: move to existing posix block on line 262

[Jay Civelli, 2017-02-13 23:51:49]

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

[Jay Civelli, 2017-02-13 23:51:58]

https://codereview.chromium.org/2684433003/diff/280001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2684433003/diff/280001/content/app/content_ma...
content/app/content_main_runner.cc:292: void PopulateFDsFromCommandLine() {
On 2017/02/13 23:10:59, jam wrote:
> nit: move to existing posix block on line 262

Done.

https://codereview.chromium.org/2684433003/diff/280001/content/public/android...
File
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java
(right):

https://codereview.chromium.org/2684433003/diff/280001/content/public/android...
content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:82:
public static final String SHARED_FILES = "shared-files";
On 2017/02/13 19:40:24, boliu wrote:
> this is no longer needed

Removed.

[commit-bot: I haz the power, 2017-02-13 23:52:27]

Dry run: 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, 2017-02-14 02:13:38]

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

[commit-bot: I haz the power, 2017-02-14 02:13:39]

Dry run: This issue passed the CQ dry run.

[nyquist, 2017-02-14 05:46:07]

https://codereview.chromium.org/2684433003/diff/300001/base/android/apk_assets.h
File base/android/apk_assets.h (right):

https://codereview.chromium.org/2684433003/diff/300001/base/android/apk_asset...
base/android/apk_assets.h:34: const std::string& file_path);
Well now it feels like this should be changed to base::FilePath& too. Especially
since it seems like the only caller already retrieves a base::FilePath (
https://cs.chromium.org/chromium/src/android_webview/lib/main/aw_main_delegat...
)

I think the code it's calling is from here: //gin/v8_initializer.h

And you already have an //android_webview reviewer.

WDYT?

[Jay Civelli, 2017-02-14 17:46:18]

https://codereview.chromium.org/2684433003/diff/300001/base/android/apk_assets.h
File base/android/apk_assets.h (right):

https://codereview.chromium.org/2684433003/diff/300001/base/android/apk_asset...
base/android/apk_assets.h:34: const std::string& file_path);
On 2017/02/14 05:46:06, nyquist wrote:
> Well now it feels like this should be changed to base::FilePath& too.
Especially
> since it seems like the only caller already retrieves a base::FilePath (
>
https://cs.chromium.org/chromium/src/android_webview/lib/main/aw_main_delegat...
> )
> 
> I think the code it's calling is from here: //gin/v8_initializer.h
> 
> And you already have an //android_webview reviewer.
> 
> WDYT?

What if I told you that this method can actually be removed? :-)
(the only caller is the one I changed. I removed it)

[dcheng, 2017-02-15 08:05:28]

Looks good over all, my comments are mostly nits (in particular, I think we can
simplify some of this by not using std::unique_ptr<std::map<...>>)

https://codereview.chromium.org/2684433003/diff/160001/base/file_descriptor_s...
File base/file_descriptor_store.cc (right):

https://codereview.chromium.org/2684433003/diff/160001/base/file_descriptor_s...
base/file_descriptor_store.cc:32: FileDescriptorStore::Descriptor&& other) {
On 2017/02/13 18:48:25, Jay Civelli wrote:
> On 2017/02/10 08:44:15, dcheng wrote:
> > I think this and the move ctor can be explicitly defaulted.
> 
> If I default the  move constructor, I get a "Complex class/struct needs an
> explicit out-of-line copy constructor."
> The = operator works though.

Sorry, to clarify, they should only be declared in the header, not defined.

Then the .cc file can write
FileDescriptorStore::Descriptor::Descriptor(Descriptor&&) = default;

https://codereview.chromium.org/2684433003/diff/320001/base/file_descriptor_s...
File base/file_descriptor_store.h (right):

https://codereview.chromium.org/2684433003/diff/320001/base/file_descriptor_s...
base/file_descriptor_store.h:44: // Gets a descriptor given a key and also
populates |region| if non null.
Nit: remove optional-ness of |region|

https://codereview.chromium.org/2684433003/diff/320001/base/posix/global_desc...
File base/posix/global_descriptors.h (right):

https://codereview.chromium.org/2684433003/diff/320001/base/posix/global_desc...
base/posix/global_descriptors.h:38: class BASE_EXPORT GlobalDescriptors {
Note that this is deprecated and new code should use FileDescriptorStore, with a
https://crbug.com/xyz link for tracking this cleanup.

https://codereview.chromium.org/2684433003/diff/320001/base/posix/global_desc...
base/posix/global_descriptors.h:68: // Also populates |region| if it's not null.
I might be missing something, but it seems like nothing actually passes null for
|region|. I'd rather just require it be present (ditto for the new class)

https://codereview.chromium.org/2684433003/diff/320001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2684433003/diff/320001/content/app/content_ma...
content/app/content_main_runner.cc:274: std::string key = descriptor.second;
const std::string& and save a copy

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
File content/browser/child_process_launcher_helper_posix.cc (right):

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:31: std::map<std::string,
std::unique_ptr<catalog::RequiredFileMap>>>
Nit: no unique_ptr indirection, using RequiredFileMap directly with std::move()
should work

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:34:
base::LazyInstance<OpenedFileMap> g_opened_files = LAZY_INSTANCE_INITIALIZER;
Nit: use function-local statics instead of LazyInstance?

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:81: static
std::map<std::string, std::string>* service_name_resolver = nullptr;
Another alternative:

static auto* service_name_resolver =
    new std::map<std::string, std::string>()({
  {switches::kGpuProcess, "content_gpu"},
  {switches::kPpapiPluginProcess, "content_plugin"},
  ...
});

As a bonus, it's automatically thread-safe.

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:99:
CHECK(service_name_iter != service_name_resolver->end())
DCHECK, unless the input can be controlled by arbitrary content (which I would
imagine shouldn't be the case)

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:101: std::string
service_name = service_name_iter->second;
Nit: const std::string& to save a copy

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:102: const auto&
files_iter = g_required_files_by_service.Get().find(service_name);
But this can just be auto, since iterators are lightweight value types
(relatively)--and in this case, lifetime extension applies anyway.

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:110:
OpenFileIfNecessary(base::FilePath(key_path_iter.second), &region);
Can the map just hold a FilePath?

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:112: LOG(ERROR) <<
"Ignoring invalid file " << key_path_iter.second;
DLOG instead of LOG? This comment applies to many of the other newly added
logging messages.

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:133:
std::unique_ptr<catalog::RequiredFileMap> required_files) {
Consider just passing by value and moving below, rather than indirecting through
unique_ptr

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:137: if
(!base::StartsWith(service_name, "content",
Maybe content_ to be safe?

https://codereview.chromium.org/2684433003/diff/320001/services/catalog/entry.cc
File services/catalog/entry.cc (right):

https://codereview.chromium.org/2684433003/diff/320001/services/catalog/entry...
services/catalog/entry.cc:182: entry->AddRequiredFileDescritor(iter.first,
iter.second);
The naming also feels a little unusual: there's no actual file descriptor at
this layer, but the function is called AddRequiredFD instead of
AddRequiredFilePath or the like.

https://codereview.chromium.org/2684433003/diff/320001/services/catalog/entry.h
File services/catalog/entry.h (right):

https://codereview.chromium.org/2684433003/diff/320001/services/catalog/entry...
services/catalog/entry.h:67: void AddRequiredFileDescritor(const std::string&
name,
Nit: AddRequiredFileDescriptor

https://codereview.chromium.org/2684433003/diff/320001/services/catalog/publi...
File services/catalog/public/cpp/manifest_parsing_util.cc (right):

https://codereview.chromium.org/2684433003/diff/320001/services/catalog/publi...
services/catalog/public/cpp/manifest_parsing_util.cc:37: bool
PopulateRequiredFiles(const base::Value& manifest_value,
Would it make sense to return a base::Optional<RequiredFileMap>? I notice some
call sites (such as entry.cc) don't actually check the return value.

https://codereview.chromium.org/2684433003/diff/320001/services/catalog/publi...
services/catalog/public/cpp/manifest_parsing_util.cc:65: for (size_t i = 0; i <
all_platform_values->GetSize(); i++) {
It's possible to use range-based for loops with base::ListValue, FWIW

https://codereview.chromium.org/2684433003/diff/320001/services/service_manag...
File services/service_manager/public/cpp/lib/shared_file_util.cc (right):

https://codereview.chromium.org/2684433003/diff/320001/services/service_manag...
services/service_manager/public/cpp/lib/shared_file_util.cc:22: std::map<int,
std::string>* values) {
Ditto: does it make sense to return a base::Optional<std::map<int, std::string>>
here?

(I don't have a strong preference for using it, I'm really just exploring
options)

https://codereview.chromium.org/2684433003/diff/320001/services/service_manag...
File services/service_manager/public/cpp/shared_file_util.h (right):

https://codereview.chromium.org/2684433003/diff/320001/services/service_manag...
services/service_manager/public/cpp/shared_file_util.h:17: void AddEntry(const
std::string& key_str, int key_id);
This code isn't actually being used yet, right? Is a followup going to use it?

[Jay Civelli, 2017-02-15 19:53:48]

https://codereview.chromium.org/2684433003/diff/320001/base/file_descriptor_s...
File base/file_descriptor_store.h (right):

https://codereview.chromium.org/2684433003/diff/320001/base/file_descriptor_s...
base/file_descriptor_store.h:44: // Gets a descriptor given a key and also
populates |region| if non null.
On 2017/02/15 08:05:27, dcheng wrote:
> Nit: remove optional-ness of |region|

Done.

https://codereview.chromium.org/2684433003/diff/320001/base/posix/global_desc...
File base/posix/global_descriptors.h (right):

https://codereview.chromium.org/2684433003/diff/320001/base/posix/global_desc...
base/posix/global_descriptors.h:38: class BASE_EXPORT GlobalDescriptors {
On 2017/02/15 08:05:27, dcheng wrote:
> Note that this is deprecated and new code should use FileDescriptorStore, with
a
> https://crbug.com/xyz link for tracking this cleanup.

Done.

https://codereview.chromium.org/2684433003/diff/320001/base/posix/global_desc...
base/posix/global_descriptors.h:68: // Also populates |region| if it's not null.
On 2017/02/15 08:05:27, dcheng wrote:
> I might be missing something, but it seems like nothing actually passes null
for
> |region|. I'd rather just require it be present (ditto for the new class)

Done.

https://codereview.chromium.org/2684433003/diff/320001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2684433003/diff/320001/content/app/content_ma...
content/app/content_main_runner.cc:274: std::string key = descriptor.second;
On 2017/02/15 08:05:27, dcheng wrote:
> const std::string& and save a copy

Done.

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
File content/browser/child_process_launcher_helper_posix.cc (right):

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:31: std::map<std::string,
std::unique_ptr<catalog::RequiredFileMap>>>
On 2017/02/15 08:05:27, dcheng wrote:
> Nit: no unique_ptr indirection, using RequiredFileMap directly with
std::move()
> should work

Done.

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:34:
base::LazyInstance<OpenedFileMap> g_opened_files = LAZY_INSTANCE_INITIALIZER;
On 2017/02/15 08:05:27, dcheng wrote:
> Nit: use function-local statics instead of LazyInstance?

Done.

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:81: static
std::map<std::string, std::string>* service_name_resolver = nullptr;
On 2017/02/15 08:05:27, dcheng wrote:
> Another alternative:
> 
> static auto* service_name_resolver =
>     new std::map<std::string, std::string>()({
>   {switches::kGpuProcess, "content_gpu"},
>   {switches::kPpapiPluginProcess, "content_plugin"},
>   ...
> });
> 
> As a bonus, it's automatically thread-safe.

Nice! Done.

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:99:
CHECK(service_name_iter != service_name_resolver->end())
On 2017/02/15 08:05:27, dcheng wrote:
> DCHECK, unless the input can be controlled by arbitrary content (which I would
> imagine shouldn't be the case)

Done.

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:101: std::string
service_name = service_name_iter->second;
On 2017/02/15 08:05:27, dcheng wrote:
> Nit: const std::string& to save a copy

Done.

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:102: const auto&
files_iter = g_required_files_by_service.Get().find(service_name);
On 2017/02/15 08:05:27, dcheng wrote:
> But this can just be auto, since iterators are lightweight value types
> (relatively)--and in this case, lifetime extension applies anyway.

Done.

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:110:
OpenFileIfNecessary(base::FilePath(key_path_iter.second), &region);
On 2017/02/15 08:05:27, dcheng wrote:
> Can the map just hold a FilePath?

Good idea, done.

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:112: LOG(ERROR) <<
"Ignoring invalid file " << key_path_iter.second;
On 2017/02/15 08:05:27, dcheng wrote:
> DLOG instead of LOG? This comment applies to many of the other newly added
> logging messages.

Went through all of them and changed them to DLOG.

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:133:
std::unique_ptr<catalog::RequiredFileMap> required_files) {
On 2017/02/15 08:05:27, dcheng wrote:
> Consider just passing by value and moving below, rather than indirecting
through
> unique_ptr

Done.

https://codereview.chromium.org/2684433003/diff/320001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:137: if
(!base::StartsWith(service_name, "content",
On 2017/02/15 08:05:27, dcheng wrote:
> Maybe content_ to be safe?

Done.

https://codereview.chromium.org/2684433003/diff/320001/services/catalog/entry.cc
File services/catalog/entry.cc (right):

https://codereview.chromium.org/2684433003/diff/320001/services/catalog/entry...
services/catalog/entry.cc:182: entry->AddRequiredFileDescritor(iter.first,
iter.second);
On 2017/02/15 08:05:27, dcheng wrote:
> The naming also feels a little unusual: there's no actual file descriptor at
> this layer, but the function is called AddRequiredFD instead of
> AddRequiredFilePath or the like.
You are right, especially since everything else uses paths in their names.
Changed to AddRequiredFilePath.

https://codereview.chromium.org/2684433003/diff/320001/services/catalog/entry.h
File services/catalog/entry.h (right):

https://codereview.chromium.org/2684433003/diff/320001/services/catalog/entry...
services/catalog/entry.h:67: void AddRequiredFileDescritor(const std::string&
name,
On 2017/02/15 08:05:27, dcheng wrote:
> Nit: AddRequiredFileDescriptor

Renamed to AddRequiredFilePath.

https://codereview.chromium.org/2684433003/diff/320001/services/catalog/publi...
File services/catalog/public/cpp/manifest_parsing_util.cc (right):

https://codereview.chromium.org/2684433003/diff/320001/services/catalog/publi...
services/catalog/public/cpp/manifest_parsing_util.cc:37: bool
PopulateRequiredFiles(const base::Value& manifest_value,
On 2017/02/15 08:05:27, dcheng wrote:
> Would it make sense to return a base::Optional<RequiredFileMap>? I notice some
> call sites (such as entry.cc) don't actually check the return value.

Good idea. (I didn't know about Optional)

https://codereview.chromium.org/2684433003/diff/320001/services/catalog/publi...
services/catalog/public/cpp/manifest_parsing_util.cc:65: for (size_t i = 0; i <
all_platform_values->GetSize(); i++) {
On 2017/02/15 08:05:28, dcheng wrote:
> It's possible to use range-based for loops with base::ListValue, FWIW

But then I'd lose the index value which I use in the debug logs.

https://codereview.chromium.org/2684433003/diff/320001/services/service_manag...
File services/service_manager/public/cpp/lib/shared_file_util.cc (right):

https://codereview.chromium.org/2684433003/diff/320001/services/service_manag...
services/service_manager/public/cpp/lib/shared_file_util.cc:22: std::map<int,
std::string>* values) {
On 2017/02/15 08:05:28, dcheng wrote:
> Ditto: does it make sense to return a base::Optional<std::map<int,
std::string>>
> here?
> 
> (I don't have a strong preference for using it, I'm really just exploring
> options)

Yes, done.

https://codereview.chromium.org/2684433003/diff/320001/services/service_manag...
File services/service_manager/public/cpp/shared_file_util.h (right):

https://codereview.chromium.org/2684433003/diff/320001/services/service_manag...
services/service_manager/public/cpp/shared_file_util.h:17: void AddEntry(const
std::string& key_str, int key_id);
On 2017/02/15 08:05:28, dcheng wrote:
> This code isn't actually being used yet, right? Is a followup going to use it?

Oh, forgot to use it where I was supposed to!
It is now used in child_process_launcher_helper_posix.cc

[dcheng, 2017-02-15 22:01:03]

lgtm

https://codereview.chromium.org/2684433003/diff/340001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2684433003/diff/340001/content/app/content_ma...
content/app/content_main_runner.cc:274: for (const auto& descriptor :
shared_file_descriptors.value()) {
Shorter alternative is *shared_file_descriptors

(I don't have a preference here, just pointing it out)

https://codereview.chromium.org/2684433003/diff/340001/content/browser/child_...
File content/browser/child_process_launcher_helper_posix.cc (right):

https://codereview.chromium.org/2684433003/diff/340001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:28:
g_required_files_by_service = LAZY_INSTANCE_INITIALIZER;
This can be a function-level static as well too (just wrap it inside a function
to hold and return it)

https://codereview.chromium.org/2684433003/diff/340001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:90:
{"nacl-loader-nonsfi", "nacl_loader_nonsfi"}});
Ditto: from a code organization perspective, it might be nice to break this out
into a helper function that holds the static map.

https://codereview.chromium.org/2684433003/diff/340001/services/service_manag...
File services/service_manager/public/cpp/lib/shared_file_util.cc (right):

https://codereview.chromium.org/2684433003/diff/340001/services/service_manag...
services/service_manager/public/cpp/lib/shared_file_util.cc:32: return {};
I believe the Chromium guidelines would discourage uniform initialization syntax
here, and would prefer:

return base::nullopt;

See
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...

https://codereview.chromium.org/2684433003/diff/340001/services/service_manag...
services/service_manager/public/cpp/lib/shared_file_util.cc:46: return
{std::move(values)};
Ditto: I believe the spirit of the rules would indicate a preference for:

return base::make_optional(std::move(values));

though it's a little more verbose.

[nyquist, 2017-02-15 22:01:40]

base/android lgtm

https://codereview.chromium.org/2684433003/diff/300001/base/android/apk_assets.h
File base/android/apk_assets.h (right):

https://codereview.chromium.org/2684433003/diff/300001/base/android/apk_asset...
base/android/apk_assets.h:34: const std::string& file_path);
On 2017/02/14 17:46:18, Jay Civelli wrote:
> On 2017/02/14 05:46:06, nyquist wrote:
> > Well now it feels like this should be changed to base::FilePath& too.
> Especially
> > since it seems like the only caller already retrieves a base::FilePath (
> >
>
https://cs.chromium.org/chromium/src/android_webview/lib/main/aw_main_delegat...
> > )
> > 
> > I think the code it's calling is from here: //gin/v8_initializer.h
> > 
> > And you already have an //android_webview reviewer.
> > 
> > WDYT?
> 
> What if I told you that this method can actually be removed? :-)
> (the only caller is the one I changed. I removed it)

Oh, right. You already removed that usage. Well, that makes me happy!

[boliu, 2017-02-15 22:23:32]

android_webview and android under content lgtm

[Jay Civelli, 2017-02-15 23:08:06]

https://codereview.chromium.org/2684433003/diff/340001/content/app/content_ma...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2684433003/diff/340001/content/app/content_ma...
content/app/content_main_runner.cc:274: for (const auto& descriptor :
shared_file_descriptors.value()) {
On 2017/02/15 22:01:03, dcheng wrote:
> Shorter alternative is *shared_file_descriptors
> 
> (I don't have a preference here, just pointing it out)

Good to know.

https://codereview.chromium.org/2684433003/diff/340001/content/browser/child_...
File content/browser/child_process_launcher_helper_posix.cc (right):

https://codereview.chromium.org/2684433003/diff/340001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:28:
g_required_files_by_service = LAZY_INSTANCE_INITIALIZER;
On 2017/02/15 22:01:03, dcheng wrote:
> This can be a function-level static as well too (just wrap it inside a
function
> to hold and return it)

Done.

https://codereview.chromium.org/2684433003/diff/340001/content/browser/child_...
content/browser/child_process_launcher_helper_posix.cc:90:
{"nacl-loader-nonsfi", "nacl_loader_nonsfi"}});
On 2017/02/15 22:01:03, dcheng wrote:
> Ditto: from a code organization perspective, it might be nice to break this
out
> into a helper function that holds the static map.

Done.

https://codereview.chromium.org/2684433003/diff/340001/services/service_manag...
File services/service_manager/public/cpp/lib/shared_file_util.cc (right):

https://codereview.chromium.org/2684433003/diff/340001/services/service_manag...
services/service_manager/public/cpp/lib/shared_file_util.cc:32: return {};
On 2017/02/15 22:01:03, dcheng wrote:
> I believe the Chromium guidelines would discourage uniform initialization
syntax
> here, and would prefer:
> 
> return base::nullopt;
> 
> See
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...

Done.

https://codereview.chromium.org/2684433003/diff/340001/services/service_manag...
services/service_manager/public/cpp/lib/shared_file_util.cc:46: return
{std::move(values)};
On 2017/02/15 22:01:03, dcheng wrote:
> Ditto: I believe the spirit of the rules would indicate a preference for:
> 
> return base::make_optional(std::move(values));
> 
> though it's a little more verbose.

Done.

[Jay Civelli, 2017-02-15 23:08:16]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2017-02-15 23:08:17]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org, boliu@chromium.org, dcheng@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2684433003/#ps380001
(title: "Synced")

[commit-bot: I haz the power, 2017-02-15 23:09:41]

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, 2017-02-15 23:38:06]

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

[commit-bot: I haz the power, 2017-02-15 23:38:08]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Jay Civelli, 2017-02-16 00:38:21]

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

[commit-bot: I haz the power, 2017-02-16 00:39:14]

Dry run: 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, 2017-02-16 02:43:15]

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

[commit-bot: I haz the power, 2017-02-16 02:43:16]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Jay Civelli, 2017-02-16 16:44:44]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2017-02-16 16:44:45]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jam@chromium.org, boliu@chromium.org, dcheng@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2684433003/#ps400001
(title: "Fix build.")

[commit-bot: I haz the power, 2017-02-16 16:45:30]

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, 2017-02-16 18:39:05]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1487263484411990,
"parent_rev": "71fcc2bfa4b36ce4085ac5f0346b1cdc86b98f5d", "commit_rev":
"dad0cefaebae56eb5ed984b62ccebee165723a6a"}

[commit-bot: I haz the power, 2017-02-16 18:39:42]

Description was changed from

==========
Files required by a service now listed in manifest.

The files required in a service process can now be listed in the service's
manifest.
In the manifest the file path is given with a string ID and a platform. The file
is then opened in the parent process and passed to the child process, its ID is
specified with a new command-line switch that associates the manifest specified
key and a GlobalDescriptor ID. There are used to populate the new
FileDescriptorStore which is similar to GlobalDescriptor but uses string as keys
(which makes it easier to prevent ID collisions between multiple services).
The service code can then retrieve the FD from the FileDescriptorStore.
The goal is to eventually switch entirely to only using the FileDescriptorStore,
and getting rid of the GlobalDescriptor.

BUG=690189
==========

to

==========
Files required by a service now listed in manifest.

The files required in a service process can now be listed in the service's
manifest.
In the manifest the file path is given with a string ID and a platform. The file
is then opened in the parent process and passed to the child process, its ID is
specified with a new command-line switch that associates the manifest specified
key and a GlobalDescriptor ID. There are used to populate the new
FileDescriptorStore which is similar to GlobalDescriptor but uses string as keys
(which makes it easier to prevent ID collisions between multiple services).
The service code can then retrieve the FD from the FileDescriptorStore.
The goal is to eventually switch entirely to only using the FileDescriptorStore,
and getting rid of the GlobalDescriptor.

BUG=690189

Review-Url: https://codereview.chromium.org/2684433003
Cr-Commit-Position: refs/heads/master@{#451019}
Committed:
https://chromium.googlesource.com/chromium/src/+/dad0cefaebae56eb5ed984b62cce...
==========

[commit-bot: I haz the power, 2017-02-16 18:39:44]

Committed patchset #21 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/dad0cefaebae56eb5ed984b62cce...