Make WebSpellCheckClient aware of leak detection

Make WebSpellCheckClient aware of leak detection

WebSpellCheckClient stores a set of WebTextCheckingCompletion objects,
which may store references to already invoked SpellCheckRequests. This
patch clears these references before a leak detection so that they will
not become a leak source.

This CL is a preparation for https://codereview.chromium.org/2224103003

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

Committed: https://crrev.com/8fcbc602fb75514f4270b86377009cd787c24222
Cr-Commit-Position: refs/heads/master@{#410969}

[Xiaocheng, 2016-08-09 12:04:48]

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

[commit-bot: I haz the power, 2016-08-09 12:05:06]

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-09 12:14:32]

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

[commit-bot: I haz the power, 2016-08-09 12:14:34]

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

[Xiaocheng, 2016-08-09 13:15:15]

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

[commit-bot: I haz the power, 2016-08-09 13:15:39]

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-09 15:12:14]

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

[commit-bot: I haz the power, 2016-08-09 15:12:30]

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-09 15:30:30]

Description was changed from

==========
Make WebSpellCheckClient aware of leak detection

WebSpellCheckClient stores a set of WebTextCheckingCompletion objects,
which may store references to already invoked SpellCheckRequests. These
references should be cleared before a leak detection in order not to
become a leak source.

This CL is a preparation for https://codereview.chromium.org/2224103003

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

to

==========
Make WebSpellCheckClient aware of leak detection

WebSpellCheckClient stores a set of WebTextCheckingCompletion objects,
which may store references to already invoked SpellCheckRequests. This
patch clears these references before a leak detection so that they will
not become a leak source.

This CL is a preparation for https://codereview.chromium.org/2224103003

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

[Xiaocheng, 2016-08-09 15:30:31]

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

[Xiaocheng, 2016-08-09 15:32:18]

PTAL.

The background story is that I tried to make unified text checker enabled by
default in all tests in another patch, which failed some browser tests due to
memory leak.

[commit-bot: I haz the power, 2016-08-09 16:28:16]

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

[commit-bot: I haz the power, 2016-08-09 16:28:17]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[please use gerrit instead, 2016-08-09 16:47:57]

https://codereview.chromium.org/2226833003/diff/40001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

https://codereview.chromium.org/2226833003/diff/40001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_provider.cc:178: void
SpellCheckProvider::prepareForLeakDetection() {
Let's use a name that better describes what happens in this function. For
example, "cancelCheckingAllText". What do you think?

[Xiaocheng, 2016-08-10 01:14:47]

Thanks and PTAL at Patch 4.

https://codereview.chromium.org/2226833003/diff/40001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

https://codereview.chromium.org/2226833003/diff/40001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_provider.cc:178: void
SpellCheckProvider::prepareForLeakDetection() {
On 2016/08/09 at 16:47:57, rouslan wrote:
> Let's use a name that better describes what happens in this function. For
example, "cancelCheckingAllText". What do you think?

Renamed to "cancelAllPendingRequests", which sounds better to me.

[Xiaocheng, 2016-08-10 01:14:58]

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

[commit-bot: I haz the power, 2016-08-10 01:15:57]

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

[tkent, 2016-08-10 01:33:52]

third_party/WebKit lgtm

[tkent, 2016-08-10 01:34:10]

test_runner too :)

[yosin_UTC9, 2016-08-10 01:34:34]

lgtm

https://codereview.chromium.org/2226833003/diff/60001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

https://codereview.chromium.org/2226833003/diff/60001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_provider.cc:180: !iter.IsAtEnd();
iter.Advance())
nit: We should have brace "{}" for loop body since for-statement spread into two
lines.

https://codereview.chromium.org/2226833003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2226833003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5269: void
cancelAllPendingRequests() override
Off-topic: we might want to re-structure spell checker related tests in
"WebFrameTest.cpp".
None of them don't verify functions in WebFrame. They verify
WebView::spellingMarkers(),
DocumentMarkerController.

Since WebFrameTest.cpp contains 8834 lines, it is worth to reduce source code
size.

[please use gerrit instead, 2016-08-10 01:35:21]

chrome/renderer/spellchecker/spellcheck_provider.* lgtm

[Xiaocheng, 2016-08-10 01:45:32]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2016-08-10 01:45:33]

Thanks a lot for the review!

I'm committing Patch 5 directly as it only fixes a nit.

https://codereview.chromium.org/2226833003/diff/60001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

https://codereview.chromium.org/2226833003/diff/60001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_provider.cc:180: !iter.IsAtEnd();
iter.Advance())
On 2016/08/10 at 01:34:34, Yosi_UTC9 wrote:
> nit: We should have brace "{}" for loop body since for-statement spread into
two lines.

Done.

https://codereview.chromium.org/2226833003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2226833003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5269: void
cancelAllPendingRequests() override
On 2016/08/10 at 01:34:34, Yosi_UTC9 wrote:
> Off-topic: we might want to re-structure spell checker related tests in
"WebFrameTest.cpp".
> None of them don't verify functions in WebFrame. They verify
WebView::spellingMarkers(),
> DocumentMarkerController.
> 
> Since WebFrameTest.cpp contains 8834 lines, it is worth to reduce source code
size.

Sounds reasonable.

[Xiaocheng, 2016-08-10 01:45:33]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
rouslan@chromium.org, yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2226833003/#ps80001
(title: "Minor style fix")

[commit-bot: I haz the power, 2016-08-10 01:46:49]

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-10 03:00:34]

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

[commit-bot: I haz the power, 2016-08-10 03:00:37]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Xiaocheng, 2016-08-10 04:33:54]

The CQ bit was checked by xiaochengh@chromium.org

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

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

[Xiaocheng, 2016-08-10 04:39:59]

Description was changed from

==========
Make WebSpellCheckClient aware of leak detection

WebSpellCheckClient stores a set of WebTextCheckingCompletion objects,
which may store references to already invoked SpellCheckRequests. This
patch clears these references before a leak detection so that they will
not become a leak source.

This CL is a preparation for https://codereview.chromium.org/2224103003

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

to

==========
Make WebSpellCheckClient aware of leak detection

WebSpellCheckClient stores a set of WebTextCheckingCompletion objects,
which may store references to already invoked SpellCheckRequests. This
patch clears these references before a leak detection so that they will
not become a leak source.

This CL is a preparation for https://codereview.chromium.org/2224103003

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

[commit-bot: I haz the power, 2016-08-10 05:45:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-10 05:47:11]

Description was changed from

==========
Make WebSpellCheckClient aware of leak detection

WebSpellCheckClient stores a set of WebTextCheckingCompletion objects,
which may store references to already invoked SpellCheckRequests. This
patch clears these references before a leak detection so that they will
not become a leak source.

This CL is a preparation for https://codereview.chromium.org/2224103003

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

to

==========
Make WebSpellCheckClient aware of leak detection

WebSpellCheckClient stores a set of WebTextCheckingCompletion objects,
which may store references to already invoked SpellCheckRequests. This
patch clears these references before a leak detection so that they will
not become a leak source.

This CL is a preparation for https://codereview.chromium.org/2224103003

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

Committed: https://crrev.com/8fcbc602fb75514f4270b86377009cd787c24222
Cr-Commit-Position: refs/heads/master@{#410969}
==========

[commit-bot: I haz the power, 2016-08-10 05:47:13]

Patchset 5 (id:??) landed as
https://crrev.com/8fcbc602fb75514f4270b86377009cd787c24222
Cr-Commit-Position: refs/heads/master@{#410969}