Bug 73616 - Asynchronous path and synchronous path of SpellChecker should share the code to mark misspellings
Summary: Asynchronous path and synchronous path of SpellChecker should share the code ...
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: 73628 73889 74033 74035
Blocks: 56368
  Show dependency treegraph
 
Reported: 2011-12-01 20:30 PST by Shinya Kawanaka
Modified: 2011-12-11 20:41 PST (History)
4 users (show)

See Also:


Attachments
Patch (27.40 KB, patch)
2011-12-06 03:43 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (27.21 KB, patch)
2011-12-07 02:59 PST, Shinya Kawanaka
morrita: review-
Details | Formatted Diff | Diff
Patch (4.46 KB, patch)
2011-12-08 22:44 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (8.08 KB, patch)
2011-12-09 04:16 PST, 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-12-01 20:30:36 PST
Currently the unified spellchecker and un-unified spellchecker do not share the code to mark misspellings.
They should be merged.
Comment 1 Shinya Kawanaka 2011-12-06 03:43:27 PST
Created attachment 118019 [details]
Patch
Comment 2 Shinya Kawanaka 2011-12-07 02:59:56 PST
Created attachment 118196 [details]
Patch
Comment 3 Hajime Morrita 2011-12-07 03:30:47 PST
Comment on attachment 118196 [details]
Patch

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

Could you split the patch into some trivial part (like renaming, exporting an internal class, etc.) and possibly-observable change like make marking code shared?

> Source/WebCore/ChangeLog:6
> +        Need a short description and bug URL (OOPS!)

You don't need this.

> Source/WebCore/editing/SpellChecker.h:80
> +    class InternalSpellCheckRequest;

This doesn't need to be a nested class.
Comment 4 Shinya Kawanaka 2011-12-08 22:44:48 PST
Created attachment 118534 [details]
Patch
Comment 5 WebKit Review Bot 2011-12-08 22:48:47 PST
Comment on attachment 118534 [details]
Patch

Attachment 118534 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10780559
Comment 6 Hajime Morrita 2011-12-09 01:00:38 PST
Comment on attachment 118534 [details]
Patch

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

> Source/WebCore/editing/SpellChecker.cpp:175
> +    m_frame->editor()->markAndReplaceFor(m_processingRequest->mask(), results, m_processingRequest->checkingRange(), m_processingRequest->paragraphRange());

Could you pass SpellCheckRequest directly markAndReplaceFor() , instead of passing independent parameters.
Comment 7 Shinya Kawanaka 2011-12-09 04:16:04 PST
Created attachment 118558 [details]
Patch
Comment 8 Hajime Morrita 2011-12-11 17:51:50 PST
Comment on attachment 118558 [details]
Patch

Looks OK. Thanks for doing this!
Comment 9 Hajime Morrita 2011-12-11 17:53:02 PST
Once this landed, could you write a test for Bug 56368?
Comment 10 WebKit Review Bot 2011-12-11 19:12:22 PST
Comment on attachment 118558 [details]
Patch

Clearing flags on attachment: 118558

Committed r102553: <http://trac.webkit.org/changeset/102553>
Comment 11 WebKit Review Bot 2011-12-11 19:12:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Shinya Kawanaka 2011-12-11 20:41:59 PST
(In reply to comment #9)
> Once this landed, could you write a test for Bug 56368?

Yes.