Eliminate class TextCheckingHelper

Eliminate class TextCheckingHelper

The class TextCheckingHelper was introduced for helping synchronous spell
checking, which can now only be initiated from |advanceToNextMisspelling()|.
Given its very limited use today and the simplicity of its logic, this patch
eliminates the class and merges its code into the SpellChecker class to
simplify the code base.

BUG=633509
TEST=n/a; no behavior change

[Xiaocheng, 2016-08-16 01:54:35]

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

[commit-bot: I haz the power, 2016-08-16 01:55:01]

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

[Xiaocheng, 2016-08-16 02:17:02]

Description was changed from

==========
Eliminate class TextCheckingHelper

BUG=
==========

to

==========
Eliminate class TextCheckingHelper

The class TextCheckingHelper was introduced for helping synchronous spell
checking, which can now only be initiated from |advanceToNextMisspelling()|.
Given its very limited use today and the simplicity of its logic, this patch
eliminates the class and merges its code into the SpellChecker class to
simplify the code base.

BUG=633509
TEST=n/a; no behavior change
==========

[commit-bot: I haz the power, 2016-08-16 05:22:19]

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

[commit-bot: I haz the power, 2016-08-16 05:22:20]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2016-08-16 05:32:33]

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

[Xiaocheng, 2016-08-16 05:32:33]

PTAL.

[yosin_UTC9, 2016-08-16 06:04:06]

Since, |SpellChecker::findMisspellings()| seems to be a new function rather than
moving.
Let's move it into |SpellChecker| in-place. Then move to "SpellChecker.cpp" for
ease of reviewing.

https://codereview.chromium.org/2247003005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right):

https://codereview.chromium.org/2247003005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:161: void
SpellChecker::findMisspellings(const String& text, Vector<TextCheckingResult>*
results)
Since WTF::Vector is now movable[1], let's return WTF::Vector instead of passing
it.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/Vector.h?q...

https://codereview.chromium.org/2247003005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:180: if (0 <
misspellingLength) {
I prefer |misspellingLength > 0|

https://codereview.chromium.org/2247003005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:181:
DCHECK_LE(0, misspellingLocation);
I prefer DCHECK_GE(misspellingLocation, 0);

https://codereview.chromium.org/2247003005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:183:
DCHECK_LT(0, misspellingLength);
I prefer DCHECK_GT(misspellingLength, 0)

https://codereview.chromium.org/2247003005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:191: 
nit: Please remove an extra blank line.

https://codereview.chromium.org/2247003005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/spellcheck/TextCheckingParagraph.cpp
(left):

https://codereview.chromium.org/2247003005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/TextCheckingParagraph.cpp:44:
static void findMisspellings(TextCheckerClient& client, const String& text,
Vector<TextCheckingResult>& results)
Let's move class here, then re-factor.

[Xiaocheng, 2016-08-16 06:21:36]

> Since, |SpellChecker::findMisspellings()| seems to be a new function rather
than moving.
> Let's move it into |SpellChecker| in-place. Then move to "SpellChecker.cpp"
for ease of reviewing.

Sorry I'm not following. Could you explain?

https://codereview.chromium.org/2247003005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right):

https://codereview.chromium.org/2247003005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:161: void
SpellChecker::findMisspellings(const String& text, Vector<TextCheckingResult>*
results)
On 2016/08/16 at 06:04:06, Yosi_UTC9 wrote:
> Since WTF::Vector is now movable[1], let's return WTF::Vector instead of
passing it.
> 
> [1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/Vector.h?q...

Didn't know that. Done.

https://codereview.chromium.org/2247003005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:180: if (0 <
misspellingLength) {
On 2016/08/16 at 06:04:06, Yosi_UTC9 wrote:
> I prefer |misspellingLength > 0|

Done.

https://codereview.chromium.org/2247003005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:181:
DCHECK_LE(0, misspellingLocation);
On 2016/08/16 at 06:04:06, Yosi_UTC9 wrote:
> I prefer DCHECK_GE(misspellingLocation, 0);

Done.

https://codereview.chromium.org/2247003005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:183:
DCHECK_LT(0, misspellingLength);
On 2016/08/16 at 06:04:06, Yosi_UTC9 wrote:
> I prefer DCHECK_GT(misspellingLength, 0)

Done.

https://codereview.chromium.org/2247003005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:191: 
On 2016/08/16 at 06:04:06, Yosi_UTC9 wrote:
> nit: Please remove an extra blank line.

Done.

[yosin_UTC9, 2016-08-16 06:41:47]

On 2016/08/16 at 06:21:36, xiaochengh wrote:
> > Since, |SpellChecker::findMisspellings()| seems to be a new function rather
than moving.
> > Let's move it into |SpellChecker| in-place. Then move to "SpellChecker.cpp"
for ease of reviewing.
> 
> Sorry I'm not following. Could you explain?

We should have two patches:
1. Change "TextCheckingHelper.cpp" to change |static void findMisspellings()| to
|SpellChecker::findMispellings()| and its call sites. Note: no re-factoring.
2. Move |SpellChecker::findMispellings()| to "SpellChecker.cpp"

[Xiaocheng, 2016-08-16 06:58:15]

On 2016/08/16 at 06:41:47, yosin wrote:
> On 2016/08/16 at 06:21:36, xiaochengh wrote:
> > > Since, |SpellChecker::findMisspellings()| seems to be a new function
rather than moving.
> > > Let's move it into |SpellChecker| in-place. Then move to
"SpellChecker.cpp" for ease of reviewing.
> > 
> > Sorry I'm not following. Could you explain?
> 
> We should have two patches:
> 1. Change "TextCheckingHelper.cpp" to change |static void findMisspellings()|
to |SpellChecker::findMispellings()| and its call sites. Note: no re-factoring.
> 2. Move |SpellChecker::findMispellings()| to "SpellChecker.cpp"

I still don't understand. What's the benefit of having two patches, at the cost
of complicating stuff by introducing an intermediate state?

Can we talk offline?