Validate MessageHeader before using

Validate MessageHeader before using

On validation error, mimic a MessageReceiver returning false. Make that trigger the error state of Connector.

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

[darin (slow to review), 2014-04-25 17:47:06]

This is not all ready, but I'd like to get your take on this approach to
validation testing.

What you see here is just some validation code and testing of the message
header. I'd like feedback on the framework. One of my goals is to make it easy
to add more tests without having to update the .cc file or re-compile.

I plan to also add a variant that tests against a fixed mojom (say
TestInterface). For that, I will string together MessageHeaderValidator and the
generated TestInterfaceStub class. I will implement TestInterface that just logs
the received methods and parameters. Tests that work this way will be prefixed
with "validate_payload_" in the tests/data directory.

Note: I'm trying to do all of this testing without involving Connector or
Router. My hope is that this will be sufficient since those classes don't do any
message parsing, etc.

[viettrungluu, 2014-04-25 21:55:58]

I like the general approach. I wish that the input/expectation files were a bit
more expressive though. (Wish list: A bit more structure, handle comments,
single file for both input and expectations....)

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
File mojo/common/test/test_support_impl.cc (right):

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
mojo/common/test/test_support_impl.cc:6: 
#include "base/files/file_path.h"

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
mojo/common/test/test_support_impl.cc:6: 
#include <stdlib.h>
#include <string.h>

(for calloc and strdup)

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
mojo/common/test/test_support_impl.cc:48: return
base::OpenFile(ResolveSourceRootRelativePath(relative_path), "rt");
Do we really want "rt"?

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
mojo/common/test/test_support_impl.cc:62: rv[names.size()] = NULL;
calloc actually zeros memory.

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
File mojo/common/test/test_support_impl.h (right):

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
mojo/common/test/test_support_impl.h:7: 
nit: #include <stdio.h>

https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/message_header_validator.cc (right):

https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/message_header_validator.cc:23: if
(header->num_fields == 2 &&
Probably this should be structured as:

if (header->num_fields == 2) {
  if (... != ...)
    return false;
} else if (header->num_fields == 3) {
  ...
} else {  // |header->num_fields > 3|.
  if (header->num_bytes < sizeof(internal::MessageHeaderWithRequestID))
    return false;
}

https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/message_header_validator.cc:31: if (header->flags &
Do we want to disallow future flags?

https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/message_header_validator.cc:40: 
Also check that "expects response" and "is response" aren't both set?

https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/message_header_validator.h (right):

https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/message_header_validator.h:12:
MessageHeaderValidator(MessageReceiver* next);
explicit

[yzshen1, 2014-04-27 22:49:38]

drive-by comments:

https://codereview.chromium.org/229683005/diff/60001/mojo/public/c/test_suppo...
File mojo/public/c/test_support/test_support.h (right):

https://codereview.chromium.org/229683005/diff/60001/mojo/public/c/test_suppo...
mojo/public/c/test_support/test_support.h:29: // heap-allocated strings. Each
much be free'd separately.
nit: much -> must

https://codereview.chromium.org/229683005/diff/60001/mojo/public/c/test_suppo...
mojo/public/c/test_support/test_support.h:33: //   char** rv = (char**)
calloc(N, sizeof(char*));
nit: should be N+1.

https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/tests/validation_unittest.cc (right):

https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/tests/validation_unittest.cc:80: EXPECT_EQ(expected,
result);
Does it make sense to also output |root| to help people easily figure out which
test is failing?

[darin (slow to review), 2014-04-29 06:31:44]

> I like the general approach. I wish that the input/expectation
> files were a bit more expressive though. (Wish list: A bit more
> structure, handle comments, single file for both input and
> expectations....)

Good ideas. I'm not sure I get the single file suggestion. Maybe you should
describe how you think it would be useful. The other ideas seem great.

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
File mojo/common/test/test_support_impl.cc (right):

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
mojo/common/test/test_support_impl.cc:6: 
On 2014/04/25 21:55:59, viettrungluu wrote:
> #include "base/files/file_path.h"

Hmm, I generally follow an approach of only including what is needed, rather
than including everything that is used. The former helps avoid including too
much, but I can see how it might be annoying to not include what you use if
someone ends up removing an include from say file_util.h, etc.

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
mojo/common/test/test_support_impl.cc:48: return
base::OpenFile(ResolveSourceRootRelativePath(relative_path), "rt");
On 2014/04/25 21:55:59, viettrungluu wrote:
> Do we really want "rt"?

Oh, hmm... we can leave off the "t". That is just force of habit when dealing
with text files on Windows. I don't think it is relevant in this case though.

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
mojo/common/test/test_support_impl.cc:62: rv[names.size()] = NULL;
On 2014/04/25 21:55:59, viettrungluu wrote:
> calloc actually zeros memory.

Yeah. Perhaps I'll draw attention to the extra NULL entry with a comment.

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
File mojo/common/test/test_support_impl.h (right):

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
mojo/common/test/test_support_impl.h:7: 
On 2014/04/25 21:55:59, viettrungluu wrote:
> nit: #include <stdio.h>

I think we can count on stdio.h from test_support_private.h. We are not
declaring anything here that it isn't guaranteed to also declare.

https://codereview.chromium.org/229683005/diff/60001/mojo/public/c/test_suppo...
File mojo/public/c/test_support/test_support.h (right):

https://codereview.chromium.org/229683005/diff/60001/mojo/public/c/test_suppo...
mojo/public/c/test_support/test_support.h:29: // heap-allocated strings. Each
much be free'd separately.
On 2014/04/27 22:49:38, yzshen1 wrote:
> nit: much -> must

Done.

https://codereview.chromium.org/229683005/diff/60001/mojo/public/c/test_suppo...
mojo/public/c/test_support/test_support.h:33: //   char** rv = (char**)
calloc(N, sizeof(char*));
On 2014/04/27 22:49:38, yzshen1 wrote:
> nit: should be N+1.

Done.

https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/message_header_validator.cc (right):

https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/message_header_validator.cc:23: if
(header->num_fields == 2 &&
On 2014/04/25 21:55:59, viettrungluu wrote:
> Probably this should be structured as:
> 
> if (header->num_fields == 2) {
>   if (... != ...)
>     return false;
> } else if (header->num_fields == 3) {
>   ...
> } else {  // |header->num_fields > 3|.
>   if (header->num_bytes < sizeof(internal::MessageHeaderWithRequestID))
>     return false;
> }

Done.

https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/message_header_validator.cc:31: if (header->flags &
On 2014/04/25 21:55:59, viettrungluu wrote:
> Do we want to disallow future flags?

No, I don't think we do. Revising code.

https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/message_header_validator.cc:40: 
On 2014/04/25 21:55:59, viettrungluu wrote:
> Also check that "expects response" and "is response" aren't both set?

Done.

https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/tests/validation_unittest.cc (right):

https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/tests/validation_unittest.cc:80: EXPECT_EQ(expected,
result);
On 2014/04/27 22:49:38, yzshen1 wrote:
> Does it make sense to also output |root| to help people easily figure out
which
> test is failing?

Done.

[viettrungluu, 2014-04-29 20:35:05]

On 2014/04/29 06:31:44, darin wrote:
> > I like the general approach. I wish that the input/expectation
> > files were a bit more expressive though. (Wish list: A bit more
> > structure, handle comments, single file for both input and
> > expectations....)
> 
> Good ideas. I'm not sure I get the single file suggestion. Maybe you should
> describe how you think it would be useful.

It seems to me that having separate files is mainly useful if either:
- the input file is some generic format, or generated (rather than
hand-crafted);
- the output file is generated (and something you might just want to
automatically update a golden file for).

Neither seems to apply in this case. Especially with a richer input
specification format (designed to make hand-crafting test inputs more
palatable), it'd make sense to have both the input and expectations in the same
place, and suitably annotated.

Here, we seem to be ending up with a potentially huge proliferation of files,
with the main annotation mechanism being the filename.

> The other ideas seem great.
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
> File mojo/common/test/test_support_impl.cc (right):
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
> mojo/common/test/test_support_impl.cc:6: 
> On 2014/04/25 21:55:59, viettrungluu wrote:
> > #include "base/files/file_path.h"
> 
> Hmm, I generally follow an approach of only including what is needed, rather
> than including everything that is used. The former helps avoid including too
> much, but I can see how it might be annoying to not include what you use if
> someone ends up removing an include from say file_util.h, etc.
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
> mojo/common/test/test_support_impl.cc:48: return
> base::OpenFile(ResolveSourceRootRelativePath(relative_path), "rt");
> On 2014/04/25 21:55:59, viettrungluu wrote:
> > Do we really want "rt"?
> 
> Oh, hmm... we can leave off the "t". That is just force of habit when dealing
> with text files on Windows. I don't think it is relevant in this case though.
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
> mojo/common/test/test_support_impl.cc:62: rv[names.size()] = NULL;
> On 2014/04/25 21:55:59, viettrungluu wrote:
> > calloc actually zeros memory.
> 
> Yeah. Perhaps I'll draw attention to the extra NULL entry with a comment.
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
> File mojo/common/test/test_support_impl.h (right):
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
> mojo/common/test/test_support_impl.h:7: 
> On 2014/04/25 21:55:59, viettrungluu wrote:
> > nit: #include <stdio.h>
> 
> I think we can count on stdio.h from test_support_private.h. We are not
> declaring anything here that it isn't guaranteed to also declare.
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/c/test_suppo...
> File mojo/public/c/test_support/test_support.h (right):
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/c/test_suppo...
> mojo/public/c/test_support/test_support.h:29: // heap-allocated strings. Each
> much be free'd separately.
> On 2014/04/27 22:49:38, yzshen1 wrote:
> > nit: much -> must
> 
> Done.
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/c/test_suppo...
> mojo/public/c/test_support/test_support.h:33: //   char** rv = (char**)
> calloc(N, sizeof(char*));
> On 2014/04/27 22:49:38, yzshen1 wrote:
> > nit: should be N+1.
> 
> Done.
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
> File mojo/public/cpp/bindings/lib/message_header_validator.cc (right):
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
> mojo/public/cpp/bindings/lib/message_header_validator.cc:23: if
> (header->num_fields == 2 &&
> On 2014/04/25 21:55:59, viettrungluu wrote:
> > Probably this should be structured as:
> > 
> > if (header->num_fields == 2) {
> >   if (... != ...)
> >     return false;
> > } else if (header->num_fields == 3) {
> >   ...
> > } else {  // |header->num_fields > 3|.
> >   if (header->num_bytes < sizeof(internal::MessageHeaderWithRequestID))
> >     return false;
> > }
> 
> Done.
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
> mojo/public/cpp/bindings/lib/message_header_validator.cc:31: if (header->flags
&
> On 2014/04/25 21:55:59, viettrungluu wrote:
> > Do we want to disallow future flags?
> 
> No, I don't think we do. Revising code.
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
> mojo/public/cpp/bindings/lib/message_header_validator.cc:40: 
> On 2014/04/25 21:55:59, viettrungluu wrote:
> > Also check that "expects response" and "is response" aren't both set?
> 
> Done.
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
> File mojo/public/cpp/bindings/tests/validation_unittest.cc (right):
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
> mojo/public/cpp/bindings/tests/validation_unittest.cc:80: EXPECT_EQ(expected,
> result);
> On 2014/04/27 22:49:38, yzshen1 wrote:
> > Does it make sense to also output |root| to help people easily figure out
> which
> > test is failing?
> 
> Done.

[viettrungluu, 2014-04-29 20:42:01]

On 2014/04/29 20:35:05, viettrungluu wrote:
> On 2014/04/29 06:31:44, darin wrote:
> > > I like the general approach. I wish that the input/expectation
> > > files were a bit more expressive though. (Wish list: A bit more
> > > structure, handle comments, single file for both input and
> > > expectations....)
> > 
> > Good ideas. I'm not sure I get the single file suggestion. Maybe you should
> > describe how you think it would be useful.
> 
> It seems to me that having separate files is mainly useful if either:
> - the input file is some generic format, or generated (rather than
> hand-crafted);
> - the output file is generated (and something you might just want to
> automatically update a golden file for).
> 
> Neither seems to apply in this case.

Looking at this some more, I can see advantages to both approaches. (I guess
having separate expectation files means that it's easier to add more data to the
output, and doing a mass updating of the expectations.) Hrm.

> Especially with a richer input
> specification format (designed to make hand-crafting test inputs more
> palatable), it'd make sense to have both the input and expectations in the
same
> place, and suitably annotated.
> 
> Here, we seem to be ending up with a potentially huge proliferation of files,
> with the main annotation mechanism being the filename.
> 
> > The other ideas seem great.
> > 
> >
>
https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
> > File mojo/common/test/test_support_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
> > mojo/common/test/test_support_impl.cc:6: 
> > On 2014/04/25 21:55:59, viettrungluu wrote:
> > > #include "base/files/file_path.h"
> > 
> > Hmm, I generally follow an approach of only including what is needed, rather
> > than including everything that is used. The former helps avoid including too
> > much, but I can see how it might be annoying to not include what you use if
> > someone ends up removing an include from say file_util.h, etc.
> > 
> >
>
https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
> > mojo/common/test/test_support_impl.cc:48: return
> > base::OpenFile(ResolveSourceRootRelativePath(relative_path), "rt");
> > On 2014/04/25 21:55:59, viettrungluu wrote:
> > > Do we really want "rt"?
> > 
> > Oh, hmm... we can leave off the "t". That is just force of habit when
dealing
> > with text files on Windows. I don't think it is relevant in this case
though.
> > 
> >
>
https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
> > mojo/common/test/test_support_impl.cc:62: rv[names.size()] = NULL;
> > On 2014/04/25 21:55:59, viettrungluu wrote:
> > > calloc actually zeros memory.
> > 
> > Yeah. Perhaps I'll draw attention to the extra NULL entry with a comment.
> > 
> >
>
https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
> > File mojo/common/test/test_support_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
> > mojo/common/test/test_support_impl.h:7: 
> > On 2014/04/25 21:55:59, viettrungluu wrote:
> > > nit: #include <stdio.h>
> > 
> > I think we can count on stdio.h from test_support_private.h. We are not
> > declaring anything here that it isn't guaranteed to also declare.
> > 
> >
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/c/test_suppo...
> > File mojo/public/c/test_support/test_support.h (right):
> > 
> >
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/c/test_suppo...
> > mojo/public/c/test_support/test_support.h:29: // heap-allocated strings.
Each
> > much be free'd separately.
> > On 2014/04/27 22:49:38, yzshen1 wrote:
> > > nit: much -> must
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/c/test_suppo...
> > mojo/public/c/test_support/test_support.h:33: //   char** rv = (char**)
> > calloc(N, sizeof(char*));
> > On 2014/04/27 22:49:38, yzshen1 wrote:
> > > nit: should be N+1.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
> > File mojo/public/cpp/bindings/lib/message_header_validator.cc (right):
> > 
> >
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
> > mojo/public/cpp/bindings/lib/message_header_validator.cc:23: if
> > (header->num_fields == 2 &&
> > On 2014/04/25 21:55:59, viettrungluu wrote:
> > > Probably this should be structured as:
> > > 
> > > if (header->num_fields == 2) {
> > >   if (... != ...)
> > >     return false;
> > > } else if (header->num_fields == 3) {
> > >   ...
> > > } else {  // |header->num_fields > 3|.
> > >   if (header->num_bytes < sizeof(internal::MessageHeaderWithRequestID))
> > >     return false;
> > > }
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
> > mojo/public/cpp/bindings/lib/message_header_validator.cc:31: if
(header->flags
> &
> > On 2014/04/25 21:55:59, viettrungluu wrote:
> > > Do we want to disallow future flags?
> > 
> > No, I don't think we do. Revising code.
> > 
> >
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
> > mojo/public/cpp/bindings/lib/message_header_validator.cc:40: 
> > On 2014/04/25 21:55:59, viettrungluu wrote:
> > > Also check that "expects response" and "is response" aren't both set?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
> > File mojo/public/cpp/bindings/tests/validation_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/229683005/diff/60001/mojo/public/cpp/bindings...
> > mojo/public/cpp/bindings/tests/validation_unittest.cc:80:
EXPECT_EQ(expected,
> > result);
> > On 2014/04/27 22:49:38, yzshen1 wrote:
> > > Does it make sense to also output |root| to help people easily figure out
> > which
> > > test is failing?
> > 
> > Done.

[viettrungluu, 2014-04-29 20:55:00]

LGTM with nits, I guess.

I guess we can ponder the test file format some more.

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
File mojo/common/test/test_support_impl.cc (right):

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
mojo/common/test/test_support_impl.cc:6: 
On 2014/04/29 06:31:45, darin wrote:
> On 2014/04/25 21:55:59, viettrungluu wrote:
> > #include "base/files/file_path.h"
> 
> Hmm, I generally follow an approach of only including what is needed, rather
> than including everything that is used. The former helps avoid including too
> much, but I can see how it might be annoying to not include what you use if
> someone ends up removing an include from say file_util.h, etc.

In this case, file_util.h really should only forward declare FilePath....

(In general, I try to include the header for a type T if either nothing I
use/include requires that T be defined[*] or if I have a use of T that's
independent of uses that imply that T be already be defined.)

[*] Many includes in headers are incidental to the implementation (e.g., so that
you can put a |Foo foo_;| member) and not implied by the interface. I'd argue
that it's bad to rely on this.

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
mojo/common/test/test_support_impl.cc:48: return
base::OpenFile(ResolveSourceRootRelativePath(relative_path), "rt");
On 2014/04/29 06:31:45, darin wrote:
> On 2014/04/25 21:55:59, viettrungluu wrote:
> > Do we really want "rt"?
> 
> Oh, hmm... we can leave off the "t". That is just force of habit when dealing
> with text files on Windows. I don't think it is relevant in this case though.

(Makes me think that we should do "rb", actually!)

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
File mojo/common/test/test_support_impl.h (right):

https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
mojo/common/test/test_support_impl.h:7: 
On 2014/04/29 06:31:45, darin wrote:
> On 2014/04/25 21:55:59, viettrungluu wrote:
> > nit: #include <stdio.h>
> 
> I think we can count on stdio.h from test_support_private.h. We are not
> declaring anything here that it isn't guaranteed to also declare.

True.

https://codereview.chromium.org/229683005/diff/80001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/tests/validation_unittest.cc (right):

https://codereview.chromium.org/229683005/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/tests/validation_unittest.cc:24: size_t root_length =
names[i].size() - suffix.size();
What if names[i].size() < suffix.size()? (I guess it's okay, if not clearly so.)

https://codereview.chromium.org/229683005/diff/80001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/tests/validation_unittest.cc:25: if
(names[i].find(prefix) == 0 &&
Probably using:

names[i].size >= suffix.size() &&
names[i].substr(0, prefix.size()) == prefix &&
names[i].substr(names[i].size - suffix.size()) == suffix

is more self-evidently correct.

[darin (slow to review), 2014-04-29 22:53:11]

On 2014/04/29 20:55:00, viettrungluu wrote:
> LGTM with nits, I guess.
> 
> I guess we can ponder the test file format some more.

Yeah, I can imagine other choices here with varying pros and cons.
The current setup has the benefit of being fairly simple.


>
https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
> File mojo/common/test/test_support_impl.cc (right):
> 
>
https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
> mojo/common/test/test_support_impl.cc:6: 
> On 2014/04/29 06:31:45, darin wrote:
> > On 2014/04/25 21:55:59, viettrungluu wrote:
> > > #include "base/files/file_path.h"
> > 
> > Hmm, I generally follow an approach of only including what is needed, rather
> > than including everything that is used. The former helps avoid including too
> > much, but I can see how it might be annoying to not include what you use if
> > someone ends up removing an include from say file_util.h, etc.
> 
> In this case, file_util.h really should only forward declare FilePath....
> 
> (In general, I try to include the header for a type T if either nothing I
> use/include requires that T be defined[*] or if I have a use of T that's
> independent of uses that imply that T be already be defined.)
> 
> [*] Many includes in headers are incidental to the implementation (e.g., so
that
> you can put a |Foo foo_;| member) and not implied by the interface. I'd argue
> that it's bad to rely on this.

Good point. I like that line approach.


>
https://codereview.chromium.org/229683005/diff/60001/mojo/common/test/test_su...
> mojo/common/test/test_support_impl.cc:48: return
> base::OpenFile(ResolveSourceRootRelativePath(relative_path), "rt");
> On 2014/04/29 06:31:45, darin wrote:
> > On 2014/04/25 21:55:59, viettrungluu wrote:
> > > Do we really want "rt"?
> > 
> > Oh, hmm... we can leave off the "t". That is just force of habit when
dealing
> > with text files on Windows. I don't think it is relevant in this case
though.
> 
> (Makes me think that we should do "rb", actually!)

Yeah, changed.


>
https://codereview.chromium.org/229683005/diff/80001/mojo/public/cpp/bindings...
> File mojo/public/cpp/bindings/tests/validation_unittest.cc (right):
> 
>
https://codereview.chromium.org/229683005/diff/80001/mojo/public/cpp/bindings...
> mojo/public/cpp/bindings/tests/validation_unittest.cc:24: size_t root_length =
> names[i].size() - suffix.size();
> What if names[i].size() < suffix.size()? (I guess it's okay, if not clearly
so.)
> 
>
https://codereview.chromium.org/229683005/diff/80001/mojo/public/cpp/bindings...
> mojo/public/cpp/bindings/tests/validation_unittest.cc:25: if
> (names[i].find(prefix) == 0 &&
> Probably using:
> 
> names[i].size >= suffix.size() &&
> names[i].substr(0, prefix.size()) == prefix &&
> names[i].substr(names[i].size - suffix.size()) == suffix
> 
> is more self-evidently correct.

Good idea.

Thanks!

[darin (slow to review), 2014-04-30 03:50:21]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-04-30 03:51:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/229683005/100001

[commit-bot: I haz the power, 2014-04-30 05:17:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 05:17:01]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium

[darin (slow to review), 2014-05-01 06:06:29]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-05-01 06:06:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/229683005/170001

[commit-bot: I haz the power, 2014-05-01 06:09:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-01 06:09:29]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium

[darin (slow to review), 2014-05-01 15:45:26]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-05-01 15:46:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/229683005/170001

[commit-bot: I haz the power, 2014-05-01 18:24:50]

Change committed as 267588

[Tom Sepez, 2014-05-05 19:10:05]

On 2014/04/29 20:55:00, viettrungluu wrote:
> LGTM with nits, I guess.
> 
> I guess we can ponder the test file format some more.
> 
Drive-by:  In the historical past, I've sometimes selected a format for these
kinds of tests such that the input and the output format are the same, and human
readable.  If you can ingest what you've spewed out, then some things get
easier.

[darin (slow to review), 2014-05-06 03:24:17]

I think that's worth pondering, especially as we start to develop tests for
validating message payloads. Yuzhu plans to pick up this work. Best to chat
with him about the details.

-Darin


On Mon, May 5, 2014 at 12:10 PM, <tsepez@chromium.org> wrote:

> On 2014/04/29 20:55:00, viettrungluu wrote:
>
>> LGTM with nits, I guess.
>>
>
>  I guess we can ponder the test file format some more.
>>
>
> Drive-by:  In the historical past, I've sometimes selected a format for
> these
> kinds of tests such that the input and the output format are the same, and
> human
> readable.  If you can ingest what you've spewed out, then some things get
> easier.
>
> https://codereview.chromium.org/229683005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2014-05-06 03:44:40]

Yeah, I will pick up the validation work this week, and would be happy to
discuss about the in/out format.

Thanks!


On Mon, May 5, 2014 at 8:24 PM, Darin Fisher <darin@chromium.org> wrote:

> I think that's worth pondering, especially as we start to develop tests
> for validating message payloads. Yuzhu plans to pick up this work. Best to
> chat with him about the details.
>
> -Darin
>
>
> On Mon, May 5, 2014 at 12:10 PM, <tsepez@chromium.org> wrote:
>
>> On 2014/04/29 20:55:00, viettrungluu wrote:
>>
>>> LGTM with nits, I guess.
>>>
>>
>>  I guess we can ponder the test file format some more.
>>>
>>
>> Drive-by:  In the historical past, I've sometimes selected a format for
>> these
>> kinds of tests such that the input and the output format are the same,
>> and human
>> readable.  If you can ingest what you've spewed out, then some things get
>> easier.
>>
>> https://codereview.chromium.org/229683005/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.