[webkit-reviews] review denied: [Bug 83047] On OS X, Mark text with dictation alternative with blue underline. : [Attachment 137472] Patch (v3)

[bugzilla-daemon at webkit.org]

Enrica Casucci <enrica at apple.com> has denied Jia Pu <jpu at apple.com>'s request
for review:
Bug 83047: On OS X, Mark text with dictation alternative with blue underline.
https://bugs.webkit.org/show_bug.cgi?id=83047

Attachment 137472: Patch (v3)
https://bugs.webkit.org/attachment.cgi?id=137472&action=review

------- Additional Comments from Enrica Casucci <enrica at apple.com>
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?