[webkit-reviews] review granted: [Bug 59693] [Feature Request] Need SpellCheck API : [Attachment 95413] Patch v4 (only for Chromium)

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Hironori Bono
<hbono at chromium.org>'s request for review:
Bug 59693: [Feature Request] Need SpellCheck API
https://bugs.webkit.org/show_bug.cgi?id=59693

Attachment 95413: Patch v4 (only for Chromium)
https://bugs.webkit.org/attachment.cgi?id=95413&action=review

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=95413&action=review

What a great, complete, patch!	This looks like it would be easy to implement
under Windows (and Mac OS X).  Once this initial patch lands I can help get it
working under Windows and/or Mac if you would like.

r+, but please correct the couple of minor comments before landing.

> Source/WebCore/dom/DocumentMarker.h:64
> +	   // this marker is almost identical as the above Spelling marker, we
need

... almost identical TO the above Spelling marker ...

> Source/WebCore/dom/DocumentMarker.h:105
> +    int length() const { return m_endOffset - m_startOffset; }

Is this ever allowed to be negative?  Should this also be unsigned?

> Source/WebCore/dom/DocumentMarkerController.cpp:665
> +	   return result.release();

No need for release here.

> Source/WebCore/dom/DocumentMarkerController.cpp:679
> +    return result.release();

Ditto

> Source/WebCore/html/SpellcheckRange.h:31
> +#include "DOMStringList.h"

Can this be a forward declaration?  I can't remember if PassRefPtr needs the
full implementation.