Add a browser test for checking spelling in out-of-process subframes

Add a browser test for checking spelling in out-of-process subframes

This patch adds a regression test for a bug that was there but has been
fixed: spellcheck messages were not sent out from out-of-process iframes.

BUG=638361
Review-Url: https://codereview.chromium.org/2819463002
Cr-Commit-Position: refs/heads/master@{#464658}
Committed: https://chromium.googlesource.com/chromium/src/+/b6c55a8e1595e5d65760be3adba1e26e0e03e95e

[Xiaocheng, 2017-04-12 23:50:02]

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

[commit-bot: I haz the power, 2017-04-12 23:50:40]

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

[Xiaocheng, 2017-04-12 23:54:49]

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

[commit-bot: I haz the power, 2017-04-12 23:55:16]

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-04-13 00:11:41]

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

[commit-bot: I haz the power, 2017-04-13 00:11:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xiaocheng, 2017-04-13 02:53:58]

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

[commit-bot: I haz the power, 2017-04-13 02:54:35]

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-04-13 03:43:30]

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

[commit-bot: I haz the power, 2017-04-13 03:43:31]

Dry run: 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_...)

[Xiaocheng, 2017-04-13 18:25:16]

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

[commit-bot: I haz the power, 2017-04-13 18:26:23]

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

[Xiaocheng, 2017-04-13 19:06:12]

Description was changed from

==========
Add a browser test for checking spelling in out-of-process subframes

BUG=638361
==========

to

==========
Add a browser test for checking spelling in out-of-process subframes

This patch adds a regression test for a bug that was there but has been
fixed: spellcheck messages were not sent out from out-of-process iframes.

BUG=638361
==========

[commit-bot: I haz the power, 2017-04-13 19:51:26]

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

[commit-bot: I haz the power, 2017-04-13 19:51:27]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-04-13 20:43:46]

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

[Xiaocheng, 2017-04-13 20:43:46]

PTAL

[Xiaocheng, 2017-04-13 21:55:40]

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

[Xiaocheng, 2017-04-13 21:55:40]

+alexmos

[alexmos, 2017-04-13 22:39:10]

LGTM

https://codereview.chromium.org/2819463002/diff/60001/chrome/browser/chrome_s...
File chrome/browser/chrome_site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2819463002/diff/60001/chrome/browser/chrome_s...
chrome/browser/chrome_site_per_process_browsertest.cc:608: // Tests that
spelling in out-of-process subframes are checked.
nit: s/are/is/

https://codereview.chromium.org/2819463002/diff/60001/chrome/test/data/page_w...
File chrome/test/data/page_with_contenteditable.html (right):

https://codereview.chromium.org/2819463002/diff/60001/chrome/test/data/page_w...
chrome/test/data/page_with_contenteditable.html:9:
document.querySelector('div').focus();
Just curious, is the spellcheck IPC triggered by this focus(), or is it
independent of it?

(If it's triggered by focus(), another approach to the test could be just to use
ExecuteScript to do the focus().  This would allow you to install the filter on
the subframe's RenderFrameHost before that call, avoiding the need for
TestBrowserClientForSpellCheck, IIUC.  But what you have is fine too. :) )

[Xiaocheng, 2017-04-13 23:16:38]

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

[commit-bot: I haz the power, 2017-04-13 23:17:18]

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

[Xiaocheng, 2017-04-13 23:17:20]

Thanks for reviewing!

https://codereview.chromium.org/2819463002/diff/60001/chrome/browser/chrome_s...
File chrome/browser/chrome_site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2819463002/diff/60001/chrome/browser/chrome_s...
chrome/browser/chrome_site_per_process_browsertest.cc:608: // Tests that
spelling in out-of-process subframes are checked.
On 2017/04/13 at 22:39:10, alexmos wrote:
> nit: s/are/is/

Done.

https://codereview.chromium.org/2819463002/diff/60001/chrome/test/data/page_w...
File chrome/test/data/page_with_contenteditable.html (right):

https://codereview.chromium.org/2819463002/diff/60001/chrome/test/data/page_w...
chrome/test/data/page_with_contenteditable.html:9:
document.querySelector('div').focus();
On 2017/04/13 at 22:39:10, alexmos wrote:
> Just curious, is the spellcheck IPC triggered by this focus(), or is it
independent of it?
> 
> (If it's triggered by focus(), another approach to the test could be just to
use ExecuteScript to do the focus().  This would allow you to install the filter
on the subframe's RenderFrameHost before that call, avoiding the need for
TestBrowserClientForSpellCheck, IIUC.  But what you have is fine too. :) )

Yes, setting focus in contenteditable triggers spellchecking.

I tried adding filter before an ExecuteScript that sets focus, but it didn't
work. I think it's because SpellCheckMessageFilter is added first, which handles
the message and prevents other filters from handling the same message.

[dcheng, 2017-04-14 00:20:12]

LGTM

https://codereview.chromium.org/2819463002/diff/60001/chrome/browser/chrome_s...
File chrome/browser/chrome_site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2819463002/diff/60001/chrome/browser/chrome_s...
chrome/browser/chrome_site_per_process_browsertest.cc:528:
message_loop_runner_(new content::MessageLoopRunner) {}
Nit: base::MakeShared<content::MessageLoopRunner>()

https://codereview.chromium.org/2819463002/diff/60001/chrome/browser/chrome_s...
chrome/browser/chrome_site_per_process_browsertest.cc:632: #endif
Nit: add a comment:
// BUILDFLAG(ENABLE_SPELLCHECK)

[Xiaocheng, 2017-04-14 01:03:10]

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

[commit-bot: I haz the power, 2017-04-14 01:03:19]

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

[Xiaocheng, 2017-04-14 01:03:53]

Thanks for reviewing!

https://codereview.chromium.org/2819463002/diff/60001/chrome/browser/chrome_s...
File chrome/browser/chrome_site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2819463002/diff/60001/chrome/browser/chrome_s...
chrome/browser/chrome_site_per_process_browsertest.cc:528:
message_loop_runner_(new content::MessageLoopRunner) {}
On 2017/04/14 at 00:20:12, dcheng wrote:
> Nit: base::MakeShared<content::MessageLoopRunner>()

Done.

https://codereview.chromium.org/2819463002/diff/60001/chrome/browser/chrome_s...
chrome/browser/chrome_site_per_process_browsertest.cc:632: #endif
On 2017/04/14 at 00:20:12, dcheng wrote:
> Nit: add a comment:
> // BUILDFLAG(ENABLE_SPELLCHECK)

Done.

[commit-bot: I haz the power, 2017-04-14 02:06:11]

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

[commit-bot: I haz the power, 2017-04-14 02:06:12]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-04-14 02:08:40]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-04-14 02:08:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexmos@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2819463002/#ps100001
(title: "Address dcheng's comments")

[commit-bot: I haz the power, 2017-04-14 02:09:10]

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-04-14 02:15:05]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492135720711090,
"parent_rev": "461b329afc65973c9d25edbae1eff9e0b6979d07", "commit_rev":
"b6c55a8e1595e5d65760be3adba1e26e0e03e95e"}

[commit-bot: I haz the power, 2017-04-14 02:16:00]

Description was changed from

==========
Add a browser test for checking spelling in out-of-process subframes

This patch adds a regression test for a bug that was there but has been
fixed: spellcheck messages were not sent out from out-of-process iframes.

BUG=638361
==========

to

==========
Add a browser test for checking spelling in out-of-process subframes

This patch adds a regression test for a bug that was there but has been
fixed: spellcheck messages were not sent out from out-of-process iframes.

BUG=638361

Review-Url: https://codereview.chromium.org/2819463002
Cr-Commit-Position: refs/heads/master@{#464658}
Committed:
https://chromium.googlesource.com/chromium/src/+/b6c55a8e1595e5d65760be3adba1...
==========

[commit-bot: I haz the power, 2017-04-14 02:16:00]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/b6c55a8e1595e5d65760be3adba1...