[webkit-reviews] review granted: [Bug 55396] [Mac] Make "=?UTF-8?Q?Change=20back=20to=20=E2=80=A6?=" contextual menu item work with new autocorrection. : [Attachment 84102] Proposed patch (v1)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted jpu at apple.com's request for review:
Bug 55396: [Mac] Make "Change back to …" contextual menu item work with new
autocorrection.
https://bugs.webkit.org/show_bug.cgi?id=55396

Attachment 84102: Proposed patch (v1)
https://bugs.webkit.org/attachment.cgi?id=84102&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=84102&action=review

> Source/WebCore/editing/Editor.cpp:2496
> +    String replacement = plainText(selection.get());
> +   
client()->recordAutocorrectionResponse(EditorClient::AutocorrectionReverted,
replacedString, replacement);

You could write this without the local variable. The local variable does
provide a bit of documentation, so I guess it could go either way.

> Source/WebCore/rendering/InlineTextBox.cpp:1098
> +	       case DocumentMarker::Replacement:
> +		   computeRectForReplacementMarker(marker, style, font);
> +		   break;

Is there a way to test this?

> Source/WebCore/rendering/InlineTextBox.cpp:1100
>	       default:
>		   ASSERT_NOT_REACHED();

If this default wasn’t here, then the compiler would have warned about the fact
that the enum value was not handled, so we might have spotted this earlier.
Generally speaking, it’s best to structure things so that switches based on
enums do not have a default case. Not sure how easy that would be in this case,
without sacrificing the “bad enum value” assertion.