Spellcheck : Fix caching in cases where text is deleted.

Spellcheck : Fix caching in cases where text is deleted.

This patch fixes spellcheck caching in the following case:
1. paragraph is edited by removing text from the back and
2. paragraph does not contain misspelled words.

Currently this case results in a cache miss and consequently
spellcheck requests being sent to the spellchecking service
while completely unnecessary. This fix applies to all
platforms.

This patch also adds a few unit tests for the relevant
caching specific behavior.

BUG=664247, 629609
Committed: https://crrev.com/cb1e0169211135db1dd2e0220434bcec2efd7f16
Cr-Commit-Position: refs/heads/master@{#434710}

[timvolodine, 2016-11-11 15:15:41]

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

[commit-bot: I haz the power, 2016-11-11 15:16:02]

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-11-11 16:01:51]

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

[commit-bot: I haz the power, 2016-11-11 16:01:52]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2016-11-11 16:20:10]

timvolodine@chromium.org changed reviewers:
+ groby@chromium.org

[groby-ooo-7-16, 2016-11-15 19:36:57]

Could you possibly add some unit tests that cover the cases your description
mentions?

Thank you!

https://codereview.chromium.org/2494123002/diff/1/components/spellcheck/rende...
File components/spellcheck/renderer/spellcheck_provider.cc (right):

https://codereview.chromium.org/2494123002/diff/1/components/spellcheck/rende...
components/spellcheck/renderer/spellcheck_provider.cc:332:
blink::WebVector<blink::WebTextCheckingResult> results(result_size);
So, I might be misreading this, but it looks to me like a shorter approach is

blink::WebVector<...> results (last_results_.data(), result_size);

While you're here :)

https://codereview.chromium.org/2494123002/diff/1/components/spellcheck/rende...
components/spellcheck/renderer/spellcheck_provider.cc:340: return true;
I don't think this works as intended. SatisfyRequestFromCache is only supposed
to invoke the completion/return true when result_size != 0

Is that causing the issues that you're fixing? (Essentially, I don't understand
how this change fixes the problems you mention)

[timvolodine, 2016-11-17 19:35:31]

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

[commit-bot: I haz the power, 2016-11-17 19:36:26]

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-11-17 19:43:50]

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

[commit-bot: I haz the power, 2016-11-17 19:43:51]

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

[timvolodine, 2016-11-17 20:15:22]

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

[commit-bot: I haz the power, 2016-11-17 20:16:42]

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

[timvolodine, 2016-11-17 20:24:15]

Description was changed from

==========
Spellcheck : Fix caching in cases where text is deleted.

This patch fixes spellcheck caching in the following case:
1. paragraph is edited by removing text from the back and
2. paragraph does not contain misspelled words.

Currently this case results in a cache miss and consequently
spellcheck requests being sent to the spellchecking service
while completely unnecessary. This fix applies to all
platforms.

BUG=664247
==========

to

==========
Spellcheck : Fix caching in cases where text is deleted.

This patch fixes spellcheck caching in the following case:
1. paragraph is edited by removing text from the back and
2. paragraph does not contain misspelled words.

Currently this case results in a cache miss and consequently
spellcheck requests being sent to the spellchecking service
while completely unnecessary. This fix applies to all
platforms.

This patch also adds a few unit tests for the relevant
caching specific behavior.

BUG=664247
==========

[timvolodine, 2016-11-17 20:53:15]

Thanks for the comments! Added unit tests, replies below.. PTAL

https://codereview.chromium.org/2494123002/diff/1/components/spellcheck/rende...
File components/spellcheck/renderer/spellcheck_provider.cc (right):

https://codereview.chromium.org/2494123002/diff/1/components/spellcheck/rende...
components/spellcheck/renderer/spellcheck_provider.cc:332:
blink::WebVector<blink::WebTextCheckingResult> results(result_size);
On 2016/11/15 19:36:57, groby wrote:
> So, I might be misreading this, but it looks to me like a shorter approach is
> 
> blink::WebVector<...> results (last_results_.data(), result_size);
> 
> While you're here :)

Yes much more compact indeed.
Done.

https://codereview.chromium.org/2494123002/diff/1/components/spellcheck/rende...
components/spellcheck/renderer/spellcheck_provider.cc:340: return true;
On 2016/11/15 19:36:57, groby wrote:
> I don't think this works as intended. SatisfyRequestFromCache is only supposed
> to invoke the completion/return true when result_size != 0
> 
> Is that causing the issues that you're fixing? (Essentially, I don't
understand
> how this change fixes the problems you mention)

So currently a substring without misspellings will result in cache miss, this is
what this patch is fixing.
I've added a SpellCheckProviderCacheTest.SubstringWithoutMisspellings to
illustrate.

[commit-bot: I haz the power, 2016-11-18 02:19:27]

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

[commit-bot: I haz the power, 2016-11-18 02:19:27]

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_...)

[groby-ooo-7-16, 2016-11-23 19:49:06]

LGTM w/ naming nit

https://codereview.chromium.org/2494123002/diff/40001/components/spellcheck/r...
File components/spellcheck/renderer/spellcheck_provider_cache_unittest.cc
(right):

https://codereview.chromium.org/2494123002/diff/40001/components/spellcheck/r...
components/spellcheck/renderer/spellcheck_provider_cache_unittest.cc:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
If you could move this to spellcheck_provider_unittest.cc ?

That way, the next person writing non-platform tests has a home already :)

[timvolodine, 2016-11-24 18:07:39]

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

[commit-bot: I haz the power, 2016-11-24 18:08: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, 2016-11-24 19:24:33]

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

[commit-bot: I haz the power, 2016-11-24 19:24:34]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2016-11-28 16:45:46]

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

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

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-11-28 18:09:44]

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

[commit-bot: I haz the power, 2016-11-28 18:09:45]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2016-11-28 18:48:17]

Thanks for the review!

https://codereview.chromium.org/2494123002/diff/40001/components/spellcheck/r...
File components/spellcheck/renderer/spellcheck_provider_cache_unittest.cc
(right):

https://codereview.chromium.org/2494123002/diff/40001/components/spellcheck/r...
components/spellcheck/renderer/spellcheck_provider_cache_unittest.cc:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/11/23 19:49:06, groby wrote:
> If you could move this to spellcheck_provider_unittest.cc ?
> 
> That way, the next person writing non-platform tests has a home already :)

Done.

[timvolodine, 2016-11-28 18:48:25]

The CQ bit was checked by timvolodine@chromium.org

[timvolodine, 2016-11-28 18:48:26]

The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org
Link to the patchset: https://codereview.chromium.org/2494123002/#ps80001
(title: "rebase + fix year")

[commit-bot: I haz the power, 2016-11-28 18:48:58]

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

[timvolodine, 2016-11-28 18:49:42]

The CQ bit was unchecked by timvolodine@chromium.org

[timvolodine, 2016-11-28 19:05:05]

Description was changed from

==========
Spellcheck : Fix caching in cases where text is deleted.

This patch fixes spellcheck caching in the following case:
1. paragraph is edited by removing text from the back and
2. paragraph does not contain misspelled words.

Currently this case results in a cache miss and consequently
spellcheck requests being sent to the spellchecking service
while completely unnecessary. This fix applies to all
platforms.

This patch also adds a few unit tests for the relevant
caching specific behavior.

BUG=664247
==========

to

==========
Spellcheck : Fix caching in cases where text is deleted.

This patch fixes spellcheck caching in the following case:
1. paragraph is edited by removing text from the back and
2. paragraph does not contain misspelled words.

Currently this case results in a cache miss and consequently
spellcheck requests being sent to the spellchecking service
while completely unnecessary. This fix applies to all
platforms.

This patch also adds a few unit tests for the relevant
caching specific behavior.

BUG=664247,629609
==========

[timvolodine, 2016-11-28 19:06:32]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2016-11-28 19:07:04]

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-11-28 20:28:36]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480359992234710,
"parent_rev": "c8351feca9eb48134db39e10137d313b140d975e", "commit_rev":
"335a78eae1985df8d6a2ff1364e5191e01e35b52"}

[commit-bot: I haz the power, 2016-11-28 20:29:02]

Description was changed from

==========
Spellcheck : Fix caching in cases where text is deleted.

This patch fixes spellcheck caching in the following case:
1. paragraph is edited by removing text from the back and
2. paragraph does not contain misspelled words.

Currently this case results in a cache miss and consequently
spellcheck requests being sent to the spellchecking service
while completely unnecessary. This fix applies to all
platforms.

This patch also adds a few unit tests for the relevant
caching specific behavior.

BUG=664247,629609
==========

to

==========
Spellcheck : Fix caching in cases where text is deleted.

This patch fixes spellcheck caching in the following case:
1. paragraph is edited by removing text from the back and
2. paragraph does not contain misspelled words.

Currently this case results in a cache miss and consequently
spellcheck requests being sent to the spellchecking service
while completely unnecessary. This fix applies to all
platforms.

This patch also adds a few unit tests for the relevant
caching specific behavior.

BUG=664247,629609
==========

[commit-bot: I haz the power, 2016-11-28 20:29:04]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-28 20:32:35]

Description was changed from

==========
Spellcheck : Fix caching in cases where text is deleted.

This patch fixes spellcheck caching in the following case:
1. paragraph is edited by removing text from the back and
2. paragraph does not contain misspelled words.

Currently this case results in a cache miss and consequently
spellcheck requests being sent to the spellchecking service
while completely unnecessary. This fix applies to all
platforms.

This patch also adds a few unit tests for the relevant
caching specific behavior.

BUG=664247,629609
==========

to

==========
Spellcheck : Fix caching in cases where text is deleted.

This patch fixes spellcheck caching in the following case:
1. paragraph is edited by removing text from the back and
2. paragraph does not contain misspelled words.

Currently this case results in a cache miss and consequently
spellcheck requests being sent to the spellchecking service
while completely unnecessary. This fix applies to all
platforms.

This patch also adds a few unit tests for the relevant
caching specific behavior.

BUG=664247,629609

Committed: https://crrev.com/cb1e0169211135db1dd2e0220434bcec2efd7f16
Cr-Commit-Position: refs/heads/master@{#434710}
==========

[commit-bot: I haz the power, 2016-11-28 20:32:37]

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