Remove timeout from idle time spell checker's cold mode request

Remove timeout from idle time spell checker's cold mode request

Idle time spell checker's cold mode is not essential task -- it
doesn't even matter that much if it starves, as it doesn't have
a strong requirement on latency.

Hence, this patch removes timeout when requesting cold mode via
requestIdleCallback.

This patch also fixes the unit test failure in crrev.com/2734013002

BUG=517298
Review-Url: https://codereview.chromium.org/2740503002
Cr-Commit-Position: refs/heads/master@{#455336}
Committed: https://chromium.googlesource.com/chromium/src/+/3164ef05aedfd208cf1d9e43558d7e8d59b14a2c

[Xiaocheng, 2017-03-07 06:37:55]

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

[commit-bot: I haz the power, 2017-03-07 06:39:04]

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

[Xiaocheng, 2017-03-07 06:44:56]

Description was changed from

==========
Remove timeout from idle time spell checker's cold mode request

BUG=517298
==========

to

==========
Remove timeout from idle time spell checker's cold mode request

Idle time spell checker's cold mode is not essential task -- it
doesn't even matter that much if it starves, as it doesn't have
a strong requirement on latency.

Hence, this patch removes timeout when requesting cold mode via
requestIdleCallback.

This patch also fixes the unit test failure in crrev.com/2734013002

BUG=517298
==========

[Xiaocheng, 2017-03-07 06:45:02]

xiaochengh@chromium.org changed reviewers:
+ yosin@chromium.org

[Xiaocheng, 2017-03-07 06:45:03]

PTAL.

[yosin_UTC9, 2017-03-07 08:15:30]

code changes are good except for whether using base::TimeDelta or not.

I'm confusing about description since description says "Remove timeout from ..."
but code changes
introduces |timeoutMS| parameter.

This maybe my English skill issue, but it is nice if you can use another phrase.
;-)

https://codereview.chromium.org/2740503002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp
(right):

https://codereview.chromium.org/2740503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:67:
void IdleSpellCheckCallback::requestInvocation(int timeoutMS) {
Should we use base::TimeDeltta, available in WTF/Time.h?

[commit-bot: I haz the power, 2017-03-07 09:03:44]

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

[commit-bot: I haz the power, 2017-03-07 09:03:45]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Xiaocheng, 2017-03-07 21:25:07]

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

[commit-bot: I haz the power, 2017-03-07 21:26:06]

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

[Xiaocheng, 2017-03-07 21:27:44]

Updated. I realized that function requestInvocation() is no longer necessary, as
now the two modes don't share much code when calling requestIdleCallback(). So I
just made the requesting part verbose.

It should be more intuitive now. PTAL.

https://codereview.chromium.org/2740503002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp
(right):

https://codereview.chromium.org/2740503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:67:
void IdleSpellCheckCallback::requestInvocation(int timeoutMS) {
On 2017/03/07 at 08:15:30, yosin_UTC9 wrote:
> Should we use base::TimeDeltta, available in WTF/Time.h?

I still prefer just an |int|, as IdleRequestOptions take |int| as parameter.

[yosin_UTC9, 2017-03-08 01:19:30]

On 2017/03/07 at 21:27:44, xiaochengh wrote:
> Updated. I realized that function requestInvocation() is no longer necessary,
as now the two modes don't share much code when calling requestIdleCallback().
So I just made the requesting part verbose.
> 
> It should be more intuitive now. PTAL.
> 
>
https://codereview.chromium.org/2740503002/diff/1/third_party/WebKit/Source/c...
> File
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp
(right):
> 
>
https://codereview.chromium.org/2740503002/diff/1/third_party/WebKit/Source/c...
>
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:67:
void IdleSpellCheckCallback::requestInvocation(int timeoutMS) {
> On 2017/03/07 at 08:15:30, yosin_UTC9 wrote:
> > Should we use base::TimeDeltta, available in WTF/Time.h?
> 
> I still prefer just an |int|, as IdleRequestOptions take |int| as parameter.

Agree.

When we use TimeDeleat, we could write:

options.setTimeout(static_cast<int>(TimeDelta::FromMilliseconds(kHotModeRequestTimeout).inMilliseconds());

Fumm... for constant, it is too verbose. Since IdleRequestOptions is generated
from IDL, we can't change it to use TimeDelta.

[yosin_UTC9, 2017-03-08 01:19:39]

lgtm

[Xiaocheng, 2017-03-08 01:21:59]

The CQ bit was unchecked by xiaochengh@chromium.org

[Xiaocheng, 2017-03-08 01:22:05]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-03-08 01:22:06]

Thanks!

[commit-bot: I haz the power, 2017-03-08 01:23:32]

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-03-08 01:52:52]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488936125576670,
"parent_rev": "5bac656c5eca0d89ab234781dd5aa5f6c40c1a8d", "commit_rev":
"3164ef05aedfd208cf1d9e43558d7e8d59b14a2c"}

[commit-bot: I haz the power, 2017-03-08 01:53:35]

Description was changed from

==========
Remove timeout from idle time spell checker's cold mode request

Idle time spell checker's cold mode is not essential task -- it
doesn't even matter that much if it starves, as it doesn't have
a strong requirement on latency.

Hence, this patch removes timeout when requesting cold mode via
requestIdleCallback.

This patch also fixes the unit test failure in crrev.com/2734013002

BUG=517298
==========

to

==========
Remove timeout from idle time spell checker's cold mode request

Idle time spell checker's cold mode is not essential task -- it
doesn't even matter that much if it starves, as it doesn't have
a strong requirement on latency.

Hence, this patch removes timeout when requesting cold mode via
requestIdleCallback.

This patch also fixes the unit test failure in crrev.com/2734013002

BUG=517298

Review-Url: https://codereview.chromium.org/2740503002
Cr-Commit-Position: refs/heads/master@{#455336}
Committed:
https://chromium.googlesource.com/chromium/src/+/3164ef05aedfd208cf1d9e43558d...
==========

[commit-bot: I haz the power, 2017-03-08 01:53:36]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/3164ef05aedfd208cf1d9e43558d...