Stop SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar from using TextCheckingParagrah

Stop SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar from using TextCheckingParagrah

SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar() does not really need
a TextCheckingParagraph as its parameter; an EphemeralRange of text to check is
sufficient.

Hence, this patch changes its parameter from a TextCheckingParagraph to an
EphemeralRange, simplifying code and improving performance by eliminating the
overhead for finding the paragraph containing the text to be checked.

BUG=n/a
TEST=n/a; no visible change

Committed: https://crrev.com/41165826c11cc0b7f1457b4c455d7e2eb2de0819
Cr-Commit-Position: refs/heads/master@{#414328}

[Xiaocheng, 2016-08-23 10:41:50]

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

[commit-bot: I haz the power, 2016-08-23 10:42:10]

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, 2016-08-23 11:58:42]

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

[commit-bot: I haz the power, 2016-08-23 11:58:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xiaocheng, 2016-08-24 09:46:19]

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

[commit-bot: I haz the power, 2016-08-24 09:46: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, 2016-08-24 11:53:08]

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

[commit-bot: I haz the power, 2016-08-24 11:53:09]

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

[Xiaocheng, 2016-08-25 01:20:32]

Description was changed from

==========
Make the use of TextCheckingParagraph restricted in markAndReplaceFor

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

to

==========
Stop SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar from using
TextCheckingParagrah

SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar() does not really need
a TextCheckingParagraph as its parameter; an EphemeralRange of text to check is
sufficient.

Hence, this patch changes its parameter from a TextCheckingParagraph to an
EphemeralRange, simplifying code and improving performance by eliminating the
overhead for finding the paragraph containing the text to be checked.

BUG=n/a
TEST=n/a; no visible change
==========

[Xiaocheng, 2016-08-25 01:21:49]

Description was changed from

==========
Stop SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar from using
TextCheckingParagrah

SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar() does not really need
a TextCheckingParagraph as its parameter; an EphemeralRange of text to check is
sufficient.

Hence, this patch changes its parameter from a TextCheckingParagraph to an
EphemeralRange, simplifying code and improving performance by eliminating the
overhead for finding the paragraph containing the text to be checked.

BUG=n/a
TEST=n/a; no visible change
==========

to

==========
Stop SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar from using
TextCheckingParagrah

SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar() does not really need
a TextCheckingParagraph as its parameter; an EphemeralRange of text to check is
sufficient.

Hence, this patch changes its parameter from a TextCheckingParagraph to an
EphemeralRange, simplifying code and improving performance by eliminating the
overhead for finding the paragraph containing the text to be checked.

This patch is part of the effort for reducing the use of TextCheckingParagraph.
After this patch, SpellChecker::markAndReplaceFor will become the only client
of it.

BUG=n/a
TEST=n/a; no visible change
==========

[Xiaocheng, 2016-08-25 01:21:50]

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

[Xiaocheng, 2016-08-25 01:23:15]

PTAL.

[Xiaocheng, 2016-08-25 01:23:30]

Description was changed from

==========
Stop SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar from using
TextCheckingParagrah

SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar() does not really need
a TextCheckingParagraph as its parameter; an EphemeralRange of text to check is
sufficient.

Hence, this patch changes its parameter from a TextCheckingParagraph to an
EphemeralRange, simplifying code and improving performance by eliminating the
overhead for finding the paragraph containing the text to be checked.

This patch is part of the effort for reducing the use of TextCheckingParagraph.
After this patch, SpellChecker::markAndReplaceFor will become the only client
of it.

BUG=n/a
TEST=n/a; no visible change
==========

to

==========
Stop SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar from using
TextCheckingParagrah

SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar() does not really need
a TextCheckingParagraph as its parameter; an EphemeralRange of text to check is
sufficient.

Hence, this patch changes its parameter from a TextCheckingParagraph to an
EphemeralRange, simplifying code and improving performance by eliminating the
overhead for finding the paragraph containing the text to be checked.

BUG=n/a
TEST=n/a; no visible change
==========

[yosin_UTC9, 2016-08-25 01:32:56]

lgtm

What's a wonderful change! Nice finding! (^_^)b

[Xiaocheng, 2016-08-25 01:55:57]

On 2016/08/25 at 01:32:56, yosin wrote:
> lgtm
> 
> What's a wonderful change! Nice finding! (^_^)b

In retrospect, why are we tuning the text range by word/sentence/paragraph
boundaries before creating a SpellCheckRequest? If we do as what this patch
does, we are already dumping everything in the root editable element into the
request in most cases (when the text is short), making the previous tuning
useless (unless the full text range is super long).

Or maybe we should make the check |if (fullTextLength <= kChunkSize)| prior to
all the tuning?

[Xiaocheng, 2016-08-25 04:00:26]

The CQ bit was checked by xiaochengh@chromium.org

[commit-bot: I haz the power, 2016-08-25 04:00:51]

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, 2016-08-25 05:39:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-25 05:42:19]

Description was changed from

==========
Stop SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar from using
TextCheckingParagrah

SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar() does not really need
a TextCheckingParagraph as its parameter; an EphemeralRange of text to check is
sufficient.

Hence, this patch changes its parameter from a TextCheckingParagraph to an
EphemeralRange, simplifying code and improving performance by eliminating the
overhead for finding the paragraph containing the text to be checked.

BUG=n/a
TEST=n/a; no visible change
==========

to

==========
Stop SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar from using
TextCheckingParagrah

SpellChecker::chunkAndMarkAllMisspellingsAndBadGrammar() does not really need
a TextCheckingParagraph as its parameter; an EphemeralRange of text to check is
sufficient.

Hence, this patch changes its parameter from a TextCheckingParagraph to an
EphemeralRange, simplifying code and improving performance by eliminating the
overhead for finding the paragraph containing the text to be checked.

BUG=n/a
TEST=n/a; no visible change

Committed: https://crrev.com/41165826c11cc0b7f1457b4c455d7e2eb2de0819
Cr-Commit-Position: refs/heads/master@{#414328}
==========

[commit-bot: I haz the power, 2016-08-25 05:42:21]

Patchset 2 (id:??) landed as
https://crrev.com/41165826c11cc0b7f1457b4c455d7e2eb2de0819
Cr-Commit-Position: refs/heads/master@{#414328}

[groby-ooo-7-16, 2016-08-25 14:28:43]

On 2016/08/25 01:55:57, Xiaocheng wrote:
> On 2016/08/25 at 01:32:56, yosin wrote:
> > lgtm
> > 
> > What's a wonderful change! Nice finding! (^_^)b
> 
> In retrospect, why are we tuning the text range by word/sentence/paragraph
> boundaries before creating a SpellCheckRequest? If we do as what this patch
> does, we are already dumping everything in the root editable element into the
> request in most cases (when the text is short), making the previous tuning
> useless (unless the full text range is super long).
> 
> Or maybe we should make the check |if (fullTextLength <= kChunkSize)| prior to
> all the tuning?

"The full range is super-long" is unfortunately a fairly common use case. People
copy/paste what seems entire novels, which can cause significant UI jank. 

And a question, since I'm not too familiar with the Blink code: Doesn't this
patch lose us the fact checks are extended to sentence boundaries? (That's a
somewhat relevant feature for server-side checking, which relies on context)

[Xiaocheng, 2016-08-26 01:29:47]

On 2016/08/25 at 14:28:43, groby wrote:
> On 2016/08/25 01:55:57, Xiaocheng wrote:
> > On 2016/08/25 at 01:32:56, yosin wrote:
> > > lgtm
> > > 
> > > What's a wonderful change! Nice finding! (^_^)b
> > 
> > In retrospect, why are we tuning the text range by word/sentence/paragraph
> > boundaries before creating a SpellCheckRequest? If we do as what this patch
> > does, we are already dumping everything in the root editable element into
the
> > request in most cases (when the text is short), making the previous tuning
> > useless (unless the full text range is super long).
> > 
> > Or maybe we should make the check |if (fullTextLength <= kChunkSize)| prior
to
> > all the tuning?
> 
> "The full range is super-long" is unfortunately a fairly common use case.
People copy/paste what seems entire novels, which can cause significant UI jank.


Yeah, but editing to short text has to be the most common use case. And this
patch shouldn't change anything when pasting long text.

> 
> And a question, since I'm not too familiar with the Blink code: Doesn't this
patch lose us the fact checks are extended to sentence boundaries? (That's a
somewhat relevant feature for server-side checking, which relies on context)

It should still keep passing full sentences. There are two cases:
- When the full text in the root editable element is short, the full text is
checked, which is fine;
- When the full text is long, we are chunking the checking range, extend each
chunk to sentence boundaries and check each extended chunk, which is still fine.

[groby-ooo-7-16, 2016-08-26 01:44:40]

On 2016/08/26 01:29:47, Xiaocheng wrote:
> On 2016/08/25 at 14:28:43, groby wrote:
> > On 2016/08/25 01:55:57, Xiaocheng wrote:
> > > On 2016/08/25 at 01:32:56, yosin wrote:
> > > > lgtm
> > > > 
> > > > What's a wonderful change! Nice finding! (^_^)b
> > > 
> > > In retrospect, why are we tuning the text range by word/sentence/paragraph
> > > boundaries before creating a SpellCheckRequest? If we do as what this
patch
> > > does, we are already dumping everything in the root editable element into
> the
> > > request in most cases (when the text is short), making the previous tuning
> > > useless (unless the full text range is super long).
> > > 
> > > Or maybe we should make the check |if (fullTextLength <= kChunkSize)|
prior
> to
> > > all the tuning?
> > 
> > "The full range is super-long" is unfortunately a fairly common use case.
> People copy/paste what seems entire novels, which can cause significant UI
jank.
> 
> 
> Yeah, but editing to short text has to be the most common use case. And this
> patch shouldn't change anything when pasting long text.
> 
> > 
> > And a question, since I'm not too familiar with the Blink code: Doesn't this
> patch lose us the fact checks are extended to sentence boundaries? (That's a
> somewhat relevant feature for server-side checking, which relies on context)
> 
> It should still keep passing full sentences. There are two cases:
> - When the full text in the root editable element is short, the full text is
> checked, which is fine;
> - When the full text is long, we are chunking the checking range, extend each
> chunk to sentence boundaries and check each extended chunk, which is still
fine.

Thank you for the explanation!

[Xiaocheng, 2016-08-26 01:51:41]

On 2016/08/26 at 01:44:40, groby wrote:
> On 2016/08/26 01:29:47, Xiaocheng wrote:
> > On 2016/08/25 at 14:28:43, groby wrote:
> > > On 2016/08/25 01:55:57, Xiaocheng wrote:
> > > > On 2016/08/25 at 01:32:56, yosin wrote:
> > > > > lgtm
> > > > > 
> > > > > What's a wonderful change! Nice finding! (^_^)b
> > > > 
> > > > In retrospect, why are we tuning the text range by
word/sentence/paragraph
> > > > boundaries before creating a SpellCheckRequest? If we do as what this
patch
> > > > does, we are already dumping everything in the root editable element
into
> > the
> > > > request in most cases (when the text is short), making the previous
tuning
> > > > useless (unless the full text range is super long).
> > > > 
> > > > Or maybe we should make the check |if (fullTextLength <= kChunkSize)|
prior
> > to
> > > > all the tuning?
> > > 
> > > "The full range is super-long" is unfortunately a fairly common use case.
> > People copy/paste what seems entire novels, which can cause significant UI
jank.
> > 
> > 
> > Yeah, but editing to short text has to be the most common use case. And this
> > patch shouldn't change anything when pasting long text.
> > 
> > > 
> > > And a question, since I'm not too familiar with the Blink code: Doesn't
this
> > patch lose us the fact checks are extended to sentence boundaries? (That's a
> > somewhat relevant feature for server-side checking, which relies on context)
> > 
> > It should still keep passing full sentences. There are two cases:
> > - When the full text in the root editable element is short, the full text is
> > checked, which is fine;
> > - When the full text is long, we are chunking the checking range, extend
each
> > chunk to sentence boundaries and check each extended chunk, which is still
fine.
> 
> Thank you for the explanation!

... and this patch is going to be reverted due to crbug.com/641083.

After a discussion with yosin@, we think there's enough refactoring and local
optimization to the existing spell checker. We should start working on the idle
time spell checker.