Reduce code size of BindingState

Reduce code size of BindingState

BindingState is a templated class that gets expanded for every Mojo impl,
causing code bloat. By moving the functions that don't depend on the template
parameter out of line we save roughly 2.3k bytes in safe_json.mojom (on
Linux). I expect similar gains for every .mojom.

BUG=597125
Committed: https://crrev.com/ad0c0b3bab4a19dfafb51bda1dad56024c2dc9e2
Cr-Commit-Position: refs/heads/master@{#409691}

[tibell, 2016-08-03 00:53:33]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 00:54:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-08-03 00:54:21]

tibell@chromium.org changed reviewers:
+ rockot@chromium.org

[tibell, 2016-08-03 00:57:27]

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

https://codereview.chromium.org/2203243002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/filter_chain.h:32: void Append(MessageFilter*
filter);
This is needed so we can call this from non-templated code.

[tibell, 2016-08-03 00:58:51]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 00:59:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-08-03 01:00:24]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 01:00:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-08-03 01:02:15]

I can't make the change look like a move. Sorry!

https://codereview.chromium.org/2203243002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/binding_state.cc (right):

https://codereview.chromium.org/2203243002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/binding_state.cc:89: 
I moved the line

    stub_.serialization_context()->group_controller = router_;

from here into the caller (i.e. it will happen after the below lines after this
refactoring). From what I can tell that is safe, but please verify.

[commit-bot: I haz the power, 2016-08-03 03:13:42]

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

[commit-bot: I haz the power, 2016-08-03 03:13:43]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-08-03 14:55:28]

Very nice! Seems like a huge win. LGTM

https://codereview.chromium.org/2203243002/diff/40001/mojo/mojo_public.gypi
File mojo/mojo_public.gypi (right):

https://codereview.chromium.org/2203243002/diff/40001/mojo/mojo_public.gypi#n...
mojo/mojo_public.gypi:36: 'public/cpp/bindings/lib/binding_state.cc',
Please omit this change. We don't need to maintain GYP builds anymore.

https://codereview.chromium.org/2203243002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/binding_state.h (right):

https://codereview.chromium.org/2203243002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/binding_state.h:35: // mojom file.
nit: Maybe document that it's a base class used for templated Binding primitives
which bind a pipe exclusively to a single interface. The detail about code
generation size isn't particularly interesting to someone who just wants to
understand the code, though I don't think it hurts to include that information
as well.

https://codereview.chromium.org/2203243002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/binding_state.h:36: class SimpleBindingStateBase {
nit: Maybe drop the "Base" suffix? Seems superfluous.

https://codereview.chromium.org/2203243002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/binding_state.h:41: void Bind(
nitty nit: I'd prefer we make this protected and call it something like
BindInternal

https://codereview.chromium.org/2203243002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/binding_state.h:128: class
MultiplexedBindingStateBase {
nit: Roughly the same comments about "Base" and documentation here

[tibell, 2016-08-03 23:37:43]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 23:38:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-08-03 23:42:51]

Thanks for reviewing.

I checked out how big impact this has on the current use of Mojo. It only saves
24k on a stripped Linux binary, but hopefully the pay-off is a bit bigger once
everything is converted to Mojo. Baby steps I guess. :)

https://codereview.chromium.org/2203243002/diff/40001/mojo/mojo_public.gypi
File mojo/mojo_public.gypi (right):

https://codereview.chromium.org/2203243002/diff/40001/mojo/mojo_public.gypi#n...
mojo/mojo_public.gypi:36: 'public/cpp/bindings/lib/binding_state.cc',
On 2016/08/03 14:55:27, Ken Rockot wrote:
> Please omit this change. We don't need to maintain GYP builds anymore.

Done.

https://codereview.chromium.org/2203243002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/binding_state.h (right):

https://codereview.chromium.org/2203243002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/binding_state.h:36: class SimpleBindingStateBase {
On 2016/08/03 14:55:28, Ken Rockot wrote:
> nit: Maybe drop the "Base" suffix? Seems superfluous.

Done.

https://codereview.chromium.org/2203243002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/binding_state.h:41: void Bind(
On 2016/08/03 14:55:28, Ken Rockot wrote:
> nitty nit: I'd prefer we make this protected and call it something like
> BindInternal

Done.

https://codereview.chromium.org/2203243002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/binding_state.h:128: class
MultiplexedBindingStateBase {
On 2016/08/03 14:55:28, Ken Rockot wrote:
> nit: Roughly the same comments about "Base" and documentation here

Done.

[commit-bot: I haz the power, 2016-08-04 01:08:13]

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

[commit-bot: I haz the power, 2016-08-04 01:08:14]

Dry run: This issue passed the CQ dry run.

[tibell, 2016-08-04 01:08:53]

The CQ bit was checked by tibell@chromium.org

[tibell, 2016-08-04 01:08:54]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2203243002/#ps60001
(title: "Address rockot's review comments")

[commit-bot: I haz the power, 2016-08-04 01:09:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 01:13:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-04 01:16:58]

Description was changed from

==========
Reduce code size of BindingState

BindingState is a templated class that gets expanded for every Mojo impl,
causing code bloat. By moving the functions that don't depend on the template
parameter out of line we save roughly 2.3k bytes in safe_json.mojom (on
Linux). I expect similar gains for every .mojom.

BUG=597125
==========

to

==========
Reduce code size of BindingState

BindingState is a templated class that gets expanded for every Mojo impl,
causing code bloat. By moving the functions that don't depend on the template
parameter out of line we save roughly 2.3k bytes in safe_json.mojom (on
Linux). I expect similar gains for every .mojom.

BUG=597125

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

[commit-bot: I haz the power, 2016-08-04 01:16:59]

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