RESOLVED FIXED 83047
Mark text with text alternative with blue underline.
https://bugs.webkit.org/show_bug.cgi?id=83047
Summary Mark text with text alternative with blue underline.
Jia Pu
Reported 2012-04-03 10:57:33 PDT
This is part of the changes for bug 82503. <rdar://problem/11175647>
Attachments
Patch (v1) (95.62 KB, patch)
2012-04-16 17:13 PDT, Jia Pu
no flags
Patch (v2) (90.70 KB, patch)
2012-04-16 19:44 PDT, Jia Pu
no flags
Patch (v3) (90.70 KB, patch)
2012-04-16 20:48 PDT, Jia Pu
no flags
Moved code shared by WebKit and WebKit2 into WebCore. (75.97 KB, patch)
2012-04-18 19:38 PDT, Jia Pu
no flags
Minor Adjustment (76.13 KB, patch)
2012-04-18 20:04 PDT, Jia Pu
no flags
Fix build. (78.00 KB, patch)
2012-04-18 21:28 PDT, Jia Pu
no flags
Patch (v1). (128.25 KB, patch)
2012-06-11 17:42 PDT, Jia Pu
no flags
Patch (v2). (128.18 KB, patch)
2012-06-11 19:37 PDT, Jia Pu
no flags
Fixing Lion build. (128.26 KB, patch)
2012-06-13 00:27 PDT, Jia Pu
no flags
Fixing Lion build. (129.31 KB, patch)
2012-06-13 10:09 PDT, Jia Pu
no flags
Remove block code to make gcc happy. (128.99 KB, patch)
2012-06-13 18:44 PDT, Jia Pu
no flags
Update to TOT. (128.95 KB, patch)
2012-06-13 21:00 PDT, Jia Pu
enrica: review+
Jia Pu
Comment 1 2012-04-16 17:13:18 PDT
Created attachment 137438 [details] Patch (v1)
Build Bot
Comment 2 2012-04-16 17:33:07 PDT
Comment on attachment 137438 [details] Patch (v1) Attachment 137438 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12415473
Early Warning System Bot
Comment 3 2012-04-16 17:45:50 PDT
Comment on attachment 137438 [details] Patch (v1) Attachment 137438 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12412656
Build Bot
Comment 4 2012-04-16 17:47:04 PDT
Comment on attachment 137438 [details] Patch (v1) Attachment 137438 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12410953
Philippe Normand
Comment 5 2012-04-16 18:00:12 PDT
Comment on attachment 137438 [details] Patch (v1) Attachment 137438 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12412668
Jia Pu
Comment 6 2012-04-16 19:44:44 PDT
Created attachment 137464 [details] Patch (v2)
Jia Pu
Comment 7 2012-04-16 20:48:55 PDT
Created attachment 137472 [details] Patch (v3)
Enrica Casucci
Comment 8 2012-04-18 11:50:49 PDT
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?
Jia Pu
Comment 9 2012-04-18 19:38:55 PDT
Created attachment 137822 [details] Moved code shared by WebKit and WebKit2 into WebCore.
Jia Pu
Comment 10 2012-04-18 19:40:30 PDT
(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.
Jia Pu
Comment 11 2012-04-18 20:04:13 PDT
Created attachment 137825 [details] Minor Adjustment
Build Bot
Comment 12 2012-04-18 20:13:17 PDT
Comment on attachment 137825 [details] Minor Adjustment Attachment 137825 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12297189
Jia Pu
Comment 13 2012-04-18 21:28:50 PDT
Created attachment 137834 [details] Fix build.
Jia Pu
Comment 14 2012-06-11 17:42:54 PDT
Created attachment 146978 [details] Patch (v1).
Jia Pu
Comment 15 2012-06-11 19:37:52 PDT
Created attachment 146995 [details] Patch (v2).
Build Bot
Comment 16 2012-06-11 20:03:49 PDT
Comment on attachment 146995 [details] Patch (v2). Attachment 146995 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12944397
Jia Pu
Comment 17 2012-06-13 00:27:51 PDT
Created attachment 147251 [details] Fixing Lion build.
Build Bot
Comment 18 2012-06-13 00:50:27 PDT
Comment on attachment 147251 [details] Fixing Lion build. Attachment 147251 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12943819
Jia Pu
Comment 19 2012-06-13 10:09:12 PDT
Created attachment 147351 [details] Fixing Lion build.
Build Bot
Comment 20 2012-06-13 10:33:38 PDT
Comment on attachment 147351 [details] Fixing Lion build. Attachment 147351 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12950404
Jia Pu
Comment 21 2012-06-13 18:44:25 PDT
Created attachment 147463 [details] Remove block code to make gcc happy.
Jia Pu
Comment 22 2012-06-13 21:00:27 PDT
Created attachment 147479 [details] Update to TOT.
Enrica Casucci
Comment 23 2012-06-14 12:13:35 PDT
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.
Jia Pu
Comment 24 2012-06-14 13:25:06 PDT
Alexey Proskuryakov
Comment 25 2012-06-15 12:13:09 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.