Reduce boilerplate needed to configure mojo::Binding

Reduce boilerplate needed to configure mojo::Binding

BUG=579645
TEST=none
R=yzshen@chromium.org

Committed: https://crrev.com/ad19bf46a6bea7ad579a19e044de847a9fd08e7d
Cr-Commit-Position: refs/heads/master@{#370741}

[sky, 2016-01-21 01:07:21]

I only converted the mash ones so that you can see it in use. I'll convert the
other uses once done.

[yzshen1, 2016-01-21 01:12:09]

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

https://codereview.chromium.org/1614553002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding.h:116: InterfacePtr<Interface>
CreateInterfacePtrAndBind() {
Please add a |waiter| param (just like line 127), and pass it into Bind().

https://codereview.chromium.org/1614553002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding.h:119: return std::move(interface_ptr);
I think std::move() is not needed.

[sky, 2016-01-21 14:35:26]

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

https://codereview.chromium.org/1614553002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding.h:116: InterfacePtr<Interface>
CreateInterfacePtrAndBind() {
On 2016/01/21 01:12:09, yzshen1 wrote:
> Please add a |waiter| param (just like line 127), and pass it into Bind().

Do we really need the waiter param? And doesn't the style guide say no default
args? How about I add it if we actually use it somewhere.

[yzshen1, 2016-01-21 15:23:35]

lgtm, please address the other nit.


On 2016/01/21 14:35:26, sky wrote:
>
https://codereview.chromium.org/1614553002/diff/20001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/binding.h (right):
> 
>
https://codereview.chromium.org/1614553002/diff/20001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/binding.h:116: InterfacePtr<Interface>
> CreateInterfacePtrAndBind() {
> On 2016/01/21 01:12:09, yzshen1 wrote:
> > Please add a |waiter| param (just like line 127), and pass it into Bind().
> 
> Do we really need the waiter param? And doesn't the style guide say no default
> args? How about I add it if we actually use it somewhere.

Okay. We don't use it anywhere. Eventually  we will refacor the environment
stuff and remove this anyway.

[sky, 2016-01-21 17:50:24]

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

https://codereview.chromium.org/1614553002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding.h:119: return std::move(interface_ptr);
On 2016/01/21 01:12:09, yzshen1 wrote:
> I think std::move() is not needed.

Done.

[sky, 2016-01-21 17:50:42]

The CQ bit was checked by sky@chromium.org

[sky, 2016-01-21 17:50:42]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1614553002/#ps40001
(title: "nuke std::move")

[commit-bot: I haz the power, 2016-01-21 17:50:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1614553002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1614553002/40001

[commit-bot: I haz the power, 2016-01-21 18:59:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-21 19:01:52]

Description was changed from

==========
Reduce boilerplate needed to configure mojo::Binding

BUG=579645
TEST=none
R=yzshen@chromium.org
==========

to

==========
Reduce boilerplate needed to configure mojo::Binding

BUG=579645
TEST=none
R=yzshen@chromium.org

Committed: https://crrev.com/ad19bf46a6bea7ad579a19e044de847a9fd08e7d
Cr-Commit-Position: refs/heads/master@{#370741}
==========

[commit-bot: I haz the power, 2016-01-21 19:01:53]

Patchset 3 (id:??) landed as
https://crrev.com/ad19bf46a6bea7ad579a19e044de847a9fd08e7d
Cr-Commit-Position: refs/heads/master@{#370741}

[Elliot Glaysher, 2016-01-21 19:15:01]

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

[Elliot Glaysher, 2016-01-21 19:15:01]

Hi, I believe this broke ToT mash:

[0121/111257:FATAL:binding.h(218)] Check failed: is_bound(). 
#0 0x7f229a0b290e base::debug::StackTrace::StackTrace()
#1 0x7f229a100cff logging::LogMessage::~LogMessage()
#2 0x7f2297eac6f2 mojo::Binding<>::set_connection_error_handler()
#3 0x7f2297eabc26
mash::browser_driver::BrowserDriverApplicationDelegate::AddAccelerators()
#4 0x7f2297eabab4
mash::browser_driver::BrowserDriverApplicationDelegate::Initialize()

It looks like a binding needs to be bound to set a connection error handler.
browser_driver consistently crashes on mash startup now.

[Elliot Glaysher, 2016-01-21 19:23:54]

On 2016/01/21 19:15:01, Elliot Glaysher wrote:
> Hi, I believe this broke ToT mash:
> 
> [0121/111257:FATAL:binding.h(218)] Check failed: is_bound(). 
> #0 0x7f229a0b290e base::debug::StackTrace::StackTrace()
> #1 0x7f229a100cff logging::LogMessage::~LogMessage()
> #2 0x7f2297eac6f2 mojo::Binding<>::set_connection_error_handler()
> #3 0x7f2297eabc26
> mash::browser_driver::BrowserDriverApplicationDelegate::AddAccelerators()
> #4 0x7f2297eabab4
> mash::browser_driver::BrowserDriverApplicationDelegate::Initialize()
> 
> It looks like a binding needs to be bound to set a connection error handler.
> browser_driver consistently crashes on mash startup now.

Created https://codereview.chromium.org/1617893003/

[sky, 2016-01-21 20:20:38]

Sorry for the breakage and thanks for the fix.

On Thu, Jan 21, 2016 at 11:23 AM,  <erg@chromium.org> wrote:
> On 2016/01/21 19:15:01, Elliot Glaysher wrote:
>>
>> Hi, I believe this broke ToT mash:
>
>
>> [0121/111257:FATAL:binding.h(218)] Check failed: is_bound().
>> #0 0x7f229a0b290e base::debug::StackTrace::StackTrace()
>> #1 0x7f229a100cff logging::LogMessage::~LogMessage()
>> #2 0x7f2297eac6f2 mojo::Binding<>::set_connection_error_handler()
>> #3 0x7f2297eabc26
>> mash::browser_driver::BrowserDriverApplicationDelegate::AddAccelerators()
>> #4 0x7f2297eabab4
>> mash::browser_driver::BrowserDriverApplicationDelegate::Initialize()
>
>
>> It looks like a binding needs to be bound to set a connection error
>> handler.
>> browser_driver consistently crashes on mash startup now.
>
>
> Created https://codereview.chromium.org/1617893003/
>
> https://codereview.chromium.org/1614553002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.