Bug 69128 - Unifying spell-checking code path should be easy
Summary: Unifying spell-checking code path should be easy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 69241 69242
Blocks: 65849
  Show dependency treegraph
 
Reported: 2011-09-29 23:17 PDT by Shinya Kawanaka
Modified: 2011-11-10 17:19 PST (History)
9 users (show)

See Also:


Attachments
Patch (35.20 KB, patch)
2011-09-30 00:00 PDT, Shinya Kawanaka
gustavo.noronha: commit-queue-
Details | Formatted Diff | Diff
Test Patch (35.34 KB, patch)
2011-09-30 01:20 PDT, Shinya Kawanaka
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Test Patch (42.65 KB, patch)
2011-09-30 03:15 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (42.65 KB, patch)
2011-09-30 04:16 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (47.01 KB, patch)
2011-09-30 05:23 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2011-09-29 23:17:14 PDT
Editor class has two difference code paths for spell-checking:
(1) checkTextOfParagraph() for Snow Leopard or later
(2) checkSpellingOfString() and checkGrammarOfString() for other platforms.
These paths should be unified so that improving spell-checking code is easy.

At the first step, we should call checkTextOfParagraph() indirectly so that we can implement a function to mimic checkTextOfParagraph() in non-SL or non-Lion platform.
Also, we should introduce a flag to choose a code path which calls checkTextOfParagraph() in non-SL or non-Lion platform.
Comment 1 Shinya Kawanaka 2011-09-30 00:00:14 PDT
Created attachment 109258 [details]
Patch
Comment 2 WebKit Review Bot 2011-09-30 00:02:41 PDT
Attachment 109258 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/accessibility/Accessibility..." exit_code: 1

Source/WebCore/editing/TextCheckingHelper.h:98:  The parameter name "client" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/editing/Editor.cpp:1892:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/editing/Editor.cpp:1980:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/editing/Editor.cpp:3238:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 4 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Collabora GTK+ EWS bot 2011-09-30 00:08:22 PDT
Comment on attachment 109258 [details]
Patch

Attachment 109258 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9899334
Comment 4 Gyuyoung Kim 2011-09-30 00:09:25 PDT
Comment on attachment 109258 [details]
Patch

Attachment 109258 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9888737
Comment 5 WebKit Review Bot 2011-09-30 00:12:36 PDT
Comment on attachment 109258 [details]
Patch

Attachment 109258 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9901303
Comment 6 Early Warning System Bot 2011-09-30 00:14:00 PDT
Comment on attachment 109258 [details]
Patch

Attachment 109258 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9899335
Comment 7 Shinya Kawanaka 2011-09-30 01:20:11 PDT
Created attachment 109263 [details]
Test Patch
Comment 8 WebKit Review Bot 2011-09-30 01:23:32 PDT
Attachment 109263 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/accessibility/Accessibility..." exit_code: 1

Source/WebCore/editing/TextCheckingHelper.h:98:  The parameter name "client" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/editing/Editor.cpp:1892:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/editing/Editor.cpp:1980:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/editing/Editor.cpp:3238:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 4 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Gyuyoung Kim 2011-09-30 01:31:23 PDT
Comment on attachment 109263 [details]
Test Patch

Attachment 109263 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9898327
Comment 10 Early Warning System Bot 2011-09-30 01:32:07 PDT
Comment on attachment 109263 [details]
Test Patch

Attachment 109263 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9901320
Comment 11 Collabora GTK+ EWS bot 2011-09-30 02:24:50 PDT
Comment on attachment 109263 [details]
Test Patch

Attachment 109263 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9900350
Comment 12 Gustavo Noronha (kov) 2011-09-30 02:48:32 PDT
Comment on attachment 109263 [details]
Test Patch

Attachment 109263 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9893601
Comment 13 Shinya Kawanaka 2011-09-30 03:15:51 PDT
Created attachment 109274 [details]
Test Patch
Comment 14 Shinya Kawanaka 2011-09-30 04:16:15 PDT
Created attachment 109280 [details]
Patch
Comment 15 Shinya Kawanaka 2011-09-30 05:23:46 PDT
Created attachment 109286 [details]
Patch
Comment 16 Ryosuke Niwa 2011-10-02 21:15:36 PDT
Can we split this patch into smaller pieces?
Comment 17 Shinya Kawanaka 2011-10-02 21:35:30 PDT
(In reply to comment #16)
> Can we split this patch into smaller pieces?

Hmm...

Maybe we can split this patch into two patches:
1) introduces a wrapper function of TextClientChecker::checkTextOfParagraph(), and uses the wrapper instead of the original one.
2) removing static-#if statements by introducing unifiedTextCheckerEnabled.

Let me try.
Comment 18 Hajime Morrita 2011-10-02 22:28:28 PDT
> Maybe we can split this patch into two patches:
> 1) introduces a wrapper function of TextClientChecker::checkTextOfParagraph(), and uses the wrapper instead of the original one.
> 2) removing static-#if statements by introducing unifiedTextCheckerEnabled.
> 
> Let me try.
Sounds nice. In that case, please create sub-bugs which depends on this bug.
Comment 19 Hajime Morrita 2011-11-10 17:19:58 PST
Closing this since dependent bugs are closed.