[webkit-reviews] review requested: [Bug 59693] [Feature Request] Need SpellCheck API : [Attachment 95891] Patch v5 (only for Chromium)

[bugzilla-daemon at webkit.org]

Hironori Bono <hbono at chromium.org> has asked  for review:
Bug 59693: [Feature Request] Need SpellCheck API
https://bugs.webkit.org/show_bug.cgi?id=59693

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

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

Thank you for your review and comment.

(In reply to comment #18)
> (From update of attachment 95413 [details])
> 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 ...

Done. Thank you for correcting my description.
 
> > 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?

Thank you for noticing it. Probably we do not need this variable to be int. I
have changed the result type to unsigned and added an ASSERT() here.
 
> > Source/WebCore/dom/DocumentMarkerController.cpp:665
> > +	     return result.release();
> 
> No need for release here.

Thank you for correction. I forgot fixing this when Morita-san told it.

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

Thank you for noticing it. Unfortunately, I cannot make it a forward
declaration because it causes compile errors while compiling an
automatically-generated code for v8 as listed below.

distcc[70342] ERROR: compile
/Users/hbono/Chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../
../../xcodebuild/DerivedSources/Release/webkit/bindings/V8DerivedSources19.cpp
on localhost:3632/2 failed
distcc[70342] (dcc_build_somewhere) Warning: remote compilation of
'/Users/hbono/Chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../..
/../../xcodebuild/DerivedSources/Release/webkit/bindings/V8DerivedSources19.cpp
' failed, retrying locally
distcc[70342] Warning: failed to distribute
/Users/hbono/Chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../
../../xcodebuild/DerivedSources/Release/webkit/bindings/V8DerivedSources19.cpp
to localhost:3632/2, running locally instead
In file included from
/Users/hbono/Chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../
../../xcodebuild/DerivedSources/Release/webkit/bindings/V8DerivedSources19.cpp:
85:
../../JavaScriptCore/wtf/PassRefPtr.h: In function 'void
WTF::derefIfNotNull(T*) [with T = WebCore::DOMStringList]':
../../JavaScriptCore/wtf/PassRefPtr.h:74:   instantiated from
'WTF::PassRefPtr<T>::~PassRefPtr() [with T = WebCore::DOMStringList]'
../html/SpellcheckRange.h:45:	instantiated from here
../../JavaScriptCore/wtf/PassRefPtr.h:59: error: invalid use of incomplete type
'struct WebCore::DOMStringList'
../bindings/v8/V8Binding.h:44: error: forward declaration of 'struct
WebCore::DOMStringList'
../../JavaScriptCore/wtf/PassRefPtr.h: In function 'void WTF::refIfNotNull(T*)
[with T = WebCore::DOMStringList]':
../../JavaScriptCore/wtf/RefPtr.h:44:	instantiated from
'WTF::RefPtr<T>::RefPtr(const WTF::RefPtr<T>&) [with T =
WebCore::DOMStringList]'
/Users/hbono/Chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../
../../xcodebuild/DerivedSources/Release/webcore/bindings/V8HTMLDivElement.cpp:7
9:   instantiated from here
../../JavaScriptCore/wtf/PassRefPtr.h:53: error: invalid use of incomplete type
'struct WebCore::DOMStringList'
../bindings/v8/V8Binding.h:44: error: forward declaration of 'struct
WebCore::DOMStringList'
distcc[70342] ERROR: compile
/Users/hbono/Chrome/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../
../../xcodebuild/DerivedSources/Release/webkit/bindings/V8DerivedSources19.cpp
on localhost failed

Even though I have read the code generator that creates this binding code, I
gave up fixing it because I could not find simple solution for this problem.
Sorry.

Regards,

Hironori Bono