Bug 108511 – [Chromium] Spelling should ignore punctuation

• Component: HTML Editing
• Status: RESOLVED FIXED

[Rouslan Solomakhin]

Punctuation marks should be treated as whitespace: spelling should ignore them. To test manually, type 'wellcome!', select 'wellcome!', and context-click on the selection. The test succeeds when the context menu has spell check suggestions.

[Rouslan Solomakhin]

Created attachment 186092 [details]
Patch

[Tony Chang]

Comment on attachment 186092 [details]
Patch

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

> Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:147
> +// Returns true for whitespace and punctuation characters. Returns false for every other type of character.

Remove this comment.

> Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:150
> +    return WTF::Unicode::isSeparatorSpace(c) || WTF::Unicode::isPunct(c);

The old code did WTF::Unicode::direction(c) == WTF::Unicode::WhiteSpaceNeutral instead of WTF::Unicode::isSeparatorSpace(c).  Is this difference intentional?  Maybe we should #include <wtf/text/StringImpl.h> and use isSpaceOrNewline(c) || WTF::Unicode::isPunct(c) to be more like the old code?

[Rouslan Solomakhin]

Created attachment 186121 [details]
Patch

[Rouslan Solomakhin]

(In reply to comment #2)
> (From update of attachment 186092 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186092&action=review
> 
> > Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:147
> > +// Returns true for whitespace and punctuation characters. Returns false for every other type of character.
> 
> Remove this comment.

Removed. What are the guidelines for function comments?

> > Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:150
> > +    return WTF::Unicode::isSeparatorSpace(c) || WTF::Unicode::isPunct(c);
> 
> The old code did WTF::Unicode::direction(c) == WTF::Unicode::WhiteSpaceNeutral instead of WTF::Unicode::isSeparatorSpace(c).  Is this difference intentional?  Maybe we should #include <wtf/text/StringImpl.h> and use isSpaceOrNewline(c) || WTF::Unicode::isPunct(c) to be more like the old code?

The difference is not intentional. I switched to "isSpaceOrNewLine(c) || WTF::Unicode::isPunct(c)" as you suggested. I did not need to include StringImpl.h.

[Tony Chang]

Comment on attachment 186121 [details]
Patch

Most functions don't have comments in WebKit.  Instead, we try to use descriptive function names.

[WebKit Review Bot]

Comment on attachment 186121 [details]
Patch

Clearing flags on attachment: 186121

Committed r141650: <http://trac.webkit.org/changeset/141650>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.