Rename HandleConverter to NaClMessageScanner.

components/nacl/loader/nacl_ipc_adapter.cc

 // Copyright 2013 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/loader/nacl_ipc_adapter.h"
 
 #include <limits.h>
 #include <string.h>
 
 #include "base/basictypes.h"
@@ skipping 359 matching lines @@
 #endif
 
 bool NaClIPCAdapter::OnMessageReceived(const IPC::Message& msg) {
   {
     base::AutoLock lock(lock_);
 
     scoped_refptr<RewrittenMessage> rewritten_msg(new RewrittenMessage);
 
     typedef std::vector<ppapi::proxy::SerializedHandle> Handles;
     Handles handles;
-    scoped_ptr<IPC::Message> new_msg_ptr;
-    bool success = locked_data_.handle_converter_.ConvertNativeHandlesToPosix(
-        msg, &handles, &new_msg_ptr);
-    if (!success)
+    scoped_ptr<IPC::Message> new_msg;
+    if (!locked_data_.nacl_msg_scanner_.ScanMessage(msg, &handles, &new_msg))
       return false;
 
     // Now add any descriptors we found to rewritten_msg. |handles| is usually
     // empty, unless we read a message containing a FD or handle.
     for (Handles::const_iterator iter = handles.begin();
          iter != handles.end();
          ++iter) {
       scoped_ptr<NaClDescWrapper> nacl_desc;
       switch (iter->type()) {
         case ppapi::proxy::SerializedHandle::SHARED_MEMORY: {
@@ skipping 58 matching lines @@
         case ppapi::proxy::SerializedHandle::INVALID: {
           // Nothing to do. TODO(dmichael): Should we log this? Or is it
           // sometimes okay to pass an INVALID handle?
           break;
         }
         // No default, so the compiler will warn us if new types get added.
       }
       if (nacl_desc.get())
         rewritten_msg->AddDescriptor(nacl_desc.release());
     }
-    if (new_msg_ptr && !handles.empty())
-      SaveMessage(*new_msg_ptr, rewritten_msg.get());
+    if (new_msg && !handles.empty())

[bbudge, 2013/10/30 19:01:34]

I wonder if we really need to check 'handles' after we've gone to the trouble of
rewriting the message.

[dmichael (off chromium), 2013/10/30 20:57:57]

I think the issue is that ScanMessage only writes new_msg if we explicitly
recognize the message in there. I.e., for messages without handles (most of
them), new_msg will be empty.

At least a comment might help. It took me a while to remember why it is this
way.

We could probably make ScanMessage only actually set new_msg if it did any work.
Then you wouldn't need the handles.empty() check. That would be easier to
understand, at least here.

+      SaveMessage(*new_msg, rewritten_msg.get());
     else
       SaveMessage(msg, rewritten_msg.get());
   }
   cond_var_.Signal();
   return true;
 }
 
 void NaClIPCAdapter::OnChannelConnected(int32 peer_pid) {
 }
 
@@ skipping 33 matching lines @@
       reinterpret_cast<const NaClMessageHeader*>(buffer);
 
   // Length of the message not including the body. The data passed to us by the
   // plugin should match that in the message header. This should have already
   // been validated by GetBufferStatus.
   int body_len = static_cast<int>(buffer_len - sizeof(NaClMessageHeader));
   DCHECK(body_len == static_cast<int>(header->payload_size));
 
   // We actually discard the flags and only copy the ones we care about. This
   // is just because message doesn't have a constructor that takes raw flags.
-  scoped_ptr<IPC::Message> msg(
-      new IPC::Message(header->routing, header->type));
+  scoped_ptr<IPC::Message> msg(new IPC::Message(header->routing, header->type));
   if (header->flags & IPC::Message::SYNC_BIT)
     msg->set_sync();
   if (header->flags & IPC::Message::REPLY_BIT)
     msg->set_reply();
   if (header->flags & IPC::Message::REPLY_ERROR_BIT)
     msg->set_reply_error();
   if (header->flags & IPC::Message::UNBLOCK_BIT)
     msg->set_unblock(true);
 
   msg->WriteBytes(&buffer[sizeof(NaClMessageHeader)], body_len);
 
   // Technically we didn't have to do any of the previous work in the lock. But
   // sometimes our buffer will point to the to_be_sent_ string which is
   // protected by the lock, and it's messier to factor Send() such that it can
   // unlock for us. Holding the lock for the message construction, which is
   // just some memcpys, shouldn't be a big deal.
   lock_.AssertAcquired();
   if (locked_data_.channel_closed_)
     return false;  // TODO(brettw) clean up handles here when we add support!
 
   if (msg->is_sync()) {
-    locked_data_.handle_converter_.RegisterSyncMessageForReply(*msg);
+    locked_data_.nacl_msg_scanner_.RegisterSyncMessageForReply(*msg);
   }
   // Actual send must be done on the I/O thread.
   task_runner_->PostTask(FROM_HERE,
       base::Bind(&NaClIPCAdapter::SendMessageOnIOThread, this,
                  base::Passed(&msg)));
   return true;
 }
 
 void NaClIPCAdapter::ClearToBeSent() {
   lock_.AssertAcquired();
@@ skipping 31 matching lines @@
   header.flags = msg.flags();
   header.num_fds = static_cast<int>(rewritten_msg->desc_count());
 
   rewritten_msg->SetData(header, msg.payload(), msg.payload_size());
   locked_data_.to_be_received_.push(rewritten_msg);
 }
 
 int TranslatePepperFileReadWriteOpenFlagsForTesting(int32_t pp_open_flags) {
   return TranslatePepperFileReadWriteOpenFlags(pp_open_flags);
 }