Stop using Resource::ResourceCallback and make Resource have a TaskHandle

Stop using Resource::ResourceCallback and make Resource have a TaskHandle

Resource::ResourceCallback was made to be static local by crrev.com/16365011.
It was intended to uses a single timer for all Resources.
But I don't think this optimization has an impact in the real world web.
To make the code simple, I'd like to remove the ResourceCallback and make the
Resource have a TaskHandle.

This change will make it easy to support off-main-thread fetch.

BUG=443374
Review-Url: https://codereview.chromium.org/2876453002
Cr-Commit-Position: refs/heads/master@{#470897}
Committed: https://chromium.googlesource.com/chromium/src/+/1b18cecc5e51e5946b1c224bcbfd8fd71bd7c23d

[horo, 2017-05-10 03:38:41]

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

[commit-bot: I haz the power, 2017-05-10 03:39:13]

Dry run: CQ is trying da patch.

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

[horo, 2017-05-10 03:43:49]

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

[commit-bot: I haz the power, 2017-05-10 03:44:55]

Dry run: CQ is trying da patch.

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

[horo, 2017-05-10 05:12:24]

Patchset #1 (id:1) has been deleted

[horo, 2017-05-10 05:12:30]

Patchset #1 (id:20001) has been deleted

[horo, 2017-05-10 05:14:02]

Description was changed from

==========
Stop using Resource::ResourceCallback

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

to

==========
Stop using Resource::ResourceCallback

Resource::ResourceCallback was made to be static local by crrev.com/16365011.
It was intended to uses a single timer for all Resources.
But I don't think this optimization has an impact in the real world web.
To make the code simple, I'd like to remove the ResourceCallback and make the
Resource have a TaskHandle.

This change will make it easy to support off-main-thread fetch.

BUG=443374
==========

[horo, 2017-05-10 05:17:42]

Description was changed from

==========
Stop using Resource::ResourceCallback

Resource::ResourceCallback was made to be static local by crrev.com/16365011.
It was intended to uses a single timer for all Resources.
But I don't think this optimization has an impact in the real world web.
To make the code simple, I'd like to remove the ResourceCallback and make the
Resource have a TaskHandle.

This change will make it easy to support off-main-thread fetch.

BUG=443374
==========

to

==========
Stop using Resource::ResourceCallback and make Resource have a TaskHandle

Resource::ResourceCallback was made to be static local by crrev.com/16365011.
It was intended to uses a single timer for all Resources.
But I don't think this optimization has an impact in the real world web.
To make the code simple, I'd like to remove the ResourceCallback and make the
Resource have a TaskHandle.

This change will make it easy to support off-main-thread fetch.

BUG=443374
==========

[horo, 2017-05-10 05:43:49]

horo@chromium.org changed reviewers:
+ yhirano@chromium.org

[horo, 2017-05-10 05:43:50]

yhirano@
Please review this.

[commit-bot: I haz the power, 2017-05-10 06:57:00]

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

[commit-bot: I haz the power, 2017-05-10 06:57:00]

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

[yhirano, 2017-05-10 06:59:17]

https://codereview.chromium.org/2876453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/Resource.cpp (right):

https://codereview.chromium.org/2876453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/Resource.cpp:695: if
(clients_awaiting_callback_.IsEmpty())
The original code contains |task_runner_.IsActive()| in the condition.

... that said, it looks Cancel() does nothing when not active. Removing the
check is no problem, but I think L778-L780 should be consistent on the point.

[horo, 2017-05-10 07:04:42]

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

[commit-bot: I haz the power, 2017-05-10 07:05:10]

Dry run: CQ is trying da patch.

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

[horo, 2017-05-10 07:05:30]

https://codereview.chromium.org/2876453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/Resource.cpp (right):

https://codereview.chromium.org/2876453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/Resource.cpp:695: if
(clients_awaiting_callback_.IsEmpty())
On 2017/05/10 06:59:17, yhirano wrote:
> The original code contains |task_runner_.IsActive()| in the condition.
> 
> ... that said, it looks Cancel() does nothing when not active. Removing the
> check is no problem, but I think L778-L780 should be consistent on the point.

Done.

[yhirano, 2017-05-10 07:08:45]

lgtm

[kinuko, 2017-05-10 07:18:09]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2017-05-10 07:18:10]

cool! (lgtm/2)

https://codereview.chromium.org/2876453002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/Resource.h (right):

https://codereview.chromium.org/2876453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/Resource.h:483: TaskHandle
async_callback_task_handle_;
nit: async_finish_pending_clients_task_ or something like that?

[horo, 2017-05-10 07:33:44]

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

[commit-bot: I haz the power, 2017-05-10 07:34:12]

Dry run: CQ is trying da patch.

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

[horo, 2017-05-10 07:35:09]

Thank you.

https://codereview.chromium.org/2876453002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/loader/fetch/Resource.h (right):

https://codereview.chromium.org/2876453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/loader/fetch/Resource.h:483: TaskHandle
async_callback_task_handle_;
On 2017/05/10 07:18:09, kinuko wrote:
> nit: async_finish_pending_clients_task_ or something like that?

Done.

[horo, 2017-05-10 09:40:18]

horo@chromium.org changed reviewers:
+ japhet@chromium.org

[horo, 2017-05-10 09:40:19]

japhet@
Could you please review this?

[commit-bot: I haz the power, 2017-05-10 10:37:41]

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

[commit-bot: I haz the power, 2017-05-10 10:37:41]

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

[Nate Chapin, 2017-05-10 17:30:58]

lgtm

[horo, 2017-05-10 20:51:04]

The CQ bit was checked by horo@chromium.org

[horo, 2017-05-10 20:51:05]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2876453002/#ps80001
(title: "s/async_callback_task_handle_/async_finish_pending_clients_task_/")

[commit-bot: I haz the power, 2017-05-10 20:52: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, 2017-05-10 23:47:06]

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

[commit-bot: I haz the power, 2017-05-10 23:47:07]

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

[horo, 2017-05-11 00:55:10]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2017-05-11 00:56:40]

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, 2017-05-11 03:57:23]

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

[commit-bot: I haz the power, 2017-05-11 03:57:25]

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

[horo, 2017-05-11 04:01:38]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2017-05-11 04:03:37]

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, 2017-05-11 07:08:00]

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

[commit-bot: I haz the power, 2017-05-11 07:08:02]

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

[horo, 2017-05-11 07:12:58]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2017-05-11 07:13: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, 2017-05-11 07:52:20]

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

[commit-bot: I haz the power, 2017-05-11 07:52:21]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[horo, 2017-05-11 07:53:28]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2017-05-11 07:53:59]

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, 2017-05-11 10:17:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494489208309820,
"parent_rev": "2a628672163491c90fde73c7c467fb059455dc28", "commit_rev":
"1b18cecc5e51e5946b1c224bcbfd8fd71bd7c23d"}

[commit-bot: I haz the power, 2017-05-11 10:17:56]

Description was changed from

==========
Stop using Resource::ResourceCallback and make Resource have a TaskHandle

Resource::ResourceCallback was made to be static local by crrev.com/16365011.
It was intended to uses a single timer for all Resources.
But I don't think this optimization has an impact in the real world web.
To make the code simple, I'd like to remove the ResourceCallback and make the
Resource have a TaskHandle.

This change will make it easy to support off-main-thread fetch.

BUG=443374
==========

to

==========
Stop using Resource::ResourceCallback and make Resource have a TaskHandle

Resource::ResourceCallback was made to be static local by crrev.com/16365011.
It was intended to uses a single timer for all Resources.
But I don't think this optimization has an impact in the real world web.
To make the code simple, I'd like to remove the ResourceCallback and make the
Resource have a TaskHandle.

This change will make it easy to support off-main-thread fetch.

BUG=443374

Review-Url: https://codereview.chromium.org/2876453002
Cr-Commit-Position: refs/heads/master@{#470897}
Committed:
https://chromium.googlesource.com/chromium/src/+/1b18cecc5e51e5946b1c224bcbfd...
==========

[commit-bot: I haz the power, 2017-05-11 10:17:57]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/1b18cecc5e51e5946b1c224bcbfd...