Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1269343005:
Updates SpellcheckWordIterator::GetNextWord to return an enum. (Closed)
|
Created:
5 years, 4 months ago by Julius Modified:
4 years, 11 months ago CC:
chromium-reviews, groby+spellwatch_chromium.org, rlp+watch_chromium.org, rouslan+spellwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@break-iter Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdates SpellcheckWordIterator::GetNextWord to return an enum.
GetNextWord should return an enum so we can determine if the iterator
has found chracters that the spellchecker can skip over.
BUG=5102
Committed: https://crrev.com/4f336a6bca18d7a71660d79e7a56d018d74dabaf
Cr-Commit-Position: refs/heads/master@{#343145}
Patch Set 1 : #Patch Set 2 : Updates comments and preserves functionality in tests. #
Total comments: 20
Patch Set 3 : Cleaned up comments and loops. #
Total comments: 6
Patch Set 4 : Addressed comments. #
Total comments: 6
Patch Set 5 : Addressed comments. #
Total comments: 2
Messages
Total messages: 28 (13 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #2 (id:80001) has been deleted
juliusa@google.com changed reviewers: + rouslan@chromium.org
Rouslan, PTAL at Patch Set #2.
fyi, the tests will fail, because the bots don't know to apply http://crrev.com/1272683002 before this patch.
Looking nice. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_language.cc (right): https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_language.cc:72: while (status != SpellcheckWordIterator::IS_END_OF_TEXT) { Is this loop easier to understand? Should be less typing, fewer calls to GetNextWord(), hence smaller chance of making a mistake. // clang-format me for (SpellcheckWordIterator::WordIteratorStatus status = text_iterator_.GetNextWord(&word, &word_start, &word_length); status != SpellcheckWordIterator::IS_END_OF_TEXT; status = text_iterator_.GetNextWord( &word, &word_start, &word_length)) { // Preserve the existing comments. if (status == SpellcheckWordIterator::IS_SKIPPABLE_CHAR) continue; if (platform_spelling_engine_->CheckSpelling(word, tag)) continue; if (IsValidContraction(word, tag)) continue; ... } https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_language.cc:129: while (status != SpellcheckWordIterator::IS_END_OF_TEXT) { for loop again. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator.cc (right): https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator.cc:393: return IS_SKIPPABLE_CHAR; You should update the |word_string|, |word_start|, and |word_length| here, too. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator.h (left): https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator.h:140: // while(iterator.GetNextWord(&word, &offset, &length)) Keep this example in the comment please. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator.h (right): https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator.h:121: IS_SKIPPABLE_CHAR, IS_SKIPPABLE Putting "_CHAR" in there might confuse people into thinking that only a single character was encountered. Although the implementation might behave this way, let's not expose too much implementation details in the API for the next layer up the stack. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator.h:146: // - Returns IS_WORD if the iterator just found the end of a sequence of word Put a newline before a bullet point to make the comment easier to read. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc (right): https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:145: iterator.GetNextWord(&actual_word, &actual_start, &actual_end); for loop the heck out of this one. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:181: SpellcheckWordIterator::WordIteratorStatus status; for loop. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:187: EXPECT_EQ(status, SpellcheckWordIterator::WordIteratorStatus::IS_END_OF_TEXT); Expected value (SpellcheckWordIterator::WordIteratorStatus::IS_END_OF_TEXT) goes into EXPECT_EQ() first: EXPECT_EQ(expected, actual); i.e.: EXPECT_EQ(SpellcheckWordIterator::WordIteratorStatus::IS_END_OF_TEXT, status); https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:248: SpellcheckWordIterator::WordIteratorStatus status; for loop
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
Rouslan, PTAL at Patch Set #3. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_language.cc (right): https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_language.cc:72: while (status != SpellcheckWordIterator::IS_END_OF_TEXT) { On 2015/08/11 22:54:10, Rouslan wrote: > Is this loop easier to understand? Should be less typing, fewer calls to > GetNextWord(), hence smaller chance of making a mistake. > > // clang-format me > for (SpellcheckWordIterator::WordIteratorStatus status = > text_iterator_.GetNextWord(&word, &word_start, &word_length); > status != SpellcheckWordIterator::IS_END_OF_TEXT; > status = text_iterator_.GetNextWord( > &word, &word_start, &word_length)) { > // Preserve the existing comments. > if (status == SpellcheckWordIterator::IS_SKIPPABLE_CHAR) > continue; > > if (platform_spelling_engine_->CheckSpelling(word, tag)) > continue; > > if (IsValidContraction(word, tag)) > continue; > > ... > } Done. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_language.cc:129: while (status != SpellcheckWordIterator::IS_END_OF_TEXT) { On 2015/08/11 22:54:10, Rouslan wrote: > for loop again. Done. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator.cc (right): https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator.cc:393: return IS_SKIPPABLE_CHAR; On 2015/08/11 22:54:10, Rouslan wrote: > You should update the |word_string|, |word_start|, and |word_length| here, too. Done. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator.h (left): https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator.h:140: // while(iterator.GetNextWord(&word, &offset, &length)) On 2015/08/11 22:54:10, Rouslan wrote: > Keep this example in the comment please. Done. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator.h (right): https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator.h:121: IS_SKIPPABLE_CHAR, On 2015/08/11 22:54:10, Rouslan wrote: > IS_SKIPPABLE > > Putting "_CHAR" in there might confuse people into thinking that only a single > character was encountered. Although the implementation might behave this way, > let's not expose too much implementation details in the API for the next layer > up the stack. Done. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator.h:146: // - Returns IS_WORD if the iterator just found the end of a sequence of word On 2015/08/11 22:54:10, Rouslan wrote: > Put a newline before a bullet point to make the comment easier to read. Done. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc (right): https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:145: iterator.GetNextWord(&actual_word, &actual_start, &actual_end); On 2015/08/11 22:54:11, Rouslan wrote: > for loop the heck out of this one. Done. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:181: SpellcheckWordIterator::WordIteratorStatus status; On 2015/08/11 22:54:11, Rouslan wrote: > for loop. Done. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:187: EXPECT_EQ(status, SpellcheckWordIterator::WordIteratorStatus::IS_END_OF_TEXT); On 2015/08/11 22:54:11, Rouslan wrote: > Expected value (SpellcheckWordIterator::WordIteratorStatus::IS_END_OF_TEXT) goes > into EXPECT_EQ() first: > > EXPECT_EQ(expected, actual); > > i.e.: > > EXPECT_EQ(SpellcheckWordIterator::WordIteratorStatus::IS_END_OF_TEXT, status); Done. https://codereview.chromium.org/1269343005/diff/100001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:248: SpellcheckWordIterator::WordIteratorStatus status; On 2015/08/11 22:54:11, Rouslan wrote: > for > > > > > > > > > > > > > > > > loop D o n e .
Looking good! Minor nits. https://codereview.chromium.org/1269343005/diff/180001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator.cc (right): https://codereview.chromium.org/1269343005/diff/180001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator.cc:385: if (break_status == BreakIterator::IS_WORD_BREAK) { You should use a switch statement without a "default" case to explicitly specify what happens for every possible WordBreakStatus value. https://codereview.chromium.org/1269343005/diff/180001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc (right): https://codereview.chromium.org/1269343005/diff/180001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:175: // iterator.GetNextWord() call gets stuck in an infinite loop. Therefore, this This comment was written with a single iterator.GetNextWord() call in mind. Please update the comment to describe the for loop instead. https://codereview.chromium.org/1269343005/diff/180001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:183: continue; We usually use curly braces for the body of a for loop where either for(...) or the body spans multiple lines. This for(...) has three lines, so you should use curly braces around "continue;".
Patchset #4 (id:200001) has been deleted
Rouslan, PTAL at Patch Set #4. https://codereview.chromium.org/1269343005/diff/180001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator.cc (right): https://codereview.chromium.org/1269343005/diff/180001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator.cc:385: if (break_status == BreakIterator::IS_WORD_BREAK) { On 2015/08/12 21:01:09, Rouslan wrote: > You should use a switch statement without a "default" case to explicitly specify > what happens for every possible WordBreakStatus value. Done. https://codereview.chromium.org/1269343005/diff/180001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc (right): https://codereview.chromium.org/1269343005/diff/180001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:175: // iterator.GetNextWord() call gets stuck in an infinite loop. Therefore, this On 2015/08/12 21:01:09, Rouslan wrote: > This comment was written with a single iterator.GetNextWord() call in mind. > Please update the comment to describe the for loop instead. Done. https://codereview.chromium.org/1269343005/diff/180001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:183: continue; On 2015/08/12 21:01:09, Rouslan wrote: > We usually use curly braces for the body of a for loop where either for(...) or > the body spans multiple lines. This for(...) has three lines, so you should use > curly braces around "continue;". Yeah, wasn't quite sure about this. Didn't see it in the style guide but might have missed it.
lgtm % nits https://codereview.chromium.org/1269343005/diff/220001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator.cc (right): https://codereview.chromium.org/1269343005/diff/220001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator.cc:383: BreakIterator::WordBreakStatus break_status = inline this variable. https://codereview.chromium.org/1269343005/diff/220001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator.cc:400: // |iterator_| is RULE_BASED so |break_status| should never be If you inline |break_status|, then update the comment to say simply "break status". https://codereview.chromium.org/1269343005/diff/220001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc (right): https://codereview.chromium.org/1269343005/diff/220001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:41: // characters are found that are not from the specified language we skip them. "we" can be confusing in code. Specify the subject. I think it's "the test" that skips the characters.
Fixed final nits and submitting the patch. https://codereview.chromium.org/1269343005/diff/220001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator.cc (right): https://codereview.chromium.org/1269343005/diff/220001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator.cc:383: BreakIterator::WordBreakStatus break_status = On 2015/08/13 00:13:28, Rouslan wrote: > inline this variable. Done. https://codereview.chromium.org/1269343005/diff/220001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator.cc:400: // |iterator_| is RULE_BASED so |break_status| should never be On 2015/08/13 00:13:28, Rouslan wrote: > If you inline |break_status|, then update the comment to say simply "break > status". Done. https://codereview.chromium.org/1269343005/diff/220001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc (right): https://codereview.chromium.org/1269343005/diff/220001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:41: // characters are found that are not from the specified language we skip them. On 2015/08/13 00:13:28, Rouslan wrote: > "we" can be confusing in code. Specify the subject. I think it's "the test" that > skips the characters. Done.
The CQ bit was checked by juliusa@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/1269343005/#ps240001 (title: "Addressed comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1269343005/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1269343005/240001
Message was sent while issue was closed.
Committed patchset #5 (id:240001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4f336a6bca18d7a71660d79e7a56d018d74dabaf Cr-Commit-Position: refs/heads/master@{#343145}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1269343005/diff/240001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc (right): https://codereview.chromium.org/1269343005/diff/240001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:311: iterator.GetNextWord(&actual_word, &actual_start, &actual_end)) { You forgot to assign to status here (you got it right in line 184). A new warning I'm evaluating: ../../chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:313:10: error: variable 'status' used in loop condition not modified in loop body [-Werror,-Wfor-loop-analysis] status == SpellcheckWordIterator::IS_SKIPPABLE; ^~~~~~ How come the test still passes?
Message was sent while issue was closed.
groby@chromium.org changed reviewers: + groby@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1269343005/diff/240001/chrome/renderer/spellc... File chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc (right): https://codereview.chromium.org/1269343005/diff/240001/chrome/renderer/spellc... chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:311: iterator.GetNextWord(&actual_word, &actual_start, &actual_end)) { On 2016/01/05 22:13:23, Nico wrote: > You forgot to assign to status here (you got it right in line 184). A new > warning I'm evaluating: > > ../../chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:313:10: > error: variable 'status' used in loop condition not modified in loop body > [-Werror,-Wfor-loop-analysis] > status == SpellcheckWordIterator::IS_SKIPPABLE; > ^~~~~~ > > How come the test still passes? I _think_ the test still passes because the very first word delivered back is marked IS_WORD :) Currently investigating, fix coming shortly.
Message was sent while issue was closed.
Update: Fix at https://codereview.chromium.org/1561813002/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
