88618 2012-06-07 22:08:02 -0700 [chromium] Use the marker region to replace a misspelled word with a suggestion 2012-06-13 21:08:39 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 hbono webkit-unassigned abarth ap dglazkov enrica fishd jamesr jiapu.mail morrita rniwa tkent tkent+wkapi webkit.review.bot oldest_to_newest 644322 0 hbono 2012-06-07 22:08:02 -0700 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 644351 1 146482 hbono 2012-06-07 22:48:23 -0700 Created attachment 146482 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 644354 2 webkit.review.bot 2012-06-07 22:51:26 -0700 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. 644364 3 146482 rniwa 2012-06-07 23:10:31 -0700 Comment on attachment 146482 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. 644426 4 146482 morrita 2012-06-08 00:39:23 -0700 Comment on attachment 146482 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. 644438 5 hbono 2012-06-08 00:52:53 -0700 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. 644446 6 rniwa 2012-06-08 01:00:48 -0700 Alexey and Enrica might know where this code is located in Mac port. 646560 7 hbono 2012-06-11 19:30:53 -0700 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. 648718 8 146482 tkent 2012-06-13 20:05:10 -0700 Comment on attachment 146482 Patch v0 Looks ok. We can share code later if one wants to do so. 648769 9 146482 webkit.review.bot 2012-06-13 21:08:33 -0700 Comment on attachment 146482 Patch v0 Clearing flags on attachment: 146482 Committed r120274: <http://trac.webkit.org/changeset/120274> 648770 10 webkit.review.bot 2012-06-13 21:08:39 -0700 All reviewed patches have been landed. Closing bug. 146482 2012-06-07 22:48:23 -0700 2012-06-13 21:08:33 -0700 Patch v0 issue88618-patch0.txt text/plain 4016 hbono SW5kZXg6IFNvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNv dXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCShyZXZpc2lvbiAxMTk3OTkpCisrKyBTb3Vy Y2UvV2ViS2l0L2Nocm9taXVtL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIy IEBACisyMDEyLTA2LTA3ICBIaXJvbm9yaSBCb25vICA8aGJvbm9AY2hyb21pdW0ub3JnPgorCisg ICAgICAgIFtjaHJvbWl1bV0gQWRkIFdlYkZyYW1lOjpyZXBsYWNlTWlzc3BlbGxlZFJhbmdlCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04ODYxOAorCisg ICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoaXMgY2hhbmdl IGFkZHMgV2ViRnJhbWU6OnJlcGxhY2VNaXNzcGVsbGVkUmFuZ2UsIHdoaWNoIHJlcGxhY2VzIHRo ZSByYW5nZSBvZiBhCisgICAgICAgIG1pc3NwZWxsZWQgbWFya2VyIHdpdGggdGV4dCBzbyBDaHJv bWl1bSBjYW4gdXNlIGl0IGZvciByZXBsYWNpbmcgbWlzc3BlbGxlZAorICAgICAgICB3b3JkcyB3 aXRoIHN1Z2dldGlvbnMuCisKKyAgICAgICAgKiBwdWJsaWMvV2ViRnJhbWUuaDoKKyAgICAgICAg KFdlYkZyYW1lKToKKyAgICAgICAgKiBzcmMvV2ViRnJhbWVJbXBsLmNwcDoKKyAgICAgICAgKFdl YktpdDo6V2ViRnJhbWVJbXBsOjpyZXBsYWNlTWlzc3BlbGxlZFJhbmdlKToKKyAgICAgICAgKFdl YktpdCk6CisgICAgICAgICogc3JjL1dlYkZyYW1lSW1wbC5oOgorICAgICAgICAoV2ViRnJhbWVJ bXBsKToKKwogMjAxMi0wNi0wNyAgTmljbyBXZWJlciAgPHRoYWtpc0BjaHJvbWl1bS5vcmc+CiAK ICAgICAgICAgW2Nocm9taXVtL21hY10gSW1wcm92ZSBkZXZpY2VEUEksIHJlY3QsIGFuZCBhdmFp bGFibGVSZWN0IGNvbXB1dGF0aW9uCkluZGV4OiBTb3VyY2UvV2ViS2l0L2Nocm9taXVtL3B1Ymxp Yy9XZWJGcmFtZS5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJLaXQvY2hyb21pdW0vcHVibGlj L1dlYkZyYW1lLmgJKHJldmlzaW9uIDExOTc4NSkKKysrIFNvdXJjZS9XZWJLaXQvY2hyb21pdW0v cHVibGljL1dlYkZyYW1lLmgJKHdvcmtpbmcgY29weSkKQEAgLTQzNCw2ICs0MzQsNyBAQCBwdWJs aWM6CiAgICAgdmlydHVhbCB2b2lkIGVuYWJsZUNvbnRpbnVvdXNTcGVsbENoZWNraW5nKGJvb2wp ID0gMDsKICAgICB2aXJ0dWFsIGJvb2wgaXNDb250aW51b3VzU3BlbGxDaGVja2luZ0VuYWJsZWQo KSBjb25zdCA9IDA7CiAgICAgdmlydHVhbCB2b2lkIHJlcXVlc3RUZXh0Q2hlY2tpbmcoY29uc3Qg V2ViRWxlbWVudCYpID0gMDsKKyAgICB2aXJ0dWFsIHZvaWQgcmVwbGFjZU1pc3NwZWxsZWRSYW5n ZShjb25zdCBXZWJTdHJpbmcmKSA9IDA7CiAKICAgICAvLyBTZWxlY3Rpb24gLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0KIApJbmRleDog U291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvV2ViRnJhbWVJbXBsLmNwcAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJGcmFtZUltcGwuY3BwCShyZXZpc2lvbiAx MTk3ODUpCisrKyBTb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJGcmFtZUltcGwuY3BwCSh3 b3JraW5nIGNvcHkpCkBAIC0xMzQ1LDYgKzEzNDUsMjQgQEAgdm9pZCBXZWJGcmFtZUltcGw6OnJl cXVlc3RUZXh0Q2hlY2tpbmcoYwogICAgIGZyYW1lKCktPmVkaXRvcigpLT5zcGVsbENoZWNrZXIo KS0+cmVxdWVzdENoZWNraW5nRm9yKFNwZWxsQ2hlY2tSZXF1ZXN0OjpjcmVhdGUoVGV4dENoZWNr aW5nVHlwZVNwZWxsaW5nIHwgVGV4dENoZWNraW5nVHlwZUdyYW1tYXIsIFRleHRDaGVja2luZ1By b2Nlc3NCYXRjaCwgcmFuZ2VUb0NoZWNrLCByYW5nZVRvQ2hlY2spKTsKIH0KIAordm9pZCBXZWJG cmFtZUltcGw6OnJlcGxhY2VNaXNzcGVsbGVkUmFuZ2UoY29uc3QgV2ViU3RyaW5nJiB0ZXh0KQor eworICAgIC8vIElmIHRoaXMgY2FyZXQgc2VsZWN0aW9uIGhhcyB0d28gb3IgbW9yZSBtYXJrZXJz LCB0aGlzIGZ1bmN0aW9uIHJlcGxhY2UgdGhlIHJhbmdlIGNvdmVyZWQgYnkgdGhlIGZpcnN0IG1h cmtlciB3aXRoIHRoZSBzcGVjaWZpZWQgd29yZCBhcyBNaWNyb3NvZnQgV29yZCBkb2VzLgorICAg IGlmIChwbHVnaW5Db250YWluZXJGcm9tRnJhbWUoZnJhbWUoKSkpCisgICAgICAgIHJldHVybjsK KyAgICBSZWZQdHI8UmFuZ2U+IGNhcmV0UmFuZ2UgPSBmcmFtZSgpLT5zZWxlY3Rpb24oKS0+dG9O b3JtYWxpemVkUmFuZ2UoKTsKKyAgICBpZiAoIWNhcmV0UmFuZ2UpCisgICAgICAgIHJldHVybjsK KyAgICBWZWN0b3I8RG9jdW1lbnRNYXJrZXIqPiBtYXJrZXJzID0gZnJhbWUoKS0+ZG9jdW1lbnQo KS0+bWFya2VycygpLT5tYXJrZXJzSW5SYW5nZShjYXJldFJhbmdlLmdldCgpLCBEb2N1bWVudE1h cmtlcjo6U3BlbGxpbmcgfCBEb2N1bWVudE1hcmtlcjo6R3JhbW1hcik7CisgICAgaWYgKG1hcmtl cnMuc2l6ZSgpIDwgMSB8fCBtYXJrZXJzWzBdLT5zdGFydE9mZnNldCgpID49IG1hcmtlcnNbMF0t PmVuZE9mZnNldCgpKQorICAgICAgICByZXR1cm47CisgICAgUmVmUHRyPFJhbmdlPiBtYXJrZXJS YW5nZSA9IFRleHRJdGVyYXRvcjo6cmFuZ2VGcm9tTG9jYXRpb25BbmRMZW5ndGgoZnJhbWUoKS0+ c2VsZWN0aW9uKCktPnJvb3RFZGl0YWJsZUVsZW1lbnRPckRvY3VtZW50RWxlbWVudCgpLCBtYXJr ZXJzWzBdLT5zdGFydE9mZnNldCgpLCBtYXJrZXJzWzBdLT5lbmRPZmZzZXQoKSAtIG1hcmtlcnNb MF0tPnN0YXJ0T2Zmc2V0KCkpOworICAgIGlmICghbWFya2VyUmFuZ2UuZ2V0KCkgfHwgIWZyYW1l KCktPnNlbGVjdGlvbigpLT5zaG91bGRDaGFuZ2VTZWxlY3Rpb24obWFya2VyUmFuZ2UuZ2V0KCkp KQorICAgICAgICByZXR1cm47CisgICAgZnJhbWUoKS0+c2VsZWN0aW9uKCktPnNldFNlbGVjdGlv bihtYXJrZXJSYW5nZS5nZXQoKSwgQ2hhcmFjdGVyR3JhbnVsYXJpdHkpOworICAgIGZyYW1lKCkt PmVkaXRvcigpLT5yZXBsYWNlU2VsZWN0aW9uV2l0aFRleHQodGV4dCwgZmFsc2UsIHRydWUpOwor fQorCiBib29sIFdlYkZyYW1lSW1wbDo6aGFzU2VsZWN0aW9uKCkgY29uc3QKIHsKICAgICBXZWJQ bHVnaW5Db250YWluZXJJbXBsKiBwbHVnaW5Db250YWluZXIgPSBwbHVnaW5Db250YWluZXJGcm9t RnJhbWUoZnJhbWUoKSk7CkluZGV4OiBTb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJGcmFt ZUltcGwuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJGcmFt ZUltcGwuaAkocmV2aXNpb24gMTE5Nzg1KQorKysgU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMv V2ViRnJhbWVJbXBsLmgJKHdvcmtpbmcgY29weSkKQEAgLTE3MSw2ICsxNzEsNyBAQCBwdWJsaWM6 CiAgICAgdmlydHVhbCB2b2lkIGVuYWJsZUNvbnRpbnVvdXNTcGVsbENoZWNraW5nKGJvb2wpOwog ICAgIHZpcnR1YWwgYm9vbCBpc0NvbnRpbnVvdXNTcGVsbENoZWNraW5nRW5hYmxlZCgpIGNvbnN0 OwogICAgIHZpcnR1YWwgdm9pZCByZXF1ZXN0VGV4dENoZWNraW5nKGNvbnN0IFdlYkVsZW1lbnQm KTsKKyAgICB2aXJ0dWFsIHZvaWQgcmVwbGFjZU1pc3NwZWxsZWRSYW5nZShjb25zdCBXZWJTdHJp bmcmKTsKICAgICB2aXJ0dWFsIGJvb2wgaGFzU2VsZWN0aW9uKCkgY29uc3Q7CiAgICAgdmlydHVh bCBXZWJSYW5nZSBzZWxlY3Rpb25SYW5nZSgpIGNvbnN0OwogICAgIHZpcnR1YWwgV2ViU3RyaW5n IHNlbGVjdGlvbkFzVGV4dCgpIGNvbnN0Owo=