Change SpellCheckProvider into a RenderFrameObserver

Change SpellCheckProvider into a RenderFrameObserver

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

This patch changes SpellCheckProvider into a RenderFrameObserver, so that
spellcheck messages from OOPIFs can be successfully sent to browser, making
spell-checking work in OOPIFs.

BUG=638361, 710044, 625068
Review-Url: https://codereview.chromium.org/2799923003
Cr-Commit-Position: refs/heads/master@{#463912}
Committed: https://chromium.googlesource.com/chromium/src/+/0c92b1cfa21b855c0e8c19f5550ae5209a325936

[Xiaocheng, 2017-04-07 01:36:58]

Description was changed from

==========
WIP Change SpellCheckProvider into a RenderFrameObserver



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

to

==========
WIP Change SpellCheckProvider into a RenderFrameObserver

BLOCKED RenderFrameCreated() called before core frame set up

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

[Xiaocheng, 2017-04-07 01:43:44]

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

[Xiaocheng, 2017-04-07 01:43:46]

Hi thestig@:

This patch is still WIP, but I encountered a life cycle issue that I'd like to
discuss.

This patch makes renderer crash when creating a subframe, because for a
subframe, the current initialization order is:

(in WebLocalFrameImpl::createChildFrame)

1. RenderFrameImpl creation starts
2. WebLocalFrame created
3. RenderFrameImpl creation ends, with RenderFrameCreated() called
4. WebLocalFrame sets up the core frame

Is this ordering correct? Should RenderFrameCreated() be called after setting up
the core frame?

The current ordering makes renderer crash because when creating a
SpellCheckProvider in RenderFrameCreated(), it attempts to visit the core frame,
which is not set up yet.

https://codereview.chromium.org/2799923003/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2799923003/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:571: new
SpellCheckProvider(render_frame, spellcheck_.get());
SpellCheckProvider created at RenderFrameCreated()

https://codereview.chromium.org/2799923003/diff/1/components/spellcheck/rende...
File components/spellcheck/renderer/spellcheck_provider.cc (right):

https://codereview.chromium.org/2799923003/diff/1/components/spellcheck/rende...
components/spellcheck/renderer/spellcheck_provider.cc:45:
EnableSpellcheck(spellcheck_->IsSpellcheckEnabled());
Crashes here, because EnableSpellcheck() visits blink::LocalFrame, which is not
set up yet.

https://codereview.chromium.org/2799923003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2799923003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1077: CHECK(frame());
Crash site.

[Xiaocheng, 2017-04-07 01:48:41]

Btw, I can make SpellCheckProvider not to visit the core frame in its ctor. But
still, I would like to have a discussion on the initialization order to make
sure things are done correctly.

[Lei Zhang, 2017-04-07 02:03:53]

I don't really work on Blink so I have no idea what happens on that side. You
can try asking content/ folks, since you want to add RenderFrameObserver anyway.

[Xiaocheng, 2017-04-07 02:09:49]

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

[Xiaocheng, 2017-04-07 02:09:49]

Thanks anyway.

+nick, could you help?

[ncarter (slow), 2017-04-07 17:24:58]

nick@chromium.org changed reviewers:
+ lfg@chromium.org

[ncarter (slow), 2017-04-07 17:24:58]

+lfg might know off the top of his head

[lfg, 2017-04-07 19:10:33]

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

[lfg, 2017-04-07 19:10:34]

On 2017/04/07 01:43:46, Xiaocheng wrote:
> 
> 1. RenderFrameImpl creation starts
> 2. WebLocalFrame created
> 3. RenderFrameImpl creation ends, with RenderFrameCreated() called
> 4. WebLocalFrame sets up the core frame
> 
> Is this ordering correct? Should RenderFrameCreated() be called after setting
up
> the core frame?

The ordering for 1, 2 and 4 are correct. To create a WebLocalFrame we first need
the WebFrameClient, which is the RenderFrameImpl. And to create the LocalFrame
(the frame representation in core/), we need the WebLocalFrame so that the frame
exists in the frame tree.

I _think_ that 3 could in theory happen after 4, it's just that nothing needs it
to happen in that order. To do that we'll probably need to add a
didCreateFrame()
call to WebFrameClient that dispatches the RenderFrameCreated callback.

Alternatively, you may be able to query if spellchecking should be enable when
creating the SpellChecker in the LocalFrame constructor.

+dcheng may have other ideas.

[Xiaocheng, 2017-04-07 23:55:43]

On 2017/04/07 at 19:10:34, lfg wrote:
> On 2017/04/07 01:43:46, Xiaocheng wrote:
> > 
> > 1. RenderFrameImpl creation starts
> > 2. WebLocalFrame created
> > 3. RenderFrameImpl creation ends, with RenderFrameCreated() called
> > 4. WebLocalFrame sets up the core frame
> > 
> > Is this ordering correct? Should RenderFrameCreated() be called after
setting up
> > the core frame?
> 
> The ordering for 1, 2 and 4 are correct. To create a WebLocalFrame we first
need
> the WebFrameClient, which is the RenderFrameImpl. And to create the LocalFrame
> (the frame representation in core/), we need the WebLocalFrame so that the
frame
> exists in the frame tree.
> 
> I _think_ that 3 could in theory happen after 4, it's just that nothing needs
it
> to happen in that order. To do that we'll probably need to add a
didCreateFrame()
> call to WebFrameClient that dispatches the RenderFrameCreated callback.

With more investigation, I think the order shouldn't be changed.

It seems that WebLocalFrameImpl::initializeCoreFrame may detach the frame in
'load' handler, in which case RenderFrameImpl will be deleted before
initializeCoreFrame() returns. If we change the initialization order, then
RenderFrameCreated() will never be called for the subframe.

So maybe the correct solution is just not to access the core frame in
RenderFrameCreated().

> 
> Alternatively, you may be able to query if spellchecking should be enable when
> creating the SpellChecker in the LocalFrame constructor.
> 
> +dcheng may have other ideas.

[Xiaocheng, 2017-04-10 19:13:11]

Description was changed from

==========
WIP Change SpellCheckProvider into a RenderFrameObserver

BLOCKED RenderFrameCreated() called before core frame set up

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

to

==========
Change SpellCheckProvider into a RenderFrameObserver

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

This patch changes SpellCheckProvider into a RenderFrameObserver, so that
spellcheck messages from OOPIFs can be successfully sent to browser, making
spell-checking work in OOPIFs.

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

[Xiaocheng, 2017-04-10 19:20:55]

Description was changed from

==========
Change SpellCheckProvider into a RenderFrameObserver

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

This patch changes SpellCheckProvider into a RenderFrameObserver, so that
spellcheck messages from OOPIFs can be successfully sent to browser, making
spell-checking work in OOPIFs.

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

to

==========
Change SpellCheckProvider into a RenderFrameObserver

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

This patch changes SpellCheckProvider into a RenderFrameObserver, so that
spellcheck messages from OOPIFs can be successfully sent to browser, making
spell-checking work in OOPIFs.

BUG=638361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TEST=WIP...
==========

[Xiaocheng, 2017-04-10 19:21:00]

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

[commit-bot: I haz the power, 2017-04-10 19:22:07]

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-10 19:40:45]

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

[commit-bot: I haz the power, 2017-04-10 19:40:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xiaocheng, 2017-04-10 20:28:05]

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

[commit-bot: I haz the power, 2017-04-10 20:28:38]

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-10 20:49:04]

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

[commit-bot: I haz the power, 2017-04-10 20:49:05]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Xiaocheng, 2017-04-10 21:07:21]

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

[commit-bot: I haz the power, 2017-04-10 21:08:15]

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-10 22:21:41]

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

[Xiaocheng, 2017-04-10 22:21:41]

+torne for android_webview/

I manually tested this patch, which indeed fixes spellchecking in OOPIFs

I'm stuck at adding a regression test... I have never worked on any
renderer-side OOPIF test before. Is there any pattern that I can follow?

[Xiaocheng, 2017-04-10 22:33:19]

Description was changed from

==========
Change SpellCheckProvider into a RenderFrameObserver

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

This patch changes SpellCheckProvider into a RenderFrameObserver, so that
spellcheck messages from OOPIFs can be successfully sent to browser, making
spell-checking work in OOPIFs.

BUG=638361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TEST=WIP...
==========

to

==========
Change SpellCheckProvider into a RenderFrameObserver

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

This patch changes SpellCheckProvider into a RenderFrameObserver, so that
spellcheck messages from OOPIFs can be successfully sent to browser, making
spell-checking work in OOPIFs.

BUG=638361,710044,625068
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TEST=WIP...
==========

[Torne, 2017-04-10 22:43:06]

android_webview LGTM

[Xiaocheng, 2017-04-10 22:53:41]

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

[Xiaocheng, 2017-04-10 22:53:42]

Whoops, forgot rouslan@ for compoents/spellcheck...

[commit-bot: I haz the power, 2017-04-10 22:56:26]

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

[commit-bot: I haz the power, 2017-04-10 22:56:27]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2017-04-11 14:02:58]

lgtm

[Xiaocheng, 2017-04-12 00:06:34]

Description was changed from

==========
Change SpellCheckProvider into a RenderFrameObserver

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

This patch changes SpellCheckProvider into a RenderFrameObserver, so that
spellcheck messages from OOPIFs can be successfully sent to browser, making
spell-checking work in OOPIFs.

BUG=638361,710044,625068
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TEST=WIP...
==========

to

==========
Change SpellCheckProvider into a RenderFrameObserver

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

This patch changes SpellCheckProvider into a RenderFrameObserver, so that
spellcheck messages from OOPIFs can be successfully sent to browser, making
spell-checking work in OOPIFs.

BUG=638361,710044,625068
==========

[Xiaocheng, 2017-04-12 00:08:31]

thestig@: PTAL?

Since M59 branch date is approaching, I'd like to land this fix first without
test case without test case.

I'm still working on the test case, and will add it later.

[Xiaocheng, 2017-04-12 00:08:43]

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

[commit-bot: I haz the power, 2017-04-12 00:09:15]

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-12 00:34:28]

LGTM. Feel free to fix the nits below later.

https://codereview.chromium.org/2799923003/diff/100001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2799923003/diff/100001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:261: bool
Visit(content::RenderFrame* render_frame) override;
BTW, can you add a dtor and remember to use override keyword?

https://codereview.chromium.org/2799923003/diff/100001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:264: SpellCheck* spellcheck_; 
// New shared spellcheck for all views. Weak Ptr.
SpellCheck* const, since it never changes.

[Xiaocheng, 2017-04-12 00:54:03]

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

[Xiaocheng, 2017-04-12 00:54:13]

Thanks for reviewing!

https://codereview.chromium.org/2799923003/diff/100001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2799923003/diff/100001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:261: bool
Visit(content::RenderFrame* render_frame) override;
On 2017/04/12 at 00:34:28, Lei Zhang wrote:
> BTW, can you add a dtor and remember to use override keyword?

Done.

https://codereview.chromium.org/2799923003/diff/100001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:264: SpellCheck* spellcheck_; 
// New shared spellcheck for all views. Weak Ptr.
On 2017/04/12 at 00:34:28, Lei Zhang wrote:
> SpellCheck* const, since it never changes.

Done.

[commit-bot: I haz the power, 2017-04-12 00:54:38]

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 01:50:14]

The CQ bit was unchecked by xiaochengh@chromium.org

[Xiaocheng, 2017-04-12 01:50:15]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-04-12 01:50:16]

The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org,
rouslan@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2799923003/#ps120001
(title: "Tue Apr 11 17:50:52 PDT 2017")

[commit-bot: I haz the power, 2017-04-12 01:50:41]

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

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491961815977800,
"parent_rev": "f0df49dd3525a1fb2b5068edb981593256476986", "commit_rev":
"0c92b1cfa21b855c0e8c19f5550ae5209a325936"}

[commit-bot: I haz the power, 2017-04-12 03:07:53]

Description was changed from

==========
Change SpellCheckProvider into a RenderFrameObserver

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

This patch changes SpellCheckProvider into a RenderFrameObserver, so that
spellcheck messages from OOPIFs can be successfully sent to browser, making
spell-checking work in OOPIFs.

BUG=638361,710044,625068
==========

to

==========
Change SpellCheckProvider into a RenderFrameObserver

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

This patch changes SpellCheckProvider into a RenderFrameObserver, so that
spellcheck messages from OOPIFs can be successfully sent to browser, making
spell-checking work in OOPIFs.

BUG=638361,710044,625068

Review-Url: https://codereview.chromium.org/2799923003
Cr-Commit-Position: refs/heads/master@{#463912}
Committed:
https://chromium.googlesource.com/chromium/src/+/0c92b1cfa21b855c0e8c19f5550a...
==========

[commit-bot: I haz the power, 2017-04-12 03:07:55]

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