[webkit-dev] Spellcheck code cleanup

[Hajime Morrita]

Hi folks,

My team is planning to simplify spellchecking code in WebCore by
migrating old code path to new code path.
I'd like to hear your advice before moving forward.

Background:
Currently WebCore has two separate logics for spellchecking.
The first version relies on TextCheckerClient::checkTextOfParagraph(),
which is available Snow Leopard or later.
We call it "unified" version since the code path is guarded by
USE(UNIFIED_TEXT_CHECKING).
Another version only uses simpler (but less powerful) API like
TextCheckerClient::checkSpellingOfString().
We call it "legacy" version.

Although both "unified" and "legacy" version have almost same role,
their code paths are completely different and are mutual exclusive.
"Unified" version is inside #if block and "legacy" is in #else.
This makes it hard to change editing code
because we need to take care of both but we cannot check both at once.

Our goal is to make the code simpler by removing "legacy" path and
replacing it with "unified" path.
It should be possible by implementing checkTextOfParagraph()
equivalent with other TextCheckerClient API.

Here is a tracking bug for this: https://bugs.webkit.org/show_bug.cgi?id=65849
I uploaded prototype version of the change.

Theoretically, this can be a harmless one-shot change.
But we're worrying about unexpected side effect thus we'd like to do
it in small steps.
Here is a rough plan:

Step 1: Add Setting::isUnifiedTextCheckerEnabled(), which is true only
for platform who has checkTextOfParagraph().
Step 2: Implement Editor::checkTextOfParagraph() which wraps
TextCheckerClient::checkTextOfParagraph()
            and replace TextCheckerClient::checkTextOfParagraph() call
with Editor's indirection.
Step 3: Replace compile-time flag "#if USE(UNIFIED_TEXT_CHECKING)"
with the runtime-flag check using isUnifiedTextCheckerEnabled().
             This is only for files under editing/.
Step 4: Implement TextCheckerClient::checkTextOfParagraph() equivalent
in Editor::checkTextOfParagraph(),
             which is used with platforms which don't have
TextCheckerClient::checkTextOfParagraph().
Step 5: Flip isUnifiedTextCheckerEnabled() to true on Chromium (maybe
behind the experimental flag.)
            Then see how it works.
Step 6: Stabilize (Hopes there is no need to do it ;-))
Step 7: Once step 6 goes well, flip isUnifiedTextCheckerEnabled() for
all platforms,
             maybe behind the compilation flags like
ENABLE_EDITOR_UNIFIED_TEXT_CHECKING.
Step 8: Remove "legacy" code path and the runtime flag.

Possible side effects in my mind are:

A: Temporal code size growth
Since we enable both legacy and unified code path, it can impact the code size.
But in practice, the grows is around a few hundred lines of code and
there is no template heavy class.
Thus the impact seems negligible.

B: Performance impact.
This change can affect performance since the "unified" version
requires larger text chunks to check spelling
when the grammar checker is enabled.
This affect only Leopard since it is the only platform that has
grammar checker but doesn't have unified checker.
And in my understanding, Safari disables grammar checker by default.
So this looks to affect only
small number of people who 1.use Leopard 2.enable grammar checking
3.use latest WebKit.
And the spellchecking only happens by typing, and there is no batch spellcheck.
Thus it would be little recognizable difference in real use.

>From my personal viewpoint, Step 1-5 looks fine, but 6-8 may be controversial.
How do you think? Any feedback is appreciated.

Regards,
--
morrita