Code cleanup related to unified text checker

Code cleanup related to unified text checker

Unified text checker is always enabled in chrome, and never assumed to
be disabled in any test case. Hence, this patch removes:
- All code conditioning on unified text checker
- Dead code assuming unified text checker is disabled
- The switch |unifiedTextCheckerEnabled| from Settings

This patch also simplifies code related to conditioning of
isContinuousSpellCheckingEnabled() in SpellChecker.cpp, given that
unified text checker is always enabled.

The following layout tests are marked as NeedsRebaseline because their
pixel results are affected by the appearance of spelling markers after
enabling unified text checker. They do not explicitly test any
functionality of spell checker, though:
- fast/dom/blur-contenteditable.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
- fast/dom/focus-contenteditable.html
- virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- fast/forms/text/input-double-click-selection-gap-bug.html
- compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=633509
TEST=n/a; no behavior change, but several test files are changed

Committed: https://crrev.com/accb28ab54cbdf525c3e319c3857a492bb27f04c
Cr-Commit-Position: refs/heads/master@{#412164}

[Xiaocheng, 2016-08-10 05:07:21]

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

[commit-bot: I haz the power, 2016-08-10 05:07:49]

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-10 05:12:56]

Description was changed from

==========
Code cleanup related to unified text checker

Unified text checker is enabled in all cases, and there is no client
trying to disable it. Hence, this patch removes:
- All code conditioning on unified text checker
- Dead code assuming unified text checker is disabled
- The switch |unifiedTextCheckerEnabled| from Settings

BUG=635509
TEST=n/a; no behavior change, but several test files are changed
==========

to

==========
Code cleanup related to unified text checker

Unified text checker is enabled in all cases, and there is no client
trying to disable it. Hence, this patch removes:
- All code conditioning on unified text checker
- Dead code assuming unified text checker is disabled
- The switch |unifiedTextCheckerEnabled| from Settings

This patch also simplifies code related to conditioning of
isContinuousSpellCheckingEnabled() in SpellChecker.cpp.

BUG=635509
TEST=n/a; no behavior change, but several test files are changed
==========

[Xiaocheng, 2016-08-10 06:45:29]

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

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

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-10 08:38:14]

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

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

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2016-08-10 09:13:59]

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

[commit-bot: I haz the power, 2016-08-10 09:14:07]

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-10 11:32:01]

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

[commit-bot: I haz the power, 2016-08-10 11:32:02]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2016-08-12 02:31:24]

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

[commit-bot: I haz the power, 2016-08-12 02:32:08]

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-12 02:33:24]

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

[commit-bot: I haz the power, 2016-08-12 02:33:53]

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-12 02:36:22]

Description was changed from

==========
Code cleanup related to unified text checker

Unified text checker is enabled in all cases, and there is no client
trying to disable it. Hence, this patch removes:
- All code conditioning on unified text checker
- Dead code assuming unified text checker is disabled
- The switch |unifiedTextCheckerEnabled| from Settings

This patch also simplifies code related to conditioning of
isContinuousSpellCheckingEnabled() in SpellChecker.cpp.

BUG=635509
TEST=n/a; no behavior change, but several test files are changed
==========

to

==========
Code cleanup related to unified text checker

Unified text checker is enabled in all cases, and there is no client
trying to disable it. Hence, this patch removes:
- All code conditioning on unified text checker
- Dead code assuming unified text checker is disabled
- The switch |unifiedTextCheckerEnabled| from Settings

This patch also simplifies code related to conditioning of
isContinuousSpellCheckingEnabled() in SpellChecker.cpp.

The following layout tests are marked as NeedsRebaseline because their
pixel results are affected by the appearance of spelling markers after
enabling unified text checker. They do not explicitly test any functionality
of spell checker, though:
- fast/dom/blur-contenteditable.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
- fast/dom/focus-contenteditable.html
-
virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- fast/forms/text/input-double-click-selection-gap-bug.html
-
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=635509
TEST=n/a; no behavior change, but several test files are changed
==========

[Xiaocheng, 2016-08-12 02:43:20]

Description was changed from

==========
Code cleanup related to unified text checker

Unified text checker is enabled in all cases, and there is no client
trying to disable it. Hence, this patch removes:
- All code conditioning on unified text checker
- Dead code assuming unified text checker is disabled
- The switch |unifiedTextCheckerEnabled| from Settings

This patch also simplifies code related to conditioning of
isContinuousSpellCheckingEnabled() in SpellChecker.cpp.

The following layout tests are marked as NeedsRebaseline because their
pixel results are affected by the appearance of spelling markers after
enabling unified text checker. They do not explicitly test any functionality
of spell checker, though:
- fast/dom/blur-contenteditable.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
- fast/dom/focus-contenteditable.html
-
virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- fast/forms/text/input-double-click-selection-gap-bug.html
-
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=635509
TEST=n/a; no behavior change, but several test files are changed
==========

to

==========
Code cleanup related to unified text checker

Unified text checker is enabled in all cases, and there is no client
trying to disable it. Hence, this patch removes:
- All code conditioning on unified text checker
- Dead code assuming unified text checker is disabled
- The switch |unifiedTextCheckerEnabled| from Settings

This patch also simplifies code related to conditioning of
isContinuousSpellCheckingEnabled() in SpellChecker.cpp, given that
unified text checker is always enabled.

The following layout tests are marked as NeedsRebaseline because their
pixel results are affected by the appearance of spelling markers after
enabling unified text checker. They do not explicitly test any
functionality of spell checker, though:
- fast/dom/blur-contenteditable.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
- fast/dom/focus-contenteditable.html
-
virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- fast/forms/text/input-double-click-selection-gap-bug.html
-
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=635509
TEST=n/a; no behavior change, but several test files are changed
==========

[commit-bot: I haz the power, 2016-08-12 03:46:02]

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

[commit-bot: I haz the power, 2016-08-12 03:46:03]

Dry run: This issue passed the CQ dry run.

[Xiaocheng, 2016-08-12 04:37:55]

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

[commit-bot: I haz the power, 2016-08-12 04:38:08]

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-12 05:46:14]

Description was changed from

==========
Code cleanup related to unified text checker

Unified text checker is enabled in all cases, and there is no client
trying to disable it. Hence, this patch removes:
- All code conditioning on unified text checker
- Dead code assuming unified text checker is disabled
- The switch |unifiedTextCheckerEnabled| from Settings

This patch also simplifies code related to conditioning of
isContinuousSpellCheckingEnabled() in SpellChecker.cpp, given that
unified text checker is always enabled.

The following layout tests are marked as NeedsRebaseline because their
pixel results are affected by the appearance of spelling markers after
enabling unified text checker. They do not explicitly test any
functionality of spell checker, though:
- fast/dom/blur-contenteditable.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
- fast/dom/focus-contenteditable.html
-
virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- fast/forms/text/input-double-click-selection-gap-bug.html
-
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=635509
TEST=n/a; no behavior change, but several test files are changed
==========

to

==========
Code cleanup related to unified text checker

Unified text checker is always enabled in chrome, and never assumed to
be disabled in any test case. Hence, this patch removes:
- All code conditioning on unified text checker
- Dead code assuming unified text checker is disabled
- The switch |unifiedTextCheckerEnabled| from Settings

This patch also simplifies code related to conditioning of
isContinuousSpellCheckingEnabled() in SpellChecker.cpp, given that
unified text checker is always enabled.

The following layout tests are marked as NeedsRebaseline because their
pixel results are affected by the appearance of spelling markers after
enabling unified text checker. They do not explicitly test any
functionality of spell checker, though:
- fast/dom/blur-contenteditable.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
- fast/dom/focus-contenteditable.html
-
virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- fast/forms/text/input-double-click-selection-gap-bug.html
-
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=635509
TEST=n/a; no behavior change, but several test files are changed
==========

[Xiaocheng, 2016-08-12 05:46:14]

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

[Xiaocheng, 2016-08-12 05:46:43]

PTAL.

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

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

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

Dry run: This issue passed the CQ dry run.

[tkent, 2016-08-12 06:42:00]

third_party/WebKit lgtm

[ncarter (slow), 2016-08-12 16:48:01]

content lgtm

[Xiaocheng, 2016-08-12 22:32:00]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2016-08-12 22:32:00]

Thanks.

[commit-bot: I haz the power, 2016-08-12 22:33:02]

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-12 22:47:13]

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

[commit-bot: I haz the power, 2016-08-12 22:47:14]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Xiaocheng, 2016-08-12 23:05:38]

xiaochengh@chromium.org changed reviewers:
+ ochang@chromium.org

[Xiaocheng, 2016-08-12 23:05:38]

+ochang for ipc security review

[Oliver Chang, 2016-08-15 19:10:50]

ipc lgtm

[Xiaocheng, 2016-08-15 23:43:18]

The CQ bit was checked by xiaochengh@chromium.org

[Xiaocheng, 2016-08-15 23:43:19]

Thanks!

[commit-bot: I haz the power, 2016-08-15 23:43:46]

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-16 04:12:49]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-16 04:15:06]

Description was changed from

==========
Code cleanup related to unified text checker

Unified text checker is always enabled in chrome, and never assumed to
be disabled in any test case. Hence, this patch removes:
- All code conditioning on unified text checker
- Dead code assuming unified text checker is disabled
- The switch |unifiedTextCheckerEnabled| from Settings

This patch also simplifies code related to conditioning of
isContinuousSpellCheckingEnabled() in SpellChecker.cpp, given that
unified text checker is always enabled.

The following layout tests are marked as NeedsRebaseline because their
pixel results are affected by the appearance of spelling markers after
enabling unified text checker. They do not explicitly test any
functionality of spell checker, though:
- fast/dom/blur-contenteditable.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
- fast/dom/focus-contenteditable.html
-
virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- fast/forms/text/input-double-click-selection-gap-bug.html
-
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=635509
TEST=n/a; no behavior change, but several test files are changed
==========

to

==========
Code cleanup related to unified text checker

Unified text checker is always enabled in chrome, and never assumed to
be disabled in any test case. Hence, this patch removes:
- All code conditioning on unified text checker
- Dead code assuming unified text checker is disabled
- The switch |unifiedTextCheckerEnabled| from Settings

This patch also simplifies code related to conditioning of
isContinuousSpellCheckingEnabled() in SpellChecker.cpp, given that
unified text checker is always enabled.

The following layout tests are marked as NeedsRebaseline because their
pixel results are affected by the appearance of spelling markers after
enabling unified text checker. They do not explicitly test any
functionality of spell checker, though:
- fast/dom/blur-contenteditable.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
- fast/dom/focus-contenteditable.html
-
virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- fast/forms/text/input-double-click-selection-gap-bug.html
-
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=635509
TEST=n/a; no behavior change, but several test files are changed

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

[commit-bot: I haz the power, 2016-08-16 04:15:08]

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

[Xiaocheng, 2016-08-16 05:35:52]

Description was changed from

==========
Code cleanup related to unified text checker

Unified text checker is always enabled in chrome, and never assumed to
be disabled in any test case. Hence, this patch removes:
- All code conditioning on unified text checker
- Dead code assuming unified text checker is disabled
- The switch |unifiedTextCheckerEnabled| from Settings

This patch also simplifies code related to conditioning of
isContinuousSpellCheckingEnabled() in SpellChecker.cpp, given that
unified text checker is always enabled.

The following layout tests are marked as NeedsRebaseline because their
pixel results are affected by the appearance of spelling markers after
enabling unified text checker. They do not explicitly test any
functionality of spell checker, though:
- fast/dom/blur-contenteditable.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
- fast/dom/focus-contenteditable.html
-
virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- fast/forms/text/input-double-click-selection-gap-bug.html
-
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=635509
TEST=n/a; no behavior change, but several test files are changed

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

to

==========
Code cleanup related to unified text checker

Unified text checker is always enabled in chrome, and never assumed to
be disabled in any test case. Hence, this patch removes:
- All code conditioning on unified text checker
- Dead code assuming unified text checker is disabled
- The switch |unifiedTextCheckerEnabled| from Settings

This patch also simplifies code related to conditioning of
isContinuousSpellCheckingEnabled() in SpellChecker.cpp, given that
unified text checker is always enabled.

The following layout tests are marked as NeedsRebaseline because their
pixel results are affected by the appearance of spelling markers after
enabling unified text checker. They do not explicitly test any
functionality of spell checker, though:
- fast/dom/blur-contenteditable.html
- fast/inline-block/14498-positionForCoordinates.html
- fast/repaint/inline-outline-repaint.html
- fast/dom/focus-contenteditable.html
-
virtual/prefer_compositing_to_lcd_text/compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html
- fast/forms/text/input-double-click-selection-gap-bug.html
-
compositing/overflow/do-not-paint-outline-into-composited-scrolling-contents.html

BUG=633509
TEST=n/a; no behavior change, but several test files are changed

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

[timvolodine, 2016-08-19 13:58:54]

On 2016/08/16 04:15:08, commit-bot: I haz the power wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/accb28ab54cbdf525c3e319c3857a492bb27f04c
> Cr-Commit-Position: refs/heads/master@{#412164}

unified_textchecker_enabled is actually false by default in android webview.
Does this have any performance consequences?
I guess we are considering enabling spellchecking in webview anyway so this may
still be ok.

[Xiaocheng, 2016-08-22 01:53:04]

On 2016/08/19 at 13:58:54, timvolodine wrote:
> On 2016/08/16 04:15:08, commit-bot: I haz the power wrote:
> > Patchset 5 (id:??) landed as
> > https://crrev.com/accb28ab54cbdf525c3e319c3857a492bb27f04c
> > Cr-Commit-Position: refs/heads/master@{#412164}
> 
> unified_textchecker_enabled is actually false by default in android webview.
Does this have any performance consequences?
> I guess we are considering enabling spellchecking in webview anyway so this
may still be ok.

Sorry but I can't find the code where the flag is set. Could you provide a link?

IMO enabling unified text checker only improves the performance. The code path
with unified text checker disabled invokes synchronous spell checking, which is
something we are trying to get rid of (see crbug.com/517298); The code path with
it enabled does things in a semi-asynchronous way: requests are generated
synchronously but resolved asynchronously.

ps. "unified text checker" is a really bad legacy name that no longer reflects
what it does...

[timvolodine, 2016-08-23 14:30:07]

On 2016/08/22 01:53:04, Xiaocheng wrote:
> On 2016/08/19 at 13:58:54, timvolodine wrote:
> > On 2016/08/16 04:15:08, commit-bot: I haz the power wrote:
> > > Patchset 5 (id:??) landed as
> > > https://crrev.com/accb28ab54cbdf525c3e319c3857a492bb27f04c
> > > Cr-Commit-Position: refs/heads/master@{#412164}
> > 
> > unified_textchecker_enabled is actually false by default in android webview.
> Does this have any performance consequences?
> > I guess we are considering enabling spellchecking in webview anyway so this
> may still be ok.
> 
> Sorry but I can't find the code where the flag is set. Could you provide a
link?
> 

It was only set to true in chrome_content_browser_client.cc (which you removed)
but it's false by default.

> IMO enabling unified text checker only improves the performance. The code path
> with unified text checker disabled invokes synchronous spell checking, which
is
> something we are trying to get rid of (see crbug.com/517298); The code path
with
> it enabled does things in a semi-asynchronous way: requests are generated
> synchronously but resolved asynchronously.
> 
> ps. "unified text checker" is a really bad legacy name that no longer reflects
> what it does...

right, I don't know what unified spellchecker exactly used to do but at the time
it seemed like a number of code paths exited early due to it being disabled (as
in webview). Hence my wondering if it unnecessarily invokes more code now on
webview. However if we enable spellchecking in webview as well it may be needed
anyway..

[Xiaocheng, 2016-08-24 02:15:47]

On 2016/08/23 at 14:30:07, timvolodine wrote:
> On 2016/08/22 01:53:04, Xiaocheng wrote:
> > On 2016/08/19 at 13:58:54, timvolodine wrote:
> > > On 2016/08/16 04:15:08, commit-bot: I haz the power wrote:
> > > > Patchset 5 (id:??) landed as
> > > > https://crrev.com/accb28ab54cbdf525c3e319c3857a492bb27f04c
> > > > Cr-Commit-Position: refs/heads/master@{#412164}
> > > 
> > > unified_textchecker_enabled is actually false by default in android
webview.
> > Does this have any performance consequences?
> > > I guess we are considering enabling spellchecking in webview anyway so
this
> > may still be ok.
> > 
> > Sorry but I can't find the code where the flag is set. Could you provide a
link?
> > 
> 
> It was only set to true in chrome_content_browser_client.cc (which you
removed) but it's false by default.
> 
> > IMO enabling unified text checker only improves the performance. The code
path
> > with unified text checker disabled invokes synchronous spell checking, which
is
> > something we are trying to get rid of (see crbug.com/517298); The code path
with
> > it enabled does things in a semi-asynchronous way: requests are generated
> > synchronously but resolved asynchronously.
> > 
> > ps. "unified text checker" is a really bad legacy name that no longer
reflects
> > what it does...
> 
> right, I don't know what unified spellchecker exactly used to do but at the
time it seemed like a number of code paths exited early due to it being disabled
(as in webview). Hence my wondering if it unnecessarily invokes more code now on
webview. However if we enable spellchecking in webview as well it may be needed
anyway..

Agreed.