[webkit-reviews] review denied: [Bug 82503] Provide UI to show alternative strings of dictated text. : [Attachment 134616] Patch (v4)

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Jia Pu <jpu at apple.com>'s request for
review:
Bug 82503: Provide UI to show alternative strings of dictated text.
https://bugs.webkit.org/show_bug.cgi?id=82503

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=134616&action=review


r- due to style and build EWS failures. You should also restructure this code
so that it doesn't add so much code.

> Source/WebCore/ChangeLog:16
> +	   3. Introduced DictationCommand which will add DictationAlternative
markers to text
> +	      inserted by the command.

It seems like this class is unnecessary. We probably want to add a new flag or
something to InsertTextCommand instead.

> Source/WebCore/editing/AlternativeTextController.cpp:76
> +    VisibleSelection currentSelection(m_frame->selection()->selection());
> +    if (!currentSelection.isCaret() || currentSelection == oldSelection)
> +	   return;
> +    
> +    VisiblePosition selectionPosition = currentSelection.start();
> +    // Creating a Visible position triggers a layout and there is no
> +    // guarantee that the selection is still valid.
> +    if (selectionPosition.isNull())
> +	   return;
> +    
> +    VisiblePosition endPositionOfWord = endOfWord(selectionPosition,
LeftWordIfOnBoundary);
> +    if (selectionPosition != endPositionOfWord)
> +	   return;
> +    
> +    Position position = endPositionOfWord.deepEquivalent();
> +    if (position.anchorType() != Position::PositionIsOffsetInAnchor)
> +	   return;

This chunk of code is duplicated in
SpellingCorrectionController::respondToChangedSelection.
We should at least share some code with SpellingCorrectionController.
I'm not convinced at all that we need this new class for just supporting
dictation alternatives.
Can't we generalize SpellingCorrectionController to support dictations as well?


> Source/WebCore/editing/AlternativeTextController.cpp:129
> +PassRefPtr<Range> replaceTextInRange(const String& newText, Range* range,
Frame* frame, bool forSpellingCorrection)
> +{

This function probably belongs in Editor.cpp

> Source/WebKit/mac/WebView/WebHTMLView.mm:5975
> +#if USE(DICTATION_ALTERNATIVES)
> +	   if (dictationAlternatives.size())
> +	       eventHandled = [[self _webView] _insertDictatedText:eventText
alternatives:dictationAlternatives triggeringEvent:event];
> +	   else
> +#endif
> +	       eventHandled = coreFrame->editor()->insertText(eventText,
event);

You should obtain the list of dictation alternatives here, and then add some
optional argument to insertText.