Rename HandleConverter to NaClMessageScanner.

Rename HandleConverter to NaClMessageScanner.
Renames the class and its methods to reflect its role as a message
cracker, translator, and auditor.
Combines several out params into a single ScanOutput structure, to make it
easier to add other functions to the scan.

BUG=194304
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232603

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

https://codereview.chromium.org/53123002/diff/20001/components/nacl/loader/na...
File components/nacl/loader/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/53123002/diff/20001/components/nacl/loader/na...
components/nacl/loader/nacl_ipc_adapter.cc:460: if (new_msg && !handles.empty())
I wonder if we really need to check 'handles' after we've gone to the trouble of
rewriting the message.

[bbudge, 2013-10-30 19:02:17]

Mark for components/nacl
David for everything

[Mark Seaborn, 2013-10-30 20:05:31]

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.cc:26: struct ScanOutput {
Can you add a note in the commit message to say that you're factoring out some
function arguments into this struct?  Might this factoring out be better done as
a separate change?

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.cc:30: Handles* handles;
Add a comment?  Is this an input or output of scanning?

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.cc:31: int handle_index;
Add a comment for this field?  Is it an index into 'handles'?  Is it different
from handles->size()?

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.cc:32: IPC::Message* msg;
Add comments for 'msg' and 'audit_msg'?

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.cc:33: IPC::Message* audit_msg;
I don't see any users of this field.

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.cc:79: // NOTE: We only catch handles from
host->NaCl. The only kind of
"catch" seems more vague than "translate" (the original).

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
File ppapi/proxy/nacl_message_scanner.h (right):

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
Can you use "git cl upload --similarity=..." to get Git to treat this file as
moved, perhaps?

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.h:31: // Translates the message if necessary
(e.g. on Windows) and returns it in
If it converts the message too, then maybe the name "MessageScanner" isn't
appropriate?  "Scanning" suggests a pass over the message to check it, without
modifying the message.  "MessageConverter" or "MessageTranslator" might be more
appropriate?

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

New PS didn't upload right. Can you try again, please?

https://codereview.chromium.org/53123002/diff/20001/components/nacl/loader/na...
File components/nacl/loader/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/53123002/diff/20001/components/nacl/loader/na...
components/nacl/loader/nacl_ipc_adapter.cc:460: if (new_msg && !handles.empty())
On 2013/10/30 19:01:34, bbudge1 wrote:
> I wonder if we really need to check 'handles' after we've gone to the trouble
of
> rewriting the message.
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.

[bbudge, 2013-10-30 21:02:48]

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.cc:26: struct ScanOutput {
On 2013/10/30 20:05:31, Mark Seaborn wrote:
> Can you add a note in the commit message to say that you're factoring out some
> function arguments into this struct?  Might this factoring out be better done
as
> a separate change?

Done. (Added comment to commit message.)

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.cc:30: Handles* handles;
On 2013/10/30 20:05:31, Mark Seaborn wrote:
> Add a comment?  Is this an input or output of scanning?

Done.

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.cc:31: int handle_index;
On 2013/10/30 20:05:31, Mark Seaborn wrote:
> Add a comment for this field?  Is it an index into 'handles'?  Is it different
> from handles->size()?

Done.

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.cc:32: IPC::Message* msg;
On 2013/10/30 20:05:31, Mark Seaborn wrote:
> Add comments for 'msg' and 'audit_msg'?

Done.

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.cc:33: IPC::Message* audit_msg;
On 2013/10/30 20:05:31, Mark Seaborn wrote:
> I don't see any users of this field.

This is the whole point of the CL. But as it's unused, I'll remove it and the
overload that catches nested messages for a future CL.

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.cc:79: // NOTE: We only catch handles from
host->NaCl. The only kind of
On 2013/10/30 20:05:31, Mark Seaborn wrote:
> "catch" seems more vague than "translate" (the original).

Done.

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
File ppapi/proxy/nacl_message_scanner.h (right):

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/10/30 20:05:31, Mark Seaborn wrote:
> Can you use "git cl upload --similarity=..." to get Git to treat this file as
> moved, perhaps?

Ah, thanks, I'll give that a try next upload.

https://codereview.chromium.org/53123002/diff/20001/ppapi/proxy/nacl_message_...
ppapi/proxy/nacl_message_scanner.h:31: // Translates the message if necessary
(e.g. on Windows) and returns it in
On 2013/10/30 20:05:31, Mark Seaborn wrote:
> If it converts the message too, then maybe the name "MessageScanner" isn't
> appropriate?  "Scanning" suggests a pass over the message to check it, without
> modifying the message.  "MessageConverter" or "MessageTranslator" might be
more
> appropriate?

Renaming this has been tricky, because the class does several related things.
Currently it cracks the messages and picks out handles, conditionally rewriting
the message. But the whole point of this renaming is to support the auditing of
certain PPAPI messages. Some names I've considered:

NaClMessageAuditor
NaClMessageMonitor
NaClMessageTranslator
NaClMessageScanner

The reason I settled on the last one is that this class is a bunch of stuff hung
off the core message cracking machinery. But I think NaClMessageCracker would be
an odd name, so settled for NaClMessageScanner.

[bbudge, 2013-10-30 22:29:27]

Fixed a long-standing issue with rewriting resource replies on Windows. PTAL.

[bbudge, 2013-10-31 00:55:28]

Patchset #5 is to fix failing Windows tests. I think there must be another case
where we create bad translated messages when there are no handles.

David, any ideas?

[dmichael (off chromium), 2013-10-31 18:21:33]

Looks like a nice improvement to me, Thanks!

Most of my comments are about improving documentation. O/w lgtm

https://codereview.chromium.org/53123002/diff/440001/components/nacl/loader/n...
File components/nacl/loader/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/53123002/diff/440001/components/nacl/loader/n...
components/nacl/loader/nacl_ipc_adapter.cc:534: return false;  // TODO(brettw)
clean up handles here when we add support!
unrelated nit: I wonder if we still need this comment? Or if maybe it should say
something more like:
"We should clean up handles here, if we ever add support for passing handles out
of the plugin."

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:26: struct ScanOutput {
nit: This name is ambiguous; it also sounds like an action ("Scan the output"),
when it's really the result of the scan. Maybe ResultOfScan, ScanningResult, or
something would be clearer?

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:31: scoped_ptr<IPC::Message> msg;  // Output
message, possibly NULL.
I think it would be nice to be super-duper clear about the NULL thing. I.e.,
// Output message. This could be NULL, so all ScanParam overloads must
// check for NULL before writing to |msg|. In some cases, a ScanParam
// overload may set |msg| to NULL to indicate that there are no
// parameters that need conversion. (See ResourceMessageReplyParams
// overload).

Or something. It's pretty tricky, so may be worthy of a long comment.

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:83: // rewritten.
Maybe note that this only works because ResourceMessageReply *only* carries
handles inside ResourceMessageReplyParams. Otherwise, we might have to collect
handles from other params.

Also...  as we start looking at other parameters (e.g. for quota), the comment
might need to be more general. We need to be sure *no* parameters have to be
changed in flight in order for this optimization to work. I think that's only
handles for the foreseeable future, though, so maybe I'm being paranoid :-)

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:85: output->msg.reset(NULL);
you could return early here, I think. Doesn't appear to matter, but might make
the intent clearer.

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:162: output->msg.get()->set_reply();
Why use get() here?

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:164: output->msg.get()->WriteInt(id);
ditto?

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:218: // received in a POSIX environment (in
this case, NaCl).
"must be received in"
->
"will be in a format compatible with"

or something. I found the comment a little confusing as written, but I'm easily
confused :)

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:263: if (!output.handles.empty()) {
We probably fall in here in the PPPMessaging_HandleMessage case. Vars only
*sometimes* contain a handle :-p

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
File ppapi/proxy/nacl_message_scanner.h (right):

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.h:32: // |new_msg_ptr|.
Shouldn't we point out that new_msg_ptr is left NULL if ScanMessage didn't do
any work?

[bbudge, 2013-11-01 00:34:45]

https://codereview.chromium.org/53123002/diff/440001/components/nacl/loader/n...
File components/nacl/loader/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/53123002/diff/440001/components/nacl/loader/n...
components/nacl/loader/nacl_ipc_adapter.cc:534: return false;  // TODO(brettw)
clean up handles here when we add support!
On 2013/10/31 18:21:33, dmichael wrote:
> unrelated nit: I wonder if we still need this comment? Or if maybe it should
say
> something more like:
> "We should clean up handles here, if we ever add support for passing handles
out
> of the plugin."

Done.

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:26: struct ScanOutput {
On 2013/10/31 18:21:33, dmichael wrote:
> nit: This name is ambiguous; it also sounds like an action ("Scan the
output"),
> when it's really the result of the scan. Maybe ResultOfScan, ScanningResult,
or
> something would be clearer?

I thought about ScanningContext or even just Context, but since this is mostly
for output, I think ScanningResults is best for now. Done.

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:31: scoped_ptr<IPC::Message> msg;  // Output
message, possibly NULL.
On 2013/10/31 18:21:33, dmichael wrote:
> I think it would be nice to be super-duper clear about the NULL thing. I.e.,
> // Output message. This could be NULL, so all ScanParam overloads must
> // check for NULL before writing to |msg|. In some cases, a ScanParam
> // overload may set |msg| to NULL to indicate that there are no
> // parameters that need conversion. (See ResourceMessageReplyParams
> // overload).
> 
> Or something. It's pretty tricky, so may be worthy of a long comment.

Done. Also renamed the field to make it a little clearer.

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:66: return;
I removed the early out. If another SerializedVar param does contain handles,
the rewritten message will be missing data.

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:83: // rewritten.
On 2013/10/31 18:21:33, dmichael wrote:
> Maybe note that this only works because ResourceMessageReply *only* carries
> handles inside ResourceMessageReplyParams. Otherwise, we might have to collect
> handles from other params.
> 
> Also...  as we start looking at other parameters (e.g. for quota), the comment
> might need to be more general. We need to be sure *no* parameters have to be
> changed in flight in order for this optimization to work. I think that's only
> handles for the foreseeable future, though, so maybe I'm being paranoid :-)

Done.

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:85: output->msg.reset(NULL);
On 2013/10/31 18:21:33, dmichael wrote:
> you could return early here, I think. Doesn't appear to matter, but might make
> the intent clearer.

Done.

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:162: output->msg.get()->set_reply();
On 2013/10/31 18:21:33, dmichael wrote:
> Why use get() here?

Done.

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:164: output->msg.get()->WriteInt(id);
On 2013/10/31 18:21:33, dmichael wrote:
> ditto?

Done.

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:218: // received in a POSIX environment (in
this case, NaCl).
On 2013/10/31 18:21:33, dmichael wrote:
> "must be received in"
> ->
> "will be in a format compatible with"
> 
> or something. I found the comment a little confusing as written, but I'm
easily
> confused :)

I completely rewrote this. The comment is now above the method definition.

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:263: if (!output.handles.empty()) {
On 2013/10/31 18:21:33, dmichael wrote:
> We probably fall in here in the PPPMessaging_HandleMessage case. Vars only
> *sometimes* contain a handle :-p

I think the "early out" in the SerializedVar overload may be the cause of this.
Apparently this works OK now but if we someday need to rewrite messages with
multiple SerializedVars, the early out won't work.

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
File ppapi/proxy/nacl_message_scanner.h (right):

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.h:32: // |new_msg_ptr|.
On 2013/10/31 18:21:33, dmichael wrote:
> Shouldn't we point out that new_msg_ptr is left NULL if ScanMessage didn't do
> any work?

Done.

[dmichael (off chromium), 2013-11-01 19:35:16]

moreso lgtm

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/53123002/diff/440001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:66: return;
On 2013/11/01 00:34:45, bbudge1 wrote:
> I removed the early out. If another SerializedVar param does contain handles,
> the rewritten message will be missing data.
Agreed. I missed that on my last review. I think we'd be okay in practice right
now, but it will bite us eventually if we break early here.

https://codereview.chromium.org/53123002/diff/680001/ppapi/proxy/nacl_message...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/53123002/diff/680001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:240: // determine that there are no handles,
cancel the rewriting.
s/cancel/we will cancel
?

[Mark Seaborn, 2013-11-01 20:23:18]

On 2013/10/30 22:29:27, bbudge1 wrote:
> Fixed a long-standing issue with rewriting resource replies on Windows. PTAL.

> Fixes a problem on Windows where we needlessly copy resource reply messages
> that turn out to *not* contain any handles.

I don't understand this part.  The code change ("if (!output.handles.empty())")
looks like it's just an optimisation, but your description above makes this
sound like a bug fix.  If it's a bug fix, it should really be separate from
moving and refactoring the code.  Does the problem only surface with the
moving+refactoring applied?

[bbudge, 2013-11-01 22:10:28]

On 2013/11/01 20:23:18, Mark Seaborn wrote:
> On 2013/10/30 22:29:27, bbudge1 wrote:
> > Fixed a long-standing issue with rewriting resource replies on Windows.
PTAL.
> 
> > Fixes a problem on Windows where we needlessly copy resource reply messages
> > that turn out to *not* contain any handles.
> 
> I don't understand this part.  The code change ("if
(!output.handles.empty())")
> looks like it's just an optimisation, but your description above makes this
> sound like a bug fix.  If it's a bug fix, it should really be separate from
> moving and refactoring the code.  Does the problem only surface with the
> moving+refactoring applied?

This check used to be in NaClIPCAdapter, where we decide whether to use the new
message. It was needed because when there are no handles, the rewritten message
may be incomplete. This was because we had an early-out in the SerializedVar
overload of ConvertHandlesInParam. This is incorrect in general, so the check
was needed.

I removed the early out from the overload but I think we should still use the
handles.empty() test to not return the new message. I'm not 100% sure we
correctly scan the entire message and all the nesting possibilities so would
prefer to be conservative.

I can split this out into a separate change if you prefer. WDYT?

[bbudge, 2013-11-01 23:27:28]

I removed the SerializedVar fix for a later CL and added a TODO.

Reworked some comments. PTAL

https://codereview.chromium.org/53123002/diff/680001/ppapi/proxy/nacl_message...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/53123002/diff/680001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:240: // determine that there are no handles,
cancel the rewriting.
On 2013/11/01 19:35:16, dmichael wrote:
> s/cancel/we will cancel
> ?

Done.

[Mark Seaborn, 2013-11-02 00:57:44]

> I removed the SerializedVar fix for a later CL and added a TODO.

In the commit message you still have "Fixes a problem on Windows where we
needlessly copy resource reply messages that turn out to *not* contain any
handles".

I thought that was the part you were going to remove and do as a separate
change?  LGTM if you do that separately, with commit message updated.

https://codereview.chromium.org/53123002/diff/780001/components/nacl/loader/n...
File components/nacl/loader/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/53123002/diff/780001/components/nacl/loader/n...
components/nacl/loader/nacl_ipc_adapter.cc:460: if (new_msg)
Wasn't this change part of the changes to the early-out handling that you were
going to move to a separate change?

https://codereview.chromium.org/53123002/diff/780001/ppapi/proxy/nacl_message...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/53123002/diff/780001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:32: int handle_index;
Can you expand on the comment to explain how this relates to handles.size()?  It
looks like handle_index <= handles.size().

https://codereview.chromium.org/53123002/diff/780001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:36: // that need conversion. (See the
ResourceMessageReplyParams overload).
Nit: put '.' inside ()s

https://codereview.chromium.org/53123002/diff/780001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:96: return;
Is this another early-out that you added in a more recent version?

https://codereview.chromium.org/53123002/diff/780001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:281: if (!results.handles.empty()) {
Wasn't this change also part of the changes to the early-out handling that you
were going to move to a separate change?

[bbudge, 2013-11-02 01:18:49]

https://codereview.chromium.org/53123002/diff/780001/components/nacl/loader/n...
File components/nacl/loader/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/53123002/diff/780001/components/nacl/loader/n...
components/nacl/loader/nacl_ipc_adapter.cc:460: if (new_msg)
On 2013/11/02 00:57:44, Mark Seaborn wrote:
> Wasn't this change part of the changes to the early-out handling that you were
> going to move to a separate change?

No, this protects us from badly formed messages in the case where there are no
handles. It doesn't belong here, so I moved it into the message scanner, which
should decide whether the message should be returned.

https://codereview.chromium.org/53123002/diff/780001/ppapi/proxy/nacl_message...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/53123002/diff/780001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:32: int handle_index;
On 2013/11/02 00:57:44, Mark Seaborn wrote:
> Can you expand on the comment to explain how this relates to handles.size()? 
It
> looks like handle_index <= handles.size().

Done.

https://codereview.chromium.org/53123002/diff/780001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:36: // that need conversion. (See the
ResourceMessageReplyParams overload).
On 2013/11/02 00:57:44, Mark Seaborn wrote:
> Nit: put '.' inside ()s

I removed this part of the comment since without the early out optimization it
doesn't really apply.

https://codereview.chromium.org/53123002/diff/780001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:96: return;
On 2013/11/02 00:57:44, Mark Seaborn wrote:
> Is this another early-out that you added in a more recent version?

This is the legitimate early out. I'll add it back in a follow on CL.

https://codereview.chromium.org/53123002/diff/780001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:281: if (!results.handles.empty()) {
On 2013/11/02 00:57:44, Mark Seaborn wrote:
> Wasn't this change also part of the changes to the early-out handling that you
> were going to move to a separate change?

This corresponds to the check in NaClIPCAdapter that I removed.

[commit-bot: I haz the power, 2013-11-02 01:21:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/53123002/870001

[commit-bot: I haz the power, 2013-11-02 04:07:17]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-02 05:09:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/53123002/870001

[commit-bot: I haz the power, 2013-11-02 13:38:33]

Change committed as 232603