Bug 114344 - Refactor Editor::markAndReplaceFor before fixing autocorrection bugs
Summary: Refactor Editor::markAndReplaceFor before fixing autocorrection bugs
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: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 114337
  Show dependency tree / graph
 
Reported: 2013-04-10 02:18 PDT by Ryosuke Niwa
Modified: 2013-04-10 12:15 PDT (History)
5 users (show)

See Also:

Attachments
Cleanup (14.27 KB, patch)
2013-04-10 02:23 PDT, Ryosuke Niwa 		no flags Details | Formatted Diff | Diff
More cleanups (16.06 KB, patch)
2013-04-10 02:32 PDT, Ryosuke Niwa 		enrica: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-04-10 02:18:33 PDT 
Refactor Editor::markAndReplaceFor before fixing autocorrection bugs
Comment 1 Ryosuke Niwa 2013-04-10 02:23:46 PDT 
Created attachment 197227 [details]
Cleanup
Comment 2 Ryosuke Niwa 2013-04-10 02:32:56 PDT 
Created attachment 197230 [details]
More cleanups
Comment 3 Enrica Casucci 2013-04-10 11:20:04 PDT 
Comment on attachment 197230 [details]
More cleanups

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

Looks ok to me. just few nits in the change log.

> Source/WebCore/ChangeLog:8
> +        This patch refactors Editor::markAndReplaceFor so that we can start fixing bugs in a sane staet.

Typo: state.

> Source/WebCore/ChangeLog:12
> +        In particular, shouldMarkSpelling used to be set false when shouldShowCorrectionPanel was true

to false

> Source/WebCore/ChangeLog:14
> +        since shouldMarkSpelling was only referred once after the assignment.

typo referenced.

> Source/WebCore/editing/Editor.cpp:2265
> +            bool existingMarkersPermitReplacement = m_alternativeTextController->processMarkersOnTextToBeReplacedByResult(&results[i], rangeToReplace.get(), replacedString);

Can this be const as well?
Comment 4 Ryosuke Niwa 2013-04-10 11:26:06 PDT 
Thanks for the review! Will land after fixing these nits.
Comment 5 Ryosuke Niwa 2013-04-10 12:15:25 PDT 
Committed r148124: <http://trac.webkit.org/changeset/148124>