New issue
Advanced search Search tips

Issue 710097 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: 2017-12-11
OS: ----
Pri: 2
Type: Task



Sign in to add a comment

Duplicated maintenance of spellcheck enabled state

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

Issue description

The spellcheck enabled state is maintained at two places:

1. SpellCheck::spellcheck_enabled_ (in components/spellcheck/renderer)
2. SpellCheckerClientImpl::spell_check_this_field_status_ (in Blink, web/ layer)

The logic keeping them in sync is complicated and error-prone.

Maybe we should remove (2).
 
Owner: xiaoche...@chromium.org
Status: Assigned (was: Available)
Assigning since there is a CL with a TODO($username) referencing this bug.
Components: -UI>Browser>Spellcheck UI>Browser>Language>Spellcheck
NextAction: 2017-12-11
I'll have another try in mid Dec.
The NextAction date has arrived: 2017-12-11
Labels: -Pri-3 Pri-2
Status: Started (was: Assigned)
Attack plan: https://goo.gl/grtURZ

All-in-one patch: crrev.com/c/821496

Splitting the above patch into smaller pieces to land...
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 14 2017

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

commit 737544e60cf68b92e9379f3506a7914ce24835bb
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Thu Dec 14 00:42:20 2017

Remove internals.setSpelCheckingEnabled()

As Blink-side maintenance of spellcheck enabled status will be removed,
toggling the Blink-side status no longer makes sense. Hence, any test
code toggling the Blink-side status should be removed.

This patch:

1. Removes internals.setSpellCheckingEnabled() since it is the tool for
   toggling the Blink-side status

2. Removes all the callers from layout tests, most of which are
   redundant. Test cases that really depend on the removed function are
   also removed.

3. An Android test is temporarily disabled since it depends on the
   removed function. It will be re-enabled when the cleanup is finished.

All-in-one patch: crrev.com/c/821496

Removal plan of Blink-side status: https://goo.gl/grtURZ

Bug:  710097 
Change-Id: I0ace095c3d082e9168a342164479a69b44813abc
Reviewed-on: https://chromium-review.googlesource.com/825250
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Ryan Landay <rlanday@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523956}
[modify] https://crrev.com/737544e60cf68b92e9379f3506a7914ce24835bb/content/public/android/javatests/src/org/chromium/content/browser/input/TextSuggestionMenuTest.java
[modify] https://crrev.com/737544e60cf68b92e9379f3506a7914ce24835bb/third_party/WebKit/LayoutTests/dom/text/normalize-crash-in-spell-checker.html
[delete] https://crrev.com/4b6bbb0348e4ec1ccbef8993a5d54bdc62a6f888/third_party/WebKit/LayoutTests/editing/spelling/spellcheck-disable-enable.html
[modify] https://crrev.com/737544e60cf68b92e9379f3506a7914ce24835bb/third_party/WebKit/LayoutTests/editing/spelling/spellcheck-mixed-editable-long-text-crash.html
[modify] https://crrev.com/737544e60cf68b92e9379f3506a7914ce24835bb/third_party/WebKit/LayoutTests/editing/spelling/spellcheck-paste-disabled.html
[modify] https://crrev.com/737544e60cf68b92e9379f3506a7914ce24835bb/third_party/WebKit/Source/core/testing/Internals.cpp
[modify] https://crrev.com/737544e60cf68b92e9379f3506a7914ce24835bb/third_party/WebKit/Source/core/testing/Internals.h
[modify] https://crrev.com/737544e60cf68b92e9379f3506a7914ce24835bb/third_party/WebKit/Source/core/testing/Internals.idl

Project Member

Comment 7 by bugdroid1@chromium.org, Dec 14 2017

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

commit 8d7d05b92fee53029a6592abcbc6889aba937b67
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Thu Dec 14 04:20:54 2017

Unify marker removal logic when spellchecking is globally disabled

When spellchecking is globally disabled (via context menu or
settings), all spelling markers should be removed. Currently, the
marker removal logic is duplicated at two places.

This patch unifies the logic and ensures that the removal is done
exactly once for each frame.

This is also a preparation patch for removing Blink-side maintenance
of the global spellcheck enabled status.

All-in-one patch: crrev.com/c/821496

Removal plan of Blink-side status: https://goo.gl/grtURZ

Bug:  710097 
Change-Id: I84d33cef5db2aa6ffeb1b66568dee037cba4a737
Reviewed-on: https://chromium-review.googlesource.com/826259
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524003}
[modify] https://crrev.com/8d7d05b92fee53029a6592abcbc6889aba937b67/components/spellcheck/renderer/spellcheck.cc
[modify] https://crrev.com/8d7d05b92fee53029a6592abcbc6889aba937b67/components/spellcheck/renderer/spellcheck_provider.cc
[modify] https://crrev.com/8d7d05b92fee53029a6592abcbc6889aba937b67/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 19 2017

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

commit 03563dd1635a028d286beaf8b4f3d637cc4a8830
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Tue Dec 19 22:27:19 2017

Make Blink check the Chromium-side spellcheck-enabled status

We currently maintain two spellcheck-enabled status, one in Blink
and one in Chromium. This patch makes the Chromium-side status the
only effective one, by changing all querying of the Blink-side status
to query the Chromium-side one instead. In this way, we reduce
maintenance cost, and also fix a bug in syncing the the two statuses
(regression test included).

After this patch, the Blink-side status becomes useless. A follow-up
patch (crrev.com/c/827558) will remove it and its relevant dead code.

Bug:  789273 ,  710097 
Change-Id: I5d4ee851934a3cb2139f0df9aa4df949071644a6
Reviewed-on: https://chromium-review.googlesource.com/821496
Reviewed-by: Rachel Blum <groby@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525157}
[modify] https://crrev.com/03563dd1635a028d286beaf8b4f3d637cc4a8830/chrome/browser/chrome_site_per_process_browsertest.cc
[modify] https://crrev.com/03563dd1635a028d286beaf8b4f3d637cc4a8830/components/spellcheck/renderer/spellcheck_provider.cc
[modify] https://crrev.com/03563dd1635a028d286beaf8b4f3d637cc4a8830/components/spellcheck/renderer/spellcheck_provider.h
[modify] https://crrev.com/03563dd1635a028d286beaf8b4f3d637cc4a8830/content/shell/test_runner/spell_check_client.cc
[modify] https://crrev.com/03563dd1635a028d286beaf8b4f3d637cc4a8830/content/shell/test_runner/spell_check_client.h
[modify] https://crrev.com/03563dd1635a028d286beaf8b4f3d637cc4a8830/third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallback.h
[modify] https://crrev.com/03563dd1635a028d286beaf8b4f3d637cc4a8830/third_party/WebKit/Source/core/editing/spellcheck/IdleSpellCheckCallbackTest.cpp
[modify] https://crrev.com/03563dd1635a028d286beaf8b4f3d637cc4a8830/third_party/WebKit/Source/core/editing/spellcheck/SpellCheckTestBase.cpp
[modify] https://crrev.com/03563dd1635a028d286beaf8b4f3d637cc4a8830/third_party/WebKit/Source/core/editing/spellcheck/SpellCheckTestBase.h
[modify] https://crrev.com/03563dd1635a028d286beaf8b4f3d637cc4a8830/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp
[modify] https://crrev.com/03563dd1635a028d286beaf8b4f3d637cc4a8830/third_party/WebKit/Source/core/exported/WebFrameTest.cpp
[modify] https://crrev.com/03563dd1635a028d286beaf8b4f3d637cc4a8830/third_party/WebKit/Source/core/loader/EmptyClients.cpp
[modify] https://crrev.com/03563dd1635a028d286beaf8b4f3d637cc4a8830/third_party/WebKit/Source/core/loader/EmptyClients.h
[modify] https://crrev.com/03563dd1635a028d286beaf8b4f3d637cc4a8830/third_party/WebKit/public/web/WebTextCheckClient.h

Project Member

Comment 9 by bugdroid1@chromium.org, Dec 20 2017

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

commit 70c5627cf932dac93da8f621976a1f052643c9f6
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Wed Dec 20 20:36:06 2017

Remove Blink-side spellcheck-enabled status and relevant dead code

As r525157 has already made the Blink-side spellcheck-enabled
status useless, this patch removes all dead code maintaining it.

Bug:  710097 
Change-Id: I50de79f3166eea2753d90059e2641dbb9bc133ad
Reviewed-on: https://chromium-review.googlesource.com/827558
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Rachel Blum <groby@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525438}
[modify] https://crrev.com/70c5627cf932dac93da8f621976a1f052643c9f6/android_webview/renderer/aw_content_renderer_client.cc
[modify] https://crrev.com/70c5627cf932dac93da8f621976a1f052643c9f6/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/70c5627cf932dac93da8f621976a1f052643c9f6/components/spellcheck/renderer/spellcheck.cc
[modify] https://crrev.com/70c5627cf932dac93da8f621976a1f052643c9f6/components/spellcheck/renderer/spellcheck_provider.cc
[modify] https://crrev.com/70c5627cf932dac93da8f621976a1f052643c9f6/components/spellcheck/renderer/spellcheck_provider.h
[modify] https://crrev.com/70c5627cf932dac93da8f621976a1f052643c9f6/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp
[modify] https://crrev.com/70c5627cf932dac93da8f621976a1f052643c9f6/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.h
[modify] https://crrev.com/70c5627cf932dac93da8f621976a1f052643c9f6/third_party/WebKit/Source/core/frame/WebLocalFrameImpl.cpp
[modify] https://crrev.com/70c5627cf932dac93da8f621976a1f052643c9f6/third_party/WebKit/Source/core/frame/WebLocalFrameImpl.h
[modify] https://crrev.com/70c5627cf932dac93da8f621976a1f052643c9f6/third_party/WebKit/Source/core/page/Page.h
[modify] https://crrev.com/70c5627cf932dac93da8f621976a1f052643c9f6/third_party/WebKit/Source/core/testing/Internals.cpp
[modify] https://crrev.com/70c5627cf932dac93da8f621976a1f052643c9f6/third_party/WebKit/public/web/WebLocalFrame.h

Status: Fixed (was: Started)
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 20 2017

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

commit e7d2ad69d8f0bdf4c5b2a2acb681556124b6f648
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Wed Dec 20 21:44:43 2017

Re-Enable TextSuggestionMenuTest.testDeleteWordMarkedWithSpellingMarker

A previous CL [1] disabled the test to ease a change in spellchecker [2].

Now that the change is done, the test can be re-enabled.

[1] crrev.com/c/823604

[2] crrev.com/c/821496

Tbr: changwan@chromium.org
Bug:  710097 
Change-Id: Iaf666bd0b4f45c6dabdb3fa34537e0f6156cdbed
Reviewed-on: https://chromium-review.googlesource.com/827595
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Reviewed-by: Ryan Landay <rlanday@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525471}
[modify] https://crrev.com/e7d2ad69d8f0bdf4c5b2a2acb681556124b6f648/content/public/android/javatests/src/org/chromium/content/browser/input/TextSuggestionMenuTest.java

Sign in to add a comment