NSSpellChecker's recordResponse isn't called for unseen automatic corrections
Created attachment 297691 [details] Patch
Still have to decide how to test this. And make sure that the new call site is actually right. It *works* (and we don't get double-corrections with seen corrections), but this code is fairly mysterious.
<rdar://problem/29447824>
Attachment 297691 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 297691 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297691&action=review > Source/WebCore/editing/Editor.cpp:2585 > + m_alternativeTextController->markCorrection(replacementRange, replacedString); Should be WTFMove(replacementRange) here.
Created attachment 297928 [details] Patch
Comment on attachment 297928 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297928&action=review r=me after you fix iOS and Windows builds > Source/WebCore/editing/AlternativeTextController.cpp:521 > if (AlternativeTextClient* client = alternativeTextClient()) Should use auto*. > Source/WebCore/editing/Editor.cpp:2581 > + RefPtr<Range> replacementRange = paragraph.subrange(resultLocation, replacement.length()); How about auto here instead of writing out RefPtr? > Source/WebCore/page/AlternativeTextClient.h:57 > enum AutocorrectionResponseType { > AutocorrectionEdited, > - AutocorrectionReverted > + AutocorrectionReverted, > + AutocorrectionAccepted > }; Seems like this should be an enum class some day. > Source/WebKit/mac/WebCoreSupport/WebAlternativeTextClient.mm:65 > +static NSCorrectionResponse toCorrectionResponse(AutocorrectionResponseType responseType) inline Also seems unfortunate we have to repeat this twice. Is there some way to share this between WebKit 1 and 2? Maybe a handy place to put this in the platform directory? > Source/WebKit2/UIProcess/mac/PageClientImpl.mm:571 > +static NSCorrectionResponse toCorrectionResponse(AutocorrectionResponseType responseType) inline > Tools/DumpRenderTree/TestRunner.cpp:1785 > + if (argumentCount != 1) I would say < 1; not sure we should make a habit of silently doing nothing when we get an extra argument. Except this is test code so no big deal either way I guess. > Tools/DumpRenderTree/mac/DumpRenderTreeSpellChecker.mm:66 > + static BOOL hasSwizzled; A little funny to use BOOL here. > LayoutTests/ChangeLog:14 > + * editing/mac/spelling/accept-unseen-candidate-records-acceptance-expected.txt: Added. Seems like it would be better for this to be a reference test, or at least a "dumpAsText"; not great to land a render tree for this, I think. > LayoutTests/platform/mac-wk2/TestExpectations:176 > +webkit.org/b/105616 editing/mac/spelling/accept-unseen-candidate-records-acceptance.html [ Failure ] Still living in the past?
(In reply to comment #7) > Comment on attachment 297928 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=297928&action=review > > > Source/WebKit/mac/WebCoreSupport/WebAlternativeTextClient.mm:65 > > +static NSCorrectionResponse toCorrectionResponse(AutocorrectionResponseType responseType) > > inline > > Also seems unfortunate we have to repeat this twice. Is there some way to > share this between WebKit 1 and 2? Maybe a handy place to put this in the > platform directory? I'm never quite sure where to put things like this. Without moving other things (the enum) to platform, it can't move there. But maybe that is the right thing to do. > > Tools/DumpRenderTree/mac/DumpRenderTreeSpellChecker.mm:66 > > + static BOOL hasSwizzled; > > A little funny to use BOOL here. Bad habit. > > LayoutTests/ChangeLog:14 > > + * editing/mac/spelling/accept-unseen-candidate-records-acceptance-expected.txt: Added. > > Seems like it would be better for this to be a reference test, or at least a > "dumpAsText"; not great to land a render tree for this, I think. Sure! > > LayoutTests/platform/mac-wk2/TestExpectations:176 > > +webkit.org/b/105616 editing/mac/spelling/accept-unseen-candidate-records-acceptance.html [ Failure ] > > Still living in the past? Hmm? Because spelling tests don't work in WK2, or something else?
(In reply to comment #7) > Comment on attachment 297928 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=297928&action=review > > r=me after you fix iOS and Windows builds > > > Source/WebCore/editing/AlternativeTextController.cpp:521 > > if (AlternativeTextClient* client = alternativeTextClient()) > > Should use auto*. > > > Source/WebCore/editing/Editor.cpp:2581 > > + RefPtr<Range> replacementRange = paragraph.subrange(resultLocation, replacement.length()); > > How about auto here instead of writing out RefPtr? Here's why I don't love auto: this seems like a sensible suggestion, but performing it makes everything crash. Why? Because subrange returns a PassRefPtr, so it evaporates the first time it's passed into a RefPtr. Now, maybe it shouldn't use PassRefPtr (maybe I should even change that in this patch!), but a) I don't know that PassRefPtr is the only instance of this class of problem and b) there's still a lot of PassRefPtr.
Created attachment 297954 [details] Patch
(In reply to comment #9) > Here's why I don't love auto: this seems like a sensible suggestion, but > performing it makes everything crash. Why? Because subrange returns a > PassRefPtr, so it evaporates the first time it's passed into a RefPtr. Now, > maybe it shouldn't use PassRefPtr (maybe I should even change that in this > patch!), but a) I don't know that PassRefPtr is the only instance of this > class of problem and b) there's still a lot of PassRefPtr. Sorry! I am working hard to get rid of PassRefPtr.
(In reply to comment #11) > (In reply to comment #9) > > Here's why I don't love auto: this seems like a sensible suggestion, but > > performing it makes everything crash. Why? Because subrange returns a > > PassRefPtr, so it evaporates the first time it's passed into a RefPtr. Now, > > maybe it shouldn't use PassRefPtr (maybe I should even change that in this > > patch!), but a) I don't know that PassRefPtr is the only instance of this > > class of problem and b) there's still a lot of PassRefPtr. > > Sorry! :D > I am working hard to get rid of PassRefPtr. Is it the only problem? If it's the only similar problem, I'm much more OK with this.
Created attachment 297979 [details] Patch
Comment on attachment 297979 [details] Patch Clearing flags on attachment: 297979 Committed r210266: <http://trac.webkit.org/changeset/210266>
All reviewed patches have been landed. Closing bug.
(In reply to comment #12) > > I am working hard to get rid of PassRefPtr. > > Is it the only problem? If it's the only similar problem, I'm much more OK > with this. I’m not aware of another. The uses of rvalue references to achieve move semantics that are the modern approach that replace PassRefPtr don’t have this issue, for example.
(In reply to comment #14) > Comment on attachment 297979 [details] > Patch > > Clearing flags on attachment: 297979 > > Committed r210266: <http://trac.webkit.org/changeset/210266> And the cmake buildfix landed in https://trac.webkit.org/changeset/210274