Bug 155532 – Provide NSSpellChecker spellChecking methods with the current insertion point

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Beth Dakin]

Provide NSSpellChecker spellChecking methods with the current insertion point

rdar://problem/24066952

[Beth Dakin]

Created attachment 274172 [details]
Patch

[WebKit Commit Bot]

Attachment 274172 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:1102:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

Created attachment 274203 [details]
Patch

[Beth Dakin]

Created attachment 274206 [details]
Patch

[Beth Dakin]

Created attachment 274208 [details]
Patch

[Beth Dakin]

Created attachment 274216 [details]
Patch

[Darin Adler]

Comment on attachment 274216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274216&action=review

Since text checker is defined in terms of string views, not the DOM, seems the right architecture would be to convert the selection to offsets within the string at the call sites, rather than actually passing the VisibleSelection object down. Is there a straightforward way to do it that way instead?

> Source/WebCore/ChangeLog:10
> +        Pass the Frameâs section to a handful of spelling checking methods that call 

Frame's selection, not Frame's section

[Simon Fraser (smfr)]

Comment on attachment 274216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274216&action=review

>> Source/WebCore/ChangeLog:10
>> +        Pass the Frameâs section to a handful of spelling checking methods that call 
> 
> Frame's selection, not Frame's section

Is there any way to test this? API test perhaps?

> Source/WebKit2/UIProcess/TextChecker.h:75
> +    static Vector<WebCore::TextCheckingResult> checkTextOfParagraph(int64_t spellDocumentTag, StringView text, uint64_t insertionPoint, uint64_t checkingTypes);

I find it a bit odd that the insertionPoint is a uint64_t. Does WebCore handle 64-bit offsets?

> Source/WebKit2/UIProcess/mac/TextCheckerMac.mm:301
> +    options = [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithUnsignedInteger:insertionPoint], NSTextCheckingInsertionPointKey, nil];

You should use literals here: @{} etc.

> Source/WebKit2/UIProcess/mac/TextCheckerMac.mm:440
> +    options = [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithUnsignedInteger:insertionPoint], NSTextCheckingInsertionPointKey, nil];

Literals.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:1066
> +    options = [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithUnsignedInteger:insertionPointFromCurrentSelection(currentSelection)], NSTextCheckingInsertionPointKey, nil];

literals.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:1112
> +    options = [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithUnsignedInteger:insertionPointFromCurrentSelection(currentSelection)], NSTextCheckingInsertionPointKey, nil];

ditto.

> Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:1300
> +    options = [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithUnsignedInteger:insertionPointFromCurrentSelection(currentSelection)], NSTextCheckingInsertionPointKey, nil];

ditto. Maybe we should just wrap -checkString and -requestCheckingOfString: to avoid the repetition?

[Beth Dakin]

(In reply to comment #8)
> Comment on attachment 274216 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274216&action=review
> 
> >> Source/WebCore/ChangeLog:10
> >> +        Pass the Frameâs section to a handful of spelling checking methods that call 
> > 
> > Frame's selection, not Frame's section
> 
> Is there any way to test this? API test perhaps?
> 

Hmmmmmm. I don't think so, but I will give this more thought.

> > Source/WebKit2/UIProcess/TextChecker.h:75
> > +    static Vector<WebCore::TextCheckingResult> checkTextOfParagraph(int64_t spellDocumentTag, StringView text, uint64_t insertionPoint, uint64_t checkingTypes);
> 
> I find it a bit odd that the insertionPoint is a uint64_t. Does WebCore
> handle 64-bit offsets?
> 

I think you're right, and int_32t would be more appropriate here given the underlying WebCore types.

> > Source/WebKit2/UIProcess/mac/TextCheckerMac.mm:301
> > +    options = [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithUnsignedInteger:insertionPoint], NSTextCheckingInsertionPointKey, nil];
> 
> You should use literals here: @{} etc.
> 

Okay!

> > Source/WebKit2/UIProcess/mac/TextCheckerMac.mm:440
> > +    options = [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithUnsignedInteger:insertionPoint], NSTextCheckingInsertionPointKey, nil];
> 
> Literals.
> 

Will fix.

> > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:1066
> > +    options = [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithUnsignedInteger:insertionPointFromCurrentSelection(currentSelection)], NSTextCheckingInsertionPointKey, nil];
> 
> literals.
> 

Will fix.

> > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:1112
> > +    options = [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithUnsignedInteger:insertionPointFromCurrentSelection(currentSelection)], NSTextCheckingInsertionPointKey, nil];
> 
> ditto.
> 

Will fix.

> > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:1300
> > +    options = [NSDictionary dictionaryWithObjectsAndKeys:[NSNumber numberWithUnsignedInteger:insertionPointFromCurrentSelection(currentSelection)], NSTextCheckingInsertionPointKey, nil];
> 
> ditto. Maybe we should just wrap -checkString and -requestCheckingOfString:
> to avoid the repetition?

Hmm, I'm not sure I follow. How would this help?

[Beth Dakin]

(In reply to comment #7)
> Comment on attachment 274216 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274216&action=review
> 
> Since text checker is defined in terms of string views, not the DOM, seems
> the right architecture would be to convert the selection to offsets within
> the string at the call sites, rather than actually passing the
> VisibleSelection object down. Is there a straightforward way to do it that
> way instead?
> 

I chose to pass a VisibleSelection around WebCore, and then convert it to the offset up in WebKit (we end up passing around the offset on its own in WK2 to get it to the UIProcess) because there has been talk of possibly adding some additional keys to this NSSpellChecker dictionary, and that would cover information that could also be extracted from the VisibleSelection. So I chose to do it this way because I thought it would pave the way for some more straightforward patches in the future.

That being said, I do not have a great understanding of any architectural decisions guiding these WebCore classes. So if you think it's a better/cleaner architecture here to stick to the offset, then I would be happy to do that. Do you think that's better even if I add some additional parameters in the future?

[Beth Dakin]

http://trac.webkit.org/changeset/198306 I will follow-up with Darin to see if a follow-up patch is in order.

[Darin Adler]

(In reply to comment #10)
> That being said, I do not have a great understanding of any architectural
> decisions guiding these WebCore classes. So if you think it's a
> better/cleaner architecture here to stick to the offset, then I would be
> happy to do that. Do you think that's better even if I add some additional
> parameters in the future?

This is really a layering question.

Before this change, these functions were an abstract interface to spell checking that was independent of the DOM. After the change, they are hooks for spell checking that are not independent of the DOM.

I don’t have a strong feeling about which design is better, but I don’t like place we have landed with the division of responsibilities where extracting the text to spell check is a job for the caller, but interpreting the selection position is a job for the spell checking functions. I’d prefer that the work of extracting the text to spell check and the other contextual information needed to correctly check from the DOM was either done entirely by the caller or entirely by the spell checking functions.

This is not an urgent issue but nice to resolve in the longer term. We can talk in person about this at some point.

[Beth Dakin]

(In reply to comment #12)
> (In reply to comment #10)
> > That being said, I do not have a great understanding of any architectural
> > decisions guiding these WebCore classes. So if you think it's a
> > better/cleaner architecture here to stick to the offset, then I would be
> > happy to do that. Do you think that's better even if I add some additional
> > parameters in the future?
> 
> This is really a layering question.
> 
> Before this change, these functions were an abstract interface to spell
> checking that was independent of the DOM. After the change, they are hooks
> for spell checking that are not independent of the DOM.
> 
> I don’t have a strong feeling about which design is better, but I don’t like
> place we have landed with the division of responsibilities where extracting
> the text to spell check is a job for the caller, but interpreting the
> selection position is a job for the spell checking functions. I’d prefer
> that the work of extracting the text to spell check and the other contextual
> information needed to correctly check from the DOM was either done entirely
> by the caller or entirely by the spell checking functions.
> 
> This is not an urgent issue but nice to resolve in the longer term. We can
> talk in person about this at some point.

Sounds good! I'm happy to get this in a better place. Thanks for your input!

[Beth Dakin]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > That being said, I do not have a great understanding of any architectural
> > > decisions guiding these WebCore classes. So if you think it's a
> > > better/cleaner architecture here to stick to the offset, then I would be
> > > happy to do that. Do you think that's better even if I add some additional
> > > parameters in the future?
> > 
> > This is really a layering question.
> > 
> > Before this change, these functions were an abstract interface to spell
> > checking that was independent of the DOM. After the change, they are hooks
> > for spell checking that are not independent of the DOM.
> > 
> > I don’t have a strong feeling about which design is better, but I don’t like
> > place we have landed with the division of responsibilities where extracting
> > the text to spell check is a job for the caller, but interpreting the
> > selection position is a job for the spell checking functions. I’d prefer
> > that the work of extracting the text to spell check and the other contextual
> > information needed to correctly check from the DOM was either done entirely
> > by the caller or entirely by the spell checking functions.
> > 
> > This is not an urgent issue but nice to resolve in the longer term. We can
> > talk in person about this at some point.
> 
> Sounds good! I'm happy to get this in a better place. Thanks for your input!

I posted a patch to address this: https://bugs.webkit.org/show_bug.cgi?id=155908