[AiS] Add a structured representation of Answer JSON.

[AiS] Add a structured representation of Answer JSON.

BUG=383907
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278217

[Justin Donnelly, 2014-06-12 22:26:24]

Hi Ted - I'm adding more tests, but I was hoping you could take a quick look at
this and let me know if you think it's on the right track.

See https://chrome-internal-review.googlesource.com/#/c/166055/ for the related
downstream change.

[groby-ooo-7-16, 2014-06-12 23:10:50]

Drive-by comments...

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

https://codereview.chromium.org/332943002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionAnswer.java:52:
return null;
Might want to handle the "more than two lines" case, too. It's still a malformed
answer.

https://codereview.chromium.org/332943002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionAnswer.java:76:
* Represents a single line of an answer, contianing any number of typed text
fields and an
"containing"

https://codereview.chromium.org/332943002/diff/1/chrome/android/javatests/src...
File
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/SuggestionAnswerTest.java
(right):

https://codereview.chromium.org/332943002/diff/1/chrome/android/javatests/src...
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/SuggestionAnswerTest.java:28:
String json = "{ \"l\": [ { \"il\": { \"t\": [{\"t\": \"text\", \"tt\": 8 }] }}
] }";
That's rather hard to read - I'd prefer using single quotes inside the string,
then json.replace('\'','"');

[Ted C, 2014-06-13 22:39:37]

overall seems very reasonable to me.  I'll give it a thorough review, but I
don't see anything that would require any sort of large changes.

[Ted C, 2014-06-13 23:54:17]

Ok...i lied.  I think the suggestion positioning in the list is important, so I
think you'll want to sync with those more in the know about how best to solve
it.

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

https://codereview.chromium.org/332943002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:218:
if (!TextUtils.isEmpty(suggestion.getAnswerContents()) &&
!suggestion.hasAnswer()) {
I just remembered that we log selections based on position.  If we change the
ordering in the list, then these will be off.

Take a look at onSuggestionSelected(...) below.  I don't know if it is something
we can cull from the native results or not.  You'll probably want to check with
pkasting@ about that.  Even if we add position information to the suggestions
and separate it from the list position, I suspect it would mess up our tracking
as it would appear people are clicking on suggestions further down the list.

I don't know if falling back to a search suggestion is better or not than this.

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

https://codereview.chromium.org/332943002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionAnswer.java:101:
public List<TextField> getTextFields() {
I would javadoc these methods to give an idea of how they are to be used...same
for TextField.

i.e. I have no idea what getAdditionalText() is in relation to getTextFields()
or what status means...etc...

[Justin Donnelly, 2014-06-16 19:32:54]

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

https://codereview.chromium.org/332943002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:218:
if (!TextUtils.isEmpty(suggestion.getAnswerContents()) &&
!suggestion.hasAnswer()) {
On 2014/06/13 23:54:16, Ted C wrote:
> I just remembered that we log selections based on position.  If we change the
> ordering in the list, then these will be off.
> 
> Take a look at onSuggestionSelected(...) below.  I don't know if it is
something
> we can cull from the native results or not.  You'll probably want to check
with
> pkasting@ about that.  Even if we add position information to the suggestions
> and separate it from the list position, I suspect it would mess up our
tracking
> as it would appear people are clicking on suggestions further down the list.
> 
> I don't know if falling back to a search suggestion is better or not than
this.

Well, on further reflection, maybe fallback is better anyway.  If we get an
Answer for "weather los angeles" I think that replaces the simple "weather los
angeles" suggestion we otherwise would have gotten.  To be honest, my motivation
to avoid fallback was just that fallthrough in a switch statement makes me
queasy.  Might be better to just rework that code a bit to avoid the
fallthrough.

But then there's Peter's point in my other CL about logging something as an
Answer and then later displaying it as just a regular suggestion.  Let's discuss
in the meeting this afternoon.

[Justin Donnelly, 2014-06-17 21:34:03]

PTAL

Responded to comments, added image handling to the JSON parsing, and added more
tests.

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

https://codereview.chromium.org/332943002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:218:
if (!TextUtils.isEmpty(suggestion.getAnswerContents()) &&
!suggestion.hasAnswer()) {
On 2014/06/16 19:32:54, Justin Donnelly wrote:
> On 2014/06/13 23:54:16, Ted C wrote:
> > I just remembered that we log selections based on position.  If we change
the
> > ordering in the list, then these will be off.
> > 
> > Take a look at onSuggestionSelected(...) below.  I don't know if it is
> something
> > we can cull from the native results or not.  You'll probably want to check
> with
> > pkasting@ about that.  Even if we add position information to the
suggestions
> > and separate it from the list position, I suspect it would mess up our
> tracking
> > as it would appear people are clicking on suggestions further down the list.
> > 
> > I don't know if falling back to a search suggestion is better or not than
> this.
> 
> Well, on further reflection, maybe fallback is better anyway.  If we get an
> Answer for "weather los angeles" I think that replaces the simple "weather los
> angeles" suggestion we otherwise would have gotten.  To be honest, my
motivation
> to avoid fallback was just that fallthrough in a switch statement makes me
> queasy.  Might be better to just rework that code a bit to avoid the
> fallthrough.
> 
> But then there's Peter's point in my other CL about logging something as an
> Answer and then later displaying it as just a regular suggestion.  Let's
discuss
> in the meeting this afternoon.

Per our discussion yesterday, removed this code.  We'll stick with fallback to
display as a regular search suggestion in SuggestionView.  This means we'll
potentially log answers incorrectly if the JSON is malformed until we move the
JSON parsing and validation to the C++ side.  Since we're just launching as a 1%
experiment and there's no reason to believe that the JSON will ever be
malformed, we're not overly concerned about this.

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

https://codereview.chromium.org/332943002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionAnswer.java:52:
return null;
On 2014/06/12 23:10:49, groby wrote:
> Might want to handle the "more than two lines" case, too. It's still a
malformed
> answer.

Done.

https://codereview.chromium.org/332943002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionAnswer.java:76:
* Represents a single line of an answer, contianing any number of typed text
fields and an
On 2014/06/12 23:10:49, groby wrote:
> "containing"

Done.

https://codereview.chromium.org/332943002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionAnswer.java:101:
public List<TextField> getTextFields() {
On 2014/06/13 23:54:16, Ted C wrote:
> I would javadoc these methods to give an idea of how they are to be
used...same
> for TextField.
> 
> i.e. I have no idea what getAdditionalText() is in relation to getTextFields()
> or what status means...etc...

Done.

https://codereview.chromium.org/332943002/diff/1/chrome/android/javatests/src...
File
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/SuggestionAnswerTest.java
(right):

https://codereview.chromium.org/332943002/diff/1/chrome/android/javatests/src...
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/SuggestionAnswerTest.java:28:
String json = "{ \"l\": [ { \"il\": { \"t\": [{\"t\": \"text\", \"tt\": 8 }] }}
] }";
On 2014/06/12 23:10:49, groby wrote:
> That's rather hard to read - I'd prefer using single quotes inside the string,
> then json.replace('\'','"');

No need to replace, I should've just used single quotes in the first place. 
Fixed.

[Ted C, 2014-06-18 17:04:41]

lgtm

[Justin Donnelly, 2014-06-18 17:10:16]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-06-18 17:10:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/332943002/20001

[commit-bot: I haz the power, 2014-06-19 01:14:52]

Change committed as 278217