Remove and deprecate SEARCH_SUGGEST_ANSWER

Remove and deprecate SEARCH_SUGGEST_ANSWER.  This type is no longer used in the UI code, since answers can be attached to suggestions of any type.

BUG=430513
Committed: https://crrev.com/48569adc0d14e1c996daf22061d70b394dd89691
Cr-Commit-Position: refs/heads/master@{#314167}

[dschuyler, 2014-12-22 19:47:36]

dschuyler@chromium.org changed reviewers:
+ jdonnelly@chromium.org

[Justin Donnelly, 2014-12-23 14:54:38]

Looks good. Just a few comment suggestions. Also, change the title to "Remove
and deprecate SEARCH_SUGGEST_ANSWER".  Finally, it's helpful for the CL
description to have a why as well as a what. How about the following:

Remove and deprecate SEARCH_SUGGEST_ANSWER. This type is no longer used in the
UI code, since answers can be attached to suggestions of any type.

Also, address lint issues in OmniboxSuggestion.java.

https://codereview.chromium.org/815273002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java
(right):

https://codereview.chromium.org/815273002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:59:
SEARCH_SUGGEST_ANSWER (18);       // DEPRECATED.  TODO: remove from
Change this to TODO(dschuyler): Remove once references to this have been removed
from the Java UI code.

https://codereview.chromium.org/815273002/diff/1/components/metrics/proto/omn...
File components/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/815273002/diff/1/components/metrics/proto/omn...
components/metrics/proto/omnibox_event.proto:220: SEARCH_SUGGEST_ANSWER   = 22;
// DEPRECATED.  Use other ResultTypes for
Change to "DEPRECATED. Answers no longer have their own type but instead can be
attached to suggestions of any type."

[dschuyler, 2014-12-23 19:59:06]

Thanks - updated

[dschuyler, 2014-12-23 20:00:15]

https://codereview.chromium.org/815273002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java
(right):

https://codereview.chromium.org/815273002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:59:
SEARCH_SUGGEST_ANSWER (18);       // DEPRECATED.  TODO: remove from
On 2014/12/23 14:54:38, Justin Donnelly wrote:
> Change this to TODO(dschuyler): Remove once references to this have been
removed
> from the Java UI code.

Done.

https://codereview.chromium.org/815273002/diff/1/components/metrics/proto/omn...
File components/metrics/proto/omnibox_event.proto (right):

https://codereview.chromium.org/815273002/diff/1/components/metrics/proto/omn...
components/metrics/proto/omnibox_event.proto:220: SEARCH_SUGGEST_ANSWER   = 22;
// DEPRECATED.  Use other ResultTypes for
On 2014/12/23 14:54:38, Justin Donnelly wrote:
> Change to "DEPRECATED. Answers no longer have their own type but instead can
be
> attached to suggestions of any type."

Done.

[Justin Donnelly, 2014-12-23 20:06:49]

lgtm

[groby-ooo-7-16, 2014-12-23 20:15:21]

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

[groby-ooo-7-16, 2014-12-23 20:15:22]

A few more notes, sorry :)

https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java
(right):

https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:37:
VOICE_SUGGEST        (-100),      // A suggested search from the voice
recognizer.
Please don't change whitespace just for alignments sake. Alignment is not
required[1], and whitespace-only changes make it more difficult to find relevant
changes through history

[1]
https://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s4.6.3-hori...

https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:59:
SEARCH_SUGGEST_ANSWER (18);       // TODO(dschuyler): Remove once references to
You might want to consider filing a bug instead, and leaving the file untouched
for now. :)

https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:78:
|| this == HISTORY_BODY || this == HISTORY_KEYWORD;
Again, please don't commit style-only changes.

https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:209:
(mAnswerContents == null && suggestion.mAnswerContents == null)
See above. If you feel you truly must change the style for this file, please do
so as a separate CL - this has no bearing on functionality

https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
File components/omnibox/autocomplete_match.cc (left):

https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
components/omnibox/autocomplete_match.cc:157: IDR_OMNIBOX_SEARCH,
Question: Do we still want answers to have a search icon attached?

https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
File components/omnibox/autocomplete_match_type.h (left):

https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
components/omnibox/autocomplete_match_type.h:45: SEARCH_SUGGEST_ANSWER       =
18,  // A short result for a suggested search.
I take it this enum is never persisted?

[dschuyler, 2014-12-23 21:02:51]

On 2014/12/23 20:15:22, groby wrote:
> A few more notes, sorry :)
> 
>
https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java
> (right):
> 
>
https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:37:
> VOICE_SUGGEST        (-100),      // A suggested search from the voice
> recognizer.
> Please don't change whitespace just for alignments sake. Alignment is not
> required[1], and whitespace-only changes make it more difficult to find
relevant
> changes through history
> 
> [1]
>
https://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s4.6.3-hori...
> 
>
https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:59:
> SEARCH_SUGGEST_ANSWER (18);       // TODO(dschuyler): Remove once references
to
> You might want to consider filing a bug instead, and leaving the file
untouched
> for now. :)
> 
>
https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:78:
> || this == HISTORY_BODY || this == HISTORY_KEYWORD;
> Again, please don't commit style-only changes.
> 
>
https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:209:
> (mAnswerContents == null && suggestion.mAnswerContents == null)
> See above. If you feel you truly must change the style for this file, please
do
> so as a separate CL - this has no bearing on functionality
> 
>
https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
> File components/omnibox/autocomplete_match.cc (left):
> 
>
https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
> components/omnibox/autocomplete_match.cc:157: IDR_OMNIBOX_SEARCH,
> Question: Do we still want answers to have a search icon attached?
> 
>
https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
> File components/omnibox/autocomplete_match_type.h (left):
> 
>
https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
> components/omnibox/autocomplete_match_type.h:45: SEARCH_SUGGEST_ANSWER       =
> 18,  // A short result for a suggested search.
> I take it this enum is never persisted?

I was getting errors from the java code that prevented the upload unless the
punctuation formatting was changed.
The alignment is not necessary - I'll revert that.
Moving +, &&, || and such to the beginning of the line is what *seems* to be
required.  Maybe I can do the formatting changes first?

[Justin Donnelly, 2014-12-23 21:08:41]

https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java
(right):

https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:78:
|| this == HISTORY_BODY || this == HISTORY_KEYWORD;
On 2014/12/23 20:15:22, groby wrote:
> Again, please don't commit style-only changes.

Is it the number of lines touched that's a problem? I often make lint fixes in
files I'm touching, albeit usually just a line or two here or there.

https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
File components/omnibox/autocomplete_match.cc (left):

https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
components/omnibox/autocomplete_match.cc:157: IDR_OMNIBOX_SEARCH,
On 2014/12/23 20:15:22, groby wrote:
> Question: Do we still want answers to have a search icon attached?

The icon type will be determined by the existing type of whatever an answer is
attached to. IIRC, this is SEARCH_SUGGEST, SEARCH_SUGGEST_PERSONALIZED, and
SEARCH_HISTORY all of which have a search icon.

https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
File components/omnibox/autocomplete_match_type.h (left):

https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
components/omnibox/autocomplete_match_type.h:45: SEARCH_SUGGEST_ANSWER       =
18,  // A short result for a suggested search.
On 2014/12/23 20:15:22, groby wrote:
> I take it this enum is never persisted?

I can't say for absolutely certain, but given that it's (a) not a proto and (b)
there are no other old, deprecated types in this list makes me pretty confident
that it's not.

[groby-ooo-7-16, 2014-12-23 21:11:16]

On 2014/12/23 21:02:51, dschuyler wrote:
> On 2014/12/23 20:15:22, groby wrote:
> > A few more notes, sorry :)
> > 
> >
>
https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:37:
> > VOICE_SUGGEST        (-100),      // A suggested search from the voice
> > recognizer.
> > Please don't change whitespace just for alignments sake. Alignment is not
> > required[1], and whitespace-only changes make it more difficult to find
> relevant
> > changes through history
> > 
> > [1]
> >
>
https://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s4.6.3-hori...
> > 
> >
>
https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:59:
> > SEARCH_SUGGEST_ANSWER (18);       // TODO(dschuyler): Remove once references
> to
> > You might want to consider filing a bug instead, and leaving the file
> untouched
> > for now. :)
> > 
> >
>
https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:78:
> > || this == HISTORY_BODY || this == HISTORY_KEYWORD;
> > Again, please don't commit style-only changes.
> > 
> >
>
https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:209:
> > (mAnswerContents == null && suggestion.mAnswerContents == null)
> > See above. If you feel you truly must change the style for this file, please
> do
> > so as a separate CL - this has no bearing on functionality
> > 
> >
>
https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
> > File components/omnibox/autocomplete_match.cc (left):
> > 
> >
>
https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
> > components/omnibox/autocomplete_match.cc:157: IDR_OMNIBOX_SEARCH,
> > Question: Do we still want answers to have a search icon attached?
> > 
> >
>
https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
> > File components/omnibox/autocomplete_match_type.h (left):
> > 
> >
>
https://codereview.chromium.org/815273002/diff/20001/components/omnibox/autoc...
> > components/omnibox/autocomplete_match_type.h:45: SEARCH_SUGGEST_ANSWER      
=
> > 18,  // A short result for a suggested search.
> > I take it this enum is never persisted?
> 
> I was getting errors from the java code that prevented the upload unless the
> punctuation formatting was changed.
> The alignment is not necessary - I'll revert that.
> Moving +, &&, || and such to the beginning of the line is what *seems* to be
> required.  Maybe I can do the formatting changes first?

Meh. If I read the code correctly, you didn't actually make functional changes,
just added a TODO, correct?  Adding it to crbug is the easier fix. 

But if PRESUBMIT indeed insists of fixing line breaks in unmodified code, yeah,
a separate CL just doing that might be a better choice.

[groby-ooo-7-16, 2014-12-23 21:12:35]

https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java
(right):

https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:78:
|| this == HISTORY_BODY || this == HISTORY_KEYWORD;
On 2014/12/23 21:08:40, Justin Donnelly wrote:
> On 2014/12/23 20:15:22, groby wrote:
> > Again, please don't commit style-only changes.
> 
> Is it the number of lines touched that's a problem? I often make lint fixes in
> files I'm touching, albeit usually just a line or two here or there.

It's unrelated code. But, as Dave pointed out, it seems the Android peeps
enforce that via PRESUBMIT. I'm not going to block the CL over it. 

(I'd really suggest not doing the comment alignment, though)

[dschuyler, 2014-12-23 21:21:34]

On 2014/12/23 21:12:35, groby wrote:
>
https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java
> (right):
> 
>
https://codereview.chromium.org/815273002/diff/20001/chrome/android/java/src/...
>
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:78:
> || this == HISTORY_BODY || this == HISTORY_KEYWORD;
> On 2014/12/23 21:08:40, Justin Donnelly wrote:
> > On 2014/12/23 20:15:22, groby wrote:
> > > Again, please don't commit style-only changes.
> > 
> > Is it the number of lines touched that's a problem? I often make lint fixes
in
> > files I'm touching, albeit usually just a line or two here or there.
> 
> It's unrelated code. But, as Dave pointed out, it seems the Android peeps
> enforce that via PRESUBMIT. I'm not going to block the CL over it. 
> 
> (I'd really suggest not doing the comment alignment, though)

I'll go for rolling back changes to that java file and enter a bug to follow-up
(which is what the comment change was about).
If for no other reason than to practice entering a bug :)

[dschuyler, 2014-12-23 21:22:08]

please re-review without java change

[groby-ooo-7-16, 2014-12-23 21:31:18]

Justin already replied to my comments on the C++ side, so LGTM.

[dschuyler, 2014-12-29 19:27:32]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2014-12-29 19:27:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/815273002/40001

[commit-bot: I haz the power, 2014-12-29 19:33:54]

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

[commit-bot: I haz the power, 2014-12-29 19:33:55]

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

[Justin Donnelly, 2015-01-02 15:35:10]

On 2014/12/29 19:33:55, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Remove the "Also, address [...]" comment from the description and then assign to
pkasting to review.

[Mark P, 2015-01-02 18:20:03]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2015-01-02 18:23:42]

lgtm

Remember to update the internal Chrome-for-Android codebase immediately after
submitting this, else you'll break the Chrome-for-Android compile.  See uses of
SEARCH_SUGGEST_ANSWER in their code.

--mark

[Justin Donnelly, 2015-01-02 18:40:38]

On 2015/01/02 18:23:42, Mark P wrote:
> lgtm
> 
> Remember to update the internal Chrome-for-Android codebase immediately after
> submitting this, else you'll break the Chrome-for-Android compile.  See uses
of
> SEARCH_SUGGEST_ANSWER in their code.
> 
> --mark

The Clank code should be ok. It refers to
org.chromium.chrome.browser.omnibox.OmniboxSuggestion.SEARCH_SUGGEST_ANSWER
which this change doesn't touch.  And that class doesn't depend on any of these
files, it just assumes a numeric correspondence with the C++ values (see
OmniboxSuggestion.getTypeFromNativeType()).

But thanks for mentioning it, Mark, because I overlooked that this will break
Bling downstream.

Dave, I've come up with a better way to do this.  If you don't mind, please
split this up into two CLs. Move the one-line change to
search_suggestion_parser.cc into a new CL.  We'll submit that first.  Then I'll
make the UI code changes to Bling (iOS) and Clank (Android) and submit them.  As
the final step, you can add back in the Java change you originally had here and
submit this CL.

[groby-ooo-7-16, 2015-01-09 18:51:14]

So, is this CL dead?

[Justin Donnelly, 2015-01-09 18:54:08]

On 2015/01/09 18:51:14, groby wrote:
> So, is this CL dead?

No, it's just blocked. Before submitting it we'll submit Dave's other change
(https://codereview.chromium.org/832943002/) after the branch and remove all
references to this type in the Clank and Bling UI code.

[dschuyler, 2015-01-29 23:51:16]

This change list is updated to account for some code changes since the review
started.

[groby-ooo-7-16, 2015-01-30 01:58:46]

A short summary of the changes would probably help :)

That said, LGTM - but please wait for Justin/Mark before committing

[Justin Donnelly, 2015-01-30 22:29:49]

On 2015/01/30 01:58:46, groby wrote:
> A short summary of the changes would probably help :)

The change from the previous version was to remove the change
search_suggestion_parser.cc, which was instead changed in
https://codereview.chromium.org/832943002/.

[Justin Donnelly, 2015-01-30 22:29:56]

Looks good but can you add one more file to this? There's a reference to the
type in the upstreamed Clank code (i.e. in the main Chromium repository):

chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java

[dschuyler, 2015-01-30 23:01:03]

Done: added java file with suggest answer enum removed

[Justin Donnelly, 2015-01-30 23:06:24]

lgtm

LGTM

[Justin Donnelly, 2015-01-30 23:06:46]

jdonnelly@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Justin Donnelly, 2015-01-30 23:07:10]

+tedchoc for Java change

[dschuyler, 2015-01-30 23:09:12]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2015-01-30 23:10:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/815273002/120001

[Mark P, 2015-01-30 23:13:28]

https://codereview.chromium.org/815273002/diff/120001/components/omnibox/auto...
File components/omnibox/autocomplete_match_type.h (right):

https://codereview.chromium.org/815273002/diff/120001/components/omnibox/auto...
components/omnibox/autocomplete_match_type.h:14: // These values are stored in
ShortcutsDatabase and in GetDemotionsByType()
This comment makes me worried about this action.  Did we talk about this
already?  This type was used in the wild, right?  What's going to happen with
shortcuts with a Type value in its database that no longer exists?  (Or are
these Type values transformed *before* storing in the database?)

[Mark P, 2015-01-30 23:14:00]

The CQ bit was unchecked by mpearson@chromium.org

[Justin Donnelly, 2015-01-30 23:21:04]

https://codereview.chromium.org/815273002/diff/120001/components/omnibox/auto...
File components/omnibox/autocomplete_match_type.h (right):

https://codereview.chromium.org/815273002/diff/120001/components/omnibox/auto...
components/omnibox/autocomplete_match_type.h:14: // These values are stored in
ShortcutsDatabase and in GetDemotionsByType()
On 2015/01/30 23:13:27, Mark P wrote:
> This comment makes me worried about this action.  Did we talk about this
> already?  This type was used in the wild, right?  What's going to happen with
> shortcuts with a Type value in its database that no longer exists?  (Or are
> these Type values transformed *before* storing in the database?)

Yeah, you're right. Dave, it looks to me like the right thing to do here is to
update the comment with "(deprecated)" like EXTENSION_APP and
CONTACT_DEPRECATED. Adding _DEPRECATED to the type name seems appropriate as
well.

[Ted C, 2015-01-30 23:29:24]

androidy -- lgtm

[chromium-reviews, 2015-01-30 23:35:49]

So that would bring back a few other entries (to satisfy compile asserts):
 Do these changes look right:


$ git diff
diff --git a/chrome/browser/metrics/omnibox_metrics_provider.cc
b/chrome/browser/metrics/omnibox_metrics_provider.cc
index ffcdc9f..14403d6 100644
--- a/chrome/browser/metrics/omnibox_metrics_provider.cc
+++ b/chrome/browser/metrics/omnibox_metrics_provider.cc
@@ -62,6 +62,7 @@ OmniboxEventProto::Suggestion::ResultType
AsOmniboxEventResultType(
     case AutocompleteMatchType::NAVSUGGEST_PERSONALIZED:
       return OmniboxEventProto::Suggestion::NAVSUGGEST_PERSONALIZED;
     case AutocompleteMatchType::CONTACT_DEPRECATED:
+    case AutocompleteMatchType::SEARCH_SUGGEST_ANSWER_DEPRECATED:
     case AutocompleteMatchType::NUM_TYPES:
       break;
   }
diff --git a/components/omnibox/autocomplete_match.cc
b/components/omnibox/autocomplete_match.cc
index 89aa684..9de0a10d 100644
--- a/components/omnibox/autocomplete_match.cc
+++ b/components/omnibox/autocomplete_match.cc
@@ -155,6 +155,7 @@ int AutocompleteMatch::TypeToIcon(Type type) {
       IDR_OMNIBOX_SEARCH,         // CONTACT_DEPRECATED
       IDR_OMNIBOX_HTTP,           // BOOKMARK_TITLE
       IDR_OMNIBOX_HTTP,           // NAVSUGGEST_PERSONALIZED
+      IDR_OMNIBOX_SEARCH,         // SEARCH_SUGGEST_ANSWER
   };
 #else
   static const int kIcons[] = {
@@ -176,6 +177,7 @@ int AutocompleteMatch::TypeToIcon(Type type) {
       IDR_OMNIBOX_SEARCH,         // CONTACT_DEPRECATED
       IDR_OMNIBOX_HTTP,           // BOOKMARK_TITLE
       IDR_OMNIBOX_HTTP,           // NAVSUGGEST_PERSONALIZED
+      IDR_OMNIBOX_SEARCH,         // SEARCH_SUGGEST_ANSWER
   };
 #endif
   static_assert(arraysize(kIcons) == AutocompleteMatchType::NUM_TYPES,
diff --git a/components/omnibox/autocomplete_match_type.cc
b/components/omnibox/autocomplete_match_type.cc
index d50fbf6..a5c97b0 100644
--- a/components/omnibox/autocomplete_match_type.cc
+++ b/components/omnibox/autocomplete_match_type.cc
@@ -27,6 +27,7 @@ std::string
AutocompleteMatchType::ToString(AutocompleteMatchType::Type type) {
     "contact",
     "bookmark-title",
     "navsuggest-personalized",
+    "search-suggest-answer",
   };
   static_assert(arraysize(strings) == AutocompleteMatchType::NUM_TYPES,
                 "strings array must have NUM_TYPES elements");
diff --git a/components/omnibox/autocomplete_match_type.h
b/components/omnibox/autocomplete_match_type.h
index 9e539c3..0c107a6 100644
--- a/components/omnibox/autocomplete_match_type.h
+++ b/components/omnibox/autocomplete_match_type.h
@@ -42,6 +42,8 @@ struct AutocompleteMatchType {
     BOOKMARK_TITLE              = 16,  // A bookmark whose title contains
the
                                        // input.
     NAVSUGGEST_PERSONALIZED     = 17,  // A personalized suggestion URL.
+    SEARCH_SUGGEST_ANSWER_DEPRECATED = 18,  // DEPRECATED. A short result
+                                            // for a suggested search.
     NUM_TYPES,
   };


On Fri, Jan 30, 2015 at 3:21 PM, <jdonnelly@chromium.org> wrote:

>
> https://codereview.chromium.org/815273002/diff/120001/components/omnibox/
> autocomplete_match_type.h
> File components/omnibox/autocomplete_match_type.h (right):
>
> https://codereview.chromium.org/815273002/diff/120001/components/omnibox/
> autocomplete_match_type.h#newcode14
> components/omnibox/autocomplete_match_type.h:14: // These values are
> stored in ShortcutsDatabase and in GetDemotionsByType()
> On 2015/01/30 23:13:27, Mark P wrote:
>
>> This comment makes me worried about this action.  Did we talk about
>>
> this
>
>> already?  This type was used in the wild, right?  What's going to
>>
> happen with
>
>> shortcuts with a Type value in its database that no longer exists?
>>
> (Or are
>
>> these Type values transformed *before* storing in the database?)
>>
>
> Yeah, you're right. Dave, it looks to me like the right thing to do here
> is to update the comment with "(deprecated)" like EXTENSION_APP and
> CONTACT_DEPRECATED. Adding _DEPRECATED to the type name seems
> appropriate as well.
>
> https://codereview.chromium.org/815273002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dschuyler, 2015-01-30 23:59:51]

adding SEARCH_SUGGEST_ANSWER_DEPRECATED

[Mark P, 2015-01-31 00:06:57]

mpearson@chromium.org changed reviewers:
+ pkasting@google.com

[Mark P, 2015-01-31 00:10:22]

+pkasting

Okay, I'm sorry to jerk you around.  I just looked closer at the Shortcuts
provider code.  It appears to me that anything that's of type specialized search
is treated by Shortcuts provider as SEARCH_HISTORY.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
More importantly, it appears to me that we do this transformation *before*
writing a shortcut to the database.  This means there should be no databases
with this autocomplete_match_type value in them.  I'm adding pkasting@ to
confirm this.  If so, then you can submit patchset 6 (not 7).

--mark

[Mark P, 2015-01-31 00:11:03]

+pkasting

Okay, I'm sorry to jerk you around.  I just looked closer at the Shortcuts
provider code.  It appears to me that anything that's of type specialized search
is treated by Shortcuts provider as SEARCH_HISTORY.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
More importantly, it appears to me that we do this transformation *before*
writing a shortcut to the database.  This means there should be no databases
with this autocomplete_match_type value in them.  I'm adding pkasting@ to
confirm this.  If so, then you can submit patchset 6 (not 7).

--mark

[Peter Kasting, 2015-01-31 00:13:20]

On 2015/01/31 00:11:03, Mark P wrote:
> If so, then you can submit patchset 6 (not 7).

I think you meant "patchset 7 (not 8)".

In theory we're supposed to not write special answer types to the shortcuts DB. 
I think patchset 7 is fine.

[Mark P, 2015-01-31 00:22:22]

On 2015/01/31 00:13:20, Peter Kasting wrote:
> On 2015/01/31 00:11:03, Mark P wrote:
> > If so, then you can submit patchset 6 (not 7).
> 
> I think you meant "patchset 7 (not 8)".
> 
> In theory we're supposed to not write special answer types to the shortcuts
DB. 
> I think patchset 7 is fine.

Okay, dschuyler, can you please revert to patchset 7.  I'll be happy to lgtm
that one.

--mark

[Mark P, 2015-01-31 00:22:36]

-lgtm
(for this one)

[Mark P, 2015-01-31 00:22:49]

not lgtm
(for this one)

[dschuyler, 2015-01-31 00:36:41]

Patchset #8 (id:140001) has been deleted

[dschuyler, 2015-01-31 00:37:09]

Patch set 8 removed :)

[Mark P, 2015-01-31 00:43:33]

lgtm

[dschuyler, 2015-02-02 18:09:58]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2015-02-02 18:10:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/815273002/120001

[commit-bot: I haz the power, 2015-02-02 18:15:01]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-02 18:16:57]

Patchset 7 (id:??) landed as
https://crrev.com/48569adc0d14e1c996daf22061d70b394dd89691
Cr-Commit-Position: refs/heads/master@{#314167}