Bug 56818 - Spellcheck feature specific symbol should be defined.
Summary: Spellcheck feature specific symbol should be defined.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-22 02:19 PDT by Hajime Morrita
Modified: 2011-03-24 00:00 PDT (History)
3 users (show)

See Also:


Attachments
Patch (17.20 KB, patch)
2011-03-22 04:27 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (21.16 KB, patch)
2011-03-22 21:26 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (28.23 KB, patch)
2011-03-23 22:54 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (27.65 KB, patch)
2011-03-23 23:32 PDT, Hajime Morrita
tkent: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2011-03-22 02:19:18 PDT
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.
Comment 1 Hajime Morrita 2011-03-22 04:27:59 PDT
Created attachment 86454 [details]
Patch
Comment 2 Kent Tamura 2011-03-22 19:20:33 PDT
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
Comment 3 Hajime Morrita 2011-03-22 21:26:44 PDT
Created attachment 86568 [details]
Patch
Comment 4 Hajime Morrita 2011-03-22 21:28:18 PDT
Hi Kent-san, thanks for your suggestion!
I replaced them with using USE() macro.
Comment 5 Kent Tamura 2011-03-22 22:04:47 PDT
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.
Comment 6 Hajime Morrita 2011-03-23 19:56:26 PDT
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.
Comment 7 Kent Tamura 2011-03-23 20:05:25 PDT
(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.
Comment 8 Hajime Morrita 2011-03-23 22:54:38 PDT
Created attachment 86744 [details]
Patch
Comment 9 Hajime Morrita 2011-03-23 22:55:21 PDT
> 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.
Comment 10 Kent Tamura 2011-03-23 23:07:54 PDT
Comment on attachment 86744 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:1
> +2011-03-23  MORITA Hajime  <morrita@google.com>

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.
Comment 11 Hajime Morrita 2011-03-23 23:32:12 PDT
Created attachment 86749 [details]
Patch
Comment 12 Hajime Morrita 2011-03-23 23:32:58 PDT
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
> > +2011-03-23  MORITA Hajime  <morrita@google.com>
> 
> 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.
Comment 13 Hajime Morrita 2011-03-24 00:00:36 PDT
Committed r81855: <http://trac.webkit.org/changeset/81855>