Allow test runner to run a callback when finishing a spellcheck request

Allow test runner to run a callback when finishing a spellcheck request

This patch adds a new method setSpellCheckResolvedCallback to test
runner, so that layout tests can run script when a spell check request
is resolved, which can be used to inspect spelling markers in a
better manner. It also adds removeSpellCheckResolvedCallback for clearing
a previously set callback.

Currently layout tests can only set a timeout to wait for markers to appear.
Follow-up patches will utilize this new method in layout tests.

BUG=674819
Committed: https://crrev.com/5fe2c6a2c0339de9ad318841e26df0bde9754eff
Cr-Commit-Position: refs/heads/master@{#439725}

[Xiaocheng, 2016-12-19 08:51:18]

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

[commit-bot: I haz the power, 2016-12-19 08:51:29]

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-12-19 10:03:12]

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

[commit-bot: I haz the power, 2016-12-19 10:03:13]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2016-12-20 00:28:13]

Description was changed from

==========
Allow test runner to run a callback when finishing a spellcheck request

BUG=674819
==========

to

==========
Allow test runner to run a callback when finishing a spellcheck request

This patch adds a new method setSpellCheckResolvedCallback to test
runner, so that layout tests can run script when a spell check request
is resolved, which can be used to inspect spelling markers in a
better manner. Currently layout tests can only set a timeout to wait
for markers to appear.

Follow-up patches will utilize this new method in layout tests.

BUG=674819
==========

[Xiaocheng, 2016-12-20 00:28:43]

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

[Xiaocheng, 2016-12-20 00:28:44]

PTAL.

[tkent, 2016-12-20 00:38:44]

tkent@chromium.org changed reviewers:
+ haraken@chromium.org

[tkent, 2016-12-20 00:39:21]

haraken, would you review code interacting with v8?

[haraken, 2016-12-20 02:46:21]

https://codereview.chromium.org/2587823003/diff/1/components/test_runner/spel...
File components/test_runner/spell_check_client.cc (right):

https://codereview.chromium.org/2587823003/diff/1/components/test_runner/spel...
components/test_runner/spell_check_client.cc:158:
frame->callFunctionEvenIfScriptDisabled(

Is there any reason you want to run scripts when script execution is disabled?

https://codereview.chromium.org/2587823003/diff/1/components/test_runner/test...
File components/test_runner/test_runner.h (right):

https://codereview.chromium.org/2587823003/diff/1/components/test_runner/test...
components/test_runner/test_runner.h:89: blink::WebView* main_view() const {
return main_view_; }

Can you expose mainFrame() instead of main_view()?

[Xiaocheng, 2016-12-20 03:05:59]

Thanks for the review.

https://codereview.chromium.org/2587823003/diff/1/components/test_runner/spel...
File components/test_runner/spell_check_client.cc (right):

https://codereview.chromium.org/2587823003/diff/1/components/test_runner/spel...
components/test_runner/spell_check_client.cc:158:
frame->callFunctionEvenIfScriptDisabled(
On 2016/12/20 at 02:46:21, haraken wrote:
> Is there any reason you want to run scripts when script execution is disabled?

No specific reason. I just copied code from accessibility_controller.cc...

If we want to use a safer function, what should it be?
WebLocalFrame::requestExecuteV8Function?

https://codereview.chromium.org/2587823003/diff/1/components/test_runner/test...
File components/test_runner/test_runner.h (right):

https://codereview.chromium.org/2587823003/diff/1/components/test_runner/test...
components/test_runner/test_runner.h:89: blink::WebView* main_view() const {
return main_view_; }
On 2016/12/20 at 02:46:21, haraken wrote:
> Can you expose mainFrame() instead of main_view()?

Will do.

[haraken, 2016-12-20 03:09:13]

LGTM.

On 2016/12/20 03:05:59, Xiaocheng wrote:
> Thanks for the review.
> 
>
https://codereview.chromium.org/2587823003/diff/1/components/test_runner/spel...
> File components/test_runner/spell_check_client.cc (right):
> 
>
https://codereview.chromium.org/2587823003/diff/1/components/test_runner/spel...
> components/test_runner/spell_check_client.cc:158:
> frame->callFunctionEvenIfScriptDisabled(
> On 2016/12/20 at 02:46:21, haraken wrote:
> > Is there any reason you want to run scripts when script execution is
disabled?
> 
> No specific reason. I just copied code from accessibility_controller.cc...
> 
> If we want to use a safer function, what should it be?
> WebLocalFrame::requestExecuteV8Function?

Thanks, makes sense. Since your use case is only for testing,
callFunctionEvenIfScriptDisabled will be okay.

> 
>
https://codereview.chromium.org/2587823003/diff/1/components/test_runner/test...
> File components/test_runner/test_runner.h (right):
> 
>
https://codereview.chromium.org/2587823003/diff/1/components/test_runner/test...
> components/test_runner/test_runner.h:89: blink::WebView* main_view() const {
> return main_view_; }
> On 2016/12/20 at 02:46:21, haraken wrote:
> > Can you expose mainFrame() instead of main_view()?
> 
> Will do.

[tkent, 2016-12-20 03:16:41]

lgtm

[Xiaocheng, 2016-12-20 03:51:14]

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

[commit-bot: I haz the power, 2016-12-20 03:51:29]

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

[Xiaocheng, 2016-12-20 04:49:09]

Sorry for my carelessness... There should be one more method for clearing the
new callback.

PTAL.

https://codereview.chromium.org/2587823003/diff/1/components/test_runner/test...
File components/test_runner/test_runner.h (right):

https://codereview.chromium.org/2587823003/diff/1/components/test_runner/test...
components/test_runner/test_runner.h:89: blink::WebView* main_view() const {
return main_view_; }
On 2016/12/20 at 03:05:59, Xiaocheng wrote:
> On 2016/12/20 at 02:46:21, haraken wrote:
> > Can you expose mainFrame() instead of main_view()?
> 
> Will do.

Done.

[Xiaocheng, 2016-12-20 04:50:12]

Description was changed from

==========
Allow test runner to run a callback when finishing a spellcheck request

This patch adds a new method setSpellCheckResolvedCallback to test
runner, so that layout tests can run script when a spell check request
is resolved, which can be used to inspect spelling markers in a
better manner. Currently layout tests can only set a timeout to wait
for markers to appear.

Follow-up patches will utilize this new method in layout tests.

BUG=674819
==========

to

==========
Allow test runner to run a callback when finishing a spellcheck request

This patch adds a new method setSpellCheckResolvedCallback to test
runner, so that layout tests can run script when a spell check request
is resolved, which can be used to inspect spelling markers in a
better manner. It also adds removeSpellCheckResolvedCallback for clearing
a previously set callback.

Currently layout tests can only set a timeout to wait for markers to appear.
Follow-up patches will utilize this new method in layout tests.

BUG=674819
==========

[commit-bot: I haz the power, 2016-12-20 05:12:55]

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

[commit-bot: I haz the power, 2016-12-20 05:12:56]

Dry run: This issue passed the CQ dry run.

[tkent, 2016-12-20 06:03:51]

On 2016/12/20 04:49:09, Xiaocheng wrote:
> Sorry for my carelessness... There should be one more method for clearing the
> new callback.

lgtm

[Xiaocheng, 2016-12-20 06:06:57]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2016-12-20 06:06:58]

Thanks for the review!

[Xiaocheng, 2016-12-20 06:06:58]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2587823003/#ps40001
(title: "fix typo...")

[commit-bot: I haz the power, 2016-12-20 06:07:18]

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-12-20 06:12:03]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482214017880150,
"parent_rev": "d8bb74ed11bf9e8d1ea0b4e3a96a55b8db3d1bb3", "commit_rev":
"bdb5513340909eea60246627ef2547883f1352ca"}

[commit-bot: I haz the power, 2016-12-20 06:12:45]

Description was changed from

==========
Allow test runner to run a callback when finishing a spellcheck request

This patch adds a new method setSpellCheckResolvedCallback to test
runner, so that layout tests can run script when a spell check request
is resolved, which can be used to inspect spelling markers in a
better manner. It also adds removeSpellCheckResolvedCallback for clearing
a previously set callback.

Currently layout tests can only set a timeout to wait for markers to appear.
Follow-up patches will utilize this new method in layout tests.

BUG=674819
==========

to

==========
Allow test runner to run a callback when finishing a spellcheck request

This patch adds a new method setSpellCheckResolvedCallback to test
runner, so that layout tests can run script when a spell check request
is resolved, which can be used to inspect spelling markers in a
better manner. It also adds removeSpellCheckResolvedCallback for clearing
a previously set callback.

Currently layout tests can only set a timeout to wait for markers to appear.
Follow-up patches will utilize this new method in layout tests.

BUG=674819

Review-Url: https://codereview.chromium.org/2587823003
==========

[commit-bot: I haz the power, 2016-12-20 06:12:46]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-20 06:14:55]

Description was changed from

==========
Allow test runner to run a callback when finishing a spellcheck request

This patch adds a new method setSpellCheckResolvedCallback to test
runner, so that layout tests can run script when a spell check request
is resolved, which can be used to inspect spelling markers in a
better manner. It also adds removeSpellCheckResolvedCallback for clearing
a previously set callback.

Currently layout tests can only set a timeout to wait for markers to appear.
Follow-up patches will utilize this new method in layout tests.

BUG=674819

Review-Url: https://codereview.chromium.org/2587823003
==========

to

==========
Allow test runner to run a callback when finishing a spellcheck request

This patch adds a new method setSpellCheckResolvedCallback to test
runner, so that layout tests can run script when a spell check request
is resolved, which can be used to inspect spelling markers in a
better manner. It also adds removeSpellCheckResolvedCallback for clearing
a previously set callback.

Currently layout tests can only set a timeout to wait for markers to appear.
Follow-up patches will utilize this new method in layout tests.

BUG=674819

Committed: https://crrev.com/5fe2c6a2c0339de9ad318841e26df0bde9754eff
Cr-Commit-Position: refs/heads/master@{#439725}
==========

[commit-bot: I haz the power, 2016-12-20 06:14:56]

Patchset 3 (id:??) landed as
https://crrev.com/5fe2c6a2c0339de9ad318841e26df0bde9754eff
Cr-Commit-Position: refs/heads/master@{#439725}