Allow storing multiple replacements on SpellCheckResult

Allow storing multiple replacements on SpellCheckResult

I'm working on adding support for the Android spellcheck suggestion list that
appears in a native text box when you tap on a misspelled word. It appears that
I need to modify this piece of infrastructure to support passing multiple
suggestions through to the Spelling marker that gets added.

For reference, we construct the SpellCheckResult objects from the Android
spellchecker results in SpellCheckerSessionBridge::ProcessSpellCheckResults().

Alternatively, ContextMenuClientImpl has logic for calling the spellchecker
again to get the list of suggestions that we might be able to use:
https://chromium.googlesource.com/chromium/src/+/adc8910776c64a876f0422454da618a37d01167b/third_party/WebKit/Source/web/ContextMenuClientImpl.cpp#346

However, I talked to aelias and he said it would be better to store the
suggestions the first time we call the spellchecker, at the time we add the
marker, to avoid going back and forth unnecessarily.

BUG=715365
Review-Url: https://codereview.chromium.org/2848943002
Cr-Commit-Position: refs/heads/master@{#475058}
Committed: https://chromium.googlesource.com/chromium/src/+/c45f50742410a644a054325478d885e61d286713

[rlanday, 2017-04-28 17:55:39]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 17:56:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-04-28 18:01:17]

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

[rlanday, 2017-04-28 18:10:01]

groby: adding you as a spellcheck OWNER.

I'm working on adding support for the Android spellcheck menu that
appears when you tap a misspelled word (right now on other platforms, we
include the spellcheck options as part of a general context menu; this is
a text suggestion-specific menu on Android). The first step seemed to me to
modify this piece of infrastructure so we can store the whole list of
spelling suggestions in the Spelling DocumentMarkers when we first run
spellcheck to create them.

I'm not sure if this is the correct approach or not. There's apparently
some other code that stores lists of suggestions in DocumentMarker separated
by newline characters in a single string, but I'm not sure if it goes through
SpellCheckResult or not to do that. Another option (that seems less desirable
to some extent) is to run the spellchecker again when we're about to open the
menu to get the list of suggestions).

Does this seem like a reasonable thing to do or is there another approach I
should take?

[rlanday, 2017-04-28 18:12:22]

For reference, this is what the next step, to actually pass the results
through from the Android spellchecker, would look like:
https://codereview.chromium.org/2849933002

[commit-bot: I haz the power, 2017-04-28 20:04:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 20:04:47]

Dry run: This issue passed the CQ dry run.

[groby-ooo-7-16, 2017-04-28 23:45:55]

FWIW: By now, Render- and Browser-side spellcheckers have pretty much the same
API. I haven't dug into this, but you might be able to pull some simplifications
out of that.

Also, we probably want a test that ensures the full set of suggestions is
plumbed all the way through?

https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/commo...
File components/spellcheck/common/spellcheck_result.h (right):

https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/commo...
components/spellcheck/common/spellcheck_result.h:27: Decoration d = SPELLING,
Can we just kill the defaults here?

https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/rende...
File components/spellcheck/renderer/spellcheck.cc (right):

https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/rende...
components/spellcheck/renderer/spellcheck.cc:489: if (skip_these_replacements)
I believe this is wrong - with your change, if *any* of the suggestions is
identical except for apostrophe fixes, *all* are discarded.

This is complex enough it probably deserves a test case :)

https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/rende...
File components/spellcheck/renderer/spellcheck_provider_test.cc (right):

https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/rende...
components/spellcheck/renderer/spellcheck_provider_test.cc:86:
blink::WebVector<blink::WebString> replacements;
I don't think you need this.

https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/rende...
File components/spellcheck/renderer/spellcheck_unittest.cc (right):

https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/rende...
components/spellcheck/renderer/spellcheck_unittest.cc:1185:
std::vector<base::string16>({base::UTF8ToUTF16("I've")})));
That code is fairly ugly by now... I wonder if we should allow a convenience
ctor that just takes a string16, instead of a vector? Or at least a MakeResult
helper function (which could then go and take care of UTF8 and wstring
conversion, too)

[rlanday, 2017-05-01 20:40:01]

https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/rende...
File components/spellcheck/renderer/spellcheck.cc (right):

https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/rende...
components/spellcheck/renderer/spellcheck.cc:489: if (skip_these_replacements)
On 2017/04/28 at 23:45:55, groby wrote:
> I believe this is wrong - with your change, if *any* of the suggestions is
identical except for apostrophe fixes, *all* are discarded.
> 
> This is complex enough it probably deserves a test case :)

My thinking was that if the spellchecker thinks the word can be made to be
correctly spelled just by changing the apostrophe, we should just ignore the
misspelling.

E.g. we might have a case like

word:
"John’s"

suggestions:
"John's"
"Jon's"

where one of the suggestions is just changing the apostrophe, and some of the
others are unnecessary changes to an already correctly-misspelled word. In this
case, the code I wrote probably does the most correct thing and ignores the
"misspelling".

I suppose it's also possible in principle to have a case like this:

word:
Johhn’s

suggestions:
Johhn's (which we can correct to Johhn’s and throw out)
John's (which we can correct to John’s and keep)

In this case, would do want to keep all other suggested replacements. However,
something like this seems like an oddball case that shouldn't happen because it
requires the spellchecker to return a suggestion that changes only the
apostrophe, despite the word still being misspelled. This shouldn't happen if
the dictionary to pull replacements out of is the same for checking the word in
the first place, but might happen if one of the rules for generating suggestions
is "replace curly quotes with straight quotes".

If we're really worried about this second case, we could probably switch out the
apostrophes to straight quotes before running the spellchecker, but then we
could theoretically have the same problem in the opposite direction where some
spellchecker might think words are only spelled correctly if they use *curly*
quotes.


What do you think the behavior should be?

[groby-ooo-7-16, 2017-05-01 20:45:24]

On 2017/05/01 20:40:01, rlanday wrote:
>
https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/rende...
> File components/spellcheck/renderer/spellcheck.cc (right):
> 
>
https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/rende...
> components/spellcheck/renderer/spellcheck.cc:489: if (skip_these_replacements)
> On 2017/04/28 at 23:45:55, groby wrote:
> > I believe this is wrong - with your change, if *any* of the suggestions is
> identical except for apostrophe fixes, *all* are discarded.
> > 
> > This is complex enough it probably deserves a test case :)
> 
> My thinking was that if the spellchecker thinks the word can be made to be
> correctly spelled just by changing the apostrophe, we should just ignore the
> misspelling.
> 
> E.g. we might have a case like
> 
> word:
> "John’s"
> 
> suggestions:
> "John's"
> "Jon's"
> 
> where one of the suggestions is just changing the apostrophe, and some of the
> others are unnecessary changes to an already correctly-misspelled word. In
this
> case, the code I wrote probably does the most correct thing and ignores the
> "misspelling".
> 
> I suppose it's also possible in principle to have a case like this:
> 
> word:
> Johhn’s
> 
> suggestions:
> Johhn's (which we can correct to Johhn’s and throw out)
> John's (which we can correct to John’s and keep)
> 
> In this case, would do want to keep all other suggested replacements. However,
> something like this seems like an oddball case that shouldn't happen because
it
> requires the spellchecker to return a suggestion that changes only the
> apostrophe, despite the word still being misspelled. This shouldn't happen if
> the dictionary to pull replacements out of is the same for checking the word
in
> the first place, but might happen if one of the rules for generating
suggestions
> is "replace curly quotes with straight quotes".
> 
> If we're really worried about this second case, we could probably switch out
the
> apostrophes to straight quotes before running the spellchecker, but then we
> could theoretically have the same problem in the opposite direction where some
> spellchecker might think words are only spelled correctly if they use *curly*
> quotes.
> 
> 
> What do you think the behavior should be?

We _do_ normalize  apostrophes to straight apostrophes before we run the
checker. We just restore them after fixing spelling, because we have no control
over "curly or not", and the spell checker treats them differently. There's a
long bug with a whole lot of info on this.
https://bugs.chromium.org/p/chromium/issues/detail?id=165079

The check that you're looking at is part of the code that handles that
particular case :)

[rlanday, 2017-05-01 23:25:07]

Updated

https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/commo...
File components/spellcheck/common/spellcheck_result.h (right):

https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/commo...
components/spellcheck/common/spellcheck_result.h:27: Decoration d = SPELLING,
On 2017/04/28 at 23:45:55, groby wrote:
> Can we just kill the defaults here?

Seems that these are needed for IPC::ReadParam() to compile properly

https://codereview.chromium.org/2848943002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/spellcheck/SpellCheckerTest.cpp
(right):

https://codereview.chromium.org/2848943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/spellcheck/SpellCheckerTest.cpp:87:
TEST_F(SpellCheckerTest, MarkAndReplaceForHandlesMultipleReplacements) {
This was the closest I could easily get to testing that the multiple
replacements are plumbed all the way through to the DocumentMarker. It doesn't
catch the part where SpellCheckResults are converted to TextCheckingResults, but
it does test the actual logic that creates the DocumentMarkers.

[rlanday, 2017-05-01 23:26:06]

rlanday@chromium.org changed reviewers:
+ xiaochengh@chromium.org

[rlanday, 2017-05-02 17:53:22]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 17:53:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 18:03:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 18:03:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rlanday, 2017-05-02 21:53:24]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 21:54:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-05-02 22:14:27]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 22:14:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 00:05:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 00:05:42]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[rlanday, 2017-05-03 00:20:29]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 00:21:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 02:24:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 02:24:10]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-05-03 17:57:42]

Looks like all the trybots are finally passing again :)

[groby-ooo-7-16, 2017-05-08 15:31:22]

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

[groby-ooo-7-16, 2017-05-08 15:31:22]

+rouslan, who's more familiar with the curly apostrophe handling.

Rouslan: Am I misunderstanding our handling of curly apostrophes? We never
accounted for the multi-replacement case here. (Which makes me curious, because
hunspell always has multiple suggestions)

https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/rende...
File components/spellcheck/renderer/spellcheck.cc (right):

https://codereview.chromium.org/2848943002/diff/1/components/spellcheck/rende...
components/spellcheck/renderer/spellcheck.cc:489: if (skip_these_replacements)
On 2017/05/01 20:40:01, rlanday wrote:
> On 2017/04/28 at 23:45:55, groby wrote:
> > I believe this is wrong - with your change, if *any* of the suggestions is
> identical except for apostrophe fixes, *all* are discarded.
> > 
> > This is complex enough it probably deserves a test case :)
> 
> My thinking was that if the spellchecker thinks the word can be made to be
> correctly spelled just by changing the apostrophe, we should just ignore the
> misspelling.
> 
> E.g. we might have a case like
> 
> word:
> "John’s"
> 
> suggestions:
> "John's"
> "Jon's"
> 
> where one of the suggestions is just changing the apostrophe, and some of the
> others are unnecessary changes to an already correctly-misspelled word. In
this
> case, the code I wrote probably does the most correct thing and ignores the
> "misspelling".
> 
> I suppose it's also possible in principle to have a case like this:
> 
> word:
> Johhn’s
> 
> suggestions:
> Johhn's (which we can correct to Johhn’s and throw out)
> John's (which we can correct to John’s and keep)
> 
> In this case, would do want to keep all other suggested replacements. However,
> something like this seems like an oddball case that shouldn't happen because
it
> requires the spellchecker to return a suggestion that changes only the
> apostrophe, despite the word still being misspelled. This shouldn't happen if
> the dictionary to pull replacements out of is the same for checking the word
in
> the first place, but might happen if one of the rules for generating
suggestions
> is "replace curly quotes with straight quotes".
> 
> If we're really worried about this second case, we could probably switch out
the
> apostrophes to straight quotes before running the spellchecker, but then we
> could theoretically have the same problem in the opposite direction where some
> spellchecker might think words are only spelled correctly if they use *curly*
> quotes.

What we currently do is that for all replacements, we restore the original
apostrophes after spell checking. If after that replacement the suggestion
equals the original word, we drop *that suggestion*.

There might be multiple other suggestions that still apply. Let's assume we try
the possessive of "mart", "mart‘s" with a 'smart quote'

The suggestions coming back will be "mart's", "mark's", "mare's" - all with
straight apostrophes. We do the back substitution on the first. It's equal to
the original, it should be dropped. But all further suggestions should still be
included, with the apostrophe substitution. I.e. We'd keep "mark's" and "mare's"
with smart quotes.

We can certainly debate which is the better choice - you've opted for "it's
correct, let's drop it". I'm wondering if that's the right choice, since we also
mix results with the server-side check. (Below, filter==USE_NATIVE_CHECKER). 

I.e. if server-side suggested "mare's" as a contextually better result, I
believe it should still get a red underline. In your case, it'd get a grey one.

Yes, I know this is the edge case of an edge case :) If you'd prefer to stick
with your choice, I won't keep objecting - but please document the choice in a
comment, and add a unit test that verifies that specific behavior. 

> 
> What do you think the behavior should be?

[please use gerrit instead, 2017-05-08 15:40:31]

On 2017/05/08 15:31:22, groby wrote:
> +rouslan, who's more familiar with the curly apostrophe handling.
> 
> Rouslan: Am I misunderstanding our handling of curly apostrophes? We never
> accounted for the multi-replacement case here. (Which makes me curious,
because
> hunspell always has multiple suggestions)

Your understanding is correct, Rachel. We never accounted for the
multi-replacement case because only the server-side spellchecker has the
apostrophe-fixing behavior. The server-side spellchecker always provides only
one suggestion.

[please use gerrit instead, 2017-05-08 15:42:19]

At the time of my work on spellcheck, SpellCheck::CreateTextCheckingResults()
was called only for spellcheck results coming back from browser, i.e., from the
server spellchecker. Hunspell was not using that code path.

[please use gerrit instead, 2017-05-09 15:24:26]

Description was changed from

==========
Allow storing multiple replacements on SpellCheckResult

I'm working on adding support for the Android spellcheck suggestion list that
appears in a native text box when you tap on a misspelled word. It appears that
I need to modify this piece of infrastructure to support passing multiple
suggestions through to the Spelling marker that gets added.

For reference, we construct the SpellCheckResult objects from the Android
spellchecker results in SpellCheckerSessionBridge::ProcessSpellCheckResults().

Alternatively, ContextMenuClientImpl has logic for calling the spellchecker
again to get the list of suggestions that we might be able to use:
https://chromium.googlesource.com/chromium/src/+/adc8910776c64a876f0422454da6...

However, I talked to aelias and he said it would be better to store the
suggestions the first time we call the spellchecker, at the time we add the
marker, to avoid going back and forth unnecessarily.

BUG=715365
==========

to

==========
Allow storing multiple replacements on SpellCheckResult

I'm working on adding support for the Android spellcheck suggestion list that
appears in a native text box when you tap on a misspelled word. It appears that
I need to modify this piece of infrastructure to support passing multiple
suggestions through to the Spelling marker that gets added.

For reference, we construct the SpellCheckResult objects from the Android
spellchecker results in SpellCheckerSessionBridge::ProcessSpellCheckResults().

Alternatively, ContextMenuClientImpl has logic for calling the spellchecker
again to get the list of suggestions that we might be able to use:
https://chromium.googlesource.com/chromium/src/+/adc8910776c64a876f0422454da6...

However, I talked to aelias and he said it would be better to store the
suggestions the first time we call the spellchecker, at the time we add the
marker, to avoid going back and forth unnecessarily.

BUG=715365
==========

[please use gerrit instead, 2017-05-09 15:24:26]

rouslan@chromium.org changed reviewers:
- rouslan@chromium.org

[rlanday, 2017-05-09 17:45:03]

On 2017/05/08 at 15:31:22, groby wrote:
> What we currently do is that for all replacements, we restore the original
apostrophes after spell checking. If after that replacement the suggestion
equals the original word, we drop *that suggestion*.
> 
> There might be multiple other suggestions that still apply. Let's assume we
try the possessive of "mart", "mart‘s" with a 'smart quote'
> 
> The suggestions coming back will be "mart's", "mark's", "mare's" - all with
straight apostrophes. We do the back substitution on the first. It's equal to
the original, it should be dropped. But all further suggestions should still be
included, with the apostrophe substitution. I.e. We'd keep "mark's" and "mare's"
with smart quotes.
> 
> We can certainly debate which is the better choice - you've opted for "it's
correct, let's drop it". I'm wondering if that's the right choice, since we also
mix results with the server-side check. (Below, filter==USE_NATIVE_CHECKER). 
> 
> I.e. if server-side suggested "mare's" as a contextually better result, I
believe it should still get a red underline. In your case, it'd get a grey one.
> 
> Yes, I know this is the edge case of an edge case :) If you'd prefer to stick
with your choice, I won't keep objecting - but please document the choice in a
comment, and add a unit test that verifies that specific behavior. 

My concern is that the spellchecker might flag a word as misspelled *solely*
because the apostrophe is the wrong type, and we'd end up putting red underlines
under words that are actually correctly spelled. It appears that the current
code has logic to handle this specific case (if the replacement is the same as
the misspelled word module the apostrophe type, drop the misspelling), so this
must be something we're concerned might happen, right? And then in that case, it
might return more than one suggestion, so the presence of having additional
suggestions that change more than just the apostrophe style doesn't really
indicate anything.

And if we have two spellcheckers involved, one of which is saying that the word
is correct except for the apostrophe change, I don't think the word should be
flagged as misspelled (the user could have added the word to one dictionary but
not the other).

The only case I think really might be problematic is if we have a sort of
"rogue" spellchecker that, for any word containing a curly quote, suggests that
you replace it with a straight quote, regardless of if the new word is correctly
spelled or not. In this case, we wouldn't be able to mark any word containing a
curly quote as misspelled because our spellchecker is returning spurious
results. But I don't know of any way around that.


I'd actually changed the behavior in code to keep the misspelling unless *all*
of the suggestions are only changing the apostrophe, but based on my reasoning
above, I don't think that's correct, so I will change it back. I will add some
code comments and update the test cases I'm adding in spellcheck_unittest.cc.

[rlanday, 2017-05-09 19:10:05]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[rlanday, 2017-05-09 19:10:07]

Updated

[commit-bot: I haz the power, 2017-05-09 19:11:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 20:12:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 20:12:27]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[rlanday, 2017-05-17 22:20:09]

rlanday@chromium.org changed reviewers:
+ aelias@chromium.org

[rlanday, 2017-05-17 22:23:58]

https://codereview.chromium.org/2848943002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebTextCheckingResult.h (right):

https://codereview.chromium.org/2848943002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebTextCheckingResult.h:36: #include
"../platform/WebVector.h"
aelias, question for you since you're a third_party/WebKit/public owner:
apparently files in this directory aren't supposed to be including from
../platform/. Is this something we should fix? If so, how?

[rlanday, 2017-05-17 22:28:31]

On 2017/05/17 at 22:23:58, rlanday wrote:
>
https://codereview.chromium.org/2848943002/diff/80001/third_party/WebKit/publ...
> File third_party/WebKit/public/web/WebTextCheckingResult.h (right):
> 
>
https://codereview.chromium.org/2848943002/diff/80001/third_party/WebKit/publ...
> third_party/WebKit/public/web/WebTextCheckingResult.h:36: #include
"../platform/WebVector.h"
> aelias, question for you since you're a third_party/WebKit/public owner:
apparently files in this directory aren't supposed to be including from
../platform/. Is this something we should fix? If so, how?

Update: groby says writing "public/platform/" instead of "../platform/" fixes
the issue

[rlanday, 2017-05-18 01:38:05]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 01:39:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-05-18 02:20:37]

groby: I looked at your test case and the behavior you want for the quote marks
and I think it makes sense (I think I thought we were dealing with a different
issue than we actually are). I took your behavioral change and your test case
(with some small fixes to make it run properly) and included it in this CL.
PTAL.

Should I consolidate the two spellcheck_unittests?



Further explanation of why I changed my mind for anyone else reading this:

I thought we were dealing with a case where a spellchecker declares that a word
is misspelled simply because it has a curly quote, and I was trying to make sure
we wouldn't flag such words as misspelled.

But I guess since we already convert quotes to straight quotes before running
the spellchecker, what we're really trying to prevent is a spellchecker saying
"hey, you should make this a curly quote" when really the word already had one
and we just changed it to try not to confuse the spellchecker. See
spellcheck_unittest2.cc

[commit-bot: I haz the power, 2017-05-18 05:13:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 05:13:52]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[rlanday, 2017-05-19 20:18:59]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 20:20:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 23:10:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 23:10:09]

Dry run: This issue passed the CQ dry run.

[groby-ooo-7-16, 2017-05-22 14:47:51]

On 2017/05/18 02:20:37, rlanday wrote:
> groby: I looked at your test case and the behavior you want for the quote
marks
> and I think it makes sense (I think I thought we were dealing with a different
> issue than we actually are). I took your behavioral change and your test case
> (with some small fixes to make it run properly) and included it in this CL.
> PTAL.
> 
> Should I consolidate the two spellcheck_unittests?

If you could, much appreciated. I hacked the demo together, and worked in a
separate unittest to affect as little code as possible - but that was strictly
for easier hacking :)
> 
> 
> 
> Further explanation of why I changed my mind for anyone else reading this:
> 
> I thought we were dealing with a case where a spellchecker declares that a
word
> is misspelled simply because it has a curly quote, and I was trying to make
sure
> we wouldn't flag such words as misspelled.
> 
> But I guess since we already convert quotes to straight quotes before running
> the spellchecker, what we're really trying to prevent is a spellchecker saying
> "hey, you should make this a curly quote" when really the word already had one
> and we just changed it to try not to confuse the spellchecker. See
> spellcheck_unittest2.cc

For anyone else reading: spellcheck_unittest2.cc is here:
https://codereview.chromium.org/2891763003/ 
(written as a sketch to outline the issue)

[rlanday, 2017-05-22 20:36:58]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 20:37:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-05-22 20:37:18]

Ok, I've consolidate the test cases

[rlanday, 2017-05-22 20:44:31]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-05-22 20:44:35]

The CQ bit was unchecked by rlanday@chromium.org

[rlanday, 2017-05-23 19:45:35]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 19:46:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 00:26:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 00:26:58]

Dry run: This issue passed the CQ dry run.

[groby-ooo-7-16, 2017-05-25 21:36:14]

lgtm

[rlanday, 2017-05-25 21:59:25]

The CQ bit was checked by rlanday@chromium.org

[commit-bot: I haz the power, 2017-05-25 22:00:29]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 22:04:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-25 22:04:03]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rlanday, 2017-05-25 22:17:18]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-05-25 22:17:19]

The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org
Link to the patchset: https://codereview.chromium.org/2848943002/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-25 22:18:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 22:30:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-25 22:30:46]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[rlanday, 2017-05-25 22:31:32]

The CQ bit was unchecked by rlanday@chromium.org

[rlanday, 2017-05-25 22:34:37]

rlanday@chromium.org changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org

[rlanday, 2017-05-25 22:38:43]

Looks like I need a couple more OWNERS to review this CL:

jochen@, can you please review:
  chrome/browser/renderer_context_menu/spelling_menu_observer.cc
  content/shell/test_runner/mock_spell_check.cc
  content/shell/test_runner/spell_check_client.cc
  third_party/WebKit/Source/core/editing/spellcheck/SpellCheckRequester.h
  third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp
  third_party/WebKit/Source/core/editing/spellcheck/SpellCheckerTest.cpp
  third_party/WebKit/Source/platform/text/TextChecking.h
  third_party/WebKit/Source/web/WebTextCheckingResult.cpp
  third_party/WebKit/public/web/WebTextCheckingResult.h

mkwst@, can you please review:
  components/spellcheck/common/spellcheck_messages.h

[Mike West, 2017-05-26 04:52:59]

Changing IPC from a string to a vector LGTM.

[jochen (gone - plz use gerrit), 2017-05-26 11:38:04]

lgtm

[rlanday, 2017-05-26 16:33:54]

The CQ bit was checked by rlanday@chromium.org

[commit-bot: I haz the power, 2017-05-26 16:34:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 18:07:09]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1495816434581780,
"parent_rev": "6d0877aff7c0a14b421d2635cf60da4c4d25a948", "commit_rev":
"c45f50742410a644a054325478d885e61d286713"}

[commit-bot: I haz the power, 2017-05-26 18:07:22]

Description was changed from

==========
Allow storing multiple replacements on SpellCheckResult

I'm working on adding support for the Android spellcheck suggestion list that
appears in a native text box when you tap on a misspelled word. It appears that
I need to modify this piece of infrastructure to support passing multiple
suggestions through to the Spelling marker that gets added.

For reference, we construct the SpellCheckResult objects from the Android
spellchecker results in SpellCheckerSessionBridge::ProcessSpellCheckResults().

Alternatively, ContextMenuClientImpl has logic for calling the spellchecker
again to get the list of suggestions that we might be able to use:
https://chromium.googlesource.com/chromium/src/+/adc8910776c64a876f0422454da6...

However, I talked to aelias and he said it would be better to store the
suggestions the first time we call the spellchecker, at the time we add the
marker, to avoid going back and forth unnecessarily.

BUG=715365
==========

to

==========
Allow storing multiple replacements on SpellCheckResult

I'm working on adding support for the Android spellcheck suggestion list that
appears in a native text box when you tap on a misspelled word. It appears that
I need to modify this piece of infrastructure to support passing multiple
suggestions through to the Spelling marker that gets added.

For reference, we construct the SpellCheckResult objects from the Android
spellchecker results in SpellCheckerSessionBridge::ProcessSpellCheckResults().

Alternatively, ContextMenuClientImpl has logic for calling the spellchecker
again to get the list of suggestions that we might be able to use:
https://chromium.googlesource.com/chromium/src/+/adc8910776c64a876f0422454da6...

However, I talked to aelias and he said it would be better to store the
suggestions the first time we call the spellchecker, at the time we add the
marker, to avoid going back and forth unnecessarily.

BUG=715365

Review-Url: https://codereview.chromium.org/2848943002
Cr-Commit-Position: refs/heads/master@{#475058}
Committed:
https://chromium.googlesource.com/chromium/src/+/c45f50742410a644a054325478d8...
==========

[commit-bot: I haz the power, 2017-05-26 18:07:28]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/c45f50742410a644a054325478d8...

[Noel Gordon, 2017-05-29 03:27:07]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/2906243002/ by noel@chromium.org.

The reason for reverting is: Caused spell markers to no longer appear on OSX,
per crbug.com/727172.

[Xiaocheng, 2017-05-29 04:00:31]

https://codereview.chromium.org/2848943002/diff/200001/components/spellcheck/...
File components/spellcheck/renderer/spellcheck.cc (right):

https://codereview.chromium.org/2848943002/diff/200001/components/spellcheck/...
components/spellcheck/renderer/spellcheck.cc:499: if
(replacements_adjusted.empty())
This is the culprit of crbug.com/727172:

If |spellcheck_result| doesn't contain any replacement, the old implementation
still returns the result, but the new implementation throws the result away due
to an empty |replacements_adjusted|.

Note that Mac always creates results with empty replacement:

https://cs.chromium.org/chromium/src/components/spellcheck/browser/spellcheck...

[rlanday, 2017-05-29 17:13:27]

Thanks for finding that, xiaochengh@. I apologize for the bug. I will re-upload
a fixed version of the CL tomorrow (today is a US holiday).