Introduce NaClFileInfoAutoCloser as an RAII wrapper.

ppapi/native_client/src/trusted/plugin/file_downloader.cc

Index: ppapi/native_client/src/trusted/plugin/file_downloader.cc
diff --git a/ppapi/native_client/src/trusted/plugin/file_downloader.cc b/ppapi/native_client/src/trusted/plugin/file_downloader.cc
index f8d3a4c4572fe6e68002a684196d6c43b11f0614..b5bf5033ee52f1e7f0d68d83bd1ab142beed9c4a 100644
--- a/ppapi/native_client/src/trusted/plugin/file_downloader.cc
+++ b/ppapi/native_client/src/trusted/plugin/file_downloader.cc
@@ -57,6 +57,37 @@ int32_t ConvertFileDescriptor(PP_FileHandle handle) {
 
 namespace plugin {
 
+NaClFileInfoAutoCloser::NaClFileInfoAutoCloser() {
+  info_ = NoFileInfo();

[dmichael (off chromium), 2014/01/29 18:22:28]

Why not do this in the initializer list instead?

[bsy, 2014/01/29 20:51:28]

Done.

+}
+
+NaClFileInfoAutoCloser::NaClFileInfoAutoCloser(NaClFileInfo pass_ownership) {
+  info_ = pass_ownership;

[dmichael (off chromium), 2014/01/29 18:22:28]

ditto

[bsy, 2014/01/29 20:51:28]

Done.

+}
+
+void NaClFileInfoAutoCloser::FreeResources() {
+  if (-1 != info_.desc) {
+    PLUGIN_PRINTF(("NaClFileInfoAutoCloser::FreeResources close(%d)\n",
+                   info_.desc));
+    close(get_desc());

[dmichael (off chromium), 2014/01/29 18:22:28]

tiny stupid nit: seems better to be consistent and either use get_desc
throughout this method or info_.desc, but not both. Or am I missing something
subtle?

[bsy, 2014/01/29 20:51:28]

not missing anything subtle, just that get_desc() was added after this edit and
i missed it.

+  }
+  info_.desc = -1;
+}
+
+void NaClFileInfoAutoCloser::set(NaClFileInfo pass_ownership) {

[dmichael (off chromium), 2014/01/29 18:22:28]

nit: this should probably also be capitalized, and maybe named more
descriptively like:
TakeOwnership
WDYT?

[bsy, 2014/01/29 20:51:28]

Done.

+  PLUGIN_PRINTF(("NaClFileInfoAutoCloser::set: taking ownership of %d\n",
+                 pass_ownership.desc));
+  FreeResources();

[dmichael (off chromium), 2014/01/29 18:22:28]

nit: It might be good to check on if info_.desc == pass_ownership, and either do
nothing, or (maybe better) just assert that case doesn't happen.

[bsy, 2014/01/29 20:51:28]

Done.

+  info_ = pass_ownership;
+}
+
+NaClFileInfo NaClFileInfoAutoCloser::release() {
+  NaClFileInfo nvro;

[dmichael (off chromium), 2014/01/29 18:22:28]

Some readers won't get nvro (is named return value optimization your intent?),
and I don't think the fact that the compiler can do return optimization is
actually important here (it's cheap either way).

How about something like info_to_return?

And it seems like you could initialize it to info_ right here, rather than
separate assignment?

[bsy, 2014/01/29 20:51:28]

Done.

+  nvro = info_;
+  info_ = NoFileInfo();
+  return nvro;
+}
+
 void FileDownloader::Initialize(Plugin* instance) {
   PLUGIN_PRINTF(("FileDownloader::FileDownloader (this=%p)\n",
                  static_cast<void*>(this)));
@@ -69,7 +100,7 @@ void FileDownloader::Initialize(Plugin* instance) {
   url_loader_trusted_interface_ = static_cast<const PPB_URLLoaderTrusted*>(
       pp::Module::Get()->GetBrowserInterface(PPB_URLLOADERTRUSTED_INTERFACE));
   temp_buffer_.resize(kTempBufferSize);
-  cached_file_info_ = NoFileInfo();
+  file_info_.FreeResources();
 }
 
 bool FileDownloader::OpenStream(
@@ -101,7 +132,7 @@ bool FileDownloader::Open(
   file_open_notify_callback_ = callback;
   mode_ = mode;
   buffer_.clear();
-  cached_file_info_ = NoFileInfo();
+  file_info_.FreeResources();
   pp::URLRequestInfo url_request(instance_);
 
   // Allow CORS.
@@ -180,29 +211,31 @@ void FileDownloader::OpenFast(const nacl::string& url,
                               uint64_t file_token_lo, uint64_t file_token_hi) {
   PLUGIN_PRINTF(("FileDownloader::OpenFast (url=%s)\n", url.c_str()));
 
-  cached_file_info_ = NoFileInfo();
+  file_info_.FreeResources();
   CHECK(instance_ != NULL);
   open_time_ = NaClGetTimeOfDayMicroseconds();
   status_code_ = NACL_HTTP_STATUS_OK;
   url_to_open_ = url;
   url_ = url;
   mode_ = DOWNLOAD_NONE;
-  file_handle_ = file_handle;
-  file_token_.lo = file_token_lo;
-  file_token_.hi = file_token_hi;
+  if (not_streaming() && file_handle != PP_kInvalidFileHandle) {
+    NaClFileInfo tmp_info = NoFileInfo();
+    tmp_info.desc = ConvertFileDescriptor(file_handle);
+    tmp_info.file_token.lo = file_token_lo;
+    tmp_info.file_token.hi = file_token_hi;
+    file_info_.set(tmp_info);
+  }
 }
 
-struct NaClFileInfo FileDownloader::GetFileInfo() {
-  PLUGIN_PRINTF(("FileDownloader::GetFileInfo\n"));
-  if (cached_file_info_.desc != -1) {
-    return cached_file_info_;
-  } else if (not_streaming() && file_handle_ != PP_kInvalidFileHandle) {
-    cached_file_info_.desc = ConvertFileDescriptor(file_handle_);
-    if (cached_file_info_.desc != -1)
-      cached_file_info_.file_token = file_token_;
-    return cached_file_info_;
+NaClFileInfo FileDownloader::GetFileInfo() {
+  NaClFileInfo nrvo = NoFileInfo();
+
+  PLUGIN_PRINTF(("FileDownloader::GetFileInfo, this %p\n", this));
+  if (file_info_.get_desc() != -1) {
+    nrvo = file_info_.release();
   }
-  return NoFileInfo();
+  PLUGIN_PRINTF(("FileDownloader::GetFileInfo -- returning %d\n", nrvo.desc));
+  return nrvo;
 }
 
 int64_t FileDownloader::TimeSinceOpenMilliseconds() const {
@@ -452,8 +485,11 @@ void FileDownloader::GotFileHandleNotify(int32_t pp_error,
   PLUGIN_PRINTF((
       "FileDownloader::GotFileHandleNotify (pp_error=%" NACL_PRId32 ")\n",
       pp_error));
-  if (pp_error == PP_OK)
-    cached_file_info_.desc = ConvertFileDescriptor(handle);
+  if (pp_error == PP_OK) {
+    NaClFileInfo tmp_info = NoFileInfo();
+    tmp_info.desc = ConvertFileDescriptor(handle);
+    file_info_.set(tmp_info);
+  }
 
   stream_finish_callback_.RunAndClear(pp_error);
 }