[webkit-reviews] review canceled: [Bug 59693] [Feature Request] Need SpellCheck API : [Attachment 94769] Patch v2 (only for Chromium)

[bugzilla-daemon at webkit.org]

Hironori Bono <hbono at chromium.org> has canceled 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 94769: Patch v2 (only for Chromium)
https://bugs.webkit.org/attachment.cgi?id=94769&action=review

------- Additional Comments from Hironori Bono <hbono at chromium.org>
(In reply to comment #8)

Thank you for your review and comments. Even though I have not applied your
comments totally, I have uploaded the updated change because I have uploaded an
old version yesterday.

> > Source/WebCore/dom/DocumentMarker.h:63
> > +#if ENABLE(SPELLCHECK_API)
> 
> This adds an extra "," for the enums. I think You can just add UserSpelling
without ENABLE() guard.
> It's never used in disabled case and there is no runtime overhead.
> Auto-correction related enums do same thing.

Thank you for your suggestion. I have removed the guard here and added a comma
at the end of this line.
 
> > Source/WebCore/dom/DocumentMarkerController.cpp:689
> > +bool DocumentMarkerController::addUserSpellingMarker(Node* node, unsigned
start, int length, RefPtr<DOMStringList> suggestions, int options)
> 
> Can length be unsigned?

Ah, yes. It is better to use unsigned since the callers use unsigned.
 
> > Source/WebCore/html/SpellcheckRangeList.h:31
> > +#include "SpellcheckRange.h"
> 
> can we replace this with a forward declaration?

Thank you for your catch. Yes, we can replace it. I have replaced this include
with a forward declaration.
 
> > Source/WebCore/rendering/InlineTextBox.cpp:961
> > +#if ENABLE(SPELLCHECK_API)
> 
> We don't need these guards

Thank you for your comment. I have removed them.
 
> > Source/WebKit/chromium/src/WebRuntimeFeatures.cpp:341
> > +#if ENABLE(SPELLCHECK_API)
> 
> Please add UNUSED_PARAM() for disabled path.

Indeed. I forgot adding UNUSED_PARAM() here.

> > LayoutTests/ChangeLog:1
> > +2011-05-25  Hironori Bono	<hbono at chromium.org>
> 
> Please update skipped lists for unsupported platforms.

Ok. I have added my new test to Skipped files.

Regards,

Hironori Bono