[webkit-reviews] review granted: [Bug 74393] [Chromium] Enable grammar checking : [Attachment 143449] Patch v5 (applied comments and updated to top-of-trunk)

Wed May 23 00:34:57 PDT 2012

Ryosuke Niwa <rniwa at webkit.org> has granted Hironori Bono
<hbono at chromium.org>'s request for review:
Bug 74393: [Chromium] Enable grammar checking
https://bugs.webkit.org/show_bug.cgi?id=74393

Attachment 143449: Patch v5 (applied comments and updated to top-of-trunk)
https://bugs.webkit.org/attachment.cgi?id=143449&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=143449&action=review

>>> Source/WebKit/chromium/src/WebTextCheckingResult.cpp:51
>>> +	     detail.userDescription = replacement;
>> 
>> I don't think this is the correct use of user description.
> 
> Editor::markAndReplaceFor somehow uses this value as the description of a
grammar marker:
<http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/W
ebCore/editing/Editor.cpp&type=cs&l=2054>, i.e. this function discards
replacement and the guesses list when it adds a grammar marker.

That is very misleading. Perhaps we should rename this member variable.

>>> LayoutTests/editing/spelling/grammar-markers.html:5
>>> +</head>
>> 
>> No need for head/title.
> 
> If I recall correctly, HTML5 needs them. (In my personal opinion, I would
like to make this file compliant with HTML5 because it has "<!DOCTYPE html>".)

I don't think so.