[scheduler] Fix TaskQueueThrottler::SetQueueEnabled logic.

[scheduler] Fix TaskQueueThrottler::SetQueueEnabled logic.

- At the moment check for scheduling a call to PumpThrottledTasks in
  TaskQueueThrottler::SetQueueEnabled is wrong, should be (enabled)
  instead of (!enabled).
- Added tzik@'s test.
- Improved comments in TaskQueueThrottler::SetQueueEnabled.

BUG=639852
Committed: https://crrev.com/2aefa856b292402f4be012f6006eebb55f2abf51
Cr-Commit-Position: refs/heads/master@{#434394}

[altimin, 2016-11-24 14:22:36]

altimin@chromium.org changed reviewers:
+ alexclarke@chromium.org, skyostil@chromium.org, tzik@chromium.org

[altimin, 2016-11-24 14:22:37]

PTAL

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

https://codereview.chromium.org/2528963002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
(right):

https://codereview.chromium.org/2528963002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:54:
std::string PointerToId(void* pointer) {
Can we land this separately?  It seems unrelated to the fix.

https://codereview.chromium.org/2528963002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2528963002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:320:
// If TaskQueueThrottler does not know about this queue, just call
Please add a comment in here saying:

Both the TaskQueueThrottler and other systems want to enable and disable task
queues.  The policy we've adopted to deal with this is:

A task queue is disabled if either the TaskQueueThrottler or the caller vote to
disable it.

A task queue is enabled only if both the TaskQueueThrottler and the caller vote
to enable it.

https://codereview.chromium.org/2528963002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:327:
// Even when this queue is not throttled by task queue throttler, we
Maybe:

// Remember the caller's preference so we know what to do when the task queue
isn't throttled, or has budget to run.

https://codereview.chromium.org/2528963002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:340:
task_queue->SetQueueEnabled(false);
That doesn't look right to me.  I think it should be:

if (enabled) { 
  MaybeSchedulePumpQueue(...) 
} else { 
  task_queue->SetQueueEnabled(false);
}

[altimin, 2016-11-24 14:52:07]

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

[commit-bot: I haz the power, 2016-11-24 14:52:36]

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-24 14:56:10]

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

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

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[altimin, 2016-11-24 15:00:55]

PTAL

https://codereview.chromium.org/2528963002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
(right):

https://codereview.chromium.org/2528963002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:54:
std::string PointerToId(void* pointer) {
On 2016/11/24 14:45:07, alex clarke wrote:
> Can we land this separately?  It seems unrelated to the fix.

Oops, sorry.

https://codereview.chromium.org/2528963002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2528963002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:320:
// If TaskQueueThrottler does not know about this queue, just call
On 2016/11/24 14:45:07, alex clarke wrote:
> Please add a comment in here saying:
> 
> Both the TaskQueueThrottler and other systems want to enable and disable task
> queues.  The policy we've adopted to deal with this is:
> 
> A task queue is disabled if either the TaskQueueThrottler or the caller vote
to
> disable it.
> 
> A task queue is enabled only if both the TaskQueueThrottler and the caller
vote
> to enable it.

Done.

https://codereview.chromium.org/2528963002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:327:
// Even when this queue is not throttled by task queue throttler, we
On 2016/11/24 14:45:07, alex clarke wrote:
> Maybe:
> 
> // Remember the caller's preference so we know what to do when the task queue
> isn't throttled, or has budget to run.

Done.

https://codereview.chromium.org/2528963002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:340:
task_queue->SetQueueEnabled(false);
On 2016/11/24 14:45:07, alex clarke wrote:
> That doesn't look right to me.  I think it should be:
> 
> if (enabled) { 
>   MaybeSchedulePumpQueue(...) 
> } else { 
>   task_queue->SetQueueEnabled(false);
> }

That's intentional. Even when queue is enabled, we want to disable it at the
moment and enable in PumpThrottledTasks.

[alex clarke (OOO till 29th), 2016-11-24 15:29:30]

lgtm

[altimin, 2016-11-24 15:40:27]

The CQ bit was checked by altimin@chromium.org

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

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:43:00]

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

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

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[altimin, 2016-11-24 15:48:12]

Description was changed from

==========
[scheduler] Fix TaskQueueThrottler::SetQueueEnabled logic.

- At the moment check for scheduling a call to PumpThrottledTasks in
  TaskQueueThrottler::SetQueueEnabled is wrong, should be (enabled)
  instead of (!enabled).
- Added tzik@'s test.
- Improved comments in TaskQueueThrottler::SetQueueEnabled.
==========

to

==========
[scheduler] Fix TaskQueueThrottler::SetQueueEnabled logic.

- At the moment check for scheduling a call to PumpThrottledTasks in
  TaskQueueThrottler::SetQueueEnabled is wrong, should be (enabled)
  instead of (!enabled).
- Added tzik@'s test.
- Improved comments in TaskQueueThrottler::SetQueueEnabled.

BUG=639852
==========

[Sami, 2016-11-24 16:05:50]

https://codereview.chromium.org/2528963002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2528963002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:341:
if (!find_it->second.IsThrottled()) {
I think this needs a rebase; IsThrottled() isn't there anymore.

[altimin, 2016-11-24 16:09:36]

https://codereview.chromium.org/2528963002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
(right):

https://codereview.chromium.org/2528963002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:341:
if (!find_it->second.IsThrottled()) {
On 2016/11/24 16:05:49, Sami wrote:
> I think this needs a rebase; IsThrottled() isn't there anymore.

It does indeed, thanks.

[altimin, 2016-11-24 16:10:38]

The CQ bit was checked by altimin@chromium.org

[altimin, 2016-11-24 16:10:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2528963002/#ps60001
(title: "Rebased")

[commit-bot: I haz the power, 2016-11-24 16:10:57]

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 17:10:18]

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

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

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

[altimin, 2016-11-24 19:48:57]

The CQ bit was checked by altimin@chromium.org

[commit-bot: I haz the power, 2016-11-24 19:49:09]

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 21:50:39]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480016937958390,
"parent_rev": "77792e182ebcc0fb9e489efe5f0861e1c21d696f", "commit_rev":
"90b6bc00c2f4f6a2ec4f679f6e2ce2eb9c675ebd"}

[commit-bot: I haz the power, 2016-11-24 21:51:01]

Description was changed from

==========
[scheduler] Fix TaskQueueThrottler::SetQueueEnabled logic.

- At the moment check for scheduling a call to PumpThrottledTasks in
  TaskQueueThrottler::SetQueueEnabled is wrong, should be (enabled)
  instead of (!enabled).
- Added tzik@'s test.
- Improved comments in TaskQueueThrottler::SetQueueEnabled.

BUG=639852
==========

to

==========
[scheduler] Fix TaskQueueThrottler::SetQueueEnabled logic.

- At the moment check for scheduling a call to PumpThrottledTasks in
  TaskQueueThrottler::SetQueueEnabled is wrong, should be (enabled)
  instead of (!enabled).
- Added tzik@'s test.
- Improved comments in TaskQueueThrottler::SetQueueEnabled.

BUG=639852
==========

[commit-bot: I haz the power, 2016-11-24 21:51:02]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-24 21:54:00]

Description was changed from

==========
[scheduler] Fix TaskQueueThrottler::SetQueueEnabled logic.

- At the moment check for scheduling a call to PumpThrottledTasks in
  TaskQueueThrottler::SetQueueEnabled is wrong, should be (enabled)
  instead of (!enabled).
- Added tzik@'s test.
- Improved comments in TaskQueueThrottler::SetQueueEnabled.

BUG=639852
==========

to

==========
[scheduler] Fix TaskQueueThrottler::SetQueueEnabled logic.

- At the moment check for scheduling a call to PumpThrottledTasks in
  TaskQueueThrottler::SetQueueEnabled is wrong, should be (enabled)
  instead of (!enabled).
- Added tzik@'s test.
- Improved comments in TaskQueueThrottler::SetQueueEnabled.

BUG=639852

Committed: https://crrev.com/2aefa856b292402f4be012f6006eebb55f2abf51
Cr-Commit-Position: refs/heads/master@{#434394}
==========

[commit-bot: I haz the power, 2016-11-24 21:54:01]

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