[Spellcheck] Fixed subtly broken test.

[Spellcheck] Fixed subtly broken test.

The test in question was passing, but still contained broken code. See
https://codereview.chromium.org/1269343005/ for a discussion of the
issue.

BUG=5102
Committed: https://crrev.com/78982c99403e54d61160a6f6a68d9b3be1f36697
Cr-Commit-Position: refs/heads/master@{#367752}

[groby-ooo-7-16, 2016-01-05 23:13:28]

Description was changed from

==========
[Spellcheck] Fixed subtly broken test.

The test in question was passing, but still contained broken code. See
https://codereview.chromium.org/1269343005/ for a discussion of the
issue.

BUG=5102
==========

to

==========
[Spellcheck] Fixed subtly broken test.

The test in question was passing, but still contained broken code. See
https://codereview.chromium.org/1269343005/ for a discussion of the
issue.

BUG=5102
==========

[groby-ooo-7-16, 2016-01-05 23:13:28]

groby@chromium.org changed reviewers:
+ thakis@chromium.org

[groby-ooo-7-16, 2016-01-05 23:34:54]

PTAL - it fixes the issue you highlighted in
https://codereview.chromium.org/1269343005/

That's the first patch set. And then I cleaned up a few things that annoyed me
:)

[Nico, 2016-01-06 01:38:26]

lgtm, thanks for the quick fix!

https://codereview.chromium.org/1561813002/diff/80001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc (right):

https://codereview.chromium.org/1561813002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:42: status =
iterator->GetNextWord(word_string, word_start, word_length);
with a do-while loop you don't need to initialize status to a default

[groby-ooo-7-16, 2016-01-06 01:48:38]

Unless the do-while thing is tremendously important to you, I think I'll land
as-is. Purely for aesthetic reasons :)

https://codereview.chromium.org/1561813002/diff/80001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc (right):

https://codereview.chromium.org/1561813002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:42: status =
iterator->GetNextWord(word_string, word_start, word_length);
On 2016/01/06 01:38:26, Nico wrote:
> with a do-while loop you don't need to initialize status to a default
True, but it's one more line :) See below:

Status status;
do {
  status = GetNext();
} while (status == IS_SKIPPABLE);

vs.

Status status = SKIPPABLE
while (status == SKPPABLE)
  status = GetNext();

I could've of course opted for the *truly* horrible solution ;)

Status status;
for (;(status=GetNext()) == IS_SKIPPABLE;) {}

[groby-ooo-7-16, 2016-01-06 02:28:04]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2016-01-06 02:28:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1561813002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1561813002/80001

[commit-bot: I haz the power, 2016-01-06 02:35:19]

Description was changed from

==========
[Spellcheck] Fixed subtly broken test.

The test in question was passing, but still contained broken code. See
https://codereview.chromium.org/1269343005/ for a discussion of the
issue.

BUG=5102
==========

to

==========
[Spellcheck] Fixed subtly broken test.

The test in question was passing, but still contained broken code. See
https://codereview.chromium.org/1269343005/ for a discussion of the
issue.

BUG=5102
==========

[commit-bot: I haz the power, 2016-01-06 02:35:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-06 02:36:04]

Description was changed from

==========
[Spellcheck] Fixed subtly broken test.

The test in question was passing, but still contained broken code. See
https://codereview.chromium.org/1269343005/ for a discussion of the
issue.

BUG=5102
==========

to

==========
[Spellcheck] Fixed subtly broken test.

The test in question was passing, but still contained broken code. See
https://codereview.chromium.org/1269343005/ for a discussion of the
issue.

BUG=5102

Committed: https://crrev.com/78982c99403e54d61160a6f6a68d9b3be1f36697
Cr-Commit-Position: refs/heads/master@{#367752}
==========

[commit-bot: I haz the power, 2016-01-06 02:36:05]

Patchset 5 (id:??) landed as
https://crrev.com/78982c99403e54d61160a6f6a68d9b3be1f36697
Cr-Commit-Position: refs/heads/master@{#367752}

[Nico, 2016-01-06 02:36:16]

Please land as is. But:

https://codereview.chromium.org/1561813002/diff/80001/chrome/renderer/spellch...
File chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc (right):

https://codereview.chromium.org/1561813002/diff/80001/chrome/renderer/spellch...
chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:42: status =
iterator->GetNextWord(word_string, word_start, word_length);
On 2016/01/06 01:48:37, groby wrote:
> On 2016/01/06 01:38:26, Nico wrote:
> > with a do-while loop you don't need to initialize status to a default
> True, but it's one more line :) See below:
> 
> Status status;
> do {
>   status = GetNext();
> } while (status == IS_SKIPPABLE);
> 
> vs.
> 
> Status status = SKIPPABLE
> while (status == SKPPABLE)
>   status = GetNext();
> 
> I could've of course opted for the *truly* horrible solution ;)
> 
> Status status;
> for (;(status=GetNext()) == IS_SKIPPABLE;) {}
> 

<s>More horrible</s>You don't need the parens:

  Status status;
  do status = GetNext();
  while (status == IS_SKIPPABLE);

[groby-ooo-7-16, 2016-01-06 18:56:00]

On 2016/01/06 02:36:16, Nico wrote:
> Please land as is. But:
> 
>
https://codereview.chromium.org/1561813002/diff/80001/chrome/renderer/spellch...
> File chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc (right):
> 
>
https://codereview.chromium.org/1561813002/diff/80001/chrome/renderer/spellch...
> chrome/renderer/spellchecker/spellcheck_worditerator_unittest.cc:42: status =
> iterator->GetNextWord(word_string, word_start, word_length);
> On 2016/01/06 01:48:37, groby wrote:
> > On 2016/01/06 01:38:26, Nico wrote:
> > > with a do-while loop you don't need to initialize status to a default
> > True, but it's one more line :) See below:
> > 
> > Status status;
> > do {
> >   status = GetNext();
> > } while (status == IS_SKIPPABLE);
> > 
> > vs.
> > 
> > Status status = SKIPPABLE
> > while (status == SKPPABLE)
> >   status = GetNext();
> > 
> > I could've of course opted for the *truly* horrible solution ;)
> > 
> > Status status;
> > for (;(status=GetNext()) == IS_SKIPPABLE;) {}
> > 
> 
> <s>More horrible</s>You don't need the parens:
> 
>   Status status;
>   do status = GetNext();
>   while (status == IS_SKIPPABLE);

You know.... The style guide is not entirely clear on the formatting. And
there's a variant that almost makes sense:

  do status = GetNext(); while (status == IS_SKIPPABLE);

Alas, the line is too long due to IS_SKIPPABLE being qualified, and the variable
names being too long. And git cl format actually doesn't even like your version,
the do has to be on a separate line.

I can't believe I actually spent time checking this - best bikeshed of 2016
contender :)