Bug 55396 - [Mac] Make "Change back to …" contextual menu item work with new autocorrection.
Summary: [Mac] Make "Change back to …" contextual menu item work with new autocorrection.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-02-28 12:18 PST by Jia Pu
Modified: 2011-02-28 15:50 PST (History)
1 user (show)

See Also:


Attachments
Proposed patch (v1) (3.61 KB, patch)
2011-02-28 12:53 PST, Jia Pu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jia Pu 2011-02-28 12:18:24 PST
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>
Comment 1 Jia Pu 2011-02-28 12:53:41 PST
Created attachment 84102 [details]
Proposed patch (v1)
Comment 2 Darin Adler 2011-02-28 13:52:23 PST
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.
Comment 3 Jia Pu 2011-02-28 14:22:50 PST
(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.
Comment 4 WebKit Commit Bot 2011-02-28 15:50:43 PST
Comment on attachment 84102 [details]
Proposed patch (v1)

Clearing flags on attachment: 84102

Committed r79936: <http://trac.webkit.org/changeset/79936>
Comment 5 WebKit Commit Bot 2011-02-28 15:50:47 PST
All reviewed patches have been landed.  Closing bug.