[webkit-reviews] review canceled: [Bug 59693] [Feature Request] Need SpellCheck API : [Attachment 94961] 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 94961: Patch v2 (only for Chromium)
https://bugs.webkit.org/attachment.cgi?id=94961&action=review

------- Additional Comments from Hironori Bono <hbono at chromium.org>
Greetings Morita-san,

Thank you for your review and comments.
I have applied your comments and updated the change.

(In reply to comment #10)

> Hi Bono-san, thank you for update.
> You can add Skipped entries where other editing or spelling test is listed.
> It would reduce the chance of conflict.

Indeed. It is definitely better to avoid unnecessary conflicts in Skipped
files.
 
> > Source/WebCore/dom/DocumentMarkerController.cpp:677
> > +	     DocumentMarker marker = list->at(i);
> 
> Could you use const refrence if possible? It will save an extra copy.

Done. Thank you for noticing it.
 
> > Source/WebCore/dom/DocumentMarkerController.cpp:683
> > +	     result->append(SpellcheckRange::create(marker.startOffset(),
marker.endOffset() - marker.startOffset(), suggestions.release(), 0));
> 
> Could you add length() to DocumentMarker for here? It would be illustrative.

OK. I have added the length() method.
 
> > Source/WebCore/html/HTMLDivElement.idl:27
> > +	     void addSpellcheckRange(in unsigned long start, in unsigned long
length, in [Optional] DOMStringList suggestions, in [Optional] unsigned short
options);
> 
> Don't we throw any exceptions? It might be a question about the spec though.

Even though the current API does not send exceptions, we will hear opinions of
users with actual implementation and clearify such ambiguous issues.
 
> > LayoutTests/ChangeLog:6
> > +	     https://bugs.webkit.org/show_bug.cgi?id=59693
> 
> Can we have a test pixel test for that to ensure that the user-marker is
actually visible?

OK. I have added another test. (I prefer script tests, though.)

Regards,

Hironori Bono