Bug 55396 – [Mac] Make "Change back to …" contextual menu item work with new autocorrection.

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

[Jia Pu]

On the version of Mac OS X that uses autocorrection panel, choose "Change back to … " should notify spell checker service about this reversion.

<rdar://problem/8836093>

[Jia Pu]

Created attachment 84102 [details]
Proposed patch (v1)

[Darin Adler]

Comment on attachment 84102 [details]
Proposed patch (v1)

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.

[Jia Pu]

(In reply to comment #2)
> (From update of attachment 84102 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84102&action=review
> 
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:1098
> > +            case DocumentMarker::Replacement:
> > +                computeRectForReplacementMarker(marker, style, font);
> > +                break;
> 
> Is there a way to test this?
> 

It doesn't seem very easy to test. On low level, current javascript function tests only the existence of the marker, not any detail about the marker. On higher level, I'm not aware of ways to exam if a particular item is in the contextual menu.

[WebKit Commit Bot]

Comment on attachment 84102 [details]
Proposed patch (v1)

Clearing flags on attachment: 84102

Committed r79936: <http://trac.webkit.org/changeset/79936>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.