DevTools: introduce WebInspector.Throttler

DevTools: introduce WebInspector.Throttler

This patch introduces WebInspector.Throttler and uses it for throttling
calls to PageAgent in OverridesSupport, as well as throttling calls in StylesSourceMapping.

Throttler is a helper object that throttles execution of processes
(possibly asynchronous).
Throttler is created with a single parameter - throttle interval T.
Throttler satisfies to the following contract: for every two consecutive
runs performed by throttler, time between the end of the first run and
start of the second run is always greater or equal to T.

Throttler.schedule has additional second parameter "asSoonAsPossible" which essentially
sets throttler timeout into 0 for the next process run.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175956

[lushnikov, 2014-06-06 14:01:18]

ptal

[vsevik, 2014-06-06 16:14:46]

I think migrating StylesSourceMapping on this would help to explain the
motivation behind this change. :)

https://chromiumcodereview.appspot.com/319143002/diff/1/Source/devtools/front...
File Source/devtools/front_end/sdk/OverridesSupport.js (right):

https://chromiumcodereview.appspot.com/319143002/diff/1/Source/devtools/front...
Source/devtools/front_end/sdk/OverridesSupport.js:589:
PageAgent.clearDeviceMetricsOverride(apiCallback.bind(this, new Function()));
I think we use function() {} usually.

[lushnikov, 2014-06-06 16:38:15]

>I think migrating StylesSourceMapping on this would help to explain the
>motivation behind this change. :)

Change's coming :)

https://chromiumcodereview.appspot.com/319143002/diff/1/Source/devtools/front...
File Source/devtools/front_end/sdk/OverridesSupport.js (right):

https://chromiumcodereview.appspot.com/319143002/diff/1/Source/devtools/front...
Source/devtools/front_end/sdk/OverridesSupport.js:589:
PageAgent.clearDeviceMetricsOverride(apiCallback.bind(this, new Function()));
On 2014/06/06 16:14:45, vsevik wrote:
> I think we use function() {} usually.

Not relevant any more.

[lushnikov, 2014-06-08 14:36:33]

Migrated StylesSourceMapping onto throttler.

[lushnikov, 2014-06-08 17:19:01]

Added jsdoc for Throttler callback

[dgozman, 2014-06-09 07:01:51]

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/common/Throttler.js (right):

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/common/Throttler.js:52: if (this._processTimeout)
How can this be non-null?

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/common/Throttler.js:60: /** @typedef {function()} */
function(function) ?

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/StylesSourceMapping.js (right):

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/sdk/StylesSourceMapping.js:371: var majorChange =
this._pendingChangeType === WebInspector.StyleFile.PendingChangeType.Major;
I still think, that binding such values as parameters of throttled function is
much safer: you don't have to guarantee that |this.something| will have the
right value at the time on invocation.
Since the whole throttler stuff is hard to test, there may be bugs which you
will not catch immediately.

[lushnikov, 2014-06-09 09:45:53]

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/common/Throttler.js (right):

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/common/Throttler.js:52: if (this._processTimeout)
On 2014/06/09 07:01:51, dgozman wrote:
> How can this be non-null?

It is set to null as the timeout fires. @see _onTimeout function

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/common/Throttler.js:60: /** @typedef {function()} */
On 2014/06/09 07:01:51, dgozman wrote:
> function(function) ?

Nope, the typedef here is correct.

[dgozman, 2014-06-09 12:03:34]

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/common/Throttler.js (right):

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/common/Throttler.js:42: this._asSoonAsPossible =
this._asSoonAsPossible || !!asSoonAsPossible;
I think, the general usage of throttler is: immediately issue first operation,
postpone next ones for a certain timeout.
To achieve this, I should pass |asSoonAsPossible=true| to the first call, which
is inconvenient. And how do you I even find out it's the first call?

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/common/Throttler.js:60: /** @typedef {function()} */
On 2014/06/09 09:45:53, lushnikov wrote:
> On 2014/06/09 07:01:51, dgozman wrote:
> > function(function) ?
> 
> Nope, the typedef here is correct.

My bad.

[vsevik, 2014-06-09 12:32:58]

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/OverridesSupport.js (left):

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/sdk/OverridesSupport.js:659:
setDeviceMetricsOverride.call(this);
This one is not called synchronously anymore, could this cause a problem?

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/OverridesSupport.js (right):

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/sdk/OverridesSupport.js:45:
this._deviceMetricsThrottler = new WebInspector.Throttler(0);
why not 100?

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/StylesSourceMapping.js (right):

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/sdk/StylesSourceMapping.js:326: this._commitThrottler
= new WebInspector.Throttler(WebInspector.StyleFile.updateTimeout);
We have (at least used to have) a test that overrides this timeout. You might
need to fix it.

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/sdk/StylesSourceMapping.js:348:
this._pendingChangeType = WebInspector.StyleFile.PendingChangeType.Major;
I believe it should be enough to set this._isMajorChangePending to true here and
reset it in this._commitIncrementalEdit. All usages of this._pendingChangeType
would be redundant then.

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/sdk/StylesSourceMapping.js:371: var majorChange =
this._pendingChangeType === WebInspector.StyleFile.PendingChangeType.Major;
We can not bind pending change type.

[lushnikov, 2014-06-09 13:07:27]

Addressed your comments guys.

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/common/Throttler.js (right):

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/common/Throttler.js:42: this._asSoonAsPossible =
this._asSoonAsPossible || !!asSoonAsPossible;
On 2014/06/09 12:03:33, dgozman wrote:
> I think, the general usage of throttler is: immediately issue first operation,
> postpone next ones for a certain timeout.
> To achieve this, I should pass |asSoonAsPossible=true| to the first call,
which
> is inconvenient. And how do you I even find out it's the first call?

Agreed. Will fix this in next CL

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/OverridesSupport.js (left):

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/sdk/OverridesSupport.js:659:
setDeviceMetricsOverride.call(this);
On 2014/06/09 12:32:57, vsevik wrote:
> This one is not called synchronously anymore, could this cause a problem?

That's intentional, should be fine here.

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/OverridesSupport.js (right):

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/sdk/OverridesSupport.js:45:
this._deviceMetricsThrottler = new WebInspector.Throttler(0);
On 2014/06/09 12:32:58, vsevik wrote:
> why not 100?

Throttler is used here as a message serializer that sends requests one-by-one to
backend.

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/StylesSourceMapping.js (right):

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/sdk/StylesSourceMapping.js:326: this._commitThrottler
= new WebInspector.Throttler(WebInspector.StyleFile.updateTimeout);
On 2014/06/09 12:32:58, vsevik wrote:
> We have (at least used to have) a test that overrides this timeout. You might
> need to fix it.

All tests successfully pass.

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/sdk/StylesSourceMapping.js:348:
this._pendingChangeType = WebInspector.StyleFile.PendingChangeType.Major;
On 2014/06/09 12:32:58, vsevik wrote:
> I believe it should be enough to set this._isMajorChangePending to true here
and
> reset it in this._commitIncrementalEdit. All usages of this._pendingChangeType
> would be redundant then.

Indeed. Fixed this.

https://codereview.chromium.org/319143002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/sdk/StylesSourceMapping.js:371: var majorChange =
this._pendingChangeType === WebInspector.StyleFile.PendingChangeType.Major;
It's tempting, but we can't.

[dgozman, 2014-06-09 13:17:38]

https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
File Source/devtools/front_end/common/Throttler.js (right):

https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/common/Throttler.js:14: this._asSoonAsPossible =
false;
Consider renaming to something like |skipTimeout|.

https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/common/Throttler.js:43: this._asSoonAsPossible =
this._asSoonAsPossible || !!asSoonAsPossible || !this._isRunningProcess;
Should be !(this._isRunningProcess || this._processTimeout), since it's not a
first call if you are waiting for timeout.

Actually, you don't need two flags, just one |isBusy| flag, which you can set
before |setTimeout| and clear in |_processCompleted|.

https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/common/Throttler.js:53: if (this._processTimeout)
I'd rather move this check to the |schedule| function, since it's only possible
there.

https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/OverridesSupport.js (right):

https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/sdk/OverridesSupport.js:627:
this._deviceMetricsThrottler.schedule(clearDeviceMetricsOverride.bind(this),
true);
Can we make second parameter optional? This throttler already has timeout=0.

[dgozman, 2014-06-09 13:18:54]

OverridesSupport lgtm

[pfeldman, 2014-06-09 13:26:45]

https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
File Source/devtools/front_end/common/Throttler.js (right):

https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/common/Throttler.js:43: this._asSoonAsPossible =
this._asSoonAsPossible || !!asSoonAsPossible || !this._isRunningProcess;
This logic seems confusing. So I was scheduling something for as soon as
possible, or I am not running process and it sticks to true?

[lushnikov, 2014-06-09 15:42:28]

On 2014/06/09 13:26:45, pfeldman_ooo wrote:
>
https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
> File Source/devtools/front_end/common/Throttler.js (right):
> 
>
https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
> Source/devtools/front_end/common/Throttler.js:43: this._asSoonAsPossible =
> this._asSoonAsPossible || !!asSoonAsPossible || !this._isRunningProcess;
> This logic seems confusing. So I was scheduling something for as soon as
> possible, or I am not running process and it sticks to true?

I'm not sure I got follow your question. Anyway, the code here was not doing
what it was intended to do.
Please, take a look on the new version.

[lushnikov, 2014-06-09 15:53:42]

https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
File Source/devtools/front_end/common/Throttler.js (right):

https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/common/Throttler.js:14: this._asSoonAsPossible =
false;
On 2014/06/09 13:17:37, dgozman wrote:
> Consider renaming to something like |skipTimeout|.

That's not really "skipTimeout" - we don't want to clear timeout and run process
synchronously. I believe "asSoonAsPossible" is less confusing.

https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/common/Throttler.js:43: this._asSoonAsPossible =
this._asSoonAsPossible || !!asSoonAsPossible || !this._isRunningProcess;
On 2014/06/09 13:26:45, pfeldman_ooo wrote:
> This logic seems confusing. So I was scheduling something for as soon as
> possible, or I am not running process and it sticks to true?

The code here has changed. Is your question still relevant?

https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/common/Throttler.js:53: if (this._processTimeout)
On 2014/06/09 13:17:37, dgozman wrote:
> I'd rather move this check to the |schedule| function, since it's only
possible
> there.

I'd like to leave it as-is, because the innerSchedule is fully responsible for
timer management.

https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/OverridesSupport.js (right):

https://codereview.chromium.org/319143002/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/sdk/OverridesSupport.js:627:
this._deviceMetricsThrottler.schedule(clearDeviceMetricsOverride.bind(this),
true);
On 2014/06/09 13:17:38, dgozman wrote:
> Can we make second parameter optional? This throttler already has timeout=0.

Sure! Fixed here and below in setDeviceMetricsOverride.

[dgozman, 2014-06-09 19:43:12]

https://codereview.chromium.org/319143002/diff/30006/Source/devtools/front_en...
File Source/devtools/front_end/common/Throttler.js (right):

https://codereview.chromium.org/319143002/diff/30006/Source/devtools/front_en...
Source/devtools/front_end/common/Throttler.js:62: this._isRunningProcess =
this._asSoonAsPossible;
If you clear |this._asSoonAsPossible| here:
- no need to fake isRunningProcess;
- call to another schedule before |_onTimeout| will initiate new process, which
should be better.

[lushnikov, 2014-06-10 09:05:22]

https://codereview.chromium.org/319143002/diff/30006/Source/devtools/front_en...
File Source/devtools/front_end/common/Throttler.js (right):

https://codereview.chromium.org/319143002/diff/30006/Source/devtools/front_en...
Source/devtools/front_end/common/Throttler.js:62: this._isRunningProcess =
this._asSoonAsPossible;
On 2014/06/09 19:43:11, dgozman wrote:
> If you clear |this._asSoonAsPossible| here:
> - no need to fake isRunningProcess;
> - call to another schedule before |_onTimeout| will initiate new process,
which
> should be better.

1. Consider having for-loop, which does throttler.schedule(process, true). I
would like to avoid repeated setTimeout/clearTimeout in this case. That's the
reason for isRunningProcess faking.

2. I'm not sure I follow here - are we talking about rescheduling timer? If so,
that's not what I want.

[vsevik, 2014-06-10 11:39:00]

lgtm, but consider simplifying this with the lines below as discussed offline.

Can we also add a test (you will probably need to use fake setTimeout)?

https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
File Source/devtools/front_end/common/Throttler.js (right):

https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
Source/devtools/front_end/common/Throttler.js:45: this._process = process;
var force = asSoonAsPossible && !this._asSoonAsPossible;

https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
Source/devtools/front_end/common/Throttler.js:56: if (this._processTimeout &&
!this._asSoonAsPossible)
if (this._processTimeout && !force)
    return;

[aandrey, 2014-06-10 14:59:53]

nits

https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
File Source/devtools/front_end/common/Throttler.js (right):

https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
Source/devtools/front_end/common/Throttler.js:13: this._processTimeout = null;
remove

https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
Source/devtools/front_end/common/Throttler.js:15: this._process = null;
jsdoc

https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
Source/devtools/front_end/common/Throttler.js:28: this._processTimeout = null;
delete this._processTimeout;

[aandrey, 2014-06-10 15:08:38]

https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
File Source/devtools/front_end/common/Throttler.js (right):

https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
Source/devtools/front_end/common/Throttler.js:9: WebInspector.Throttler =
function(timeout)
IMO, the most common use case in our code is: send a protocol command only after
the previous one is finished. (i.e. when backend takes more time to process a
command than the frontend to generate a new one).

If I understand correctly, this class does not handle that cases (i.e. there
should be no timeouts at all).

Does it make sense to generalize this class even more?

[vsevik, 2014-06-10 15:35:04]

On 2014/06/10 15:08:38, aandrey wrote:
>
https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
> File Source/devtools/front_end/common/Throttler.js (right):
> 
>
https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
> Source/devtools/front_end/common/Throttler.js:9: WebInspector.Throttler =
> function(timeout)
> IMO, the most common use case in our code is: send a protocol command only
after
> the previous one is finished. (i.e. when backend takes more time to process a
> command than the frontend to generate a new one).
> 
> If I understand correctly, this class does not handle that cases (i.e. there
> should be no timeouts at all).
> 
> Does it make sense to generalize this class even more?

This is the case in OverridesSupport.js. It sets timeout to zero to achieve
that.

[lushnikov, 2014-06-10 16:05:09]

https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
File Source/devtools/front_end/common/Throttler.js (right):

https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
Source/devtools/front_end/common/Throttler.js:15: this._process = null;
On 2014/06/10 14:59:53, aandrey wrote:
> jsdoc

Done.

https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
Source/devtools/front_end/common/Throttler.js:28: this._processTimeout = null;
On 2014/06/10 14:59:53, aandrey wrote:
> delete this._processTimeout;

Done.

https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
Source/devtools/front_end/common/Throttler.js:45: this._process = process;
On 2014/06/10 11:39:00, vsevik wrote:
> var force = asSoonAsPossible && !this._asSoonAsPossible;

Done.

https://chromiumcodereview.appspot.com/319143002/diff/30006/Source/devtools/f...
Source/devtools/front_end/common/Throttler.js:56: if (this._processTimeout &&
!this._asSoonAsPossible)
On 2014/06/10 11:39:00, vsevik wrote:
> if (this._processTimeout && !force)
>     return;

Done.

[vsevik, 2014-06-10 16:28:23]

https://chromiumcodereview.appspot.com/319143002/diff/190001/Source/devtools/...
File Source/devtools/front_end/common/Throttler.js (right):

https://chromiumcodereview.appspot.com/319143002/diff/190001/Source/devtools/...
Source/devtools/front_end/common/Throttler.js:61: if (this._processTimeout &&
force)
if (this._processTimeout) is enough

[lushnikov, 2014-06-10 16:44:59]

Thanks for the review guys!

https://chromiumcodereview.appspot.com/319143002/diff/190001/Source/devtools/...
File Source/devtools/front_end/common/Throttler.js (right):

https://chromiumcodereview.appspot.com/319143002/diff/190001/Source/devtools/...
Source/devtools/front_end/common/Throttler.js:61: if (this._processTimeout &&
force)
On 2014/06/10 16:28:23, vsevik wrote:
> if (this._processTimeout) is enough

Done.

[lushnikov, 2014-06-11 12:01:30]

The CQ bit was checked by lushnikov@chromium.org

[commit-bot: I haz the power, 2014-06-11 12:02:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lushnikov@chromium.org/319143002/210001

[commit-bot: I haz the power, 2014-06-11 12:49:32]

Change committed as 175956