Move mojo::internal::MessageBuilder out of the |internal| namespace.

Move mojo::internal::MessageBuilder out of the |internal| namespace.

This can be used to build Message (e.g interface messages) from the bindings.
It might be just as easy to build Messages by constructing a |Message| manually and store it in a byte buffer, but MessageBuilder helps construct flags in a safer way (e.g., Messages can't be a response *and* expect a response).

R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/3db36ca1b7ad0c8f0338235352e1bd38da31c50f

[vardhan, 2015-11-17 23:45:28]

PTAL

[viettrungluu, 2015-11-17 23:59:36]

viettrungluu@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[viettrungluu, 2015-11-17 23:59:38]

https://codereview.chromium.org/1460463003/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/message_builder.cc (right):

https://codereview.chromium.org/1460463003/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/message_builder.cc:32: header->flags = 0;
I guess Allocate() (that is, internal::Buffer::Allocate()) already
zero-initializes, so what was there before wasn't actually wrong. <shrug>

https://codereview.chromium.org/1460463003/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/message_builder.h (right):

https://codereview.chromium.org/1460463003/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/message_builder.h:17: class MessageBuilder {
Are there specific tests for MessageBuilder, MessageWithRequestIDBuilder, etc.?
(If not, you should probably add some. :P)

https://codereview.chromium.org/1460463003/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/message_builder.h:22: // TODO(vardhan): |buffer()|
is internal and only consumed by internal classes
Maybe, in light of this comment, you should put this after |message()|?

[vardhan, 2015-11-18 22:46:40]

PTAL

https://codereview.chromium.org/1460463003/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/message_builder.cc (right):

https://codereview.chromium.org/1460463003/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/message_builder.cc:32: header->flags = 0;
On 2015/11/17 23:59:38, viettrungluu wrote:
> I guess Allocate() (that is, internal::Buffer::Allocate()) already
> zero-initializes, so what was there before wasn't actually wrong. <shrug>

I think it's because we should explicitly set a default (defensive programming?)
also, it might be why header->version was already set to 0.

https://codereview.chromium.org/1460463003/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/message_builder.h (right):

https://codereview.chromium.org/1460463003/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/message_builder.h:17: class MessageBuilder {
On 2015/11/17 23:59:38, viettrungluu wrote:
> Are there specific tests for MessageBuilder, MessageWithRequestIDBuilder,
etc.?
> (If not, you should probably add some. :P)

Done.

https://codereview.chromium.org/1460463003/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/message_builder.h:22: // TODO(vardhan): |buffer()|
is internal and only consumed by internal classes
On 2015/11/17 23:59:38, viettrungluu wrote:
> Maybe, in light of this comment, you should put this after |message()|?

Done.

[viettrungluu, 2015-11-18 23:55:59]

lgtm with comment and style nits

https://codereview.chromium.org/1460463003/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/message_builder.cc (right):

https://codereview.chromium.org/1460463003/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/message_builder.cc:11: 
It's debatable if you should delete this blank line (depending on if you believe
the rule about omitting blank line applies to totally nested pairs/triples/etc.
of namespaces or to the partial case as well).

https://codereview.chromium.org/1460463003/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/message_builder.cc:13: 
So you shouldn't delete this blank line.

https://codereview.chromium.org/1460463003/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/message_builder.cc:32: header->flags = 0;
On 2015/11/18 22:46:40, vardhan wrote:
> On 2015/11/17 23:59:38, viettrungluu wrote:
> > I guess Allocate() (that is, internal::Buffer::Allocate()) already
> > zero-initializes, so what was there before wasn't actually wrong. <shrug>
> 
> I think it's because we should explicitly set a default (defensive
programming?)
> also, it might be why header->version was already set to 0.

(I wasn't actually disagreeing with setting it explicitly.)

https://codereview.chromium.org/1460463003/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/message_builder.cc (right):

https://codereview.chromium.org/1460463003/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/message_builder.cc:20: }  // namespace
And there should be a blank line before this.

https://codereview.chromium.org/1460463003/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/message_builder.h (right):

https://codereview.chromium.org/1460463003/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/message_builder.h:20: // MessageBuilder helps
initialize and frame a |mojo::Message| which does not
nit: "which" -> "that"

https://codereview.chromium.org/1460463003/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/message_builder.h:48: namespace internal {
nit:

Generally, there should be a blank line following |namespace foo {|, unless:
* The following line is just |namespace bar {|.
* You're just forward declaring something(s).

Similarly, there should be a blank line before |}  // namespace foo|, unless the
previous line was |}  // namespace bar| or the "body" consisted only of forward
declarations.

https://codereview.chromium.org/1460463003/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/message_builder.h:58: // Builds a |mojo::Message|
that is a "request" message, which expects a
nit: ", which" -> " that" (no comma)

https://codereview.chromium.org/1460463003/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/message_builder_unittest.cc (right):

https://codereview.chromium.org/1460463003/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/message_builder_unittest.cc:14: // Tests the
Message built from using the MessageBuilder class.
nit: You can probably omit comments like this (which fall into the same category
as commenting "GetFoo()" with a comment saying "Gets Foo.").

https://codereview.chromium.org/1460463003/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/message_builder_unittest.cc:19: // The payload
should be 8-byte aligned.
nit: "8-byte aligned" is not what you mean. You mean, "should be a multiple of 8
bytes in size". (Alignment is about the (starting) address not the size.)

https://codereview.chromium.org/1460463003/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/message_builder_unittest.cc:35: // Tests the
Message built from using the RequestMessageBuilder class.
Etc.

[darin (slow to review), 2015-11-19 05:55:27]

darin@chromium.org changed reviewers:
+ darin@chromium.org

[darin (slow to review), 2015-11-19 05:55:27]

Sorry, I'm missing why it is helpful to move these classes out of the internal
namespace. They were never really designed for use outside of the bindings
implementation code.

[darin (slow to review), 2015-11-19 05:55:29]

Sorry, I'm missing why it is helpful to move these classes out of the internal
namespace. They were never really designed for use outside of the bindings
implementation code.

[viettrungluu, 2015-11-19 16:26:12]

On 2015/11/19 05:55:29, darin (slow to review) wrote:
> Sorry, I'm missing why it is helpful to move these classes out of the internal
> namespace. They were never really designed for use outside of the bindings
> implementation code.

It's useful to be able to "manually" send messages, if you want (e.g., if you
want/need to use your own message sending/receiving mechanism).

As a concrete use-case, if you're using an interface whose messages never have a
response (say, a logging interface), it's trivial to send messages from multiple
threads, but of course InterfacePtr doesn't support this.

[vardhan, 2015-11-20 06:27:28]

Committed patchset #3 (id:40001) manually as
3db36ca1b7ad0c8f0338235352e1bd38da31c50f (presubmit successful).