[Webkit-unassigned] [Bug 40092] spellcheck does not check pasted text

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=40092





--- Comment #40 from MORITA Hajime <morrita at google.com>  2010-10-21 06:00:24 PST ---
Eric and Everybody, thanks for giving a review!
I update the patch to address Eric's point.

> > WebCore/dom/DocumentMarkerController.h:77
> > +void showDocumentMarkers(const WebCore::DocumentMarkerController*);
> 
> Why is this outside of WebCore?
It follows RenderObject.cpp:showRenderTree() style. 
The function is designed to be called from gdb, so be in global namespace matters.

> 
> > WebCore/editing/Editor.cpp:3663
> > +static Node* findFirstMarkable(Node* node)
> 
> We didn't have this logic before?
Unfortunately, no. Other part of spellchecking is immediately done once
It finds a candidate to mark. So they need no extra traversing. 
But we need it because we mark them later.

> 
> > WebCore/editing/SpellChecker.h:35
> > +struct SpellCheckingResult {
> 
> Structs rarely stay structs for long...
Agreed. Made it to a class.

> 
> > WebCore/editing/SpellChecker.h:48
> > +class SpellChecker {
> 
> Seems this should inherit from NonCopyable.
Done.

> 
> > WebCore/editing/SpellChecker.h:53
> > +    bool isAsynchronousEnabled() const;
> 
> is "Enabled" needed here?
I think so. It is asking EditorClient to the setting.

> 
> > WebCore/editing/SpellChecker.h:54
> > +    bool isAsynchronousAvailableFor(Node*) const;
> 
> canCheckAsynchronously(node)?  I'm not sure what this function does so my naming suggestions may make little sense.
Done. I agree that brief name is preferable.

> 
> > WebCore/editing/SpellChecker.h:57
> > +    bool isCheckeable(Node* node) const;
> 
> isCheckable?  Also, "node" is not needed here.
Done.

> 
> > WebKit/mac/WebCoreSupport/WebEditorClient.mm:954
> > +    RetainPtr<NSArray> _results;
> 
> Seems a bit odd, but I guess this is Objective c++?  Are destructors called for obj-c++?  Isn't there a new fancy obj-c2 way to do autoretain?
It looks OK, as Darin mentioned.

> 
> > WebKit/mac/WebCoreSupport/WebEditorClient.mm:978
> > +- (WTF::Vector<WebCore::SpellCheckingResult>) _coreResults
> 
> Extra space "lt>) _coreR"
Done.

> 
> > WebKit/mac/WebCoreSupport/WebEditorClient.mm:995
> > +    for (size_t i = 0; i < [results count]; ++i) {
> > +        NSTextCheckingType type = [[results objectAtIndex:i] resultType];
> > +        if (type & NSTextCheckingTypeSpelling)
> > +            coreResults[i].type = DocumentMarker::Spelling;
> > +        else if (type & NSTextCheckingTypeGrammar)
> > +            coreResults[i].type = DocumentMarker::Grammar;
> > +        else
> > +            coreResults[i].type = DocumentMarker::AllMarkers;
> > +
> > +        NSRange range = [[results objectAtIndex:i] range];
> > +        coreResults[i].location = range.location;
> > +        coreResults[i].length = range.length;
> > +    }
> 
> This feels like a function passed to a map.  Sadly I don't think obj-c has list comprehensions.
Agreed but there is no map() from NSArray to WTF::Vector.
So I factored out the logic  to make it more map()-like feeling.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.