Refactor params of NaClProcessMsg_Start.

components/nacl/common/nacl_types.h

Index: components/nacl/common/nacl_types.h
diff --git a/components/nacl/common/nacl_types.h b/components/nacl/common/nacl_types.h
index f691edf9b2f7c6068efb3f686337ada1e994b7b6..197f9bf531d537baa5966c7bed85d773aecb6db5 100644
--- a/components/nacl/common/nacl_types.h
+++ b/components/nacl/common/nacl_types.h
@@ -82,8 +82,10 @@ struct NaClStartParams {
   base::FilePath nexe_file_path_metadata;
 
   std::vector<NaClResourceFileInfo> prefetched_resource_files;
-  std::vector<FileDescriptor> handles;
-  FileDescriptor debug_stub_server_bound_socket;
+  IPC::PlatformFileForTransit imc_bootstrap_handle;

[Mark Seaborn, 2015/04/23 00:10:11]

Splitting up 'handles' seems reasonable but the reason for it isn't clear.  Is
it a necessary part of the change or a bonus cleanup?  Please explain in the
commit message.

[hidehiko, 2015/04/23 11:51:29]

For cleaner handle check in nacl_listener.cc. Updated the description.

+  IPC::PlatformFileForTransit irt_handle;
+  IPC::PlatformFileForTransit mac_shm_fd;

[Mark Seaborn, 2015/04/23 00:10:11]

If I understand this correctly, there's a subtle bug here:

On Unix, this is a base::FileDescriptor class -- safely initialised with a
default.

On Windows, this is a base::PlatformFile, which is a HANDLE -- not safely
initialised with a default.  So you'd need to add a default initialiser value in
nacl_types.cc for this to be safe.

[hidehiko, 2015/04/23 11:51:29]

Ah, right. Fixed (including existing debug_stub_server_bound_socket).

+  IPC::PlatformFileForTransit debug_stub_server_bound_socket;
 
   bool validation_cache_enabled;
   std::string validation_cache_key;
@@ -108,6 +110,17 @@ struct NaClStartParams {
   // in nacl_types.cc.
 };
 
+// Metadata to prefetch a resource file.

[Mark Seaborn, 2015/04/23 00:10:10]

Similarly, using a struct with names fields instead of std::pair<> seems
reasonable, but the reason for it isn't clear.  Is this a bonus cleanup or a
necessary change?

[hidehiko, 2015/04/23 11:51:29]

Bonus, but I was unhappy to distribute std::vector<std::pair<std::string,
std::string>>, and the meaning of first/second was confusing to me while I read
the code.

+struct NaClResourcePrefetchInfo {

[Mark Seaborn, 2015/04/23 00:10:11]

How about putting this next to NaClResourceFileInfo (above) and making the
relationship between them clearer?

One is a request (renderer->browser), the other is the result
(browser->nacl_loader).  How about reflecting that in the names?

Maybe:
NaClResourceFilePrefetchRequest,
NaClResourceFilePrefetchResult

[hidehiko, 2015/04/23 11:51:29]

Done.

+  NaClResourcePrefetchInfo();
+  NaClResourcePrefetchInfo(const std::string& manifest_key,
+                           const std::string& resource_url);
+  ~NaClResourcePrefetchInfo();
+
+  std::string manifest_key;

[Mark Seaborn, 2015/04/23 00:10:11]

In NaClResourceFileInfo, this is called "file_key".  Can you make the two
consistent?

You could also add a comment here to match NaClResourceFileInfo's field.

[hidehiko, 2015/04/23 11:51:29]

Done.

+  std::string resource_url;
+};
+
 // Parameters sent to the browser process to have it launch a NaCl process.
 //
 // If you change this, you will also need to update the IPC serialization in
@@ -119,9 +132,7 @@ struct NaClLaunchParams {
       const IPC::PlatformFileForTransit& nexe_file,
       uint64_t nexe_token_lo,
       uint64_t nexe_token_hi,
-      // A pair of a manifest key and its resource URL.
-      const std::vector<
-        std::pair<std::string, std::string> >& resource_files_to_prefetch,
+      const std::vector<NaClResourcePrefetchInfo>& resource_prefetch_info_list,

[Mark Seaborn, 2015/04/23 00:10:11]

Hmm, "files_to_prefetch" seems more descriptive to me than "prefetch_info_list".

[hidehiko, 2015/04/23 11:51:29]

Renamed to resource_prefetch_request_list.
Note that I do not want to use files_to_prefetch, because it is not/does not
contain a file (FD/HANDLE), which is the important key for resource management
I'm fixing.

[Mark Seaborn, 2015/04/24 00:53:24]

That reason seems a bit pedantic to me.  Files are not the same thing as file
descriptors or file handles, anyway.  So I think it's OK to name something as
"files" if it represents a set of files in the abstract (such as a set of
filenames).

FWIW, for that reason, I think "base::File" wasn't well named.

But anyhow, as long as NaClResourcePrefetchRequest/Result are commented as
representing files (as you've done in nacl_types.h), I don't mind if you use
"files_to_prefetch" or "prefetch_request_list".

       int render_view_id,
       uint32 permission_bits,
       bool uses_nonsfi_mode,
@@ -135,7 +146,7 @@ struct NaClLaunchParams {
   IPC::PlatformFileForTransit nexe_file;
   uint64_t nexe_token_lo;
   uint64_t nexe_token_hi;
-  std::vector<std::pair<std::string, std::string> > resource_files_to_prefetch;
+  std::vector<NaClResourcePrefetchInfo> resource_prefetch_info_list;
 
   int render_view_id;
   uint32 permission_bits;