Summary: | [Mac] Make "Change back to …" contextual menu item work with new autocorrection. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jia Pu <jiapu.mail> | ||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | commit-queue | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Mac (Intel) | ||||||
OS: | OS X 10.6 | ||||||
Attachments: |
|
Description
Jia Pu
2011-02-28 12:18:24 PST
Created attachment 84102 [details]
Proposed patch (v1)
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. (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 on attachment 84102 [details] Proposed patch (v1) Clearing flags on attachment: 84102 Committed r79936: <http://trac.webkit.org/changeset/79936> All reviewed patches have been landed. Closing bug. |