New issue
Advanced search Search tips

Issue 716642 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 27
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug
Launch-Accessibility: NA
Launch-Legal: NA
Launch-Privacy: NA
Launch-Security: NA
Launch-Test: NA
Launch-UI: NA

Blocked on:
issue 671922
issue 706996

Blocking:
issue 824030



Sign in to add a comment

Launch tracking: Idle Time Spell Checking, Cold Mode

Project Member Reported by xiaoche...@chromium.org, Apr 28 2017

Issue description

Change description:
Run full document spell checking at renderer's idle time, aka cold mode of idle time spell checking, so that bottom-line correctness of spell checking is guaranteed.

Changes to API surface:
None.

Links:
Design doc: https://docs.google.com/document/d/1vo71cnlGOqOuXyy1zjzXSlUIjG7GsWgRjqmkrq1g5hk/edit?usp=sharing


Support in other browsers:
N/A. This is Blink-internal implementation change.
 
Blockedon: 706996

Comment 2 by owe...@chromium.org, Sep 12 2017

Labels: migrated-launch-owp Type-Task
This issue has been automatically relabelled type=task because type=launch-owp issues are now officially deprecated. The deprecation is because they were creating confusion about how to get launch approvals, which should be instead done via type=launch issues.

We recommend this issue be used for implementation tracking (for public visibility), but if you already have an issue for that, you may mark this as duplicate.

For more details see here: https://docs.google.com/document/d/1JA6RohjtZQc26bTrGoIE_bSXGXUDQz8vc6G0n_sZJ2o/edit

For any questions, please contact owencm, sshruthi, larforge
Blocking: 824030
Labels: -M-61 M-68
I don't think I have enough time to finish the implementation by M67.

Let's target M68.
Labels: -Type-Task Type-Bug
As discussed with yosin@, this is kind of a bug fix to existing feature.

No need to go through launch process.
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/72ac5c08820272be7452ebd9181f9308bf62a223

commit 72ac5c08820272be7452ebd9181f9308bf62a223
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Apr 17 19:45:36 2018

[Spellcheck] Make cold mode idle time spellchecker less aggressive

This patch revises the cold mode idle time spellchecker that, it only
checks the editable element that is currently focused, instead of
checking the full document, so that the checker is less aggressive and
less resource consuming.

Bug:  824030 ,  716642 
Change-Id: I1361b53b4dd3513252ef50d0356497c0a4054ecd
Reviewed-on: https://chromium-review.googlesource.com/1014472
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551442}
[add] https://crrev.com/72ac5c08820272be7452ebd9181f9308bf62a223/third_party/WebKit/LayoutTests/editing/spelling/cold_mode_multiline.html
[delete] https://crrev.com/3613946f7535f3a82c536e78ca80e95f04541b14/third_party/WebKit/LayoutTests/editing/spelling/cold_mode_static_page.html
[modify] https://crrev.com/72ac5c08820272be7452ebd9181f9308bf62a223/third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.html
[modify] https://crrev.com/72ac5c08820272be7452ebd9181f9308bf62a223/third_party/blink/renderer/core/editing/spellcheck/cold_mode_spell_check_requester.cc
[modify] https://crrev.com/72ac5c08820272be7452ebd9181f9308bf62a223/third_party/blink/renderer/core/editing/spellcheck/cold_mode_spell_check_requester.h
[modify] https://crrev.com/72ac5c08820272be7452ebd9181f9308bf62a223/third_party/blink/renderer/core/editing/spellcheck/idle_spell_check_callback.cc
[modify] https://crrev.com/72ac5c08820272be7452ebd9181f9308bf62a223/third_party/blink/renderer/core/editing/spellcheck/spell_check_requester.cc
[modify] https://crrev.com/72ac5c08820272be7452ebd9181f9308bf62a223/third_party/blink/renderer/core/editing/spellcheck/spell_check_requester.h

Project Member

Comment 7 by bugdroid1@chromium.org, Apr 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/96b9e995718ccad040ad709f16c9b70cf58434d8

commit 96b9e995718ccad040ad709f16c9b70cf58434d8
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Wed Apr 18 02:56:21 2018

[Spellcheck] Enable cold mode idle time spellchecker in M68

As previously planned, we will enable cold mode checker as bug fix to
the current spellchecker, and will monitor its performance.

Bug:  824030 ,  716642 
Change-Id: Ie37a08fb6d5562d2f2ff2c2ac84e8033b6ffb776
Reviewed-on: https://chromium-review.googlesource.com/1015790
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551558}
[modify] https://crrev.com/96b9e995718ccad040ad709f16c9b70cf58434d8/third_party/blink/renderer/platform/runtime_enabled_features.json5

Project Member

Comment 8 by bugdroid1@chromium.org, Apr 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/322e3b5472bcdcfcef10f58d41506136132eb6fe

commit 322e3b5472bcdcfcef10f58d41506136132eb6fe
Author: Rune Lillesveen <futhark@chromium.org>
Date: Wed Apr 18 08:01:05 2018

Revert "[Spellcheck] Enable cold mode idle time spellchecker in M68"

This reverts commit 96b9e995718ccad040ad709f16c9b70cf58434d8.

Reason for revert: introduces memory leak on youtube.com test

First failling here:

https://ci.chromium.org/buildbot/chromium.linux/Leak%20Detection%20Linux/4115

Original change's description:
> [Spellcheck] Enable cold mode idle time spellchecker in M68
> 
> As previously planned, we will enable cold mode checker as bug fix to
> the current spellchecker, and will monitor its performance.
> 
> Bug:  824030 ,  716642 
> Change-Id: Ie37a08fb6d5562d2f2ff2c2ac84e8033b6ffb776
> Reviewed-on: https://chromium-review.googlesource.com/1015790
> Reviewed-by: Kent Tamura <tkent@chromium.org>
> Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#551558}

TBR=yosin@chromium.org,tkent@chromium.org,xiaochengh@chromium.org

Change-Id: I03087aad24d41208a56aa1904eb55c719e771578
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  824030 ,  716642 
Reviewed-on: https://chromium-review.googlesource.com/1015363
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551610}
[modify] https://crrev.com/322e3b5472bcdcfcef10f58d41506136132eb6fe/third_party/blink/renderer/platform/runtime_enabled_features.json5

Project Member

Comment 9 by bugdroid1@chromium.org, Apr 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9bff4e8316fccd95ca82ed16f8830e0982ca7170

commit 9bff4e8316fccd95ca82ed16f8830e0982ca7170
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Fri Apr 20 02:47:26 2018

[Spellcheck] Stop requesting cold mode checker on document startup

Originally, cold mode checker was designed for full document checking,
which must be requested on document startup.

With crrev.com/c/1014472, cold mode checker only checks the currently
focused element, so there is no need to request it on startup. Hence,
this patch removes it.

Note: this patch also stops the memory leak seen in  crbug.com/834199 .
The root cause of the leak is still unclear.

Bug:  716642 
Change-Id: Ide93fa3ce8050b77b08079145c8cd81c2571974f
Reviewed-on: https://chromium-review.googlesource.com/1020156
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552242}
[modify] https://crrev.com/9bff4e8316fccd95ca82ed16f8830e0982ca7170/third_party/blink/renderer/core/editing/spellcheck/idle_spell_check_callback.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8712023f7d10238966ed56d02d27f10e02c8c394

commit 8712023f7d10238966ed56d02d27f10e02c8c394
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Fri Apr 20 05:55:49 2018

Reland "[Spellcheck] Enable cold mode idle time spellchecker in M68"

This is a reland of 96b9e995718ccad040ad709f16c9b70cf58434d8

Original change's description:
> [Spellcheck] Enable cold mode idle time spellchecker in M68
>
> As previously planned, we will enable cold mode checker as bug fix to
> the current spellchecker, and will monitor its performance.
>
> Bug:  824030 ,  716642 
> Change-Id: Ie37a08fb6d5562d2f2ff2c2ac84e8033b6ffb776
> Reviewed-on: https://chromium-review.googlesource.com/1015790
> Reviewed-by: Kent Tamura <tkent@chromium.org>
> Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#551558}

Tbr: yosin@chromium.org, tkent@chromium.org
Bug:  824030 ,  716642 
Change-Id: I1d0a23c321432af6e5b74bd19eb4d7ab17db5220
Reviewed-on: https://chromium-review.googlesource.com/1020740
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552270}
[modify] https://crrev.com/8712023f7d10238966ed56d02d27f10e02c8c394/third_party/blink/renderer/platform/runtime_enabled_features.json5

Project Member

Comment 11 by bugdroid1@chromium.org, May 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d1c99618c03544f0c8734c7340471b30ac67b1f1

commit d1c99618c03544f0c8734c7340471b30ac67b1f1
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Fri May 25 18:22:56 2018

Remove cold mode idle time spellchecker flag

As cold mode idle time spellchecker is shipped into M68
and no regression is obversed, this patch removes the flag
from M69 as cleanup.

Bug:  716642 
Change-Id: I8f850c52469590ee9d5e542c14f4dae842a6163a
Reviewed-on: https://chromium-review.googlesource.com/1072953
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561951}
[modify] https://crrev.com/d1c99618c03544f0c8734c7340471b30ac67b1f1/third_party/blink/renderer/core/editing/spellcheck/idle_spell_check_callback.cc
[modify] https://crrev.com/d1c99618c03544f0c8734c7340471b30ac67b1f1/third_party/blink/renderer/core/editing/spellcheck/idle_spell_check_callback_test.cc
[modify] https://crrev.com/d1c99618c03544f0c8734c7340471b30ac67b1f1/third_party/blink/renderer/platform/runtime_enabled_features.json5

Status: Fixed (was: Assigned)
Project Member

Comment 13 by bugdroid1@chromium.org, Jul 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9afcc1e595f00ea24c962ef767e35aa912df0242

commit 9afcc1e595f00ea24c962ef767e35aa912df0242
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Jul 03 02:15:51 2018

Revert "Remove cold mode idle time spellchecker flag"

This reverts commit d1c99618c03544f0c8734c7340471b30ac67b1f1.

Reason for revert: Observed many crashes in M68 & M69. Not ready
for shipping.

Original change's description:
> Remove cold mode idle time spellchecker flag
>
> As cold mode idle time spellchecker is shipped into M68
> and no regression is obversed, this patch removes the flag
> from M69 as cleanup.
>
> Bug:  716642 
> Change-Id: I8f850c52469590ee9d5e542c14f4dae842a6163a
> Reviewed-on: https://chromium-review.googlesource.com/1072953
> Reviewed-by: Kent Tamura <tkent@chromium.org>
> Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
> Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#561951}

TBR=yosin@chromium.org,tkent@chromium.org,xiaochengh@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  716642 , 856376,  859056 , 859443
Change-Id: I91426509a315a07bf4f548ebafe825c2ef856999
Reviewed-on: https://chromium-review.googlesource.com/1123120
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572086}
[modify] https://crrev.com/9afcc1e595f00ea24c962ef767e35aa912df0242/third_party/blink/renderer/core/editing/spellcheck/idle_spell_check_callback.cc
[modify] https://crrev.com/9afcc1e595f00ea24c962ef767e35aa912df0242/third_party/blink/renderer/core/editing/spellcheck/idle_spell_check_callback_test.cc
[modify] https://crrev.com/9afcc1e595f00ea24c962ef767e35aa912df0242/third_party/blink/renderer/platform/runtime_enabled_features.json5

Status: Assigned (was: Fixed)
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 10

Labels: merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/56f5ddbda1820847ab5db4bf68512a0e29281fa0

commit 56f5ddbda1820847ab5db4bf68512a0e29281fa0
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Jul 10 19:39:31 2018

[M68] Disable cold mode spellchecker

Cold mode spellchecker was introduced in M68 as a bug fix to some
spellcheck completeness issues. For risk control, it is implemented
behind a flag.

Now that we found many related crashes in M68 and M69, it turns out
that cold mode spellchecker is not ready to be pushed to the stable
channel. Hence, this patch disables it by default on M68.

It will remain enabled by default in M69 for more observation.

Bug:  716642 , 856376,  859056 , 859443
Change-Id: I8984de679d0c7c0de61123f50dabc535b2b3c3fb
Reviewed-on: https://chromium-review.googlesource.com/1124021
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#638}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/56f5ddbda1820847ab5db4bf68512a0e29281fa0/third_party/blink/renderer/platform/runtime_enabled_features.json5

Project Member

Comment 16 by bugdroid1@chromium.org, Nov 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/070773549900baa6db6fcbd18aa710b81a461d34

commit 070773549900baa6db6fcbd18aa710b81a461d34
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Nov 27 21:29:45 2018

Remove cold mode idle time spellchecker flag

As cold mode idle time spellchecker is shipped into M69 Stable
and no regression is obversed, this patch removes the flag as
cleanup.

Bug:  716642 
Change-Id: I0b75e7592be063cb53ee9eb68dbf3f0c1af48dd4
Reviewed-on: https://chromium-review.googlesource.com/c/1351869
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611335}
[modify] https://crrev.com/070773549900baa6db6fcbd18aa710b81a461d34/third_party/blink/renderer/core/editing/spellcheck/idle_spell_check_controller.cc
[modify] https://crrev.com/070773549900baa6db6fcbd18aa710b81a461d34/third_party/blink/renderer/core/editing/spellcheck/idle_spell_check_controller_test.cc
[modify] https://crrev.com/070773549900baa6db6fcbd18aa710b81a461d34/third_party/blink/renderer/platform/runtime_enabled_features.json5

Status: Fixed (was: Assigned)

Sign in to add a comment