Bug 83047 – Mark text with text alternative with blue underline.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jia Pu]

This is part of the changes for bug 82503.

<rdar://problem/11175647>

[Jia Pu]

Created attachment 137438 [details]
Patch (v1)

[Build Bot]

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 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 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 on attachment 137438 [details]
Patch (v1)

Attachment 137438 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12412668

[Jia Pu]

Created attachment 137464 [details]
Patch (v2)

[Jia Pu]

Created attachment 137472 [details]
Patch (v3)

[Enrica Casucci]

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]

Created attachment 137822 [details]
Moved code shared by WebKit and WebKit2 into WebCore.

[Jia Pu]

(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]

Created attachment 137825 [details]
Minor Adjustment

[Build Bot]

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]

Created attachment 137834 [details]
Fix build.

[Jia Pu]

Created attachment 146978 [details]
Patch (v1).

[Jia Pu]

Created attachment 146995 [details]
Patch (v2).

[Build Bot]

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]

Created attachment 147251 [details]
Fixing Lion build.

[Build Bot]

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]

Created attachment 147351 [details]
Fixing Lion build.

[Build Bot]

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]

Created attachment 147463 [details]
Remove block code to make gcc happy.

[Jia Pu]

Created attachment 147479 [details]
Update to TOT.

[Enrica Casucci]

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]

Landed manually: http://trac.webkit.org/changeset/120357

[Alexey Proskuryakov]

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.