Better spellcheck_test

Better spellcheck_test

spellcheck_test used to verify spellchecking results by repeatedly
settings timeouts and compare after timeout, which is an inefficient
and error-prone pattern.

This patch makes the verification more efficient:
- If spellchecker didn't create any result, validate immediately
- Otherwise, validate when all pending results have been resolved

BUG=517298
Review-Url: https://codereview.chromium.org/2727673005
Cr-Commit-Position: refs/heads/master@{#454958}
Committed: https://chromium.googlesource.com/chromium/src/+/266c9a16c8af6638fd43307a4aac5df5077f1c88

[Xiaocheng, 2017-03-03 04:01:50]

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

[commit-bot: I haz the power, 2017-03-03 04:02: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-03 04:03:54]

Description was changed from

==========
Better spellcheck_test

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

to

==========
Better spellcheck_test

spellcheck_test used to verify spellchecking results by repeatedly
settings timeouts and compare after timeout, which is an inefficient
and error-prone pattern.

This patch makes the verification more efficient:
- If spellchecker didn't create any result, validate immediately
- Otherwise, validate when all pending results have been resolved

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

[Xiaocheng, 2017-03-03 04:16:36]

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

[commit-bot: I haz the power, 2017-03-03 04:16:48]

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-03 04:16:51]

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

[Xiaocheng, 2017-03-03 04:16:52]

PTAL.

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

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

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

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Xiaocheng, 2017-03-03 04:44:28]

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

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

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

[yosin_UTC9, 2017-03-03 04:44:59]

lgtm w/ nits

https://codereview.chromium.org/2727673005/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js (right):

https://codereview.chromium.org/2727673005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:308: var
verificationForCurrentTest = null;
nit: s/var/let/

https://codereview.chromium.org/2727673005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:361: if
(internals.idleTimeSpellCheckerState(sample.document) === 'hot-mode-requested')
nit: We can use just one if-statement.

if (internals.idleTimeSpellCheckerState !== undefined &&
    internals.idleTimeSpellCheckerState(sample.document) ===
'hot-mode-requested) {
 ...
}

https://codereview.chromium.org/2727673005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/2727673005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/Internals.cpp:1510: case
IdleSpellCheckCallback::State::kInactive:
Optional: yet another solution is having
FOR_EACH_IDLE_SPELL_CHECK_CALLBACK_STATE(V) and
utilize here and printer.

#define FOR_EACH_IDLE_SPELL_CHECK_CALLBACK_STATE(V) \
  V(Inactive) V(HotModeReqquested) ...

enum State {
#define V(name) k##name
  FOR_EACH_IDLE_SPELL_CHECK_CALLBACK_STATE(V) 
#undef V
};

std::ostream& operator<<(...) {
  const char* const kTexts[] = {
#define V(name) #name
  FOR_EACH_IDLE_SPELL_CHECK_CALLBACK_STATE(V) 
#undef V
  };
}

etc.

See [1] for usage pattern.

[1] https://cs.chromium.org/search/?q=file:webkit+FOR_EACH_&type=cs

[commit-bot: I haz the power, 2017-03-03 06:22:29]

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

[commit-bot: I haz the power, 2017-03-03 06:22:29]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-03-03 18:56:41]

Updated. PTAL.

https://codereview.chromium.org/2727673005/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js (right):

https://codereview.chromium.org/2727673005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:308: var
verificationForCurrentTest = null;
On 2017/03/03 at 04:44:59, yosin_UTC9 wrote:
> nit: s/var/let/

Done.

https://codereview.chromium.org/2727673005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:361: if
(internals.idleTimeSpellCheckerState(sample.document) === 'hot-mode-requested')
On 2017/03/03 at 04:44:59, yosin_UTC9 wrote:
> nit: We can use just one if-statement.
> 
> if (internals.idleTimeSpellCheckerState !== undefined &&
>     internals.idleTimeSpellCheckerState(sample.document) ===
'hot-mode-requested) {
>  ...
> }

Done.

https://codereview.chromium.org/2727673005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/2727673005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/Internals.cpp:1510: case
IdleSpellCheckCallback::State::kInactive:
On 2017/03/03 at 04:44:59, yosin_UTC9 wrote:
> Optional: yet another solution is having
FOR_EACH_IDLE_SPELL_CHECK_CALLBACK_STATE(V) and
> utilize here and printer.
> 
> #define FOR_EACH_IDLE_SPELL_CHECK_CALLBACK_STATE(V) \
>   V(Inactive) V(HotModeReqquested) ...
> 
> enum State {
> #define V(name) k##name
>   FOR_EACH_IDLE_SPELL_CHECK_CALLBACK_STATE(V) 
> #undef V
> };
> 
> std::ostream& operator<<(...) {
>   const char* const kTexts[] = {
> #define V(name) #name
>   FOR_EACH_IDLE_SPELL_CHECK_CALLBACK_STATE(V) 
> #undef V
>   };
> }
> 
> etc.
> 
> See [1] for usage pattern.
> 
> [1] https://cs.chromium.org/search/?q=file:webkit+FOR_EACH_&type=cs

Updated with FOR_EACH(V)

Thanks for the advice.

[Xiaocheng, 2017-03-04 00:07:35]

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

[commit-bot: I haz the power, 2017-03-04 00:08:04]

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, 2017-03-04 04:27:45]

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

[commit-bot: I haz the power, 2017-03-04 04:27:46]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-03-06 05:01:26]

still lgtm

https://codereview.chromium.org/2727673005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h
(right):

https://codereview.chromium.org/2727673005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h:18:
V(kInactive)                                      \
I prefer not to have prefix "k".

[Xiaocheng, 2017-03-06 06:31:19]

Kent-san, could you review the changes in core/testings? Thanks!

https://codereview.chromium.org/2727673005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h
(right):

https://codereview.chromium.org/2727673005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h:18:
V(kInactive)                                      \
On 2017/03/06 at 05:01:26, yosin_UTC9 wrote:
> I prefer not to have prefix "k".

#define V(state) kstate doesn't work.

Not quite familiar with parameterized  macros...

[yosin_UTC9, 2017-03-06 06:37:49]

On 2017/03/06 at 06:31:19, xiaochengh wrote:
> Kent-san, could you review the changes in core/testings? Thanks!
> 
>
https://codereview.chromium.org/2727673005/diff/100001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h
(right):
> 
>
https://codereview.chromium.org/2727673005/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h:18:
V(kInactive)                                      \
> On 2017/03/06 at 05:01:26, yosin_UTC9 wrote:
> > I prefer not to have prefix "k".
> 
> #define V(state) kstate doesn't work.
> 
> Not quite familiar with parameterized  macros...

enum State {
#define V(state) k##state
FOR_EACH_STATE(V)
#undef V

- # id => "id"; stringify
- lhs ## rhs => lhsrhs; id concatination

[Xiaocheng, 2017-03-06 06:41:32]

https://codereview.chromium.org/2727673005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h
(right):

https://codereview.chromium.org/2727673005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h:18:
V(kInactive)                                      \
On 2017/03/06 at 06:31:19, Xiaocheng wrote:
> On 2017/03/06 at 05:01:26, yosin_UTC9 wrote:
> > I prefer not to have prefix "k".
> 
> #define V(state) kstate doesn't work.
> 
> Not quite familiar with parameterized  macros...

Will do.

Thanks...

[tkent, 2017-03-06 06:44:20]

lgtm

[Xiaocheng, 2017-03-06 19:33:00]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-03-06 19:33:01]

Committing after fixing the macros...

[Xiaocheng, 2017-03-06 19:33:01]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2727673005/#ps120001
(title: "Mon Mar  6 11:18:30 PST 2017")

[commit-bot: I haz the power, 2017-03-06 19:33:34]

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-06 21:16:00]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488828780890690,
"parent_rev": "17581175d5df6d58e62d9e3e7b699df99dca1b19", "commit_rev":
"266c9a16c8af6638fd43307a4aac5df5077f1c88"}

[commit-bot: I haz the power, 2017-03-06 21:16:43]

Description was changed from

==========
Better spellcheck_test

spellcheck_test used to verify spellchecking results by repeatedly
settings timeouts and compare after timeout, which is an inefficient
and error-prone pattern.

This patch makes the verification more efficient:
- If spellchecker didn't create any result, validate immediately
- Otherwise, validate when all pending results have been resolved

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

to

==========
Better spellcheck_test

spellcheck_test used to verify spellchecking results by repeatedly
settings timeouts and compare after timeout, which is an inefficient
and error-prone pattern.

This patch makes the verification more efficient:
- If spellchecker didn't create any result, validate immediately
- Otherwise, validate when all pending results have been resolved

BUG=517298

Review-Url: https://codereview.chromium.org/2727673005
Cr-Commit-Position: refs/heads/master@{#454958}
Committed:
https://chromium.googlesource.com/chromium/src/+/266c9a16c8af6638fd43307a4aac...
==========

[commit-bot: I haz the power, 2017-03-06 21:16:44]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/266c9a16c8af6638fd43307a4aac...