Non-SFI NaCl: Batch-open resource files

components/nacl/renderer/json_manifest.cc

Index: components/nacl/renderer/json_manifest.cc
diff --git a/components/nacl/renderer/json_manifest.cc b/components/nacl/renderer/json_manifest.cc
index e2063acd1d6c87a826917db7b7f78845827341cf..64676a123a799d2841789763a1f714c9d23c4333 100644
--- a/components/nacl/renderer/json_manifest.cc
+++ b/components/nacl/renderer/json_manifest.cc
@@ -459,6 +459,28 @@ bool JsonManifest::GetProgramURL(std::string* full_url,
   return true;
 }
 
+bool JsonManifest::GetFiles(

[Mark Seaborn, 2015/02/02 23:21:50]

This returns a bool but the caller ignores the return value.  Change to void?

[Yusuke Sato, 2015/02/04 02:00:29]

Done.

+    std::vector<std::pair<std::string, std::string> >* out_files) const {
+  if (out_files == NULL)

[Mark Seaborn, 2015/02/02 23:21:50]

Nit: no need to check for this.

[Yusuke Sato, 2015/02/04 02:00:29]

Done.

+    return false;
+
+  const Json::Value& files = dictionary_[kFilesKey];
+  if (!files.isObject()) {
+    VLOG(1) << "ResolveKey failed: no \"files\" dictionary";

[Mark Seaborn, 2015/02/02 23:21:50]

It's valid to have no "files" dict, so drop this log message?

[Yusuke Sato, 2015/02/04 02:00:29]

Done.

+    return false;
+  }
+
+  const std::vector<std::string>& keys = files.getMemberNames();
+  for (size_t i = 0; i < keys.size(); ++i) {
+    std::string full_url;
+    PP_PNaClOptions pnacl_options;

[hidehiko, 2015/01/28 09:05:20]

unused prefix? Maybe short comment why it is unused would be beneficial.

[Yusuke Sato, 2015/02/04 02:00:29]

Done.

+    if (!GetKeyUrl(files, keys[i], &full_url, &pnacl_options))
+      return false;

[hidehiko, 2015/01/28 09:05:20]

logging, too?

[Mark Seaborn, 2015/02/02 23:21:50]

Does this "return" mean that if one entry in "files" is invalid, you ignore all
later files?  That seems unnecessary.

Presumably you could write:

// We skip invalid entries in "files".
if (GetKeyUrl(files, keys[i], &full_url, &pnacl_options)) {
  out_files->push_back(std::make_pair(full_url, keys[i]));
}

[Yusuke Sato, 2015/02/04 02:00:29]

Done.

[Yusuke Sato, 2015/02/04 02:00:29]

Done.

+    out_files->push_back(std::make_pair(full_url, keys[i]));
+  }
+  return true;
+}
+
 bool JsonManifest::ResolveKey(const std::string& key,
                               std::string* full_url,
                               PP_PNaClOptions* pnacl_options) const {