Bug 56818 – Spellcheck feature specific symbol should be defined.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Hajime Morrita]

Editor class has two different path to do spellchecking.
One is using both TextCheckerClient::checkSpellingOfString() and TextCheckingClient::checkGrammarOfString() separately,
Another is using the unified text checker via TextCheckerClient::checkGrammarOfString().

These two passes should be embraced by clearly-named symbols, like SUPPORT_UNIFIED_TEXT_CHECKING.

[Hajime Morrita]

Created attachment 86454 [details]
Patch

[Kent Tamura]

Comment on attachment 86454 [details]
Patch

I think it's better to adopt USE(UNIFIED_TEXT_CHECKING) and USE(GRAMMER_CHECKING) macros.  See wtf/Platform.h

[Hajime Morrita]

Created attachment 86568 [details]
Patch

[Hajime Morrita]

Hi Kent-san, thanks for your suggestion!
I replaced them with using USE() macro.

[Kent Tamura]

Comment on attachment 86568 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86568&action=review

> Source/JavaScriptCore/wtf/Platform.h:1148
> +/* Text checking and editing */
> +#if !defined(BUILDING_ON_TIGER)
> +#define WTF_USE_GRAMMAR_CHECKING 1
> +#endif
> +
> +#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD)
> +#define WTF_USE_UNIFIED_TEXT_CHECKING 1
> +#define WTF_USE_AUTOMATIC_TEXT_REPLACEMENT 1
> +#endif

I think it's ok to define WTF_USE_* symbols in TextCheckerClient.h like the previous patch.
We have some instances defining WTF_USE_* not in Platform.h, like WebCore/platform/graphics/ImageBuffer.h.

[Hajime Morrita]

Hi Kent-san, thank you for the review again!

> 
> I think it's ok to define WTF_USE_* symbols in TextCheckerClient.h like the previous patch.
> We have some instances defining WTF_USE_* not in Platform.h, like WebCore/platform/graphics/ImageBuffer.h.

I added WTF_USE_AUTOMATIC_TEXT_REPLACEMENT at the last update 
and it is used in Editor.h, which doesn't include TextCheckingClient.h.
So I'd like to put them inside Platform.h.

I think I can move these definitions somewhere
on Bug 56085, in which I'm going to introduce a small header file
which is shared between Editror.h and TextCheckingClient.h.

[Kent Tamura]

(In reply to comment #6)
> I added WTF_USE_AUTOMATIC_TEXT_REPLACEMENT at the last update 
> and it is used in Editor.h, which doesn't include TextCheckingClient.h.
> So I'd like to put them inside Platform.h.
> 
> I think I can move these definitions somewhere
> on Bug 56085, in which I'm going to introduce a small header file
> which is shared between Editror.h and TextCheckingClient.h.

Can you introduce the small header file in this change? A Platform.h change causes rebuilding almost all of source files in all of buildbots.  We'd like to avoid it if possible.

[Hajime Morrita]

Created attachment 86744 [details]
Patch

[Hajime Morrita]

> Can you introduce the small header file in this change? A Platform.h change causes rebuilding almost all of source files in all of buildbots.  We'd like to avoid it if possible.
Sure. Updated the patch to add new TextChecking.h.

[Kent Tamura]

Comment on attachment 86744 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86744&action=review

> Source/JavaScriptCore/ChangeLog:1
> [phone]  MORITA Hajime  <[e-mail]>

Please revert the change for JavaScriptCore/ChangeLog

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:22700
> +				A77D0012133B0AEB00D6658C /* TextChecking.h in Headers */,

nit: should be inserted to the appropriate position.

[Hajime Morrita]

Created attachment 86749 [details]
Patch

[Hajime Morrita]

Hi Kent-san, thanks again!
The update patch fixed both.

(In reply to comment #10)
> (From update of attachment 86744 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86744&action=review
> 
> > Source/JavaScriptCore/ChangeLog:1
> > [phone]  MORITA Hajime  <[e-mail]>
> 
> Please revert the change for JavaScriptCore/ChangeLog
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:22700
> > +				A77D0012133B0AEB00D6658C /* TextChecking.h in Headers */,
> 
> nit: should be inserted to the appropriate position.

[Hajime Morrita]

Committed r81855: <http://trac.webkit.org/changeset/81855>