Remove BindStateHolder and have Bind() return a Callback<> object directly.

Remove BindStateHolder and have Bind() return a Callback<> object directly.

This removes some complexity and also fixes a bug where if you call Bind() with the result of Bind(), the resulting Callback would only be valid during the first call.  Ouch.

BUG=none
TEST=new unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114494

[awong, 2011-11-30 01:36:14]

Fred, your turn!

[awong, 2011-12-01 19:07:29]

ping-pong!

This should be a relatively simple change...I basically make everything take a
BindState* instead of a BindStateHolder...

[akalin, 2011-12-02 18:37:21]

On 2011/12/01 19:07:29, awong wrote:
> ping-pong!
> 
> This should be a relatively simple change...I basically make everything take a
> BindState* instead of a BindStateHolder...

Sorry, I just saw this.  Looking now...

[akalin, 2011-12-02 18:54:45]

mostly nits, but one question

http://codereview.chromium.org/8738001/diff/1001/base/bind_unittest.cc
File base/bind_unittest.cc (right):

http://codereview.chromium.org/8738001/diff/1001/base/bind_unittest.cc#newcod...
base/bind_unittest.cc:295: // Test that curring the rvalue result of Bind()
works correclty.
curring -> currying

Although don't you mean 'binding'?

http://codereview.chromium.org/8738001/diff/1001/base/bind_unittest.cc#newcod...
base/bind_unittest.cc:302: // If we implemt Bind() such that the return value
has auto_ptr-like
implemt -> implement

http://codereview.chromium.org/8738001/diff/1001/base/bind_unittest.cc#newcod...
base/bind_unittest.cc:304: // the internal BindState<> would have been
trasnfered to a *temporary*
trasnfered -> transferred

http://codereview.chromium.org/8738001/diff/1001/base/callback.h
File base/callback.h (right):

http://codereview.chromium.org/8738001/diff/1001/base/callback.h#newcode141
base/callback.h:141: // These functions, along with a set of internal templates,
are reponsible for
reponsible -> responsible

http://codereview.chromium.org/8738001/diff/1001/base/callback.h.pump
File base/callback.h.pump (right):

http://codereview.chromium.org/8738001/diff/1001/base/callback.h.pump#newcode146
base/callback.h.pump:146: // These functions, along with a set of internal
templates, are reponsible for
reponsible -> responsible

http://codereview.chromium.org/8738001/diff/1001/base/callback_internal.cc
File base/callback_internal.cc (right):

http://codereview.chromium.org/8738001/diff/1001/base/callback_internal.cc#ne...
base/callback_internal.cc:25: : bind_state_(bind_state),
This constructor should only be called with a newly-constructed bind state,
right?  Can we assert that that is so, i.e. check that bind_state->HasOneRef()
is false before we set bind_state_ to it?

http://codereview.chromium.org/8738001/diff/1001/base/callback_internal.h
File base/callback_internal.h (right):

http://codereview.chromium.org/8738001/diff/1001/base/callback_internal.h#new...
base/callback_internal.h:53: // initializtion of the scoped_refptr.  We do not
also initialize
ztion -> zation

http://codereview.chromium.org/8738001/diff/1001/base/callback_unittest.cc
File base/callback_unittest.cc (right):

http://codereview.chromium.org/8738001/diff/1001/base/callback_unittest.cc#ne...
base/callback_unittest.cc:38: // based on a type we declared in this anonymous
namespace to remove any chance
this anonymous namespace -> the anonymous namespace above

[awong, 2011-12-06 00:21:12]

sorry for the latency...here's the next draft.

http://codereview.chromium.org/8738001/diff/1001/base/bind_unittest.cc
File base/bind_unittest.cc (right):

http://codereview.chromium.org/8738001/diff/1001/base/bind_unittest.cc#newcod...
base/bind_unittest.cc:295: // Test that curring the rvalue result of Bind()
works correclty.
On 2011/12/02 18:54:45, akalin wrote:
> curring -> currying
> 
> Although don't you mean 'binding'?

Not sure there's a distinction...

http://codereview.chromium.org/8738001/diff/1001/base/bind_unittest.cc#newcod...
base/bind_unittest.cc:302: // If we implemt Bind() such that the return value
has auto_ptr-like
On 2011/12/02 18:54:45, akalin wrote:
> implemt -> implement

Done.

http://codereview.chromium.org/8738001/diff/1001/base/bind_unittest.cc#newcod...
base/bind_unittest.cc:304: // the internal BindState<> would have been
trasnfered to a *temporary*
On 2011/12/02 18:54:45, akalin wrote:
> trasnfered -> transferred

Done.

http://codereview.chromium.org/8738001/diff/1001/base/callback.h
File base/callback.h (right):

http://codereview.chromium.org/8738001/diff/1001/base/callback.h#newcode141
base/callback.h:141: // These functions, along with a set of internal templates,
are reponsible for
On 2011/12/02 18:54:45, akalin wrote:
> reponsible -> responsible

Done.

http://codereview.chromium.org/8738001/diff/1001/base/callback.h.pump
File base/callback.h.pump (right):

http://codereview.chromium.org/8738001/diff/1001/base/callback.h.pump#newcode146
base/callback.h.pump:146: // These functions, along with a set of internal
templates, are reponsible for
On 2011/12/02 18:54:45, akalin wrote:
> reponsible -> responsible

Done.

http://codereview.chromium.org/8738001/diff/1001/base/callback_internal.cc
File base/callback_internal.cc (right):

http://codereview.chromium.org/8738001/diff/1001/base/callback_internal.cc#ne...
base/callback_internal.cc:25: : bind_state_(bind_state),
On 2011/12/02 18:54:45, akalin wrote:
> This constructor should only be called with a newly-constructed bind state,
> right?  Can we assert that that is so, i.e. check that bind_state->HasOneRef()
> is false before we set bind_state_ to it?

Added a DCHECK, though it feels slightly overkill since no one should be new-ing
a base::internal struct...

http://codereview.chromium.org/8738001/diff/1001/base/callback_internal.h
File base/callback_internal.h (right):

http://codereview.chromium.org/8738001/diff/1001/base/callback_internal.h#new...
base/callback_internal.h:53: // initializtion of the scoped_refptr.  We do not
also initialize
On 2011/12/02 18:54:45, akalin wrote:
> ztion -> zation

Done.

http://codereview.chromium.org/8738001/diff/1001/base/callback_unittest.cc
File base/callback_unittest.cc (right):

http://codereview.chromium.org/8738001/diff/1001/base/callback_unittest.cc#ne...
base/callback_unittest.cc:38: // based on a type we declared in this anonymous
namespace to remove any chance
On 2011/12/02 18:54:45, akalin wrote:
> this anonymous namespace -> the anonymous namespace above

Done.

[akalin, 2011-12-06 17:57:57]

LGTM, although the trybot failures look legit

http://codereview.chromium.org/8738001/diff/1001/base/bind_unittest.cc
File base/bind_unittest.cc (right):

http://codereview.chromium.org/8738001/diff/1001/base/bind_unittest.cc#newcod...
base/bind_unittest.cc:295: // Test that curring the rvalue result of Bind()
works correclty.
On 2011/12/06 00:21:12, awong wrote:
> On 2011/12/02 18:54:45, akalin wrote:
> > curring -> currying
> > 
> > Although don't you mean 'binding'?
> 
> Not sure there's a distinction...

currying = changing a function '(A, B) -> C' to 'A -> (B -> C)'.

partial application (or binding) = taking a function '(A, B...) -> C' and a
value a of type A and returning a function '(B...) -> C'.

[awong, 2011-12-08 20:08:11]

On 2011/12/06 17:57:57, akalin wrote:
> LGTM, although the trybot failures look legit
> 
> http://codereview.chromium.org/8738001/diff/1001/base/bind_unittest.cc
> File base/bind_unittest.cc (right):
> 
>
http://codereview.chromium.org/8738001/diff/1001/base/bind_unittest.cc#newcod...
> base/bind_unittest.cc:295: // Test that curring the rvalue result of Bind()
> works correclty.
> On 2011/12/06 00:21:12, awong wrote:
> > On 2011/12/02 18:54:45, akalin wrote:
> > > curring -> currying
> > > 
> > > Although don't you mean 'binding'?
> > 
> > Not sure there's a distinction...
> 
> currying = changing a function '(A, B) -> C' to 'A -> (B -> C)'.
> 
> partial application (or binding) = taking a function '(A, B...) -> C' and a
> value a of type A and returning a function '(B...) -> C'.

Ah...replaced the wording with "binding."

*sigh* My PL-fu has grown weak.

[awong, 2011-12-13 02:22:07]

I needed to fix some type safety strictness issues.

@willchan:
   net/* + base/* is yours.
   Ignore base/bind* and base/callback*.  Fred already got those.

acolwell:
   media/* is yours

akalin:
   chrome/browser/sync/* is yours.

[willchan no longer on Chromium, 2011-12-13 02:29:13]

lgtm

[acolwell GONE FROM CHROMIUM, 2011-12-13 02:47:16]

LGTM

Do we get compile errors now for what was happening in the media code (ie
binding to a void(const T&) method, but the callback needed was a void(T))?

[awong, 2011-12-13 02:56:39]

On Mon, Dec 12, 2011 at 6:47 PM, <acolwell@chromium.org> wrote:

> LGTM
>
> Do we get compile errors now for what was happening in the media code (ie
> binding to a void(const T&) method, but the callback needed was a void(T))?
>

yep.

[awong, 2011-12-13 20:06:23]

akalin: added new code to the sync/* section.  Mind taking another look?

[akalin, 2011-12-14 20:20:33]

On 2011/12/13 20:06:23, awong wrote:
> akalin: added new code to the sync/* section.  Mind taking another look?

sync lgtm

[commit-bot: I haz the power, 2011-12-14 20:21:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/8738001/22001

[commit-bot: I haz the power, 2011-12-14 21:34:01]

Change committed as 114494

[sail, 2011-12-14 21:49:28]

Build error logs:
http://build.chromium.org/p/chromium/builders/Linux%20Builder%20%28dbg%29%28s...