Hi, While testing the spelling implementation of WebKit-EFL I found out an interesting bug in the shared code (WebCore/platform/text/enchant/TextChecker.cpp) It's connected with editing/spelling/spelling-backspace-between-lines.html. This test has been added here https://bugs.webkit.org/show_bug.cgi?id=41423. It looks like Chromium port had similar issue, already resolved https://bugs.webkit.org/show_bug.cgi?id=45438 This bug is not connected with Enchant and latest changes at all, rather with no possibilities to mark misspelled words (location and length) in checkSpellingOfString method which may take more than one word (as spelling-backspace-between-lines.html does). Let me explain step by step why spelling-backspace-between-lines.html passes in the present implementation for both WebKit-Gtk and WebKit-EFL. We've included English dictionary (size of 'm_enchantDictionaries' vector is 1) and 'checkSpellingOfString' method is called for "OK zz OK" string. 'checkSpellingOfString' sets 'dictIter' at the beginning of method! and starts to go through each word: Vector<EnchantDict*>::const_iterator dictIter = m_enchantDictionaries.begin(); for (size_t i = 0; i < numberOfCharacters + 1; i++) { // iterate through the words .... for (; dictIter != m_enchantDictionaries.end(); ++dictIter) { if (enchant_dict_check(*dictIter, cstart, numberOfBytes)) { misspellingLocation = start; misspellingLength = wordLength; } else { // Stop checking, this word is ok in at least one dict. misspellingLocation = -1; misspellingLength = 0; break; } } } 1) For first word 'OK'. We go into an inner loop and we spelling is checked. The word is fine so its location and length is saved to -1 and 0, then we break the loop, 'dictIter' is not increased! 2) For second word 'zz'. We go into an inner loop, 'dictIter' still points on the beginning of the vector. The word is wrong - misspelled location is updated to 3 and length to 2 for "OK zz OK" string. The inner loop increases 'dictIter'. 3) For third word 'OK' We go into an inner loop and we check whether 'dictIter' doesn't bound out of the vector. In this case - YES. We do not go to the loop here! Finally 'checkSpellingOfString' returns misspelled location and length from the the previous iteration of the inner loop (3,2). What is the most surprising that exactly spelling-backspace-between-lines.html expects! My proposals of resolving this bug is to initialize 'dictIter' at the beginning of the loop and: 1) temporally add spelling-backspace-between-lines.html to TestExpectations. 2) find a new way how to save misspelled location and length for multiple words for example "OK zz OK zz" (3,2) and (9,2)
Created attachment 161459 [details] initialize iteraor, add test to TestExpectations
Comment on attachment 161459 [details] initialize iteraor, add test to TestExpectations View in context: https://bugs.webkit.org/attachment.cgi?id=161459&action=review r- for the test expectations change > LayoutTests/platform/gtk/TestExpectations:1025 > +// TextCheckerEnchant::checkSpellingOfString doesn't return misspelled location and length for multiple words. > +BUGWKGTK : editing/spelling/spelling-backspace-between-lines.html = TEXT > + I don't understand why you're adding this, this test seems to run and pass on our bot: 06:57:30.535 22495 worker/22 editing/spelling/spelling-backspace-between-lines.html passed > Source/WebCore/ChangeLog:10 > + dictionaries ('dictIter') whether the word exists in at lest one dictionary. I'd remove 'whether the word exists...', that's better explained in the paragraph bellow. > Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:102 > + Vector<EnchantDict*>::const_iterator dictIter = m_enchantDictionaries.begin(); Makes sense, can we declare this inside the for? Would be more idiomatic I think.
(In reply to comment #2) > (From update of attachment 161459 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161459&action=review > > r- for the test expectations change > > > LayoutTests/platform/gtk/TestExpectations:1025 > > +// TextCheckerEnchant::checkSpellingOfString doesn't return misspelled location and length for multiple words. > > +BUGWKGTK : editing/spelling/spelling-backspace-between-lines.html = TEXT > > + > > I don't understand why you're adding this, this test seems to run and pass on our bot: Thanks for review. Believe or not this bug (that iterator is not initialized) makes that the test passes :) See the first comment, there you can find more details.
Comment on attachment 161459 [details] initialize iteraor, add test to TestExpectations View in context: https://bugs.webkit.org/attachment.cgi?id=161459&action=review >>> LayoutTests/platform/gtk/TestExpectations:1025 >>> + >> >> I don't understand why you're adding this, this test seems to run and pass on our bot: >> >> 06:57:30.535 22495 worker/22 editing/spelling/spelling-backspace-between-lines.html passed > > Thanks for review. > Believe or not this bug (that iterator is not initialized) makes that the test passes :) See the first comment, there you can find more details. Aha, thanks, sorry I didn't read the first comment.
Comment on attachment 161459 [details] initialize iteraor, add test to TestExpectations View in context: https://bugs.webkit.org/attachment.cgi?id=161459&action=review >> Source/WebCore/ChangeLog:10 >> + dictionaries ('dictIter') whether the word exists in at lest one dictionary. > > I'd remove 'whether the word exists...', that's better explained in the paragraph bellow. Done. >> Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:102 >> + Vector<EnchantDict*>::const_iterator dictIter = m_enchantDictionaries.begin(); > > Makes sense, can we declare this inside the for? Would be more idiomatic I think. Sure. Done.
Created attachment 161868 [details] updated patch
Comment on attachment 161868 [details] updated patch Clearing flags on attachment: 161868 Committed r127404: <http://trac.webkit.org/changeset/127404>
All reviewed patches have been landed. Closing bug.