Move WebTextCheckClient reference from WebViewImpl to WebLocalFrameImpl

Move WebTextCheckClient reference from WebViewImpl to WebLocalFrameImpl

This is Patch 5 of 6 for making SpellCheckProvider a RenderFrameObserver, so
that spellcheck can work in OOPIF. Full design: https://goo.gl/VfCENk

This patch moves the Blink-side reference to WebTextCheckClient from
WebViewImpl to WebLocalFrameImpl, as a preparation for changing
SpellCheckProvider from RenderViewObserver to RenderFrameObserver.

Currently, SpellCheckProvider is still view-based, and all WebLocalFrames of
the same view are holding references to the same SpellCheckProvider. Follow
up patch will change that.

For layout tests, by design there should be only one mock spell checker
instance. This patch also ensures that this instance is used by all
WebLocalFrames.

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

[Xiaocheng, 2017-04-05 02:59:49]

Description was changed from

==========
WIP Make SpellCheckProvider RenderFrameObserver

BLOCKED No RenderFrameVisitor

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

to

==========
WIP Move WebTextCheckClient reference from WebViewImpl to WebLocalFrameImpl

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

[Xiaocheng, 2017-04-05 18:43:34]

Description was changed from

==========
WIP Move WebTextCheckClient reference from WebViewImpl to WebLocalFrameImpl

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

to

==========
Move WebTextCheckClient reference from WebViewImpl to WebLocalFrameImpl

This is Patch 5 of 6 for making SpellCheckProvider a RenderFrameObserver, so
that spellcheck can work in OOPIF. Full design: https://goo.gl/VfCENk

This patch moves the Blink-side reference to WebTextCheckClient from
WebViewImpl to WebLocalFrameImpl, as a preparation for changing
SpellCheckProvider from RenderViewObserver to RenderFrameObserver.

Currently, SpellCheckProvider is still view-based, and all WebLocalFrames of
the same view are holding references to the same SpellCheckProvider. Follow
up patch will change that.

For layout tests, by design there should be only one mock spell checker
instance. This patch also ensures that this instance is used by all
WebLocalFrames.

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

[Xiaocheng, 2017-04-05 20:07:14]

xiaochengh@chromium.org changed reviewers:
+ nick@chromium.org, rouslan@chromium.org, thestig@chromium.org,
tkent@chromium.org

[Xiaocheng, 2017-04-05 20:07:15]

PTAL.

I'm not very familiar with the lifecycle of RenderView and RenderFrame. Does
this patch ensure that, every RenderFrame calls
WebLocalFrame::setTextCheckClient with a non-null SpellCheckProvider?

[Lei Zhang, 2017-04-05 20:20:45]

https://codereview.chromium.org/2797073002/diff/40001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2797073002/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:572:
SpellCheckProvider::Get(render_frame->GetRenderView())) {
Looking at RenderFrameImpl::GetRenderView() and how it's used internally, it
should never returns a nullptr. So if all RenderView creation reaches
ChromeContentRendererClient::RenderViewCreated(), where you create the
SpellCheckProvider, then this should never return a nullptr either.

[Xiaocheng, 2017-04-05 20:26:15]

Thanks for reviewing!

https://codereview.chromium.org/2797073002/diff/40001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2797073002/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:572:
SpellCheckProvider::Get(render_frame->GetRenderView())) {
On 2017/04/05 at 20:20:44, Lei Zhang wrote:
> Looking at RenderFrameImpl::GetRenderView() and how it's used internally, it
should never returns a nullptr. So if all RenderView creation reaches
ChromeContentRendererClient::RenderViewCreated(), where you create the
SpellCheckProvider, then this should never return a nullptr either.

I found that for the first RenderFrame in a RenderView, RenderFrameCreated() is
called earlier than RenderViewCreated().

So when we reach here, it's possible that the SpellCheckProvider for the
RenderView hasn't been created yet.

[Lei Zhang, 2017-04-05 20:35:35]

https://codereview.chromium.org/2797073002/diff/40001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2797073002/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:572:
SpellCheckProvider::Get(render_frame->GetRenderView())) {
On 2017/04/05 20:26:15, Xiaocheng wrote:
> On 2017/04/05 at 20:20:44, Lei Zhang wrote:
> > Looking at RenderFrameImpl::GetRenderView() and how it's used internally, it
> should never returns a nullptr. So if all RenderView creation reaches
> ChromeContentRendererClient::RenderViewCreated(), where you create the
> SpellCheckProvider, then this should never return a nullptr either.
> 
> I found that for the first RenderFrame in a RenderView, RenderFrameCreated()
is
> called earlier than RenderViewCreated().
> 
> So when we reach here, it's possible that the SpellCheckProvider for the
> RenderView hasn't been created yet.

Oh, we are in RenderFrameCreated(). I don't know enough about spell check to say
whether it's ok for the first (main?) RenderFrame to not have a client set. If
it must be set, maybe add a check to see if the client is set for the main frame
in RenderViewCreated() below and set it there instead after creating the
SpellCheckProvider?

It feels hacky, but I imagine the problem will go away once SpellCheckProvider
is a RenderFrameObserver.

[tkent, 2017-04-05 22:24:53]

third_party/WebKit lgtm

[Xiaocheng, 2017-04-05 22:32:07]

https://codereview.chromium.org/2797073002/diff/40001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2797073002/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:572:
SpellCheckProvider::Get(render_frame->GetRenderView())) {
On 2017/04/05 at 20:35:35, Lei Zhang wrote:
> On 2017/04/05 20:26:15, Xiaocheng wrote:
> > On 2017/04/05 at 20:20:44, Lei Zhang wrote:
> > > Looking at RenderFrameImpl::GetRenderView() and how it's used internally,
it
> > should never returns a nullptr. So if all RenderView creation reaches
> > ChromeContentRendererClient::RenderViewCreated(), where you create the
> > SpellCheckProvider, then this should never return a nullptr either.
> > 
> > I found that for the first RenderFrame in a RenderView, RenderFrameCreated()
is
> > called earlier than RenderViewCreated().
> > 
> > So when we reach here, it's possible that the SpellCheckProvider for the
> > RenderView hasn't been created yet.
> 
> Oh, we are in RenderFrameCreated(). I don't know enough about spell check to
say whether it's ok for the first (main?) RenderFrame to not have a client set.
If it must be set, maybe add a check to see if the client is set for the main
frame in RenderViewCreated() below and set it there instead after creating the
SpellCheckProvider?

Yes, each RenderFrame must have its client set. Do you mean:
- For a subframe, set its client in RenderFrameCreate()
- For the main frame, set its client in RenderViewCreated right after creating
the SpellCheckProvider?

> 
> It feels hacky, but I imagine the problem will go away once SpellCheckProvider
is a RenderFrameObserver.

Yeah it's hacky for now. Once SpellCheckProvider becomes RFO, we only need to
set the client in SpellCheckProvider::ctor and all hacks will be gone.

[Lei Zhang, 2017-04-05 22:38:19]

https://codereview.chromium.org/2797073002/diff/40001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2797073002/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:572:
SpellCheckProvider::Get(render_frame->GetRenderView())) {
On 2017/04/05 22:32:07, Xiaocheng wrote:
> Yes, each RenderFrame must have its client set. Do you mean:
> - For a subframe, set its client in RenderFrameCreate()
> - For the main frame, set its client in RenderViewCreated right after creating
> the SpellCheckProvider?

Based on your prior statements, my mental picture says it'll have to be done
this way.

> Yeah it's hacky for now. Once SpellCheckProvider becomes RFO, we only need to
> set the client in SpellCheckProvider::ctor and all hacks will be gone.

It sounds like we are in agreement?

[Xiaocheng, 2017-04-05 22:43:34]

https://codereview.chromium.org/2797073002/diff/40001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2797073002/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:572:
SpellCheckProvider::Get(render_frame->GetRenderView())) {
On 2017/04/05 at 22:38:19, Lei Zhang wrote:
> On 2017/04/05 22:32:07, Xiaocheng wrote:
> > Yes, each RenderFrame must have its client set. Do you mean:
> > - For a subframe, set its client in RenderFrameCreate()
> > - For the main frame, set its client in RenderViewCreated right after
creating
> > the SpellCheckProvider?
> 
> Based on your prior statements, my mental picture says it'll have to be done
this way.
> 
> > Yeah it's hacky for now. Once SpellCheckProvider becomes RFO, we only need
to
> > set the client in SpellCheckProvider::ctor and all hacks will be gone.
> 
> It sounds like we are in agreement?

Yep :)

[ncarter (slow), 2017-04-05 22:49:11]

On 2017/04/05 22:43:34, Xiaocheng wrote:
>
https://codereview.chromium.org/2797073002/diff/40001/chrome/renderer/chrome_...
> File chrome/renderer/chrome_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/2797073002/diff/40001/chrome/renderer/chrome_...
> chrome/renderer/chrome_content_renderer_client.cc:572:
> SpellCheckProvider::Get(render_frame->GetRenderView())) {
> On 2017/04/05 at 22:38:19, Lei Zhang wrote:
> > On 2017/04/05 22:32:07, Xiaocheng wrote:
> > > Yes, each RenderFrame must have its client set. Do you mean:
> > > - For a subframe, set its client in RenderFrameCreate()
> > > - For the main frame, set its client in RenderViewCreated right after
> creating
> > > the SpellCheckProvider?
> > 
> > Based on your prior statements, my mental picture says it'll have to be done
> this way.
> > 
> > > Yeah it's hacky for now. Once SpellCheckProvider becomes RFO, we only need
> to
> > > set the client in SpellCheckProvider::ctor and all hacks will be gone.
> > 
> > It sounds like we are in agreement?
> 
> Yep :)

content/lgtm.

It's unfortunate that the RV/RF initialization order (via the content renderer
client) is inconsistent for the main frame vs. subframes. But hopefully this can
be solved by dropping the RV dependency overall.

[Lei Zhang, 2017-04-05 22:56:18]

On 2017/04/05 22:43:34, Xiaocheng wrote:
> Yep :)

In which case, new patch set please.

[Xiaocheng, 2017-04-05 23:42:07]

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

[Xiaocheng, 2017-04-05 23:42:35]

Updated. PTAL

[commit-bot: I haz the power, 2017-04-05 23:42:57]

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

[Lei Zhang, 2017-04-05 23:49:19]

chrome/ lgtm

https://codereview.chromium.org/2797073002/diff/60001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2797073002/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:589: // TODO(xiaocheng):
Remove this workaround once SpellCheckProvider becomes
Your two newly added TODOs have different user names inside.

[commit-bot: I haz the power, 2017-04-06 01:29:39]

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

[commit-bot: I haz the power, 2017-04-06 01:29:40]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-04-06 02:58:30]

Updated.

https://codereview.chromium.org/2797073002/diff/60001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2797073002/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:589: // TODO(xiaocheng):
Remove this workaround once SpellCheckProvider becomes
On 2017/04/05 at 23:49:19, Lei Zhang wrote:
> Your two newly added TODOs have different user names inside.

Whoops. Thanks for catching.

[Xiaocheng, 2017-04-06 02:58:37]

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

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

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-06 04:25:59]

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

[commit-bot: I haz the power, 2017-04-06 04:26:00]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2017-04-06 14:15:17]

lgtm

[xiaochengh, 2017-04-06 17:34:54]

The CQ bit was checked by xiaochengh@google.com

[xiaochengh, 2017-04-06 17:34:55]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
nick@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2797073002/#ps80001
(title: "Spell my name correctly")

[commit-bot: I haz the power, 2017-04-06 17:35:30]

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-06 17:54:38]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491500094710170,
"parent_rev": "cb4be5b37ddc73a11c7e03538403dde2d0ca0f18", "commit_rev":
"81bfb1741ab51947f5fc7c741eadd92779fe13b2"}

[commit-bot: I haz the power, 2017-04-06 17:55:39]

Description was changed from

==========
Move WebTextCheckClient reference from WebViewImpl to WebLocalFrameImpl

This is Patch 5 of 6 for making SpellCheckProvider a RenderFrameObserver, so
that spellcheck can work in OOPIF. Full design: https://goo.gl/VfCENk

This patch moves the Blink-side reference to WebTextCheckClient from
WebViewImpl to WebLocalFrameImpl, as a preparation for changing
SpellCheckProvider from RenderViewObserver to RenderFrameObserver.

Currently, SpellCheckProvider is still view-based, and all WebLocalFrames of
the same view are holding references to the same SpellCheckProvider. Follow
up patch will change that.

For layout tests, by design there should be only one mock spell checker
instance. This patch also ensures that this instance is used by all
WebLocalFrames.

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

to

==========
Move WebTextCheckClient reference from WebViewImpl to WebLocalFrameImpl

This is Patch 5 of 6 for making SpellCheckProvider a RenderFrameObserver, so
that spellcheck can work in OOPIF. Full design: https://goo.gl/VfCENk

This patch moves the Blink-side reference to WebTextCheckClient from
WebViewImpl to WebLocalFrameImpl, as a preparation for changing
SpellCheckProvider from RenderViewObserver to RenderFrameObserver.

Currently, SpellCheckProvider is still view-based, and all WebLocalFrames of
the same view are holding references to the same SpellCheckProvider. Follow
up patch will change that.

For layout tests, by design there should be only one mock spell checker
instance. This patch also ensures that this instance is used by all
WebLocalFrames.

BUG=638361

Review-Url: https://codereview.chromium.org/2797073002
Cr-Commit-Position: refs/heads/master@{#462538}
Committed:
https://chromium.googlesource.com/chromium/src/+/81bfb1741ab51947f5fc7c741ead...
==========

[commit-bot: I haz the power, 2017-04-06 17:55:40]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/81bfb1741ab51947f5fc7c741ead...

[Xiaocheng, 2017-04-10 20:27:06]

xiaochengh@chromium.org changed reviewers:
+ timvolodine@chromium.org
- xiaochengh@google.com

[Xiaocheng, 2017-04-10 20:27:06]

I guess I broke spellchecking in Android WebView...

I'll fix it together in https://codereview.chromium.org/2799923003