Mojo cpp bindings: add support for validating incoming messages.

Mojo cpp bindings: add support for validating incoming messages.

The actual validating logic will be in a separate CL.

BUG=None
TEST=None

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

[yzshen1, 2014-05-12 16:35:25]

Hi, Darin.

Would you please take a look? Thanks!

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/interface_ptr_internal.h (right):

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/interface_ptr_internal.h:64: void
ConfigureStub(ScopedMessagePipeHandle handle,
I think this code is not used anymore, is it?

https://codereview.chromium.org/273233002/diff/30001/mojo/public/tools/bindin...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_request_validator_declaration.tmpl
(right):

https://codereview.chromium.org/273233002/diff/30001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/interface_request_validator_declaration.tmpl:1:
class {{interface.name}}RequestValidator : public
mojo::internal::MessageValidator {
FooRequestValidator inherits mojo::internal::MessageValidator, which exposes an
internal interface. I think that is okay:
- we have done similar things: internal::Bar_Data is exposed as Bar::Data.
- MessageValidator is used by our binding code internally, I think we would like
to keep it in mojo::internal::.

Please let me know if you don't think so. Thanks!

[darin (slow to review), 2014-05-12 17:28:26]

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/interface_ptr_internal.h (right):

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/interface_ptr_internal.h:52:
internal::ValidatorChain validators;
nit: already in the internal namespace

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/interface_ptr_internal.h:64: void
ConfigureStub(ScopedMessagePipeHandle handle,
On 2014/05/12 16:35:25, yzshen1 wrote:
> I think this code is not used anymore, is it?

Whoops, yes. I meant to delete that function and the ClientProxyWithStub class.
Feel free to delete that in this CL.

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/message_validator.h (right):

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/message_validator.h:16: class MessageValidator :
public MessageReceiver {
Hmm... there's nothing about this class that makes it inherently about message
validation. Maybe it would be good to use a name that more accurately describes
this class and in doing so provide a more generic system? Suppose if this were
called ChainedMessageReceiver, FilteringMessageReceiver or maybe just
MessageFilter? Yeah, I kind of like the latter.

MessageFilter could just have a "next" property that is a MessageReceiver. That
MessageReceiver could be a stub or it could be another MessageFilter.

The ValidationChain could be named FilterChain. This would make it possible to
use this system to not just validate incoming messages but also mutate them /
copy them to a logging system / etc. You might be able to build some interesting
things as message filters.

Also, perhaps it would make sense to implement the filter chain as a singly
linked list? Each MessageFilter could own the next MessageReceiver? Then,
FilterChain might just be a type that you use temporarily on the stack to help
you build up the singly linked list.

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/router.h (right):

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/router.h:20: ValidatorChain validators,
hmm, instead of passing the validator chain to Router, couldn't we just put all
of the validators being the incoming receiver?

[darin (slow to review), 2014-05-12 17:39:11]

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/router.cc (right):

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/router.cc:75:
connector_.set_incoming_receiver(validators_.GetHead());
Oh, I see an issue with my suggestion. Router wants to insert a MessageReceiver
(thunk_) between the stub (i.e., incoming_receiver_) and the validators. In that
case, we do need to specify the validators separately from the incoming
receiver. OK.

[yzshen1, 2014-05-12 21:59:57]

Thanks, Darin!

Please take another look!

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/interface_ptr_internal.h (right):

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/interface_ptr_internal.h:52:
internal::ValidatorChain validators;
On 2014/05/12 17:28:26, darin wrote:
> nit: already in the internal namespace

Done.

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/interface_ptr_internal.h:64: void
ConfigureStub(ScopedMessagePipeHandle handle,
On 2014/05/12 17:28:26, darin wrote:
> On 2014/05/12 16:35:25, yzshen1 wrote:
> > I think this code is not used anymore, is it?
> 
> Whoops, yes. I meant to delete that function and the ClientProxyWithStub
class.
> Feel free to delete that in this CL.

Done.

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/message_validator.h (right):

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/message_validator.h:16: class MessageValidator :
public MessageReceiver {
On 2014/05/12 17:28:26, darin wrote:
> Hmm... there's nothing about this class that makes it inherently about message
> validation. Maybe it would be good to use a name that more accurately
describes
> this class and in doing so provide a more generic system? Suppose if this were
> called ChainedMessageReceiver, FilteringMessageReceiver or maybe just
> MessageFilter? Yeah, I kind of like the latter.
I like the name of MessageFilter!

> 
> MessageFilter could just have a "next" property that is a MessageReceiver.
That
> MessageReceiver could be a stub or it could be another MessageFilter.
> 
> The ValidationChain could be named FilterChain. This would make it possible to
> use this system to not just validate incoming messages but also mutate them /
> copy them to a logging system / etc. You might be able to build some
interesting
> things as message filters.
> 
> Also, perhaps it would make sense to implement the filter chain as a singly
> linked list? Each MessageFilter could own the next MessageReceiver? Then,
> FilterChain might just be a type that you use temporarily on the stack to help
> you build up the singly linked list.

Making FilterChain a builder is a little awkward: we need to append several
filters to the builder, pass the builder into Router, and let Router append the
last one (Router::HandleIncomingMessageThunk) to create the singly linked list.
It seems that more refactoring is required to go with the
builder+singly-linked-list idea.

A relevant issue for discussion:
I have the feeling that it might be good to split the MessageReceiver interface.
The AcceptWithResponder() method is not needed in many cases. For example:
- the responder passed into AcceptWithResponder();
- MessageFilter;
- the receiver passed into ReadAndDispatchMessage();
- the receiver implemented by Connector, as well as the incoming receiver passed
into Connector.

Maybe we could put it in a new interface which also inherits MessageReceiver.
What do you think?

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/router.h (right):

https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/router.h:20: ValidatorChain validators,
On 2014/05/12 17:28:26, darin wrote:
> hmm, instead of passing the validator chain to Router, couldn't we just put
all
> of the validators being the incoming receiver?

We not only need to validate messages received by |incoming_receiver_|, but also
need to validate those response messages processed by Router to trigger
responders.

[yzshen1, 2014-05-14 05:41:24]

Friendly ping, Darin.
(But if you are busy, this CL could wait.)

Thanks!

[darin (slow to review), 2014-05-14 06:30:46]

On 2014/05/12 21:59:57, yzshen1 wrote:
> Thanks, Darin!
> 
> Please take another look!

Sorry for the slow response.


> > MessageFilter could just have a "next" property that is a MessageReceiver.
> That
> > MessageReceiver could be a stub or it could be another MessageFilter.
> > 
> > The ValidationChain could be named FilterChain. This would make it possible
to
> > use this system to not just validate incoming messages but also mutate them
/
> > copy them to a logging system / etc. You might be able to build some
> interesting
> > things as message filters.
> > 
> > Also, perhaps it would make sense to implement the filter chain as a singly
> > linked list? Each MessageFilter could own the next MessageReceiver? Then,
> > FilterChain might just be a type that you use temporarily on the stack to
help
> > you build up the singly linked list.
> 
> Making FilterChain a builder is a little awkward: we need to append several
> filters to the builder, pass the builder into Router, and let Router append
the
> last one (Router::HandleIncomingMessageThunk) to create the singly linked
list.
> It seems that more refactoring is required to go with the
> builder+singly-linked-list idea.

OK, yeah... that doesn't sound so great. I guess it could be done by passing
in the first and last node of the filter chain, but this seems like exactly
what FilterChain would hold onto (if it were holding the singly linked list).


> A relevant issue for discussion:
> I have the feeling that it might be good to split the MessageReceiver
interface.
> The AcceptWithResponder() method is not needed in many cases. For example:
> - the responder passed into AcceptWithResponder();
> - MessageFilter;
> - the receiver passed into ReadAndDispatchMessage();
> - the receiver implemented by Connector, as well as the incoming receiver
passed
> into Connector.
> 
> Maybe we could put it in a new interface which also inherits MessageReceiver.
> What do you think?

Good idea. It seems worth trying!


>
https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
> File mojo/public/cpp/bindings/lib/router.h (right):
> 
>
https://codereview.chromium.org/273233002/diff/30001/mojo/public/cpp/bindings...
> mojo/public/cpp/bindings/lib/router.h:20: ValidatorChain validators,
> On 2014/05/12 17:28:26, darin wrote:
> > hmm, instead of passing the validator chain to Router, couldn't we just put
> all
> > of the validators being the incoming receiver?
> 
> We not only need to validate messages received by |incoming_receiver_|, but
also
> need to validate those response messages processed by Router to trigger
> responders.

Yeah, sorry... I realized this after posting this comment.

[yzshen1, 2014-05-14 06:40:02]

> > A relevant issue for discussion:
> > I have the feeling that it might be good to split the MessageReceiver
> interface.
> > The AcceptWithResponder() method is not needed in many cases. For example:
> > - the responder passed into AcceptWithResponder();
> > - MessageFilter;
> > - the receiver passed into ReadAndDispatchMessage();
> > - the receiver implemented by Connector, as well as the incoming receiver
> passed
> > into Connector.
> > 
> > Maybe we could put it in a new interface which also inherits
MessageReceiver.
> > What do you think?
> 
> Good idea. It seems worth trying!

If you think it is okay, I will try it out in a separate CL, since It will touch
quite a lot of files.

Thanks for the reply! (I probably shouldn't send out the ping in the evening to
bother you. I thought you would see it tomorrow morning. But of course you
usually work till pretty late...)

[darin (slow to review), 2014-05-14 06:51:49]

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/router.cc (right):

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/router.cc:69: validators_(validators.Pass()),
s/validators/filters/

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/router.cc:75:
connector_.set_incoming_receiver(validators_.GetHead());
maybe set_sink and GetHead could be combined since after you set the sink it
doesn't make sense to continue appending? maybe GetHead could just take a
MessageReceiver pointer, or we could call this method something like "Finish"
instead?

FilterChain feels very much like a builder interface. You could maybe do
something like this:

  FilterChain::Builder builder;
  builder.Append(..)
         .Append(..)
         .Append(..);
  FilterChain chain = builder.Finish(sink);
  // |chain| is now immutable

Perhaps |chain.head()| could return a MessageReceiver*?

This implies passing a FilterChain::Builder to Router. That seems a bit
unfortunate and slightly awkward, but not terrible :-/

Maybe a simpler solution would be to just make Router's FilterChain member be
const but ctor parameter be non-const! Then that would deny access to Append and
Finish when used after the ctor. You could just finish the setup of FilterChain
in the Router constructor and then when done .Pass() it into the const member
variable.

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/message_filter.h (right):

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/message_filter.h:23: void set_next(MessageReceiver*
next) { next_ = next; }
it might make sense to rename "next" to "sink" because from the perspective of
the filter, it seems as though the message terminates with this receiver--this
sink. it isn't necessarily a next filter.

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/message_filter.h:34: // A trivial filter that simply
forwards every message it receives to |next_|.
nit: maybe call this class MessageForwarder? At first I thought "null filter"
would send things to /dev/null (i.e., ignore incoming messages). You could also
call it a PassThrough{Message}Filter or Forwarding{Message}Filter? I'm not sure
how important it is to have the term "Message" in those names.

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/sync_dispatcher.h (right):

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/sync_dispatcher.h:29: validators_.Append(new
internal::MessageHeaderValidator)
nit: validators_ -> filters_

[yzshen1, 2014-05-14 08:14:03]

Thanks, Darin!

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/router.cc (right):

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/router.cc:69: validators_(validators.Pass()),
On 2014/05/14 06:51:51, darin wrote:
> s/validators/filters/

Done.

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/router.cc:75:
connector_.set_incoming_receiver(validators_.GetHead());
> maybe set_sink and GetHead could be combined since after you set the sink it
> doesn't make sense to continue appending? maybe GetHead could just take a
> MessageReceiver pointer, or we could call this method something like "Finish"
> instead?
FilterChain was implemented to work correctly even if we continue appending
after setting the sink or calling GetHead(). (One caveat is that the head will
change when going from 0 to 1 filter, so GetHead() should be called to get the
new head.)

> FilterChain feels very much like a builder interface. You could maybe do
> something like this:
> 
>   FilterChain::Builder builder;
>   builder.Append(..)
>          .Append(..)
>          .Append(..);
>   FilterChain chain = builder.Finish(sink);
>   // |chain| is now immutable
> 
> Perhaps |chain.head()| could return a MessageReceiver*?
> 
> This implies passing a FilterChain::Builder to Router. That seems a bit
> unfortunate and slightly awkward, but not terrible :-/

Comparing with passing a FilterChain::Builder, maybe we could do it this way:
- FilterChain doesn't have sink-related member/methods;
- Define a new FilteredReceiver class:
class FilteredReceiver : public MessageReceiver {
  FilteredReceiver(FilterChain filters, MessageReceiver* sink); // takes
ownership of both
};

This way, FilterChain acts like a builder and we avoid the confusion of passing
a builder to Router.
One drawback is that we will have one extra layer to call:
FilteredReceiver::Accept(). Maybe that performance cost is negligible?


> 
> Maybe a simpler solution would be to just make Router's FilterChain member be
> const but ctor parameter be non-const! Then that would deny access to Append
and
> Finish when used after the ctor. You could just finish the setup of
FilterChain
> in the Router constructor and then when done .Pass() it into the const member
> variable.

What do you think?

Thanks!

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/message_filter.h (right):

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/message_filter.h:23: void set_next(MessageReceiver*
next) { next_ = next; }
On 2014/05/14 06:51:51, darin wrote:
> it might make sense to rename "next" to "sink" because from the perspective of
> the filter, it seems as though the message terminates with this receiver--this
> sink. it isn't necessarily a next filter.

Done.

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/message_filter.h:34: // A trivial filter that simply
forwards every message it receives to |next_|.
On 2014/05/14 06:51:51, darin wrote:
> nit: maybe call this class MessageForwarder? At first I thought "null filter"
> would send things to /dev/null (i.e., ignore incoming messages). You could
also
> call it a PassThrough{Message}Filter or Forwarding{Message}Filter? I'm not
sure
> how important it is to have the term "Message" in those names.

Done. I changed it to PassThroughFilter.

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/sync_dispatcher.h (right):

https://codereview.chromium.org/273233002/diff/50001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/sync_dispatcher.h:29: validators_.Append(new
internal::MessageHeaderValidator)
On 2014/05/14 06:51:51, darin wrote:
> nit: validators_ -> filters_

Done. (I was thinking validators_ is a more specific name to indicate that all
filters we put in it are validators. But I am fine with either way.)

[darin (slow to review), 2014-05-15 05:47:29]

LGTM

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

https://codereview.chromium.org/273233002/diff/70001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/message_header_validator.h:5: #ifndef
MOJO_PUBLIC_CPP_BINDINGS_LIB_MESSAGE_HEADER_VALIDATOR_H_
oops, thanks!

https://codereview.chromium.org/273233002/diff/70001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl (right):

https://codereview.chromium.org/273233002/diff/70001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl:12: #include
<assert.h>
Is this needed?

[darin (slow to review), 2014-05-15 06:56:02]

LGTM

https://codereview.chromium.org/273233002/diff/70001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/filter_chain.h (right):

https://codereview.chromium.org/273233002/diff/70001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/filter_chain.h:34: FilterChain&
Append(MessageFilter* filter);
Now I understand why you were exploring template method specializations! It
would be nice to not have to heap allocate and then destroy the
PassThroughFilter.

I suppose you could have a global function to help:

  template <typename FilterImpl>
  void Append(FilterChain* chain) {
    chain->Append(new FilterImpl());
  }

  template <>
  void Append<PassThroughFilter>(FilterChain* chain) {
  }

Then, you would write:

  FilterChain chain;
  Append<MessageHeaderValidator>(&chain);
  Append<typename Interface::RequestValidator_>(&chain);
  Append<typename Interface::Client_::ResponseValidator_>(&chain);

Yeah, that doesn't look quite as nice as what you have now.

[yzshen1, 2014-05-15 07:45:30]

Thanks Darin!

https://codereview.chromium.org/273233002/diff/70001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/filter_chain.h (right):

https://codereview.chromium.org/273233002/diff/70001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/filter_chain.h:34: FilterChain&
Append(MessageFilter* filter);
On 2014/05/15 06:56:02, darin wrote:
> Now I understand why you were exploring template method specializations! It
> would be nice to not have to heap allocate and then destroy the
> PassThroughFilter.
> 
> I suppose you could have a global function to help:
> 
>   template <typename FilterImpl>
>   void Append(FilterChain* chain) {
>     chain->Append(new FilterImpl());
>   }
> 
>   template <>
>   void Append<PassThroughFilter>(FilterChain* chain) {
>   }
> 
> Then, you would write:
> 
>   FilterChain chain;
>   Append<MessageHeaderValidator>(&chain);
>   Append<typename Interface::RequestValidator_>(&chain);
>   Append<typename Interface::Client_::ResponseValidator_>(&chain);
> 
> Yeah, that doesn't look quite as nice as what you have now.

Yeah, I experimented with template method specialization and found out that in
order to use Append() in a chain, I need to use the 'template' keyword
explicitly:

chain.Append<XXX>()
     .template Append<YYY>()
     .template Append<ZZZ>();

That seems less nice looking and probably will confuse some users (like me). :/

But there is no need to use 'template' if I break them into three statements:
chain.Append<XXX>();
chain.Append<YYY>();
chain.Append<ZZZ>();

If we want to avoid unnecessary allocation, maybe this last approach is
acceptable?

https://codereview.chromium.org/273233002/diff/70001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl (right):

https://codereview.chromium.org/273233002/diff/70001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl:12: #include
<assert.h>
On 2014/05/15 05:47:29, darin wrote:
> Is this needed?
Removed.
(I am not very clear to what extent we follow the IWYU rule.)

[yzshen1, 2014-05-15 07:45:41]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-05-15 07:46:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yzshen@chromium.org/273233002/90001

[commit-bot: I haz the power, 2014-05-15 08:16:00]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-15 08:18:43]

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

[commit-bot: I haz the power, 2014-05-15 08:18:43]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[yzshen1, 2014-05-15 17:28:51]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-05-15 17:33:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yzshen@chromium.org/273233002/110001

[darin (slow to review), 2014-05-15 21:19:05]

On 2014/05/15 07:45:30, yzshen1 (OOO until may 19) wrote:
...
> Yeah, I experimented with template method specialization and found out that in
> order to use Append() in a chain, I need to use the 'template' keyword
> explicitly:
> 
> chain.Append<XXX>()
>      .template Append<YYY>()
>      .template Append<ZZZ>();
> 
> That seems less nice looking and probably will confuse some users (like me).
:/
> 
> But there is no need to use 'template' if I break them into three statements:
> chain.Append<XXX>();
> chain.Append<YYY>();
> chain.Append<ZZZ>();
> 
> If we want to avoid unnecessary allocation, maybe this last approach is
> acceptable?

Yeah, I think that would be worth it. I always worry about the bindings
system being too heavyweight. A separate CL is fine.

[commit-bot: I haz the power, 2014-05-15 21:46:40]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-15 23:48:53]

Change committed as 270857