Bug 73628 - Refactoring: Editor::markAllMisspellingsAndBadGrammarInRanges should be refactored.
Summary: Refactoring: Editor::markAllMisspellingsAndBadGrammarInRanges should be refac...
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: 73902
Blocks: 73616
  Show dependency treegraph
 
Reported: 2011-12-01 23:04 PST by Shinya Kawanaka
Modified: 2011-12-06 21:01 PST (History)
5 users (show)

See Also:


Attachments
Patch (8.85 KB, patch)
2011-12-05 01:32 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (10.51 KB, patch)
2011-12-05 15:47 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (10.47 KB, patch)
2011-12-06 18:02 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 23:04:09 PST
Editor::markAllMisspellingsAndBadGrammarInRanges() should be refactored
 to unify the paths of Unified SpellChecker and Un-unified SpellChecker for adding markers.
Comment 1 Shinya Kawanaka 2011-12-01 23:27:00 PST
(In reply to comment #0)
> Editor::markAllMisspellingsAndBadGrammarInRanges() should be refactored
>  to unify the paths of Unified SpellChecker and Un-unified SpellChecker for adding markers.

Also, SpellChecker::requestCheckingFor should be refactored.
Comment 2 Shinya Kawanaka 2011-12-05 01:32:04 PST
Created attachment 117855 [details]
Patch
Comment 3 WebKit Review Bot 2011-12-05 02:19:47 PST
Comment on attachment 117855 [details]
Patch

Attachment 117855 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10734618

New failing tests:
svg/custom/linking-uri-01-b.svg
Comment 4 Hajime Morrita 2011-12-05 03:24:15 PST
Comment on attachment 117855 [details]
Patch

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

Basically looks nice. There are only a few nits.

> Source/WebCore/editing/Editor.cpp:2045
> +    ExceptionCode ec = 0;

It's good opportunity to eliminate this. You can use ASSERT_NO_EXCEPTION.

> Source/WebCore/editing/Editor.h:39
> +#include "TextCheckingHelper.h"

I really want hide this in .cpp file.

> Source/WebCore/editing/Editor.h:408
> +    void markAndReplaceFor(TextCheckingTypeMask, const Vector<TextCheckingResult>&, TextCheckingParagraph spellingParagraph, TextCheckingParagraph grammarParagraph);

Is it possible to pass TextCheckingParagraph as a reference or a pointer?
Comment 5 Shinya Kawanaka 2011-12-05 15:47:39 PST
Created attachment 117950 [details]
Patch
Comment 6 WebKit Review Bot 2011-12-06 02:32:13 PST
Comment on attachment 117950 [details]
Patch

Clearing flags on attachment: 117950

Committed r102111: <http://trac.webkit.org/changeset/102111>
Comment 7 WebKit Review Bot 2011-12-06 02:32:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Vsevolod Vlasov 2011-12-06 03:39:28 PST
This patch broke compilation on several platforms.

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac%20Builder%20%28CG%29/builds/3684

../editing/SpellingCorrectionController.h:58:1: error: struct 'TextCheckingResult' was previously declared as a class [-Werror,-Wmismatched-tags]
struct TextCheckingResult;
^
../editing/Editor.h:66:7: note: previous use is here
class TextCheckingResult;
      ^
1 error generated.

Rolled out: <http://trac.webkit.org/changeset/102120>

Reopening.
Comment 9 Shinya Kawanaka 2011-12-06 18:02:22 PST
Created attachment 118153 [details]
Patch
Comment 10 WebKit Review Bot 2011-12-06 21:01:14 PST
Comment on attachment 118153 [details]
Patch

Clearing flags on attachment: 118153

Committed r102218: <http://trac.webkit.org/changeset/102218>
Comment 11 WebKit Review Bot 2011-12-06 21:01:19 PST
All reviewed patches have been landed.  Closing bug.