Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskRunner::postCancellableTask (3)

Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskWebTaskRunner::postCancellableTask (3)

This CL completely removes CancellableTaskFactory.

BUG=665285
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/387c97d5cf93a15e1b547bf9f96c8c441fe9d035
Cr-Commit-Position: refs/heads/master@{#434428}

[nhiroki, 2016-11-18 05:22:11]

Description was changed from

==========
Scheduler: Deprecate CancellableTaskFactory in favor of
WebTaskScheduller::postCancellableTask (3)

This CL completely removes CancellableTaskFactory.

BUG=665285
==========

to

==========
Scheduler: Deprecate CancellableTaskFactory in favor of
WebTaskScheduller::postCancellableTask (3)

This CL completely removes CancellableTaskFactory.

BUG=665285
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[nhiroki, 2016-11-18 05:37:13]

Patchset #1 (id:1) has been deleted

[nhiroki, 2016-11-18 05:37:20]

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

[commit-bot: I haz the power, 2016-11-18 05:37:43]

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-11-18 06:26:13]

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

[commit-bot: I haz the power, 2016-11-18 06:26:14]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[nhiroki, 2016-11-18 07:29:58]

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

[commit-bot: I haz the power, 2016-11-18 07:30:21]

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

[nhiroki, 2016-11-18 08:00:31]

Description was changed from

==========
Scheduler: Deprecate CancellableTaskFactory in favor of
WebTaskScheduller::postCancellableTask (3)

This CL completely removes CancellableTaskFactory.

BUG=665285
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Scheduler: Deprecate CancellableTaskFactory in favor of
WebTaskWebTaskRunner::postCancellableTask (3)

This CL completely removes CancellableTaskFactory.

BUG=665285
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-11-18 08:35:50]

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

[commit-bot: I haz the power, 2016-11-18 08:35:51]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[nhiroki, 2016-11-21 04:30:04]

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

[commit-bot: I haz the power, 2016-11-21 04:30:17]

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-11-21 05:00:47]

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

[commit-bot: I haz the power, 2016-11-21 05:00:49]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[nhiroki, 2016-11-21 05:30:29]

Patchset #2 (id:40001) has been deleted

[nhiroki, 2016-11-21 05:40:52]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2016-11-21 05:44:32]

nhiroki@chromium.org changed reviewers:
+ bajones@chromium.org, haraken@chromium.org, tzik@chromium.org

[nhiroki, 2016-11-21 05:45:54]

Hi, can you review this?

+bajones@ for modules/webgl/
+haraken@ and tzik@ for the entire changes

(Do you know who is an appropriate reviewer of platform/mac?)

Thanks.

https://codereview.chromium.org/2514733002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/2514733002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:1067:
wrapWeakPersistent(this)),
Can we make this wrapPersistent(this)? I'm assuming that
ScrollAnimatorMac::dispose() is always called when ScrollAnimatorMac is no
longer necessary and it cuts a strong reference from a posted task to |this|.

(I tried to confirm this but I couldn't trace who calls
ScrollAnimatorMac::dispose() on the codesearch)

https://codereview.chromium.org/2514733002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:1068: 0.1);
This fails compile on Mac bots as follows:

[29712/38626] OBJCXX
obj/third_party/WebKit/Source/platform/platform/ScrollAnimatorMac.o
FAILED: obj/third_party/WebKit/Source/platform/platform/ScrollAnimatorMac.o 
../../third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:1068:7: error:
implicit conversion from 'double' to 'long long' changes value from 0.1 to 0
[-Werror,-Wliteral-conversion]
      0.1);
      ^~~
1 error generated.

I'm not sure why the original code was compilable. postDelayedTask() also takes
"long long" as the 3rd argument. Anyway, should we replace this with
postCancellableTask()?

https://codereview.chromium.org/2514733002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:1095:
wrapWeakPersistent(this)));
ditto.

[haraken, 2016-11-21 06:42:24]

tzik@: I just want to confirm, but have you already implemented a mechanism that
stops triggering cancellable tasks after the host object is marked as dead by
Oilpan's GC? (I couldn't find the code.)

It looks dangerous to increase usages of postCancellableTask without having the
mechanism.

https://codereview.chromium.org/2514733002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/2514733002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:1067:
wrapWeakPersistent(this)),
On 2016/11/21 05:45:54, nhiroki wrote:
> Can we make this wrapPersistent(this)? I'm assuming that
> ScrollAnimatorMac::dispose() is always called when ScrollAnimatorMac is no
> longer necessary and it cuts a strong reference from a posted task to |this|.
> 
> (I tried to confirm this but I couldn't trace who calls
> ScrollAnimatorMac::dispose() on the codesearch)

It's called by a pre-finalizer:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mac/S...

In other words, dispose() is called after all strong references to the scroll
animator object is gone and Oilpan has decided to destruct the object. So we
should use wrapWeakPersistent.

That said, using wrapPersistent won't cause a big issue because the timer will
be invoked and the persistent handle is dropped in a finite time period.

https://codereview.chromium.org/2514733002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:1068: 0.1);
On 2016/11/21 05:45:54, nhiroki wrote:
> This fails compile on Mac bots as follows:
> 
> [29712/38626] OBJCXX
> obj/third_party/WebKit/Source/platform/platform/ScrollAnimatorMac.o
> FAILED: obj/third_party/WebKit/Source/platform/platform/ScrollAnimatorMac.o 
> ../../third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:1068:7:
error:
> implicit conversion from 'double' to 'long long' changes value from 0.1 to 0
> [-Werror,-Wliteral-conversion]
>       0.1);
>       ^~~
> 1 error generated.
> 
> I'm not sure why the original code was compilable. postDelayedTask() also
takes
> "long long" as the 3rd argument. Anyway, should we replace this with
> postCancellableTask()?

Yeah, 0.1 looks like a random number. +1 to changing it to postCancellableTask.

[nhiroki, 2016-11-21 08:13:13]

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

[commit-bot: I haz the power, 2016-11-21 08:13:24]

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

[nhiroki, 2016-11-21 08:15:16]

Thanks for your comments. Updated.

https://codereview.chromium.org/2514733002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/2514733002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:1067:
wrapWeakPersistent(this)),
On 2016/11/21 06:42:24, haraken wrote:
> On 2016/11/21 05:45:54, nhiroki wrote:
> > Can we make this wrapPersistent(this)? I'm assuming that
> > ScrollAnimatorMac::dispose() is always called when ScrollAnimatorMac is no
> > longer necessary and it cuts a strong reference from a posted task to
|this|.
> > 
> > (I tried to confirm this but I couldn't trace who calls
> > ScrollAnimatorMac::dispose() on the codesearch)
> 
> It's called by a pre-finalizer:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mac/S...
> 
> In other words, dispose() is called after all strong references to the scroll
> animator object is gone and Oilpan has decided to destruct the object. So we
> should use wrapWeakPersistent.
> 
> That said, using wrapPersistent won't cause a big issue because the timer will
> be invoked and the persistent handle is dropped in a finite time period.

Thank you for the clarification :)

https://codereview.chromium.org/2514733002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:1068: 0.1);
On 2016/11/21 06:42:24, haraken wrote:
> On 2016/11/21 05:45:54, nhiroki wrote:
> > This fails compile on Mac bots as follows:
> > 
> > [29712/38626] OBJCXX
> > obj/third_party/WebKit/Source/platform/platform/ScrollAnimatorMac.o
> > FAILED: obj/third_party/WebKit/Source/platform/platform/ScrollAnimatorMac.o 
> > ../../third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:1068:7:
> error:
> > implicit conversion from 'double' to 'long long' changes value from 0.1 to 0
> > [-Werror,-Wliteral-conversion]
> >       0.1);
> >       ^~~
> > 1 error generated.
> > 
> > I'm not sure why the original code was compilable. postDelayedTask() also
> takes
> > "long long" as the 3rd argument. Anyway, should we replace this with
> > postCancellableTask()?
> 
> Yeah, 0.1 looks like a random number. +1 to changing it to
postCancellableTask.

Done.

[commit-bot: I haz the power, 2016-11-21 09:52:13]

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

[commit-bot: I haz the power, 2016-11-21 09:52:14]

Dry run: This issue passed the CQ dry run.

[bajones, 2016-11-21 16:06:57]

Updates to modules/webgl LGTM

[tzik, 2016-11-22 06:20:28]

On 2016/11/21 06:42:24, haraken wrote:
> tzik@: I just want to confirm, but have you already implemented a mechanism
that
> stops triggering cancellable tasks after the host object is marked as dead by
> Oilpan's GC? (I couldn't find the code.)
> 
> It looks dangerous to increase usages of postCancellableTask without having
the
> mechanism.
> 

Yes. It just relies on WeakPersistent invalidation to handle that case.
When an object is marked as dead, WeakPersistent to the object will be cleared,
and that implies the task cancellation.

[tzik, 2016-11-22 06:20:44]

lgtm

[nhiroki, 2016-11-24 08:05:10]

haraken@, ping?

[haraken, 2016-11-24 08:16:21]

On 2016/11/24 08:05:10, nhiroki (slow-sheriff) wrote:
> haraken@, ping?

Sorry, I missed this. LGTM.

[nhiroki, 2016-11-24 08:16:55]

On 2016/11/24 08:16:21, haraken wrote:
> On 2016/11/24 08:05:10, nhiroki (slow-sheriff) wrote:
> > haraken@, ping?
> 
> Sorry, I missed this. LGTM.

Np! Thank you :)

[nhiroki, 2016-11-24 08:17:09]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2016-11-24 08:17:44]

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-11-24 10:18:59]

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

[commit-bot: I haz the power, 2016-11-24 10:19:00]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_optional_gpu_tests_rel on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[nhiroki, 2016-11-24 14:40:19]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2016-11-24 14:41:15]

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-11-24 15:24:32]

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

[commit-bot: I haz the power, 2016-11-24 15:24:33]

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

[nhiroki, 2016-11-24 23:46:21]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2016-11-24 23:46:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, bajones@chromium.org, tzik@chromium.org
Link to the patchset: https://codereview.chromium.org/2514733002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-24 23:46:33]

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-11-25 00:06:31]

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

[commit-bot: I haz the power, 2016-11-25 00:06:32]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[nhiroki, 2016-11-25 00:45:14]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2016-11-25 00:45:25]

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-11-25 03:03:13]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480034714461530,
"parent_rev": "1d3c9963617d47a306bf0587913e12f6593480b0", "commit_rev":
"56b94c93b75f4d5486dc4771546cecb3e99e7d63"}

[commit-bot: I haz the power, 2016-11-25 03:03:35]

Description was changed from

==========
Scheduler: Deprecate CancellableTaskFactory in favor of
WebTaskWebTaskRunner::postCancellableTask (3)

This CL completely removes CancellableTaskFactory.

BUG=665285
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Scheduler: Deprecate CancellableTaskFactory in favor of
WebTaskWebTaskRunner::postCancellableTask (3)

This CL completely removes CancellableTaskFactory.

BUG=665285
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-11-25 03:03:36]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-11-25 03:05:50]

Description was changed from

==========
Scheduler: Deprecate CancellableTaskFactory in favor of
WebTaskWebTaskRunner::postCancellableTask (3)

This CL completely removes CancellableTaskFactory.

BUG=665285
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Scheduler: Deprecate CancellableTaskFactory in favor of
WebTaskWebTaskRunner::postCancellableTask (3)

This CL completely removes CancellableTaskFactory.

BUG=665285
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/387c97d5cf93a15e1b547bf9f96c8c441fe9d035
Cr-Commit-Position: refs/heads/master@{#434428}
==========

[commit-bot: I haz the power, 2016-11-25 03:05:52]

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