Pepper: Cleanup in PepperReverseInterface.

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

Index: ppapi/native_client/src/trusted/plugin/service_runtime.cc
diff --git a/ppapi/native_client/src/trusted/plugin/service_runtime.cc b/ppapi/native_client/src/trusted/plugin/service_runtime.cc
index 5a18f5af94cfa640c13c9c5245227014cd1b0a5d..2a95d4670d1b6741aa48cfb1ceb95585a8f07bc7 100644
--- a/ppapi/native_client/src/trusted/plugin/service_runtime.cc
+++ b/ppapi/native_client/src/trusted/plugin/service_runtime.cc
@@ -137,13 +137,7 @@ void PluginReverseInterface::PostMessage_MainThreadContinuation(
 
 bool PluginReverseInterface::EnumerateManifestKeys(
     std::set<nacl::string>* out_keys) {
-  Manifest const* mp = manifest_;
-
-  if (!mp->GetFileKeys(out_keys)) {
-    return false;
-  }
-
-  return true;
+  return manifest_->GetFileKeys(out_keys);
 }
 
 // TODO(bsy): OpenManifestEntry should use the manifest to ResolveKey
@@ -173,17 +167,21 @@ bool PluginReverseInterface::OpenManifestEntry(nacl::string url_key,
   NaClLog(4,
           "PluginReverseInterface::OpenManifestEntry:"
           " waiting on main thread\n");
-  bool shutting_down;
-  do {
+  {
     nacl::MutexLocker take(&mu_);
-    for (;;) {
+    while (true) {

[dmichael (off chromium), 2014/03/05 16:54:51]

Could this instead be:
while (!shutting_down_ && !op_complete)
  NaClXCondVarWait(&cv_, &mu);
NaClLog("... done!");  // <-- The "done!" log happens for either
                       // case in the old code
if (shutting_down_) {
  NaClLog("... plugin is shutting down\n");
  return false;
}

Or am I misunderstanding something?

       NaClLog(4,
               "PluginReverseInterface::OpenManifestEntry:"
               " got lock, checking shutdown and completion: (%s, %s)\n",
               shutting_down_ ? "yes" : "no",
               op_complete ? "yes" : "no");
-      shutting_down = shutting_down_;
-      if (op_complete || shutting_down) {
+      if (shutting_down_) {
+        NaClLog(4,
+                "PluginReverseInterface::OpenManifestEntry:"
+                " plugin is shutting down\n");
+        return false;
+      }
+      if (op_complete) {
         NaClLog(4,
                 "PluginReverseInterface::OpenManifestEntry:"
                 " done!\n");
@@ -191,16 +189,11 @@ bool PluginReverseInterface::OpenManifestEntry(nacl::string url_key,
       }
       NaClXCondVarWait(&cv_, &mu_);
     }
-  } while (0);
-  if (shutting_down) {
-    NaClLog(4,
-            "PluginReverseInterface::OpenManifestEntry:"
-            " plugin is shutting down\n");
-    return false;
   }
-  // out_desc has the returned descriptor if successful, else -1.
 
-  // The caller is responsible for not closing *out_desc.  If it is
+  // info->desc has the returned descriptor if successful, else -1.
+
+  // The caller is responsible for not closing info->desc.  If it is
   // closed prematurely, then another open could re-use the OS
   // descriptor, confusing the opened_ map.  If the caller is going to
   // want to make a NaClDesc object and transfer it etc., then the
@@ -208,8 +201,7 @@ bool PluginReverseInterface::OpenManifestEntry(nacl::string url_key,
   // value) for use by the NaClDesc object, which closes when the
   // object is destroyed.
   NaClLog(4,
-          "PluginReverseInterface::OpenManifestEntry:"
-          " *out_desc = %d\n",
+          "PluginReverseInterface::OpenManifestEntry: info->desc = %d\n",
           info->desc);
   if (info->desc == -1) {
     // TODO(bsy,ncbray): what else should we do with the error?  This
@@ -359,7 +351,6 @@ bool PluginReverseInterface::CloseManifestEntry(int32_t desc) {
   CloseManifestEntryResource* to_close =
       new CloseManifestEntryResource(desc, &op_complete, &op_result);
 
-  bool shutting_down;
   plugin::WeakRefCallOnMainThread(
       anchor_,
       0,
@@ -367,19 +358,19 @@ bool PluginReverseInterface::CloseManifestEntry(int32_t desc) {
       &plugin::PluginReverseInterface::
         CloseManifestEntry_MainThreadContinuation,
       to_close);
+
   // wait for completion or surf-away.
-  do {
+  {
     nacl::MutexLocker take(&mu_);
-    for (;;) {
-      shutting_down = shutting_down_;
-      if (op_complete || shutting_down) {
+    while (true) {

[dmichael (off chromium), 2014/03/05 16:54:51]

I think you could do the same kind of transformation here, unless I'm missing
something...

while (!shutting_down_ && !op_complete)
  NaClXCondVarWait(&cv_, &mu_);
if (shutting_down_)
  return false;

+      if (shutting_down_)
+        return false;
+      if (op_complete)
         break;
-      }
       NaClXCondVarWait(&cv_, &mu_);
     }
-  } while (0);
+  }
 
-  if (shutting_down) return false;
   // op_result true if close was successful; false otherwise (e.g., bad desc).
   return op_result;
 }