Move ownership of TextCheckerClientImpl to WebLocalFrameImpl

Move ownership of TextCheckerClientImpl to WebLocalFrameImpl

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

This patch changes TextCheckerClientImpl from view-based to frame-based, as
a preparation for changing SpellCheckProvider from view-based to frame-based.

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

[Xiaocheng, 2017-04-04 01:34:05]

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

[commit-bot: I haz the power, 2017-04-04 01:34:42]

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-04 02:03:19]

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

[Xiaocheng, 2017-04-04 02:03:22]

Kent-san: Could you help me with a question in this WIP patch?

I'm trying to add a new member to WebLocalFrameImpl and expose it to core/. Is
my current approach correct?

Thanks!

https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/LocalFrameClientImpl.h (right):

https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/LocalFrameClientImpl.h:228: TextCheckerClient*
textCheckerClient() const override;
Is it the correct way to expose it to core?

https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.h (right):

https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.h:505:
Member<TextCheckerClientImpl> m_textCheckerClient;
Defined here.

[haraken, 2017-04-04 02:21:24]

Would you elaborate on the background of this CL? Why do you want to move
ownership of TextCheckerClientImpl to WebLocalFrameImpl?

[Xiaocheng, 2017-04-04 02:48:10]

Description was changed from

==========
WIP Move ownership of TextCheckerClientImpl to WebLocalFrameImpl

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

to

==========
WIP Move ownership of TextCheckerClientImpl to WebLocalFrameImpl

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

This patch changes TextCheckerClientImpl from view-based to frame-based, as
a preparation for changing SpellCheckProvider from view-based to frame-based.

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

[Xiaocheng, 2017-04-04 02:49:52]

On 2017/04/04 at 02:21:24, haraken wrote:
> Would you elaborate on the background of this CL? Why do you want to move
ownership of TextCheckerClientImpl to WebLocalFrameImpl?

Sure.

The big picture is that we need to change SpellCheckProvider (at least most of
it) from view-based to frame-based, so that it can work correctly in OOPIF.

TextCheckerClientImpl is the Blink-side class communicating with
SpellCheckProvider, so it also need to be made frame-based.

[tkent, 2017-04-04 03:43:33]

On 2017/04/04 at 02:03:22, xiaochengh wrote:
> I'm trying to add a new member to WebLocalFrameImpl and expose it to core/. Is
my current approach correct?

Looks ok.

[haraken, 2017-04-04 05:19:22]

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

[haraken, 2017-04-04 05:19:22]

Thanks, makes sense. LGTM.

I'm just curious: Once we make TextCheckerClient frame-based, what's preventing
us from moving TextCheckerClient to core/? (i.e., why do we need to implement
TextCheckerClient in web/?)

https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/EmptyClients.h (right):

https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/EmptyClients.h:372: TextCheckerClient*
textCheckerClient() const override { return nullptr; }

Would it be possible to return an EmptyTextCheckerClient here instead of
creating it in SpellChecker.cpp?

https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/TextCheckerClientImpl.cpp (right):

https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/TextCheckerClientImpl.cpp:22: return
m_webLocalFrame->viewImpl()->textCheckClient();

Add a TODO and mention that textCheckClient should be moved to WebLocalFrame?

https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/TextCheckerClientImpl.h (right):

https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/TextCheckerClientImpl.h:20:
TextCheckerClientImpl(WebLocalFrameImpl*);

Add explicit.

[Xiaocheng, 2017-04-04 19:28:40]

Thanks for the review!

On 2017/04/04 at 05:19:22, haraken wrote:
> Thanks, makes sense. LGTM.
> 
> I'm just curious: Once we make TextCheckerClient frame-based, what's
preventing us from moving TextCheckerClient to core/? (i.e., why do we need to
implement TextCheckerClient in web/?)

I'm not very familiar with the communication between Blink and non-Blink
classes.

TextCheckerClient is the glue layer so that blink::SpellChecker can communicate
with SpellCheckProvider (in src/components/, implementing
blink::WebSpellCheckClient).

Is there any way that a core/ class can communicate with a non-Blink class
without a web/ class?

> 
>
https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/EmptyClients.h (right):
> 
>
https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/EmptyClients.h:372: TextCheckerClient*
textCheckerClient() const override { return nullptr; }
> 
> Would it be possible to return an EmptyTextCheckerClient here instead of
creating it in SpellChecker.cpp?
> 
>
https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/TextCheckerClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/TextCheckerClientImpl.cpp:22: return
m_webLocalFrame->viewImpl()->textCheckClient();
> 
> Add a TODO and mention that textCheckClient should be moved to WebLocalFrame?
> 
>
https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/TextCheckerClientImpl.h (right):
> 
>
https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/TextCheckerClientImpl.h:20:
TextCheckerClientImpl(WebLocalFrameImpl*);
> 
> Add explicit.

[Xiaocheng, 2017-04-04 19:28:56]

https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/TextCheckerClientImpl.cpp (right):

https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/TextCheckerClientImpl.cpp:22: return
m_webLocalFrame->viewImpl()->textCheckClient();
On 2017/04/04 at 05:19:22, haraken wrote:
> Add a TODO and mention that textCheckClient should be moved to WebLocalFrame?

Done.

https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/TextCheckerClientImpl.h (right):

https://codereview.chromium.org/2795113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/TextCheckerClientImpl.h:20:
TextCheckerClientImpl(WebLocalFrameImpl*);
On 2017/04/04 at 05:19:22, haraken wrote:
> Add explicit.

Done.

[Xiaocheng, 2017-04-04 22:26:12]

Description was changed from

==========
WIP Move ownership of TextCheckerClientImpl to WebLocalFrameImpl

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

This patch changes TextCheckerClientImpl from view-based to frame-based, as
a preparation for changing SpellCheckProvider from view-based to frame-based.

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

to

==========
Move ownership of TextCheckerClientImpl to WebLocalFrameImpl

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

This patch changes TextCheckerClientImpl from view-based to frame-based, as
a preparation for changing SpellCheckProvider from view-based to frame-based.

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

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

Now this patch is ready for landing.

PTAL.

[Xiaocheng, 2017-04-04 22:43:38]

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

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

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

[tkent, 2017-04-04 23:37:48]

lgtm

Core classes can use WebTextCheckClient directly if we move
public/web/WebTextCheckClient.h to public/platform/.

[Xiaocheng, 2017-04-04 23:45:37]

On 2017/04/04 at 23:37:48, tkent wrote:
> lgtm
> 
> Core classes can use WebTextCheckClient directly if we move
public/web/WebTextCheckClient.h to public/platform/.

OK. I'll try that after fixing this bug.

[haraken, 2017-04-05 00:15:26]

On 2017/04/04 23:45:37, Xiaocheng wrote:
> On 2017/04/04 at 23:37:48, tkent wrote:
> > lgtm
> > 
> > Core classes can use WebTextCheckClient directly if we move
> public/web/WebTextCheckClient.h to public/platform/.
> 
> OK. I'll try that after fixing this bug.

That sounds nicer!

[commit-bot: I haz the power, 2017-04-05 01:49:35]

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

[commit-bot: I haz the power, 2017-04-05 01:49:36]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-04-05 02:25:35]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-04-05 02:25:35]

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/2795113002/#ps80001
(title: "Rebased")

[commit-bot: I haz the power, 2017-04-05 02:26:03]

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-05 02:31:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491359135513760,
"parent_rev": "217bb29f809ae73e537ec9c0eba5ac65f48fd873", "commit_rev":
"12aaa612b318e359ab3d6664c3fbbbb3ea14f034"}

[commit-bot: I haz the power, 2017-04-05 02:31:53]

Description was changed from

==========
Move ownership of TextCheckerClientImpl to WebLocalFrameImpl

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

This patch changes TextCheckerClientImpl from view-based to frame-based, as
a preparation for changing SpellCheckProvider from view-based to frame-based.

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

to

==========
Move ownership of TextCheckerClientImpl to WebLocalFrameImpl

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

This patch changes TextCheckerClientImpl from view-based to frame-based, as
a preparation for changing SpellCheckProvider from view-based to frame-based.

BUG=638361

Review-Url: https://codereview.chromium.org/2795113002
Cr-Commit-Position: refs/heads/master@{#461945}
Committed:
https://chromium.googlesource.com/chromium/src/+/12aaa612b318e359ab3d6664c3fb...
==========

[commit-bot: I haz the power, 2017-04-05 02:31:54]

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