Bug 70299

Summary: Implement legacy text check emulation in unified text check interface.
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, gustavo.noronha, gustavo, jiapu.mail, kkristof, morrita, ojan, rniwa, shinyak, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 71031, 71792, 72251    
Bug Blocks: 65849    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Shinya Kawanaka
Reported 2011-10-17 19:45:41 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. To achieve the goal, checkTextOfParagraph() interface should be available even if its implementation cannot be used. In such a case, checkTextOfParagraph() should be implemented using checkSpellingOfString() and checkGrammarOfString().
Attachments
Patch (8.45 KB, patch)
2011-10-17 23:30 PDT, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (12.52 KB, patch)
2011-10-19 05:51 PDT, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (16.24 KB, patch)
2011-10-19 21:40 PDT, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (15.84 KB, patch)
2011-10-19 21:44 PDT, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (16.54 KB, patch)
2011-10-25 20:47 PDT, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (16.54 KB, patch)
2011-10-25 21:08 PDT, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (26.94 KB, patch)
2011-11-10 16:41 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (16.65 KB, patch)
2011-11-10 17:55 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.
Shinya Kawanaka
Comment 1 2011-10-17 23:30:17 PDT
Created attachment 111389 [details] Patch
Hajime Morrita
Comment 2 2011-10-18 23:05:38 PDT
Comment on attachment 111389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111389&action=review > Source/WebCore/page/Settings.cpp:199 > +#if USE(UNIFIED_TEXT_CHECKING) || PLATFORM(CHROMIUM) Let's do this at a separate change to allow revert easily because this can cause some unexpected, hard-to-fix regression. (I hope not though.) At this point, I recommend - to add WebCore::Internals API to flip the flag, and - to add some tests for our emulating code, especially with enabling this flag. The test would have -- a test case for some non-whitespace segmentation -- a test case containing multiple bad words -- a test case containing a bad word which located at the beginning, middle, end of the paragraph. -- a test case containing a valid word which contains a bad word as a substring.
Shinya Kawanaka
Comment 3 2011-10-19 05:51:06 PDT
Created attachment 111601 [details] Patch
Collabora GTK+ EWS bot
Comment 4 2011-10-19 07:00:25 PDT
Comment on attachment 111601 [details] Patch Attachment 111601 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10145415
Shinya Kawanaka
Comment 5 2011-10-19 21:40:37 PDT
Created attachment 111722 [details] Patch
Shinya Kawanaka
Comment 6 2011-10-19 21:41:44 PDT
Comment on attachment 111722 [details] Patch Trying to build fix win & gtk.
Shinya Kawanaka
Comment 7 2011-10-19 21:44:45 PDT
Created attachment 111724 [details] Patch
Shinya Kawanaka
Comment 8 2011-10-25 00:15:07 PDT
(In reply to comment #7) > Created an attachment (id=111724) [details] > Patch morrita-san, Could you review this?
Hajime Morrita
Comment 9 2011-10-25 10:02:03 PDT
Comment on attachment 111724 [details] Patch The code look good in general. in test, could you use unit testing framework if possible ? It makes the test expectation readable. You can see an example in existing tests like: http://trac.webkit.org/browser/trunk/LayoutTests/editing/spelling/spelling-insert-html.html for example.
Shinya Kawanaka
Comment 10 2011-10-25 20:47:23 PDT
Created attachment 112444 [details] Patch
Shinya Kawanaka
Comment 11 2011-10-25 20:57:20 PDT
(In reply to comment #10) > Created an attachment (id=112444) [details] > Patch Hi, I've change the test. Could you review it again?
Shinya Kawanaka
Comment 12 2011-10-25 21:08:34 PDT
Created attachment 112445 [details] Patch
Hajime Morrita
Comment 13 2011-10-26 06:03:31 PDT
Comment on attachment 112445 [details] Patch Thanks for the update.
WebKit Review Bot
Comment 14 2011-10-27 05:52:32 PDT
Comment on attachment 112445 [details] Patch Clearing flags on attachment: 112445 Committed r98556: <http://trac.webkit.org/changeset/98556>
WebKit Review Bot
Comment 15 2011-10-27 05:52:39 PDT
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 17 2011-10-27 10:10:09 PDT
Oops, I didn't realize but the failing test was added by this patch!
Ryosuke Niwa
Comment 18 2011-10-27 10:17:11 PDT
I'm going to rollout the patch for now because it's failing at least on Mac and GTK.
Ryosuke Niwa
Comment 19 2011-10-27 10:32:14 PDT
Reopen the bug since the patch was rolled out.
Shinya Kawanaka
Comment 20 2011-11-10 16:41:08 PST
Created attachment 114600 [details] Patch
Hajime Morrita
Comment 21 2011-11-10 17:34:23 PST
Comment on attachment 114600 [details] Patch Basically OK. But Source/WebKit2/ChangeLog is broken. Please check the character encoding setting of your editor.
Shinya Kawanaka
Comment 22 2011-11-10 17:55:09 PST
Created attachment 114610 [details] Patch
Shinya Kawanaka
Comment 23 2011-11-10 17:56:13 PST
morrita-san, sorry for bothering you...
WebKit Review Bot
Comment 24 2011-11-11 17:21:59 PST
Comment on attachment 114610 [details] Patch Clearing flags on attachment: 114610 Committed r100050: <http://trac.webkit.org/changeset/100050>
WebKit Review Bot
Comment 25 2011-11-11 17:22:05 PST
All reviewed patches have been landed. Closing bug.
Kristóf Kosztyó
Comment 26 2011-11-14 01:25:50 PST
Hi, the new test what is added by the patch (editing/spelling/spelling-unified-emulation.html) is failng on GTK and Qt. Please look at this bug: https://bugs.webkit.org/show_bug.cgi?id=72251
Note You need to log in before you can comment on or make changes to this bug.