Bug 88618 - [chromium] Use the marker region to replace a misspelled word with a suggestion
Summary: [chromium] Use the marker region to replace a misspelled word with a suggestion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-07 22:08 PDT by Hironori Bono
Modified: 2012-06-13 21:08 PDT (History)
12 users (show)

See Also:


Attachments
Patch v0 (3.92 KB, patch)
2012-06-07 22:48 PDT, Hironori Bono
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hironori Bono 2012-06-07 22:08:02 PDT
Greetings,

Our new spellchecker adds markers to wrongly-separated words. For example, when we type a sentence "dive in to the rabbit hole", it adds a marker to 'in to' and adds a suggestion 'into'. Unfortunately, when we choose the suggestion "into", Chromium replaces only the word selected by caret with the suggestion. (For this case, it replaces 'in' or 'to' with 'into'.) To fix this problem, I would like to change Chromium to use a marker region to replace a misspelled region.

Regards,

Hironori Bono
Comment 1 Hironori Bono 2012-06-07 22:48:23 PDT
Created attachment 146482 [details]
Patch v0

Greetings,

I have quickly implemented a function that replaces the range of a misspelled marker with text. Would it be possible to review this change?

Regards,

Hironori Bono
Comment 2 WebKit Review Bot 2012-06-07 22:51:26 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Ryosuke Niwa 2012-06-07 23:10:31 PDT
Comment on attachment 146482 [details]
Patch v0

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1363
> +    // If this caret selection has two or more markers, this function replace the range covered by the first marker with the specified word as Microsoft Word does.
> +    if (pluginContainerFromFrame(frame()))
> +        return;
> +    RefPtr<Range> caretRange = frame()->selection()->toNormalizedRange();
> +    if (!caretRange)
> +        return;
> +    Vector<DocumentMarker*> markers = frame()->document()->markers()->markersInRange(caretRange.get(), DocumentMarker::Spelling | DocumentMarker::Grammar);
> +    if (markers.size() < 1 || markers[0]->startOffset() >= markers[0]->endOffset())
> +        return;
> +    RefPtr<Range> markerRange = TextIterator::rangeFromLocationAndLength(frame()->selection()->rootEditableElementOrDocumentElement(), markers[0]->startOffset(), markers[0]->endOffset() - markers[0]->startOffset());
> +    if (!markerRange.get() || !frame()->selection()->shouldChangeSelection(markerRange.get()))
> +        return;
> +    frame()->selection()->setSelection(markerRange.get(), CharacterGranularity);
> +    frame()->editor()->replaceSelectionWithText(text, false, true);

This code looks awfully familiar. In fact, I saw almost exactly same comment elsewhere.
Comment 4 Hajime Morrita 2012-06-08 00:39:23 PDT
Comment on attachment 146482 [details]
Patch v0

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

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1363
>> +    frame()->editor()->replaceSelectionWithText(text, false, true);
> 
> This code looks awfully familiar. In fact, I saw almost exactly same comment elsewhere.

Searched with replaceSelectionWithText(), but I couldn't find any. 
It looks Mac port (and other ports) does this in WebKit layer for obeying AppKit protocol or whatever reason.

I hope this core logic to be a Editor method to allow future refactoring though.
Comment 5 Hironori Bono 2012-06-08 00:52:53 PDT
Greetings Niwa-san,

Thanks for your comment. To use code search, my code seems to be a subset of AlternativeTextController::handleAlternativeTextUIResult() <http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/editing/AlternativeTextController.cpp&exact_package=chromium&q=handleAlternativeTextUIResult&type=cs&l=354> in terms of functionality. (This function is for the auto-correction panel of Mac and not used by Chromium, though.)

Regards,

Hironori Bono

(In reply to comment #3)
> This code looks awfully familiar. In fact, I saw almost exactly same comment elsewhere.
Comment 6 Ryosuke Niwa 2012-06-08 01:00:48 PDT
Alexey and Enrica might know where this code is located in Mac port.
Comment 7 Hironori Bono 2012-06-11 19:30:53 PDT
Greetings Niwa-san,

For what it's worth, I wonder how I should treat this change. Is it better to move this code to WebCore (Editor), or to keep it in the WebKit layer?

Regards,

Hironori Bono

(In reply to comment #6)
> Alexey and Enrica might know where this code is located in Mac port.
Comment 8 Kent Tamura 2012-06-13 20:05:10 PDT
Comment on attachment 146482 [details]
Patch v0

Looks ok.  We can share code later if one wants to do so.
Comment 9 WebKit Review Bot 2012-06-13 21:08:33 PDT
Comment on attachment 146482 [details]
Patch v0

Clearing flags on attachment: 146482

Committed r120274: <http://trac.webkit.org/changeset/120274>
Comment 10 WebKit Review Bot 2012-06-13 21:08:39 PDT
All reviewed patches have been landed.  Closing bug.