Non-SFI NaCl: Batch-open resource files

components/nacl/renderer/json_manifest.cc

 // Copyright 2014 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.
 
 #include "components/nacl/renderer/json_manifest.h"
 
 #include <set>
 
 #include "base/containers/scoped_ptr_hash_map.h"
 #include "base/lazy_instance.h"
@@ skipping 441 matching lines @@
     error_info->string =
         "could not resolve url '" + nexe_url +
         "' relative to manifest base url '" + manifest_base_url_.c_str() +
         "'.";
     return false;
   }
   *full_url = resolved_gurl.possibly_invalid_spec();
   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 {
   // key must be one of kProgramKey or kFileKey '/' file-section-key
   if (full_url == NULL || pnacl_options == NULL)
     return false;
 
   if (key == kProgramKey)
     return GetKeyUrl(dictionary_, key, full_url, pnacl_options);
 
@@ skipping 190 matching lines @@
   } else {
     // NaCl
     *url = isa_spec[kUrlKey].asString();
     pnacl_options->translate = PP_FALSE;
   }
 
   return true;
 }
 
 }  // namespace nacl