Split BindStateBase ctor for non-cancellable Bind

Split BindStateBase ctor for non-cancellable Bind

http://crrev.com/59aa6bb1162b31d2 added a parameter of BindStateBase,
and that causes 0.8% (40kB) binary size bloat on cronet, by adding
a new instruction to the constructor invocation at BindState ctor.

To reduce the binary size, this CL splits BindStateBase into two: one
for cancellable bind and another for non-cancellable one, so that
non-cancellable bind doesn't need to pass an extra argument to
BindStateBase.

This reduces the stripped binary size of chrome on Linux by 56kB.

Committed: https://crrev.com/1fdcca3da07639548c7f37fab1c9d6c93541e4a9
Cr-Commit-Position: refs/heads/master@{#418502}

[tzik, 2016-09-09 14:15:03]

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

[commit-bot: I haz the power, 2016-09-09 14:15:23]

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

[tzik, 2016-09-09 14:22:08]

Description was changed from

==========
Split BindStateBase ctor for non-cancellable Bind
==========

to

==========
Split BindStateBase ctor for non-cancellable Bind

http://crrev.com/59aa6bb1162b31d2 added a parameter of BindStateBase,
and that causes 0.8% (40kB) binary size bloat on cronet, by adding
a new instruction to the constructor invocation at BindState ctor.

To reduce the binary size, this CL splits BindStateBase into two: one
for cancellable bind and another for non-cancellable one, so that
non-cancellable bind doesn't need to pass an extra argument to
BindStateBase.

This reduces the stripped binary size of chrome on Linux by 56kB.
==========

[tzik, 2016-09-09 14:24:02]

tzik@chromium.org changed reviewers:
+ dcheng@chromium.org, kapishnikov@chromium.org

[tzik, 2016-09-09 14:24:02]

PTAL

[kapishnikov, 2016-09-09 14:54:38]

LGTM but I don't think I am an owner. It would be good if somebody else can take
a look.

[commit-bot: I haz the power, 2016-09-09 16:15:59]

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

[commit-bot: I haz the power, 2016-09-09 16:16:00]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-09-09 20:39:02]

https://codereview.chromium.org/2322313002/diff/1/base/bind_internal.h
File base/bind_internal.h (right):

https://codereview.chromium.org/2322313002/diff/1/base/bind_internal.h#newcod...
base/bind_internal.h:423: : BindState(IsCancellable(),
Is this a C++14ism? How does it work with our toolchain? =)

[dcheng, 2016-09-09 20:39:27]

https://codereview.chromium.org/2322313002/diff/1/base/bind_internal.h
File base/bind_internal.h (right):

https://codereview.chromium.org/2322313002/diff/1/base/bind_internal.h#newcod...
base/bind_internal.h:423: : BindState(IsCancellable(),
On 2016/09/09 20:39:01, dcheng wrote:
> Is this a C++14ism? How does it work with our toolchain? =)

Also, see
https://github.com/llvm-mirror/libcxx/blob/master/include/type_traits#L505

[tzik, 2016-09-09 23:51:16]

It's not operator() call in C++14. Just a instantiation by a constructor
call, which is in C++11.

2016/09/10 5:39 <dcheng@chromium.org>:


https://codereview.chromium.org/2322313002/diff/1/base/bind_internal.h
File base/bind_internal.h (right):

https://codereview.chromium.org/2322313002/diff/1/base/
bind_internal.h#newcode423
base/bind_internal.h:423: : BindState(IsCancellable(),
On 2016/09/09 20:39:01, dcheng wrote:
> Is this a C++14ism? How does it work with our toolchain? =)

Also, see
https://github.com/llvm-mirror/libcxx/blob/master/include/type_traits#L505

https://codereview.chromium.org/2322313002/

-- 
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.

[dcheng, 2016-09-10 00:09:13]

On 2016/09/09 23:51:16, tzik wrote:
> It's not operator() call in C++14. Just a instantiation by a constructor
> call, which is in C++11.

Ahh... it's not an actual function object. Can we name this something that
doesn't suggest it's a function object then and that it's an actual type that
we're instantiating?

> 
> 2016/09/10 5:39 <mailto:dcheng@chromium.org>:
> 
> 
> https://codereview.chromium.org/2322313002/diff/1/base/bind_internal.h
> File base/bind_internal.h (right):
> 
> https://codereview.chromium.org/2322313002/diff/1/base/
> bind_internal.h#newcode423
> base/bind_internal.h:423: : BindState(IsCancellable(),
> On 2016/09/09 20:39:01, dcheng wrote:
> > Is this a C++14ism? How does it work with our toolchain? =)
> 
> Also, see
> https://github.com/llvm-mirror/libcxx/blob/master/include/type_traits#L505
> 
> https://codereview.chromium.org/2322313002/
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[tzik, 2016-09-12 08:28:41]

On 2016/09/10 00:09:13, dcheng wrote:
> On 2016/09/09 23:51:16, tzik wrote:
> > It's not operator() call in C++14. Just a instantiation by a constructor
> > call, which is in C++11.
> 
> Ahh... it's not an actual function object. Can we name this something that
> doesn't suggest it's a function object then and that it's an actual type that
> we're instantiating?

I have no idea for a good and consistent name for this.
Can we just use the uniform initialization syntax (IsCancellable{}) and add a
comment here?

> 
> > 
> > 2016/09/10 5:39 <mailto:dcheng@chromium.org>:
> > 
> > 
> > https://codereview.chromium.org/2322313002/diff/1/base/bind_internal.h
> > File base/bind_internal.h (right):
> > 
> > https://codereview.chromium.org/2322313002/diff/1/base/
> > bind_internal.h#newcode423
> > base/bind_internal.h:423: : BindState(IsCancellable(),
> > On 2016/09/09 20:39:01, dcheng wrote:
> > > Is this a C++14ism? How does it work with our toolchain? =)
> > 
> > Also, see
> > https://github.com/llvm-mirror/libcxx/blob/master/include/type_traits#L505
> > 
> > https://codereview.chromium.org/2322313002/
> > 
> > -- 
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[tzik, 2016-09-12 08:29:12]

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

[commit-bot: I haz the power, 2016-09-12 08:29:22]

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

[dcheng, 2016-09-12 10:09:45]

On 2016/09/12 08:28:41, tzik wrote:
> On 2016/09/10 00:09:13, dcheng wrote:
> > On 2016/09/09 23:51:16, tzik wrote:
> > > It's not operator() call in C++14. Just a instantiation by a constructor
> > > call, which is in C++11.
> > 
> > Ahh... it's not an actual function object. Can we name this something that
> > doesn't suggest it's a function object then and that it's an actual type
that
> > we're instantiating?
> 
> I have no idea for a good and consistent name for this.
> Can we just use the uniform initialization syntax (IsCancellable{}) and add a
> comment here?
> 
> > 
> > > 
> > > 2016/09/10 5:39 <mailto:dcheng@chromium.org>:
> > > 
> > > 
> > > https://codereview.chromium.org/2322313002/diff/1/base/bind_internal.h
> > > File base/bind_internal.h (right):
> > > 
> > > https://codereview.chromium.org/2322313002/diff/1/base/
> > > bind_internal.h#newcode423
> > > base/bind_internal.h:423: : BindState(IsCancellable(),
> > > On 2016/09/09 20:39:01, dcheng wrote:
> > > > Is this a C++14ism? How does it work with our toolchain? =)
> > > 
> > > Also, see
> > > https://github.com/llvm-mirror/libcxx/blob/master/include/type_traits#L505
> > > 
> > > https://codereview.chromium.org/2322313002/
> > > 
> > > -- 
> > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.

Would it be bad to just call it IsCancellableType? =)

[commit-bot: I haz the power, 2016-09-12 10:24:22]

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

[commit-bot: I haz the power, 2016-09-12 10:24:26]

Dry run: This issue passed the CQ dry run.

[tzik, 2016-09-12 11:09:46]

On 2016/09/12 10:09:45, dcheng wrote:
> On 2016/09/12 08:28:41, tzik wrote:
> > On 2016/09/10 00:09:13, dcheng wrote:
> > > On 2016/09/09 23:51:16, tzik wrote:
> > > > It's not operator() call in C++14. Just a instantiation by a constructor
> > > > call, which is in C++11.
> > > 
> > > Ahh... it's not an actual function object. Can we name this something that
> > > doesn't suggest it's a function object then and that it's an actual type
> that
> > > we're instantiating?
> > 
> > I have no idea for a good and consistent name for this.
> > Can we just use the uniform initialization syntax (IsCancellable{}) and add
a
> > comment here?
> > 
> > > 
> > > > 
> > > > 2016/09/10 5:39 <mailto:dcheng@chromium.org>:
> > > > 
> > > > 
> > > > https://codereview.chromium.org/2322313002/diff/1/base/bind_internal.h
> > > > File base/bind_internal.h (right):
> > > > 
> > > > https://codereview.chromium.org/2322313002/diff/1/base/
> > > > bind_internal.h#newcode423
> > > > base/bind_internal.h:423: : BindState(IsCancellable(),
> > > > On 2016/09/09 20:39:01, dcheng wrote:
> > > > > Is this a C++14ism? How does it work with our toolchain? =)
> > > > 
> > > > Also, see
> > > >
https://github.com/llvm-mirror/libcxx/blob/master/include/type_traits#L505
> > > > 
> > > > https://codereview.chromium.org/2322313002/
> > > > 
> > > > -- 
> > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Would it be bad to just call it IsCancellableType? =)

IsCancellableType sounds like "check if the given type is a cancellable type"...

[tzik, 2016-09-12 11:10:08]

Patchset #3 (id:40001) has been deleted

[tzik, 2016-09-12 11:10:10]

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

[commit-bot: I haz the power, 2016-09-12 11:10:28]

Dry run: 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-09-12 13:03:44]

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

[commit-bot: I haz the power, 2016-09-12 13:03:44]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-09-13 16:55:45]

On 2016/09/12 11:09:46, tzik wrote:
> On 2016/09/12 10:09:45, dcheng wrote:
> > On 2016/09/12 08:28:41, tzik wrote:
> > > On 2016/09/10 00:09:13, dcheng wrote:
> > > > On 2016/09/09 23:51:16, tzik wrote:
> > > > > It's not operator() call in C++14. Just a instantiation by a
constructor
> > > > > call, which is in C++11.
> > > > 
> > > > Ahh... it's not an actual function object. Can we name this something
that
> > > > doesn't suggest it's a function object then and that it's an actual type
> > that
> > > > we're instantiating?
> > > 
> > > I have no idea for a good and consistent name for this.
> > > Can we just use the uniform initialization syntax (IsCancellable{}) and
add
> a
> > > comment here?
> > > 
> > > > 
> > > > > 
> > > > > 2016/09/10 5:39 <mailto:dcheng@chromium.org>:
> > > > > 
> > > > > 
> > > > > https://codereview.chromium.org/2322313002/diff/1/base/bind_internal.h
> > > > > File base/bind_internal.h (right):
> > > > > 
> > > > > https://codereview.chromium.org/2322313002/diff/1/base/
> > > > > bind_internal.h#newcode423
> > > > > base/bind_internal.h:423: : BindState(IsCancellable(),
> > > > > On 2016/09/09 20:39:01, dcheng wrote:
> > > > > > Is this a C++14ism? How does it work with our toolchain? =)
> > > > > 
> > > > > Also, see
> > > > >
> https://github.com/llvm-mirror/libcxx/blob/master/include/type_traits#L505
> > > > > 
> > > > > https://codereview.chromium.org/2322313002/
> > > > > 
> > > > > -- 
> > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> > Would it be bad to just call it IsCancellableType? =)
> 
> IsCancellableType sounds like "check if the given type is a cancellable
type"...

Yeah, I think I agree that PS2 is better than PS3. A name like is_cancellable_t
might make it clear, but that wouldn't follow our naming conventions either
than. So... let's just go with PS2. LGTM

[tzik, 2016-09-14 05:15:15]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-09-14 05:15:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
kapishnikov@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2322313002/#ps80001
(title: "revert to PS2")

[commit-bot: I haz the power, 2016-09-14 05:15:32]

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-09-14 07:15:20]

Description was changed from

==========
Split BindStateBase ctor for non-cancellable Bind

http://crrev.com/59aa6bb1162b31d2 added a parameter of BindStateBase,
and that causes 0.8% (40kB) binary size bloat on cronet, by adding
a new instruction to the constructor invocation at BindState ctor.

To reduce the binary size, this CL splits BindStateBase into two: one
for cancellable bind and another for non-cancellable one, so that
non-cancellable bind doesn't need to pass an extra argument to
BindStateBase.

This reduces the stripped binary size of chrome on Linux by 56kB.
==========

to

==========
Split BindStateBase ctor for non-cancellable Bind

http://crrev.com/59aa6bb1162b31d2 added a parameter of BindStateBase,
and that causes 0.8% (40kB) binary size bloat on cronet, by adding
a new instruction to the constructor invocation at BindState ctor.

To reduce the binary size, this CL splits BindStateBase into two: one
for cancellable bind and another for non-cancellable one, so that
non-cancellable bind doesn't need to pass an extra argument to
BindStateBase.

This reduces the stripped binary size of chrome on Linux by 56kB.
==========

[commit-bot: I haz the power, 2016-09-14 07:15:21]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-09-14 07:17:25]

Description was changed from

==========
Split BindStateBase ctor for non-cancellable Bind

http://crrev.com/59aa6bb1162b31d2 added a parameter of BindStateBase,
and that causes 0.8% (40kB) binary size bloat on cronet, by adding
a new instruction to the constructor invocation at BindState ctor.

To reduce the binary size, this CL splits BindStateBase into two: one
for cancellable bind and another for non-cancellable one, so that
non-cancellable bind doesn't need to pass an extra argument to
BindStateBase.

This reduces the stripped binary size of chrome on Linux by 56kB.
==========

to

==========
Split BindStateBase ctor for non-cancellable Bind

http://crrev.com/59aa6bb1162b31d2 added a parameter of BindStateBase,
and that causes 0.8% (40kB) binary size bloat on cronet, by adding
a new instruction to the constructor invocation at BindState ctor.

To reduce the binary size, this CL splits BindStateBase into two: one
for cancellable bind and another for non-cancellable one, so that
non-cancellable bind doesn't need to pass an extra argument to
BindStateBase.

This reduces the stripped binary size of chrome on Linux by 56kB.

Committed: https://crrev.com/1fdcca3da07639548c7f37fab1c9d6c93541e4a9
Cr-Commit-Position: refs/heads/master@{#418502}
==========

[commit-bot: I haz the power, 2016-09-14 07:17:26]

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