Bug 83047 - Mark text with text alternative with blue underline.
Summary: Mark text with text alternative with blue underline.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.7
: P2 Normal
Assignee: Jia Pu
URL:
Keywords: InRadar
Depends on: 83549
Blocks: 82503
  Show dependency treegraph
 
Reported: 2012-04-03 10:57 PDT by Jia Pu
Modified: 2012-06-15 12:13 PDT (History)
7 users (show)

See Also:


Attachments
Patch (v1) (95.62 KB, patch)
2012-04-16 17:13 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v2) (90.70 KB, patch)
2012-04-16 19:44 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v3) (90.70 KB, patch)
2012-04-16 20:48 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Moved code shared by WebKit and WebKit2 into WebCore. (75.97 KB, patch)
2012-04-18 19:38 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Minor Adjustment (76.13 KB, patch)
2012-04-18 20:04 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Fix build. (78.00 KB, patch)
2012-04-18 21:28 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v1). (128.25 KB, patch)
2012-06-11 17:42 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Patch (v2). (128.18 KB, patch)
2012-06-11 19:37 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Fixing Lion build. (128.26 KB, patch)
2012-06-13 00:27 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Fixing Lion build. (129.31 KB, patch)
2012-06-13 10:09 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Remove block code to make gcc happy. (128.99 KB, patch)
2012-06-13 18:44 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
Update to TOT. (128.95 KB, patch)
2012-06-13 21:00 PDT, Jia Pu
enrica: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jia Pu 2012-04-03 10:57:33 PDT
This is part of the changes for bug 82503.

<rdar://problem/11175647>
Comment 1 Jia Pu 2012-04-16 17:13:18 PDT
Created attachment 137438 [details]
Patch (v1)
Comment 2 Build Bot 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
Comment 3 Early Warning System Bot 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
Comment 4 Build Bot 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
Comment 5 Philippe Normand 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
Comment 6 Jia Pu 2012-04-16 19:44:44 PDT
Created attachment 137464 [details]
Patch (v2)
Comment 7 Jia Pu 2012-04-16 20:48:55 PDT
Created attachment 137472 [details]
Patch (v3)
Comment 8 Enrica Casucci 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?
Comment 9 Jia Pu 2012-04-18 19:38:55 PDT
Created attachment 137822 [details]
Moved code shared by WebKit and WebKit2 into WebCore.
Comment 10 Jia Pu 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.
Comment 11 Jia Pu 2012-04-18 20:04:13 PDT
Created attachment 137825 [details]
Minor Adjustment
Comment 12 Build Bot 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
Comment 13 Jia Pu 2012-04-18 21:28:50 PDT
Created attachment 137834 [details]
Fix build.
Comment 14 Jia Pu 2012-06-11 17:42:54 PDT
Created attachment 146978 [details]
Patch (v1).
Comment 15 Jia Pu 2012-06-11 19:37:52 PDT
Created attachment 146995 [details]
Patch (v2).
Comment 16 Build Bot 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
Comment 17 Jia Pu 2012-06-13 00:27:51 PDT
Created attachment 147251 [details]
Fixing Lion build.
Comment 18 Build Bot 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
Comment 19 Jia Pu 2012-06-13 10:09:12 PDT
Created attachment 147351 [details]
Fixing Lion build.
Comment 20 Build Bot 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
Comment 21 Jia Pu 2012-06-13 18:44:25 PDT
Created attachment 147463 [details]
Remove block code to make gcc happy.
Comment 22 Jia Pu 2012-06-13 21:00:27 PDT
Created attachment 147479 [details]
Update to TOT.
Comment 23 Enrica Casucci 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.
Comment 24 Jia Pu 2012-06-14 13:25:06 PDT
Landed manually: http://trac.webkit.org/changeset/120357
Comment 25 Alexey Proskuryakov 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.