Introduce NaClFileInfoAutoCloser as an RAII wrapper.

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

 // Copyright (c) 2012 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 "ppapi/native_client/src/trusted/plugin/file_downloader.h"
 
 #include <stdio.h>
 #include <string.h>
 #include <string>
 
@@ skipping 39 matching lines @@
   return posix_desc;
 #else
   return handle;
 #endif
 }
 
 }  // namespace
 
 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)));
   CHECK(instance != NULL);
   CHECK(instance_ == NULL);  // Can only initialize once.
   instance_ = instance;
   callback_factory_.Initialize(this);
   file_io_private_interface_ = static_cast<const PPB_FileIO_Private*>(
       pp::Module::Get()->GetBrowserInterface(PPB_FILEIO_PRIVATE_INTERFACE));
   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(
     const nacl::string& url,
     const pp::CompletionCallback& callback,
     StreamCallbackSource* stream_callback_source) {
   open_and_stream_ = false;
   data_stream_callback_source_ = stream_callback_source;
   return Open(url, DOWNLOAD_STREAM, callback, true, NULL);
 }
@@ skipping 11 matching lines @@
     return false;
 
   CHECK(instance_ != NULL);
   open_time_ = NaClGetTimeOfDayMicroseconds();
   status_code_ = -1;
   url_to_open_ = url;
   url_ = url;
   file_open_notify_callback_ = callback;
   mode_ = mode;
   buffer_.clear();
-  cached_file_info_ = NoFileInfo();
+  file_info_.FreeResources();
   pp::URLRequestInfo url_request(instance_);
 
   // Allow CORS.
   // Note that "SetAllowCrossOriginRequests" (currently) has the side effect of
   // preventing credentials from being sent on same-origin requests.  We
   // therefore avoid setting this flag unless we know for sure it is a
   // cross-origin request, resulting in behavior similar to XMLHttpRequest.
   if (!instance_->DocumentCanRequest(url))
     url_request.SetAllowCrossOriginRequests(true);
 
@@ skipping 58 matching lines @@
                  pp_error));
   CHECK(pp_error == PP_OK_COMPLETIONPENDING);
   return true;
 }
 
 void FileDownloader::OpenFast(const nacl::string& url,
                               PP_FileHandle file_handle,
                               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 {
   int64_t now = NaClGetTimeOfDayMicroseconds();
   // If Open() wasn't called or we somehow return an earlier time now, just
   // return the 0 rather than worse nonsense values.
   if (open_time_ < 0 || now < open_time_)
     return 0;
   return (now - open_time_) / NACL_MICROS_PER_MILLI;
 }
@@ skipping 229 matching lines @@
 
 bool FileDownloader::not_streaming() const {
   return mode_ == DOWNLOAD_NONE;
 }
 
 void FileDownloader::GotFileHandleNotify(int32_t pp_error,
                                          PP_FileHandle handle) {
   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);
 }
 
 }  // namespace plugin