Add Callback::IsCancelled

Add Callback::IsCancelled

Callback::IsCancelled() checks if the Callback is cancelled by WeakPtr
invalidation.

Committed: https://crrev.com/59aa6bb1162b31d2af31c4a235c67826687b6b28
Cr-Commit-Position: refs/heads/master@{#417248}

[tzik, 2016-08-29 11:14:23]

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

[commit-bot: I haz the power, 2016-08-29 11:14:34]

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-08-29 13:03:15]

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

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

Dry run: Exceeded global retry quota

[tzik, 2016-08-30 12:27:46]

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

[commit-bot: I haz the power, 2016-08-30 12:28:02]

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-08-30 12:38:54]

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

[commit-bot: I haz the power, 2016-08-30 12:38:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tzik, 2016-08-30 13:10:30]

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

[commit-bot: I haz the power, 2016-08-30 13:10:54]

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

[tzik, 2016-08-30 14:34:53]

Description was changed from

==========
Add Callback::IsCancelled

BUG=
==========

to

==========
Add Callback::IsCancelled

TODO: Check the impact to the binary size.
 * Stripped binary size on Linux + gold -> +140kB
 * same + lld -> +440kB
 * win, mac, android?

BUG=
==========

[commit-bot: I haz the power, 2016-08-30 15:55:55]

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

[commit-bot: I haz the power, 2016-08-30 15:55:56]

Dry run: This issue passed the CQ dry run.

[tzik, 2016-09-07 08:13:59]

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

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

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-07 08:15:18]

Description was changed from

==========
Add Callback::IsCancelled

TODO: Check the impact to the binary size.
 * Stripped binary size on Linux + gold -> +140kB
 * same + lld -> +440kB
 * win, mac, android?

BUG=
==========

to

==========
Add Callback::IsCancelled

BUG=
==========

[tzik, 2016-09-07 08:15:50]

Description was changed from

==========
Add Callback::IsCancelled

BUG=
==========

to

==========
Add Callback::IsCancelled

TODO: description

BUG=
==========

[tzik, 2016-09-07 08:15:50]

tzik@chromium.org changed reviewers:
+ alexclarke@chromium.org, dcheng@chromium.org, yutak@chromium.org

[tzik, 2016-09-07 08:19:57]

Description was changed from

==========
Add Callback::IsCancelled

TODO: description

BUG=
==========

to

==========
Add Callback::IsCancelled

TODO: description & doc comment

BUG=
==========

[tzik, 2016-09-07 08:42:22]

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

[commit-bot: I haz the power, 2016-09-07 08:42:41]

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-07 08:45:23]

Description was changed from

==========
Add Callback::IsCancelled

TODO: description & doc comment

BUG=
==========

to

==========
Add Callback::IsCancelled

Callback::IsCancelled() checks if the Callback is cancelled by WeakPtr
invalidation.
==========

[tzik, 2016-09-07 08:45:39]

PTAL

[alex clarke (OOO till 29th), 2016-09-07 09:13:12]

This has the functionality I'd like, and it's significantly simpler than the
methods I was considering for doing this :)

On an official build what's the binary size delta for Android?

[Yuta Kitamura, 2016-09-07 10:31:25]

lgtm

[tzik, 2016-09-07 11:04:47]

On 2016/09/07 09:13:12, alex clarke wrote:
> This has the functionality I'd like, and it's significantly simpler than the
> methods I was considering for doing this :)
> 
> On an official build what's the binary size delta for Android?

The size delta of this CL is +41672 bytes on ChromePublic.apk, which is ~0.07%,
and the total delta of other CLs is +25268 bytes.

[alex clarke (OOO till 29th), 2016-09-07 11:06:20]

On 2016/09/07 11:04:47, tzik wrote:
> On 2016/09/07 09:13:12, alex clarke wrote:
> > This has the functionality I'd like, and it's significantly simpler than the
> > methods I was considering for doing this :)
> > 
> > On an official build what's the binary size delta for Android?
> 
> The size delta of this CL is +41672 bytes on ChromePublic.apk, which is
~0.07%,
> and the total delta of other CLs is +25268 bytes.

Good that's tiny.

LGTM

[alex clarke (OOO till 29th), 2016-09-07 11:06:45]

On 2016/09/07 11:06:20, alex clarke wrote:
> On 2016/09/07 11:04:47, tzik wrote:
> > On 2016/09/07 09:13:12, alex clarke wrote:
> > > This has the functionality I'd like, and it's significantly simpler than
the
> > > methods I was considering for doing this :)
> > > 
> > > On an official build what's the binary size delta for Android?
> > 
> > The size delta of this CL is +41672 bytes on ChromePublic.apk, which is
> ~0.07%,
> > and the total delta of other CLs is +25268 bytes.
> 
> Good that's tiny.
> 
> LGTM

I mean this CL;s delta is tiny :)

[tzik, 2016-09-07 11:08:00]

On 2016/09/07 11:04:47, tzik wrote:
> On 2016/09/07 09:13:12, alex clarke wrote:
> > This has the functionality I'd like, and it's significantly simpler than the
> > methods I was considering for doing this :)
> > 
> > On an official build what's the binary size delta for Android?
> 
> The size delta of this CL is +41672 bytes on ChromePublic.apk, which is
~0.07%,
> and the total delta of other CLs is +25268 bytes.

master: 57457059 bytes
http://crrev.com/2317563002: 57440251 bytes (delta: -16808)
http://crrev.com/2310093003: 57440655 bytes (delta: -16404)
this CL: 57482327 bytes (delta: +25268)

[commit-bot: I haz the power, 2016-09-07 12:10:39]

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

[commit-bot: I haz the power, 2016-09-07 12:10:41]

Dry run: This issue passed the CQ dry run.

[tzik, 2016-09-07 14:42:09]

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

[commit-bot: I haz the power, 2016-09-07 14:42:29]

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

[alex clarke (OOO till 29th), 2016-09-07 15:53:53]

https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal.cc
File base/callback_internal.cc (right):

https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal....
base/callback_internal.cc:41: DCHECK(bind_state_);
Can we do this instead of the DCHECK?

if (!bind_state_)
  return false;

[commit-bot: I haz the power, 2016-09-07 16:25:06]

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

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

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-09-07 21:40:27]

LGTM

https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal.cc
File base/callback_internal.cc (right):

https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal....
base/callback_internal.cc:41: DCHECK(bind_state_);
On 2016/09/07 15:53:53, alex clarke wrote:
> Can we do this instead of the DCHECK?
> 
> if (!bind_state_)
>   return false;

If there's a null callback posted to the task queue, we'll explode anyway when
we try to Run() it. So DCHECKing seems fine to me.

[tzik, 2016-09-08 03:44:05]

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

[commit-bot: I haz the power, 2016-09-08 03:44:18]

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-08 03:55:10]

https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal.cc
File base/callback_internal.cc (right):

https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal....
base/callback_internal.cc:41: DCHECK(bind_state_);
On 2016/09/07 21:40:27, dcheng wrote:
> On 2016/09/07 15:53:53, alex clarke wrote:
> > Can we do this instead of the DCHECK?
> > 
> > if (!bind_state_)
> >   return false;
> 
> If there's a null callback posted to the task queue, we'll explode anyway when
> we try to Run() it. So DCHECKing seems fine to me.

Hmm, right, it doesn't matter for the intended usage in the scheduler,  but I
feel it's natural to return false for a empty callback. Let me update this.

[dcheng, 2016-09-08 04:01:33]

https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal.cc
File base/callback_internal.cc (right):

https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal....
base/callback_internal.cc:41: DCHECK(bind_state_);
On 2016/09/08 03:55:10, tzik wrote:
> On 2016/09/07 21:40:27, dcheng wrote:
> > On 2016/09/07 15:53:53, alex clarke wrote:
> > > Can we do this instead of the DCHECK?
> > > 
> > > if (!bind_state_)
> > >   return false;
> > 
> > If there's a null callback posted to the task queue, we'll explode anyway
when
> > we try to Run() it. So DCHECKing seems fine to me.
> 
> Hmm, right, it doesn't matter for the intended usage in the scheduler,  but I
> feel it's natural to return false for a empty callback. Let me update this.

What is the use case where we'd want to call this on an empty callback? I feel
pretty strongly about keeping this a DCHECK: IsCancelled() simply doesn't have
any meaning for an empty callback.

[tzik, 2016-09-08 04:55:36]

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

[commit-bot: I haz the power, 2016-09-08 04:55:57]

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-08 05:21:46]

https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal.cc
File base/callback_internal.cc (right):

https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal....
base/callback_internal.cc:41: DCHECK(bind_state_);
On 2016/09/08 04:01:33, dcheng wrote:
> On 2016/09/08 03:55:10, tzik wrote:
> > On 2016/09/07 21:40:27, dcheng wrote:
> > > On 2016/09/07 15:53:53, alex clarke wrote:
> > > > Can we do this instead of the DCHECK?
> > > > 
> > > > if (!bind_state_)
> > > >   return false;
> > > 
> > > If there's a null callback posted to the task queue, we'll explode anyway
> when
> > > we try to Run() it. So DCHECKing seems fine to me.
> > 
> > Hmm, right, it doesn't matter for the intended usage in the scheduler,  but
I
> > feel it's natural to return false for a empty callback. Let me update this.
> 
> What is the use case where we'd want to call this on an empty callback? I feel
> pretty strongly about keeping this a DCHECK: IsCancelled() simply doesn't have
> any meaning for an empty callback.

There will be no actual use case for it. I just think state query functions
should be always valid.
Anyway, I don't have strong opinion on this. And it's OK to me to revert this to
DCHECK.

[dcheng, 2016-09-08 05:40:18]

On 2016/09/08 05:21:46, tzik wrote:
>
https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal.cc
> File base/callback_internal.cc (right):
> 
>
https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal....
> base/callback_internal.cc:41: DCHECK(bind_state_);
> On 2016/09/08 04:01:33, dcheng wrote:
> > On 2016/09/08 03:55:10, tzik wrote:
> > > On 2016/09/07 21:40:27, dcheng wrote:
> > > > On 2016/09/07 15:53:53, alex clarke wrote:
> > > > > Can we do this instead of the DCHECK?
> > > > > 
> > > > > if (!bind_state_)
> > > > >   return false;
> > > > 
> > > > If there's a null callback posted to the task queue, we'll explode
anyway
> > when
> > > > we try to Run() it. So DCHECKing seems fine to me.
> > > 
> > > Hmm, right, it doesn't matter for the intended usage in the scheduler, 
but
> I
> > > feel it's natural to return false for a empty callback. Let me update
this.
> > 
> > What is the use case where we'd want to call this on an empty callback? I
feel
> > pretty strongly about keeping this a DCHECK: IsCancelled() simply doesn't
have
> > any meaning for an empty callback.
> 
> There will be no actual use case for it. I just think state query functions
> should be always valid.
> Anyway, I don't have strong opinion on this. And it's OK to me to revert this
to
> DCHECK.

LGTM

[commit-bot: I haz the power, 2016-09-08 07:00:06]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tzik, 2016-09-08 07:28:44]

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

[commit-bot: I haz the power, 2016-09-08 07:29:02]

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

[alex clarke (OOO till 29th), 2016-09-08 08:45:03]

On 2016/09/08 04:01:33, dcheng wrote:
>
https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal.cc
> File base/callback_internal.cc (right):
> 
>
https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal....
> base/callback_internal.cc:41: DCHECK(bind_state_);
> On 2016/09/08 03:55:10, tzik wrote:
> > On 2016/09/07 21:40:27, dcheng wrote:
> > > On 2016/09/07 15:53:53, alex clarke wrote:
> > > > Can we do this instead of the DCHECK?
> > > > 
> > > > if (!bind_state_)
> > > >   return false;
> > > 
> > > If there's a null callback posted to the task queue, we'll explode anyway
> when
> > > we try to Run() it. So DCHECKing seems fine to me.
> > 
> > Hmm, right, it doesn't matter for the intended usage in the scheduler,  but
I
> > feel it's natural to return false for a empty callback. Let me update this.
> 
> What is the use case where we'd want to call this on an empty callback? I feel
> pretty strongly about keeping this a DCHECK: IsCancelled() simply doesn't have
> any meaning for an empty callback.

If the DCHECK stays I'm going to have to change a lot unit tests in the blink
scheduler which use tasks with empty closures.

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

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

[commit-bot: I haz the power, 2016-09-08 09:13:23]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2016-09-08 10:01:55]

On 2016/09/08 08:45:03, alex clarke wrote:
> On 2016/09/08 04:01:33, dcheng wrote:
> >
>
https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal.cc
> > File base/callback_internal.cc (right):
> > 
> >
>
https://codereview.chromium.org/2289703002/diff/60001/base/callback_internal....
> > base/callback_internal.cc:41: DCHECK(bind_state_);
> > On 2016/09/08 03:55:10, tzik wrote:
> > > On 2016/09/07 21:40:27, dcheng wrote:
> > > > On 2016/09/07 15:53:53, alex clarke wrote:
> > > > > Can we do this instead of the DCHECK?
> > > > > 
> > > > > if (!bind_state_)
> > > > >   return false;
> > > > 
> > > > If there's a null callback posted to the task queue, we'll explode
anyway
> > when
> > > > we try to Run() it. So DCHECKing seems fine to me.
> > > 
> > > Hmm, right, it doesn't matter for the intended usage in the scheduler, 
but
> I
> > > feel it's natural to return false for a empty callback. Let me update
this.
> > 
> > What is the use case where we'd want to call this on an empty callback? I
feel
> > pretty strongly about keeping this a DCHECK: IsCancelled() simply doesn't
have
> > any meaning for an empty callback.
> 
> If the DCHECK stays I'm going to have to change a lot unit tests in the blink
> scheduler which use tasks with empty closures.

Actually never mind.  I've just gone and fixed all the tests (in my follow on
patch)
that would break due to this.

[tzik, 2016-09-08 10:54:29]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-09-08 10:54:30]

The patchset sent to the CQ was uploaded after l-g-t-m from yutak@chromium.org,
alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2289703002/#ps140001
(title: "revert to PS5")

[commit-bot: I haz the power, 2016-09-08 10:54:45]

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-08 10:59:09]

Description was changed from

==========
Add Callback::IsCancelled

Callback::IsCancelled() checks if the Callback is cancelled by WeakPtr
invalidation.
==========

to

==========
Add Callback::IsCancelled

Callback::IsCancelled() checks if the Callback is cancelled by WeakPtr
invalidation.
==========

[commit-bot: I haz the power, 2016-09-08 10:59:11]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-08 11:02:05]

Description was changed from

==========
Add Callback::IsCancelled

Callback::IsCancelled() checks if the Callback is cancelled by WeakPtr
invalidation.
==========

to

==========
Add Callback::IsCancelled

Callback::IsCancelled() checks if the Callback is cancelled by WeakPtr
invalidation.

Committed: https://crrev.com/59aa6bb1162b31d2af31c4a235c67826687b6b28
Cr-Commit-Position: refs/heads/master@{#417248}
==========

[commit-bot: I haz the power, 2016-09-08 11:02:06]

Patchset 8 (id:??) landed as
https://crrev.com/59aa6bb1162b31d2af31c4a235c67826687b6b28
Cr-Commit-Position: refs/heads/master@{#417248}