Refactor params of NaClProcessMsg_Start.

components/nacl/common/nacl_types.h

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_NACL_COMMON_NACL_TYPES_H_
 #define COMPONENTS_NACL_COMMON_NACL_TYPES_H_
 
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 64 matching lines @@
 // Parameters sent to the NaCl process when we start it.
 struct NaClStartParams {
   NaClStartParams();
   ~NaClStartParams();
 
   IPC::PlatformFileForTransit nexe_file;
   // Used only as a key for validation caching.
   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;
   // Chrome version string. Sending the version string over IPC avoids linkage
   // issues in cases where NaCl is not compiled into the main Chromium
   // executable or DLL.
   std::string version;
 
   bool enable_debug_stub;
   bool enable_ipc_proxy;
 
   // Enables plugin code to use Mojo APIs. See mojo/nacl for details.
   bool enable_mojo;
 
   NaClAppProcessType process_type;
 
   // For NaCl <-> renderer crash information reporting.
   base::SharedMemoryHandle crash_info_shmem_handle;
 
   // NOTE: Any new fields added here must also be added to the IPC
   // serialization in nacl_messages.h and (for POD fields) the constructor
   // 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
 // nacl_host_messages.h.
 struct NaClLaunchParams {
   NaClLaunchParams();
   NaClLaunchParams(
       const std::string& manifest_url,
       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,
       NaClAppProcessType process_type);
   ~NaClLaunchParams();
 
   std::string manifest_url;
   // On Windows, the HANDLE passed here is valid in the renderer's context.
   // It's the responsibility of the browser to duplicate this handle properly
   // for passing it to the plugin.
   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;
   bool uses_nonsfi_mode;
 
   NaClAppProcessType process_type;
 };
 
 struct NaClLaunchResult {
   NaClLaunchResult();
@@ skipping 23 matching lines @@
   base::ProcessId plugin_pid;
   int plugin_child_id;
 
   // For NaCl <-> renderer crash information reporting.
   base::SharedMemoryHandle crash_info_shmem_handle;
 };
 
 }  // namespace nacl
 
 #endif  // COMPONENTS_NACL_COMMON_NACL_TYPES_H_