Split WebTextCheckClient off WebSpellCheckClient

Split WebTextCheckClient off WebSpellCheckClient

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

This patch splits the text checking functions off interface WebSpellCheckClient
into a new interface WebTextCheckClient, and makes it also referenced by WebView
(for now). Clients should call WebView::setTextCheckClient in initialization.

For subclasses implementing the interface:
- SpellCheckProvider is made to implement both interfaces
- Other subclasses are switch to implement WebTextCheckClient since they no
  longer implement any of WebSpellCheckClient's functions

BUG=638361
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2791753003
Cr-Commit-Position: refs/heads/master@{#461857}
Committed: https://chromium.googlesource.com/chromium/src/+/62dc793dc640ae5fd16d571a0c9199fe0fb740d0

[Xiaocheng, 2017-04-03 20:05:45]

Description was changed from

==========
Split WebTextCheckClient off WebSpellCheckClient

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

to

==========
Split WebTextCheckClient off WebSpellCheckClient

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

This patch splits the text checking functions off interface
WebSpellCheckClient into a new interface WebTextCheckClient,
and makes it also referenced by WebView. There is no change
to the classes implementing the interface -- they are changed
to be implementing both interfaces, for now. The only change
is that they also need to call WebView::setTextCheckClient to
make them referenced as a WebTextCheckClient.

BUG=638361
TEST=n/a; no behavior changes
==========

[Xiaocheng, 2017-04-03 20:05:51]

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

[commit-bot: I haz the power, 2017-04-03 20:06: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-03 20:10:26]

xiaochengh@chromium.org changed reviewers:
+ dcheng@chromium.org, groby@chromium.org, tkent@chromium.org

[Xiaocheng, 2017-04-03 20:10:26]

PTAL.

[Xiaocheng, 2017-04-03 21:36:41]

Description was changed from

==========
Split WebTextCheckClient off WebSpellCheckClient

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

This patch splits the text checking functions off interface
WebSpellCheckClient into a new interface WebTextCheckClient,
and makes it also referenced by WebView. There is no change
to the classes implementing the interface -- they are changed
to be implementing both interfaces, for now. The only change
is that they also need to call WebView::setTextCheckClient to
make them referenced as a WebTextCheckClient.

BUG=638361
TEST=n/a; no behavior changes
==========

to

==========
Split WebTextCheckClient off WebSpellCheckClient

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

This patch splits the text checking functions off interface WebSpellCheckClient
into a new interface WebTextCheckClient, and makes it also referenced by WebView
(for now). Clients should call WebView::setTextCheckClient in initialization.

For subclasses implementing the interface:
- SpellCheckProvider is made to implement both interfaces
- Other subclasses are switch to implement WebTextCheckClient since they no
  longer implement any of WebSpellCheckClient's functions

BUG=638361
TEST=n/a; no behavior changes
==========

[tkent, 2017-04-04 01:58:19]

third_party/WebKit lgtm

https://codereview.chromium.org/2791753003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebTextCheckClient.h (right):

https://codereview.chromium.org/2791753003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebTextCheckClient.h:8: #include
"../platform/WebString.h"
Please avoid ".." in #includes.  It should be "public/platform/WebString.h"

https://codereview.chromium.org/2791753003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebTextCheckClient.h:9: #include
"../platform/WebVector.h"
Ditto.

[Xiaocheng, 2017-04-04 02:53:24]

Thanks for the review. Updated.

https://codereview.chromium.org/2791753003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebTextCheckClient.h (right):

https://codereview.chromium.org/2791753003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebTextCheckClient.h:8: #include
"../platform/WebString.h"
On 2017/04/04 at 01:58:18, tkent wrote:
> Please avoid ".." in #includes.  It should be "public/platform/WebString.h"

Done.

[Xiaocheng, 2017-04-04 02:53:28]

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

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

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

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

[commit-bot: I haz the power, 2017-04-04 04:27:21]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dcheng, 2017-04-04 07:01:09]

The CL seems reasonable to me, but the differentiation between SpellCheckClient
and TextCheckClient seems confusing. I wonder if there's something else we can
name them to make it less ambiguous?

[Xiaocheng, 2017-04-04 19:01:59]

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

[Xiaocheng, 2017-04-04 19:02:13]

On 2017/04/04 at 07:01:09, dcheng wrote:
> The CL seems reasonable to me, but the differentiation between
SpellCheckClient and TextCheckClient seems confusing. I wonder if there's
something else we can name them to make it less ambiguous?

+rouslan

Indeed. I think we should rename the Blink-side classes as:

Classes in editing/spellcheck: remain the same

TextCheckerClient[Impl] --> SpellCheckerClient[Impl]

SpellCheckerClient[Impl] --> Split into
 - SpellingUIClient[Impl] for SpellingUI-related functions
 - SpellCheckToggler[Impl] for checking and toggling spellcheck enabled status

WebSpellCheckClient --> Split into
 - WebSpellCheckClient for checking related functions
 - WebSpellingUIClient for spelling UI related functions

I'll upload another patch for the renaming if nobody objects.

[please use gerrit instead, 2017-04-04 19:03:42]

components/spellcheck lgtm

[please use gerrit instead, 2017-04-04 19:04:11]

Renaming would be OK with me as well.

[Xiaocheng, 2017-04-04 19:47:15]

dcheng@: Can we proceed with these patches first, and perform the renaming at
the end? The renaming plan is also in the design doc: https://goo.gl/VfCENk

Landing a renaming patch first messes up all the on-going patches.

[dcheng, 2017-04-04 19:50:18]

On 2017/04/04 19:47:15, Xiaocheng wrote:
> dcheng@: Can we proceed with these patches first, and perform the renaming at
> the end? The renaming plan is also in the design doc: https://goo.gl/VfCENk
> 
> Landing a renaming patch first messes up all the on-going patches.

That's fine. Can we add some extra comments to WebTextCheckClient and
WebSpellChcekClient to help differentiate between them?

LGTM with that addressed.

[Xiaocheng, 2017-04-04 20:01:26]

Thanks! I've added a bunch of TODOs in PS#5 about the renaming and splitting.

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

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

[commit-bot: I haz the power, 2017-04-04 20:02:52]

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-04 21:43:53]

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

[commit-bot: I haz the power, 2017-04-04 21:43:54]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-04-04 21:45:04]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-04-04 21:45:05]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
rouslan@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2791753003/#ps80001
(title: "Add TODOs for class renaming")

[commit-bot: I haz the power, 2017-04-04 21:45:52]

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-04 21:59:42]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491342304944420,
"parent_rev": "0c1d4036c5f4aafb0530816aafce288cc56a691b", "commit_rev":
"62dc793dc640ae5fd16d571a0c9199fe0fb740d0"}

[commit-bot: I haz the power, 2017-04-04 22:00:31]

Description was changed from

==========
Split WebTextCheckClient off WebSpellCheckClient

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

This patch splits the text checking functions off interface WebSpellCheckClient
into a new interface WebTextCheckClient, and makes it also referenced by WebView
(for now). Clients should call WebView::setTextCheckClient in initialization.

For subclasses implementing the interface:
- SpellCheckProvider is made to implement both interfaces
- Other subclasses are switch to implement WebTextCheckClient since they no
  longer implement any of WebSpellCheckClient's functions

BUG=638361
TEST=n/a; no behavior changes
==========

to

==========
Split WebTextCheckClient off WebSpellCheckClient

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

This patch splits the text checking functions off interface WebSpellCheckClient
into a new interface WebTextCheckClient, and makes it also referenced by WebView
(for now). Clients should call WebView::setTextCheckClient in initialization.

For subclasses implementing the interface:
- SpellCheckProvider is made to implement both interfaces
- Other subclasses are switch to implement WebTextCheckClient since they no
  longer implement any of WebSpellCheckClient's functions

BUG=638361
TEST=n/a; no behavior changes

Review-Url: https://codereview.chromium.org/2791753003
Cr-Commit-Position: refs/heads/master@{#461857}
Committed:
https://chromium.googlesource.com/chromium/src/+/62dc793dc640ae5fd16d571a0c91...
==========

[commit-bot: I haz the power, 2017-04-04 22:00:33]

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