This is part of the changes for bug 82503. <rdar://problem/11175647>
Created attachment 137438 [details] Patch (v1)
Comment on attachment 137438 [details] Patch (v1) Attachment 137438 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12415473
Comment on attachment 137438 [details] Patch (v1) Attachment 137438 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12412656
Comment on attachment 137438 [details] Patch (v1) Attachment 137438 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12410953
Comment on attachment 137438 [details] Patch (v1) Attachment 137438 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12412668
Created attachment 137464 [details] Patch (v2)
Created attachment 137472 [details] Patch (v3)
Comment on attachment 137472 [details] Patch (v3) View in context: https://bugs.webkit.org/attachment.cgi?id=137472&action=review The patch seems correct. What I don't like is all the duplicated code between WebKit1 and WebKit2. You should move all the common code down to WebCore, this is the recommended approach and your patch will be a lot smaller too. R- because of all the duplicated code. > Source/WebCore/ChangeLog:3 > + On OS X, Mark text with dictation alternative with blue underline. lowercase m. > Source/WebCore/ChangeLog:18 > + (WebCore): Remove this line. > Source/WebCore/ChangeLog:20 > + (AlternativeTextController): ditto. > Source/WebCore/ChangeLog:24 > + (AlternativeTextClient): ditto, > Source/WebKit/ChangeLog:3 > + On OS X, Mark text with dictation alternative with blue underline. lower case m. > Source/WebKit/mac/ChangeLog:3 > + On OS X, Mark text with dictation alternative with blue underline. ditto. > Source/WebKit2/ChangeLog:3 > + On OS X, Mark text with dictation alternative with blue underline. lower case. > Source/WebKit2/ChangeLog:15 > + (CoreIPC::::decode): Remove 3 lines above. > Source/WebKit2/ChangeLog:23 > + (WebKit): Remove. > Source/WebKit2/ChangeLog:33 > + (PageClient): remove. > Source/WebKit2/ChangeLog:35 > + (WebKit): ditto. > Source/WebKit2/ChangeLog:38 > + (WebPageProxy): ditto. > Source/WebKit2/ChangeLog:43 > + (WebKit): ditto. > Source/WebKit2/ChangeLog:49 > + (WebKit): ditto. > Source/WebKit2/ChangeLog:66 > + (WebKit): ditto > Source/WebKit2/ChangeLog:72 > + (WebKit): ditto. > Source/WebKit2/ChangeLog:74 > + (WebPage): ditto. > Source/WebKit2/ChangeLog:78 > + (WebKit): ditto. > Source/WebCore/editing/AlternativeTextController.cpp:629 > +#if USE(DICTATION_ALTERNATIVES) Are you sure marker is always valid? > Source/WebKit/mac/WebCoreSupport/WebAlternativeTextClient.mm:78 > + [m_webView _removeDictationAlternatives:dictationContext]; probably need an #else with UNUSED_PARAM(dictationContext) > Source/WebKit2/UIProcess/API/mac/WKView.mm:1174 > +#endif It would have been nice to share this code between WebKit1 and WebKit2. Could you move this down to WebCore and export it?
Created attachment 137822 [details] Moved code shared by WebKit and WebKit2 into WebCore.
(In reply to comment #9) > Created an attachment (id=137822) [details] > Moved code shared by WebKit and WebKit2 into WebCore. This patch avoids having duplicated DictationAlternativeController and TextAlternativeWithRange classes in WebKit and WebKit2.
Created attachment 137825 [details] Minor Adjustment
Comment on attachment 137825 [details] Minor Adjustment Attachment 137825 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12297189
Created attachment 137834 [details] Fix build.
Created attachment 146978 [details] Patch (v1).
Created attachment 146995 [details] Patch (v2).
Comment on attachment 146995 [details] Patch (v2). Attachment 146995 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12944397
Created attachment 147251 [details] Fixing Lion build.
Comment on attachment 147251 [details] Fixing Lion build. Attachment 147251 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12943819
Created attachment 147351 [details] Fixing Lion build.
Comment on attachment 147351 [details] Fixing Lion build. Attachment 147351 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12950404
Created attachment 147463 [details] Remove block code to make gcc happy.
Created attachment 147479 [details] Update to TOT.
Comment on attachment 147479 [details] Update to TOT. View in context: https://bugs.webkit.org/attachment.cgi?id=147479&action=review The patch looks good. I've added a couple of comments that I would like to see fixed before you land it. I'll trust you to fix them before landing. > Source/WebCore/ChangeLog:11 > + This patch implements visual indication on dictated text with alternatives, and providing UI providing-->provides > Source/WebCore/editing/mac/AlternativeTextUIController.mm:95 > + NSSpellChecker* spellChecker = [NSSpellChecker sharedSpellChecker]; no need to use the local variable, use directly [NSSpellChecker sharedSpellChecker] on the line below.
Landed manually: http://trac.webkit.org/changeset/120357
Comment on attachment 147479 [details] Update to TOT. View in context: https://bugs.webkit.org/attachment.cgi?id=147479&action=review > Source/WebCore/editing/FrameSelection.h:121 > ClearTypingStyle = 1 << 2, > SpellCorrectionTriggered = 1 << 3, > DoNotSetFocus = 1 << 4, > + DictationTriggered = 1 << 5 I wish that we didn't have a mix of hight level and low level options here. It's not FrameSelection's job to make policy decisions on why to do when selection change is triggered by spelling or by dictation. > Source/WebCore/page/ContextMenuController.cpp:903 > + bool haveContextMenuItemsForMisspellingOrGrammer = false; "Grammer"? > Source/WebKit2/UIProcess/WebPageProxy.messages.in:288 > + DictationAlternatives(uint64_t dictationContext) -> (Vector<String> alternatives) It's a shame that we need a synchronous call from WebProcess to UIProcess to get data that's only really used in UIProcess to display a context menu.