Implement complete lifecycle transition for IdleSpellCheckCallback

Implement complete lifecycle transition for IdleSpellCheckCallback

This patch implements complete lifecycle state transition for
IdleSpellCheckCallback. With this patch, idle spell check callback
will be invoked at correct timings. The only thing remaining is to
fill in the member functions which actually perform checking when
the callback is invoked.

BUG=517298
TEST=webkit_unit_tests --gtest_filter=IdleSpellCheckCallbackTest.*

Review-Url: https://codereview.chromium.org/2701983002
Cr-Original-Commit-Position: refs/heads/master@{#453159}
Committed: https://chromium.googlesource.com/chromium/src/+/234937696e399887e0acef2ad037c35febca001c
Review-Url: https://codereview.chromium.org/2701983002
Cr-Commit-Position: refs/heads/master@{#453169}
Committed: https://chromium.googlesource.com/chromium/src/+/46b362641fc483d7d5fd0714d96db6dd39de39f6

[Xiaocheng, 2017-02-23 00:34:45]

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

[commit-bot: I haz the power, 2017-02-23 00:36:05]

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-02-23 02:21:02]

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

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

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2017-02-23 04:43:44]

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

[commit-bot: I haz the power, 2017-02-23 04:44:03]

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

[Xiaocheng, 2017-02-23 04:50:07]

Description was changed from

==========
Land complete lifecycle transition for IdleSpellCheckCallback

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

to

==========
Implement complete lifecycle transition for IdleSpellCheckCallback

This patch implements complete lifecycle state transition for
IdleSpellCheckCallback. With this patch, idle spell check callback
will be invoked at correct timings. The only thing remaining is to
fill in the member functions which actually perform checking when
the callback is invoked.

BUG=517298
TEST=webkit_unit_tests --gtest_filter=IdleSpellCheckCallbackTest.*
==========

[Xiaocheng, 2017-02-23 04:50:20]

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

[Xiaocheng, 2017-02-23 04:50:20]

PTAL.

[yosin_UTC9, 2017-02-23 05:20:49]

https://codereview.chromium.org/2701983002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/2701983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.cpp:742:
frame().setNeedsSpellChecking();
Can we make Editor owns both SpellChecker and IdleTimeSpellCheckCallback to make
LocalFrame simple?
Or make SpellChecker to own IdleTimeSpellCheckCallback?

LocalFrame is yet another kitchen sink, we want to decompose it.

[commit-bot: I haz the power, 2017-02-23 06:10:00]

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

[commit-bot: I haz the power, 2017-02-23 06:10:01]

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

[Xiaocheng, 2017-02-23 20:46:08]

https://codereview.chromium.org/2701983002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/2701983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.cpp:742:
frame().setNeedsSpellChecking();
On 2017/02/23 at 05:20:48, yosin_UTC9 wrote:
> Can we make Editor owns both SpellChecker and IdleTimeSpellCheckCallback to
make LocalFrame simple?
> Or make SpellChecker to own IdleTimeSpellCheckCallback?
> 
> LocalFrame is yet another kitchen sink, we want to decompose it.

I have the feeling that editing-related classes shouldn't be owned by LocalFrame
at all -- they don't make much sense when the document is inactive. See my
comment in crbug.com/695359

It seems to be a different issue, though. If we decide to move classes, let's
move them in different patches.

[yosin_UTC9, 2017-02-24 02:10:53]

https://codereview.chromium.org/2701983002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/2701983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/Editor.cpp:742:
frame().setNeedsSpellChecking();
On 2017/02/23 at 20:46:07, Xiaocheng wrote:
> On 2017/02/23 at 05:20:48, yosin_UTC9 wrote:
> > Can we make Editor owns both SpellChecker and IdleTimeSpellCheckCallback to
make LocalFrame simple?
> > Or make SpellChecker to own IdleTimeSpellCheckCallback?
> > 
> > LocalFrame is yet another kitchen sink, we want to decompose it.
> 
> I have the feeling that editing-related classes shouldn't be owned by
LocalFrame at all -- they don't make much sense when the document is inactive.
See my comment in crbug.com/695359
> 
> It seems to be a different issue, though. If we decide to move classes, let's
move them in different patches.

I feel it is strange that SpellChecker tells LocalFrame about spell checking. I
don't imagine LocalFrame care about spell checking.
I think IdleTimeSpellChecking is just implementation details, it should not be
expose outside SpellChecker or at most core/editing/spellcheck

BTW, it seems it is better that SpellChecker is isolate from Editor, yet another
kitchen sink. Because SpellChecker doesn't(should not) depend on Editor and
Editor should not depend on SpellChecker.

[Xiaocheng, 2017-02-24 19:21:08]

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

[commit-bot: I haz the power, 2017-02-24 19:21:38]

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-02-24 20:49:01]

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

[commit-bot: I haz the power, 2017-02-24 20:49:02]

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

[Xiaocheng, 2017-02-24 23:19:46]

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

[commit-bot: I haz the power, 2017-02-24 23:20:30]

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

[Xiaocheng, 2017-02-25 01:03:06]

Updated. PTAL.

[commit-bot: I haz the power, 2017-02-25 01:12:01]

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

[commit-bot: I haz the power, 2017-02-25 01:12:02]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-02-25 02:22:57]

lgtm

https://codereview.chromium.org/2701983002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/2701983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/Editor.cpp:744:
spellChecker().respondToChangedContents(endingSelection);
We may want to rename |respondToChangedContents()| to |didChangeContents()| in
later.
(not in this patch)

https://codereview.chromium.org/2701983002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h
(right):

https://codereview.chromium.org/2701983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h:20:
public SynchronousMutationObserver {
FYI: We may want to introduce |DocumentShutdownObserver|. I saw some speed
regression due by |SMO::didChangeChildren()| which is called by
ContainerNode::appendChild() and insertBefore() in micro benchmark tests.

Since, |IdleSpellCheckerCallback| don't observer DOM mutation, it is good
candidate of |DocumentShutdownObserver|.
Another client is |FrameSelection| once we make |Editor| to observe node removal
for closing typing command.

Anyway, let's see performance bot for results.

[Xiaocheng, 2017-02-25 02:30:06]

Thanks for the review.

On 2017/02/25 at 02:22:57, yosin wrote:
> lgtm
> 
>
https://codereview.chromium.org/2701983002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/Editor.cpp (right):
> 
>
https://codereview.chromium.org/2701983002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/Editor.cpp:744:
spellChecker().respondToChangedContents(endingSelection);
> We may want to rename |respondToChangedContents()| to |didChangeContents()| in
later.
> (not in this patch)
> 
>
https://codereview.chromium.org/2701983002/diff/100001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h
(right):
> 
>
https://codereview.chromium.org/2701983002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h:20:
public SynchronousMutationObserver {
> FYI: We may want to introduce |DocumentShutdownObserver|. I saw some speed
regression due by |SMO::didChangeChildren()| which is called by
ContainerNode::appendChild() and insertBefore() in micro benchmark tests.

I don't understand. Currently we have only 6 subclasses of SMO, all of which are
one-per-document. We shouldn't be spending a lot of time on no-op SMO callbacks.

Is there any data that I can check?

> 
> Since, |IdleSpellCheckerCallback| don't observer DOM mutation, it is good
candidate of |DocumentShutdownObserver|.
> Another client is |FrameSelection| once we make |Editor| to observe node
removal for closing typing command.
> 
> Anyway, let's see performance bot for results.

[tkent, 2017-02-26 20:35:59]

lgtm

https://codereview.chromium.org/2701983002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp
(right):

https://codereview.chromium.org/2701983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:209:
default:
Do not use |default:| in |switch| for an enum.  Please write down all of
remaining items.

https://codereview.chromium.org/2701983002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallbackTest.cpp
(right):

https://codereview.chromium.org/2701983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallbackTest.cpp:41:
default:
Do not use |default:| in |switch| for an enum.  Please write down all of
remaining items.

[Xiaocheng, 2017-02-27 04:06:34]

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

[commit-bot: I haz the power, 2017-02-27 04:06:57]

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

[Xiaocheng, 2017-02-27 04:07:18]

Thanks for the review.

https://codereview.chromium.org/2701983002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp
(right):

https://codereview.chromium.org/2701983002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.cpp:209:
default:
On 2017/02/26 at 20:35:58, tkent wrote:
> Do not use |default:| in |switch| for an enum.  Please write down all of
remaining items.

Done

[Xiaocheng, 2017-02-27 05:24:37]

The CQ bit was unchecked by xiaochengh@chromium.org

[Xiaocheng, 2017-02-27 05:24:39]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-02-27 05:24:39]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2701983002/#ps120001
(title: "Remove default from switch")

[commit-bot: I haz the power, 2017-02-27 05:24: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-02-27 05:45:32]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488173079208580,
"parent_rev": "d57cc7528932df35ce4a7d56f7cfc9dc380e5a09", "commit_rev":
"234937696e399887e0acef2ad037c35febca001c"}

[commit-bot: I haz the power, 2017-02-27 05:46:07]

Description was changed from

==========
Implement complete lifecycle transition for IdleSpellCheckCallback

This patch implements complete lifecycle state transition for
IdleSpellCheckCallback. With this patch, idle spell check callback
will be invoked at correct timings. The only thing remaining is to
fill in the member functions which actually perform checking when
the callback is invoked.

BUG=517298
TEST=webkit_unit_tests --gtest_filter=IdleSpellCheckCallbackTest.*
==========

to

==========
Implement complete lifecycle transition for IdleSpellCheckCallback

This patch implements complete lifecycle state transition for
IdleSpellCheckCallback. With this patch, idle spell check callback
will be invoked at correct timings. The only thing remaining is to
fill in the member functions which actually perform checking when
the callback is invoked.

BUG=517298
TEST=webkit_unit_tests --gtest_filter=IdleSpellCheckCallbackTest.*

Review-Url: https://codereview.chromium.org/2701983002
Cr-Commit-Position: refs/heads/master@{#453159}
Committed:
https://chromium.googlesource.com/chromium/src/+/234937696e399887e0acef2ad037...
==========

[commit-bot: I haz the power, 2017-02-27 05:46:08]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/234937696e399887e0acef2ad037...

[Xiaocheng, 2017-02-27 05:56:02]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2720663002/ by xiaochengh@chromium.org.

The reason for reverting is: Has conflict with r453158.

[hajimehoshi, 2017-02-27 05:56:33]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2715193002/ by hajimehoshi@chromium.org.

The reason for reverting is: It looks like this causes compile errors (e.g.
https://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac%20Builder&n...).

[Xiaocheng, 2017-02-27 06:14:20]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2017-02-27 06:14:20]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2701983002/#ps140001
(title: "Fix compiler error")

[commit-bot: I haz the power, 2017-02-27 06:14:35]

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-02-27 08:10:15]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488176060331720,
"parent_rev": "3e5bacc8f0fb54395c3843bcc2eff408c52c10c9", "commit_rev":
"46b362641fc483d7d5fd0714d96db6dd39de39f6"}

[commit-bot: I haz the power, 2017-02-27 08:11:15]

Description was changed from

==========
Implement complete lifecycle transition for IdleSpellCheckCallback

This patch implements complete lifecycle state transition for
IdleSpellCheckCallback. With this patch, idle spell check callback
will be invoked at correct timings. The only thing remaining is to
fill in the member functions which actually perform checking when
the callback is invoked.

BUG=517298
TEST=webkit_unit_tests --gtest_filter=IdleSpellCheckCallbackTest.*

Review-Url: https://codereview.chromium.org/2701983002
Cr-Commit-Position: refs/heads/master@{#453159}
Committed:
https://chromium.googlesource.com/chromium/src/+/234937696e399887e0acef2ad037...
==========

to

==========
Implement complete lifecycle transition for IdleSpellCheckCallback

This patch implements complete lifecycle state transition for
IdleSpellCheckCallback. With this patch, idle spell check callback
will be invoked at correct timings. The only thing remaining is to
fill in the member functions which actually perform checking when
the callback is invoked.

BUG=517298
TEST=webkit_unit_tests --gtest_filter=IdleSpellCheckCallbackTest.*

Review-Url: https://codereview.chromium.org/2701983002
Cr-Original-Commit-Position: refs/heads/master@{#453159}
Committed:
https://chromium.googlesource.com/chromium/src/+/234937696e399887e0acef2ad037...
Review-Url: https://codereview.chromium.org/2701983002
Cr-Commit-Position: refs/heads/master@{#453169}
Committed:
https://chromium.googlesource.com/chromium/src/+/46b362641fc483d7d5fd0714d96d...
==========

[commit-bot: I haz the power, 2017-02-27 08:11:17]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/46b362641fc483d7d5fd0714d96d...