Bug 106662 – [EFL][GTK] checkSpellingOfString treats the multiple words as spelled correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Grzegorz Czajkowski]

TextCheckerEnchant::checkSpellingOfString() method, treats the multiple words as spelled correctly if one of them is ok. For example, string such as "OK zz OK" is spelled correctly!

The method at the beginning assumes that the given string is spelled correctly, then it iterates over the words. In checkSpellingOfWord, if the word is ok, we mark it as spelled correctly and we do return. In the fact, the location and length of the previously misspelled words will be overwritten.

[Grzegorz Czajkowski]

Created attachment 182338 [details]
proposed patch

[Build Bot]

Comment on attachment 182338 [details]
proposed patch

Attachment 182338 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15815191

New failing tests:
svg/as-image/img-relative-height.html

[Grzegorz Czajkowski]

(In reply to comment #2)
> (From update of attachment 182338 [details])
> Attachment 182338 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/15815191
> 
> New failing tests:
> svg/as-image/img-relative-height.html

The patch touches only TextCheckerEnchant.cpp file which is building for EFL and GTK+ WebKit ports. I believe this regression is not connected with this patch.

[Sergio Villar Senin]

Comment on attachment 182338 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182338&action=review

The problem is not in checkSpellingOfWord() but in checkSpellingOfString(). What happens in the test case you mention is that after considering that zz is misspelled (and thus filling misspellingLocation and misspellingLength with the proper values), then it processes the 3rd word which is correct and thus it will overwrite the two variables with 0, -1, so the whole string will be considered correct. What you have to do is exit if one of the words is wrong.

> Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:78
>          }

This is wrong, the word is OK if it's correct according to at least one dictionary not the other way around.

[Grzegorz Czajkowski]

Created attachment 182713 [details]
stop checking next words if the current word is misspelled

[Sergio Villar Senin]

Comment on attachment 182713 [details]
stop checking next words if the current word is misspelled

Why are you adding a return value? In order to know if a word is correct you just need to check that misspellingLocation == -1, there is no need to add a boolean.

[Sergio Villar Senin]

Comment on attachment 182713 [details]
stop checking next words if the current word is misspelled

Sorry I shouldn't have set the r- not being a reviewer.

[Grzegorz Czajkowski]

(In reply to comment #6)
> (From update of attachment 182713 [details])
> Why are you adding a return value? In order to know if a word is correct you just need to check that misspellingLocation == -1, there is no need to add a boolean.

I thought about checking misspellingLocation == -1 after your review but when I started to make the proper changes, IMO the code wasn't look well. However, returning the same information (whether the word is misspelled or not) by return value and parameter is worst. Patch is coming.

[Grzegorz Czajkowski]

Created attachment 182746 [details]
apply Sergio's suggestion

[Sergio Villar Senin]

(In reply to comment #9)
> Created an attachment (id=182746) [details]
> apply Sergio's suggestion

LGTM now

[Antonio Gomes]

Comment on attachment 182746 [details]
apply Sergio's suggestion

View in context: https://bugs.webkit.org/attachment.cgi?id=182746&action=review

> Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:-72
> -    for (Vector<EnchantDict*>::const_iterator dictIter = m_enchantDictionaries.begin(); dictIter != m_enchantDictionaries.end(); ++dictIter) {

nit "{" is still needed as the 'for' content wraps line.

[Grzegorz Czajkowski]

Committed r139971: <http://trac.webkit.org/changeset/139971>