Use finer-grained checking in cold mode

Use finer-grained checking in cold mode

This patch makes idle time spell checker's cold mode invocation
more fine-controlled. Idle time spell checker used to check all
chunks of a large editable element in a single invocation, which
can introduce a long unresponsive period. This patch allows
different chunks to be checked in different invocations, so that
the renderer remains responsive.

BUG=517298
Review-Url: https://codereview.chromium.org/2768393003
Cr-Commit-Position: refs/heads/master@{#459867}
Committed: https://chromium.googlesource.com/chromium/src/+/5bb4473303cbce6262892623936b9a907d1b94c0

[Xiaocheng, 2017-03-24 05:27:43]

Description was changed from

==========
Use finer-grained checking in cold mode

BUG=517298
==========

to

==========
WIP Use finer-grained checking in cold mode

This patch makes idle time spell checker's cold mode invocation
more fine-controlled. Idle time spell checker used to check all
chunks of a large editable element in a single invocation, which
can introduce a long unresponsive period. This patch allows
different chunks to be checked in different invocations, so that
the renderer remains responsive.

BUG=517298
==========

[Xiaocheng, 2017-03-24 05:29:12]

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

[Xiaocheng, 2017-03-24 05:29:13]

How do you like this idea?

This patch is not for committing yet as test cases are needed. I would still
like to get some idea before the weekend.

[yosin_UTC9, 2017-03-24 06:38:42]

For fast layout of text, we're thinking about plain text editor for Text node to
provide:
 - track minimum/maximum change offset
 - cache paragraph start; BiDi algorithm requires a paragraph of UTF-16 string
 - cache line start/end; fast scroll up/down

For mean time, can we consider another way to get text chunk instead of
TextIterator?

Another idea is using Range for tracking where we did spell checking.

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

https://codereview.chromium.org/2768393003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/ColdModeSpellCheckRequester.cpp:127:
m_nextNode = FlatTreeTraversal::next(*m_nextNode, frame().document()->body());
We need to call |m_nextNode->updateDistribution()| which updates all
distribution of the root tree contains |m_nextNode|.

It seems we assume this function called w/ clean layout tree.

https://codereview.chromium.org/2768393003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/ColdModeSpellCheckRequester.cpp:133:
m_currentFullLength = TextIterator::rangeLength(fullRange.startPosition(),
Can we use DOM tree base character iterator?
Or use string in layout tree?
In layout NG, text is whitespace collapsed + text-transformed in UTF-16 string.
If we use it, we don't need to use TextIterator.

[Xiaocheng, 2017-03-24 21:18:43]

For plain text editor, I think we should optimize TextIterator directly instead
of optimizing here. Note that SpellCheckRequest::create() also extracts plain
text with TI, so doing an optimization here doesn't help too much.

Optimization of TextIterator for plain text editor should be part of TI NG, I
think.

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

https://codereview.chromium.org/2768393003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/ColdModeSpellCheckRequester.cpp:127:
m_nextNode = FlatTreeTraversal::next(*m_nextNode, frame().document()->body());
On 2017/03/24 at 06:38:42, yosin_UTC9 wrote:
> We need to call |m_nextNode->updateDistribution()| which updates all
distribution of the root tree contains |m_nextNode|.
> 
> It seems we assume this function called w/ clean layout tree.

This function is always called with clean layout, which is updated in
ColdModeSpellCheckRequester::invoke.

So no need to update Distribution.

https://codereview.chromium.org/2768393003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/ColdModeSpellCheckRequester.cpp:133:
m_currentFullLength = TextIterator::rangeLength(fullRange.startPosition(),
On 2017/03/24 at 06:38:42, yosin_UTC9 wrote:
> Can we use DOM tree base character iterator?
> Or use string in layout tree?
> In layout NG, text is whitespace collapsed + text-transformed in UTF-16
string.
> If we use it, we don't need to use TextIterator.

Sorry I don't understand. Could you give more details?

[Xiaocheng, 2017-03-25 00:43:14]

Description was changed from

==========
WIP Use finer-grained checking in cold mode

This patch makes idle time spell checker's cold mode invocation
more fine-controlled. Idle time spell checker used to check all
chunks of a large editable element in a single invocation, which
can introduce a long unresponsive period. This patch allows
different chunks to be checked in different invocations, so that
the renderer remains responsive.

BUG=517298
==========

to

==========
Use finer-grained checking in cold mode

This patch makes idle time spell checker's cold mode invocation
more fine-controlled. Idle time spell checker used to check all
chunks of a large editable element in a single invocation, which
can introduce a long unresponsive period. This patch allows
different chunks to be checked in different invocations, so that
the renderer remains responsive.

BUG=517298
==========

[Xiaocheng, 2017-03-25 00:44:39]

I guess I don't really need test cases. This patch is not supposed to introduce
any easily observable change.

So PTAL.

[yosin_UTC9, 2017-03-27 04:44:15]

lgtm

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

https://codereview.chromium.org/2768393003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/ColdModeSpellCheckRequester.cpp:133:
m_currentFullLength = TextIterator::rangeLength(fullRange.startPosition(),
On 2017/03/24 at 21:18:43, Xiaocheng wrote:
> On 2017/03/24 at 06:38:42, yosin_UTC9 wrote:
> > Can we use DOM tree base character iterator?
> > Or use string in layout tree?
> > In layout NG, text is whitespace collapsed + text-transformed in UTF-16
string.
> > If we use it, we don't need to use TextIterator.
> 
> Sorry I don't understand. Could you give more details?

TextIterator::rangeLength() is expensive and PlaintTextRange are expensive.
So, we want to avoid to use them.

When we use DOM tree base character iteration, we may check extra characters,
e.g. 
characters in display:none, but it may be cheaper and simpler than using
TextIterator.

Anyway, this is not for this patch.

[Xiaocheng, 2017-03-27 18:01:24]

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

[commit-bot: I haz the power, 2017-03-27 18:02:22]

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-03-27 20:26:47]

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

[commit-bot: I haz the power, 2017-03-27 20:26:47]

Dry run: This issue passed the CQ dry run.

[xiaochengh, 2017-03-27 20:32:49]

The CQ bit was checked by xiaochengh@google.com

[commit-bot: I haz the power, 2017-03-27 20:33:50]

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-03-27 20:45:14]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1490646769128570, "parent_rev":
"d9dbcf9e54cef6adb8f50288a6d907004cbfeb88", "commit_rev":
"5bb4473303cbce6262892623936b9a907d1b94c0"}

[commit-bot: I haz the power, 2017-03-27 20:45:59]

Description was changed from

==========
Use finer-grained checking in cold mode

This patch makes idle time spell checker's cold mode invocation
more fine-controlled. Idle time spell checker used to check all
chunks of a large editable element in a single invocation, which
can introduce a long unresponsive period. This patch allows
different chunks to be checked in different invocations, so that
the renderer remains responsive.

BUG=517298
==========

to

==========
Use finer-grained checking in cold mode

This patch makes idle time spell checker's cold mode invocation
more fine-controlled. Idle time spell checker used to check all
chunks of a large editable element in a single invocation, which
can introduce a long unresponsive period. This patch allows
different chunks to be checked in different invocations, so that
the renderer remains responsive.

BUG=517298

Review-Url: https://codereview.chromium.org/2768393003
Cr-Commit-Position: refs/heads/master@{#459867}
Committed:
https://chromium.googlesource.com/chromium/src/+/5bb4473303cbce6262892623936b...
==========

[commit-bot: I haz the power, 2017-03-27 20:45:59]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/5bb4473303cbce6262892623936b...