autocomplete: Add AutocompleteMatch::MergeClassifications()

autocomplete: Add AutocompleteMatch::MergeClassifications()

This just moves a classifications-merging loop from
ShortcutsProvider::ClassifyAllMatchesInString() into a
utility method.

BUG=141877
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155919

[Peter Kasting, 2012-09-10 18:56:11]

https://chromiumcodereview.appspot.com/10911188/diff/1/chrome/browser/autocom...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://chromiumcodereview.appspot.com/10911188/diff/1/chrome/browser/autocom...
chrome/browser/autocomplete/autocomplete_match.cc:237:
AutocompleteMatch::MergeClassifications(
Nit: Style guide is vague here; I usually suggest this line be indented 4

https://chromiumcodereview.appspot.com/10911188/diff/1/chrome/browser/autocom...
chrome/browser/autocomplete/autocomplete_match.cc:241: for
(ACMatchClassifications::const_iterator i = classifications1.begin(),
This is not quite safe.  If someone passes in an empty vector for
|classifications2| we'll deref garbage.

This function should get a guard like:

namespace {
bool IsTrivialClassification(const ACMatchClassifications& classifications) {
  return classifications.empty() ||
      ((classifications.size() == 1) &&
       (classifications.back().style == ACMatchClassification::NONE));
}
}

...

  if IsTrivialClassification(classifications1)
    return classifications2;
  if IsTrivialClassification(classifications2)
    return classifications1;
  ... (rest of function) ...

Then there should probably be some unittest cases for this.

https://chromiumcodereview.appspot.com/10911188/diff/1/chrome/browser/autocom...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://chromiumcodereview.appspot.com/10911188/diff/1/chrome/browser/autocom...
chrome/browser/autocomplete/shortcuts_provider.cc:298: if ((match_class.size()
== 1) &&
This conditional can be entirely removed once the guard is added to
MergeClassifications() (see comments in autocomplete_match.cc).

[Daniel Erat, 2012-09-10 19:36:17]

https://chromiumcodereview.appspot.com/10911188/diff/1/chrome/browser/autocom...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://chromiumcodereview.appspot.com/10911188/diff/1/chrome/browser/autocom...
chrome/browser/autocomplete/autocomplete_match.cc:237:
AutocompleteMatch::MergeClassifications(
On 2012/09/10 18:56:11, Peter Kasting wrote:
> Nit: Style guide is vague here; I usually suggest this line be indented 4

Done.

https://chromiumcodereview.appspot.com/10911188/diff/1/chrome/browser/autocom...
chrome/browser/autocomplete/autocomplete_match.cc:241: for
(ACMatchClassifications::const_iterator i = classifications1.begin(),
On 2012/09/10 18:56:11, Peter Kasting wrote:
> This is not quite safe.  If someone passes in an empty vector for
> |classifications2| we'll deref garbage.
> 
> This function should get a guard like:
> 
> namespace {
> bool IsTrivialClassification(const ACMatchClassifications& classifications) {
>   return classifications.empty() ||
>       ((classifications.size() == 1) &&
>        (classifications.back().style == ACMatchClassification::NONE));
> }
> }
> 
> ...
> 
>   if IsTrivialClassification(classifications1)
>     return classifications2;
>   if IsTrivialClassification(classifications2)
>     return classifications1;
>   ... (rest of function) ...
> 
> Then there should probably be some unittest cases for this.

Done.

https://chromiumcodereview.appspot.com/10911188/diff/1/chrome/browser/autocom...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://chromiumcodereview.appspot.com/10911188/diff/1/chrome/browser/autocom...
chrome/browser/autocomplete/shortcuts_provider.cc:298: if ((match_class.size()
== 1) &&
On 2012/09/10 18:56:11, Peter Kasting wrote:
> This conditional can be entirely removed once the guard is added to
> MergeClassifications() (see comments in autocomplete_match.cc).

I had to update ShortcutsProviderTest.ClassifyAllMatchesInString(); the
following test expected behavior that's changed:

  class ClassifyTest classify_test5((string16()), ACMatchClassifications());
  ACMatchClassifications spans_j = classify_test5.RunTest(ASCIIToUTF16("man"));
  ASSERT_EQ(0U, spans_j.size());

The comment above ACMatchClassification's definition says, "All text must be
associated with some kind of classification.  Even if a match has no distinct
segments, its vector should contain an entry at offset 0 with no flags."

Should MergeClassifications() enforce this?  Right now, if classifications1 is
"0,0" and classifications2 is empty, we'll return an empty vector.  Would you
prefer that I explicitly check the empty case in MergeClassifications() and
return a valid "0,0" vector instead of possibly returning an empty vector?

[Peter Kasting, 2012-09-10 19:46:42]

Garghragh.

If the string is empty, the corresponding classification vector should be empty
too.  It's wrong to have a "NONE" classification at 0 when there is no text at
0.

If the string is non-empty, we must have a non-empty classification vector.

So the test should indeed be changed, but we can't force the result of
MergeClassifications() to be non-empty.

I think MergeClassifications() should do something this:

  // We must return the empty vector only if both inputs are truly empty.  The
  // result of merging an empty vector with a single (0, NONE) classification is
  // a one-entry vector.
  if (IsTrivialClassification(classifications1)
    return classifications2.empty() ? classifications1 : classifications2;
  if (IsTrivialClassification(classifications2)
    return classifications1;

[Daniel Erat, 2012-09-10 19:56:34]

On 2012/09/10 19:46:42, Peter Kasting wrote:
> Garghragh.
> 
> If the string is empty, the corresponding classification vector should be
empty
> too.  It's wrong to have a "NONE" classification at 0 when there is no text at
> 0.
> 
> If the string is non-empty, we must have a non-empty classification vector.
> 
> So the test should indeed be changed, but we can't force the result of
> MergeClassifications() to be non-empty.
> 
> I think MergeClassifications() should do something this:
> 
>   // We must return the empty vector only if both inputs are truly empty.  The
>   // result of merging an empty vector with a single (0, NONE) classification
is
>   // a one-entry vector.
>   if (IsTrivialClassification(classifications1)
>     return classifications2.empty() ? classifications1 : classifications2;
>   if (IsTrivialClassification(classifications2)
>     return classifications1;

Makes sense; done.

[Peter Kasting, 2012-09-10 23:34:35]

LGTM

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
chrome/browser/autocomplete/autocomplete_match.cc:248: const
ACMatchClassifications& classifications1,
Nit: I would only indent these 4 and not 8?  I seriously don't know what the
style guide really wants for this case, I've seen people do all kinds of crap :/

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
chrome/browser/autocomplete/autocomplete_match.cc:250: // We must return the
empty vector only if both inputs are truly empty.
Nit: Comment should only be indented 2.

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
File chrome/browser/autocomplete/autocomplete_match_unittest.cc (right):

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
chrome/browser/autocomplete/autocomplete_match_unittest.cc:37: EXPECT_EQ("",
Nit: "" -> std::string()

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
chrome/browser/autocomplete/autocomplete_match_unittest.cc:62: EXPECT_EQ("0,1",
Nit: Might want to change one of these cases (or add one) to merge "0,1" with
"0,0"

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
chrome/browser/autocomplete/shortcuts_provider.cc:297: // Merge match-marking
data with original classifications.
Nit: Comment no longer needed

[Daniel Erat, 2012-09-10 23:46:05]

Thanks!

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
chrome/browser/autocomplete/autocomplete_match.cc:248: const
ACMatchClassifications& classifications1,
On 2012/09/10 23:34:35, Peter Kasting wrote:
> Nit: I would only indent these 4 and not 8?  I seriously don't know what the
> style guide really wants for this case, I've seen people do all kinds of crap
:/

Done.

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
chrome/browser/autocomplete/autocomplete_match.cc:250: // We must return the
empty vector only if both inputs are truly empty.
On 2012/09/10 23:34:35, Peter Kasting wrote:
> Nit: Comment should only be indented 2.

Whoops. :-/  Sorry, done.

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
File chrome/browser/autocomplete/autocomplete_match_unittest.cc (right):

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
chrome/browser/autocomplete/autocomplete_match_unittest.cc:37: EXPECT_EQ("",
On 2012/09/10 23:34:35, Peter Kasting wrote:
> Nit: "" -> std::string()

Done.

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
chrome/browser/autocomplete/autocomplete_match_unittest.cc:62: EXPECT_EQ("0,1",
On 2012/09/10 23:34:35, Peter Kasting wrote:
> Nit: Might want to change one of these cases (or add one) to merge "0,1" with
> "0,0"

Done.

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://chromiumcodereview.appspot.com/10911188/diff/4002/chrome/browser/auto...
chrome/browser/autocomplete/shortcuts_provider.cc:297: // Merge match-marking
data with original classifications.
On 2012/09/10 23:34:35, Peter Kasting wrote:
> Nit: Comment no longer needed

Done.

[commit-bot: I haz the power, 2012-09-10 23:47:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/10911188/5004

[commit-bot: I haz the power, 2012-09-11 02:48:42]

Change committed as 155919