[webkit-reviews] review denied: [Bug 40092] spellcheck does not check pasted text : [Attachment 70599] Fixing Qt WebKit2 build

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied MORITA Hajime <morrita at google.com>'s
request for review:
Bug 40092: spellcheck does not check pasted text
https://bugs.webkit.org/show_bug.cgi?id=40092

Attachment 70599: Fixing Qt WebKit2 build
https://bugs.webkit.org/attachment.cgi?id=70599&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=70599&action=review

This generally appears sane.  It would be nice if Doug or one of the Apple
folks could comment on the Obj-C.  r- for the nits.

> WebCore/dom/DocumentMarkerController.h:77
> +void showDocumentMarkers(const WebCore::DocumentMarkerController*);

Why is this outside of WebCore?

> WebCore/editing/Editor.cpp:3663
> +static Node* findFirstMarkable(Node* node)

We didn't have this logic before?

> WebCore/editing/SpellChecker.h:35
> +struct SpellCheckingResult {

Structs rarely stay structs for long...

> WebCore/editing/SpellChecker.h:48
> +class SpellChecker {

Seems this should inherit from NonCopyable.

> WebCore/editing/SpellChecker.h:53
> +    bool isAsynchronousEnabled() const;

is "Enabled" needed here?

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

> WebCore/editing/SpellChecker.h:57
> +    bool isCheckeable(Node* node) const;

isCheckable?  Also, "node" is not needed here.

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

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:978
> +- (WTF::Vector<WebCore::SpellCheckingResult>) _coreResults

Extra space "lt>) _coreR"

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