Bug 166450 – NSSpellChecker's recordResponse isn't called for unseen automatic corrections

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

NSSpellChecker's recordResponse isn't called for unseen automatic corrections

[Tim Horton]

Created attachment 297691 [details]
Patch

[Tim Horton]

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.

[Tim Horton]

<rdar://problem/29447824>

[WebKit Commit Bot]

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.

[Darin Adler]

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.

[Tim Horton]

Created attachment 297928 [details]
Patch

[Darin Adler]

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?

[Tim Horton]

(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?

[Tim Horton]

(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.

[Tim Horton]

Created attachment 297954 [details]
Patch

[Darin Adler]

(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.

[Tim Horton]

(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.

[Tim Horton]

Created attachment 297979 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 297979 [details]
Patch

Clearing flags on attachment: 297979

Committed r210266: <http://trac.webkit.org/changeset/210266>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(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.

[Csaba Osztrogonác]

(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