Upstream AutocompleteController and related classes.

Upstream AutocompleteController and related classes.

Upstreams AutocompleteController and OmniboxSuggestion on java side.
Upstreams autocomplete_bridge.cc and renames it to
autocomplete_controller_android.cc.

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

[aurimas (slooooooooow), 2014-05-12 22:55:42]

Can we add some basic tests as well?

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
File chrome/browser/android/omnibox/autocomplete_controller_android.h (right):

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.h:29: class
AutocompleteControllerAndroid : public AutocompleteControllerDelegate,
Would it make sense to name it AutocompleteController (without Android, since it
is already in the android/ directory?

[Maria, 2014-05-12 23:18:29]

Agree that it would be nice to have test coverage for this. However currently
it's difficult because of the JNI aspect. I think we should create a task to
follow up on how we can add test coverage for these types of classes.

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
File chrome/browser/android/omnibox/autocomplete_controller_android.h (right):

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.h:29: class
AutocompleteControllerAndroid : public AutocompleteControllerDelegate,
On 2014/05/12 22:55:42, aurimas wrote:
> Would it make sense to name it AutocompleteController (without Android, since
it
> is already in the android/ directory?

We use the actual chrome/browser/autocomplete AutocompleteController within this
class -- if we name this class the same, we will have to use namespaces in the
code to differentiate. This way it's more clear which class we are referring to
and it seems to be what we have done in other places where the names are the
same.

[Ted C, 2014-05-14 00:49:23]

lgtm w/ style nits

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

https://codereview.chromium.org/285633002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
2014!

https://codereview.chromium.org/285633002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:22:
private long mNativeAutocompleteControllerAndroid; // Used by native code
I think we can remove this trailing comment

https://codereview.chromium.org/285633002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:46:
* Resets the underlying autocomplete controller based on the specified profile .
there is oddly an extra blank space before the .

https://codereview.chromium.org/285633002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:168:
public void onSuggestionsReceived(
can this be protected instead of public (and really it should be private, so
maybe @VisibleForTesting)?

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

https://codereview.chromium.org/285633002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:1:
// Copyright 2010 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/285633002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:65:
if (t.mNativeType == nativeType)
make this a one liner

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
File chrome/browser/android/omnibox/autocomplete_controller_android.cc (right):

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:54: const char
kChromeUINativeNewTabURL[] = "chrome-native://newtab/";
this has in fact landed, so mind cleaning up the TODO here?

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:128: if
(autocomplete_controller_ == NULL)
I think in chrome, it is now more common to see

if (!autocomplete_controller_)
  return;

At least for pointers.

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:148: jobject
obj,
indenting is off on these params (and else where below due to class renaming)

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:382: env,
match.type, match.relevance,
I think each one of these needs to go on a new line since they don't all fit on
a single one.  Glancing up, I see it occurs a few other places in this file as
well.

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:390: if
(profile_ == NULL) {
no braces needed.

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
File chrome/browser/android/omnibox/autocomplete_controller_android.h (right):

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.h:1: // Copyright
2011 The Chromium Authors. All rights reserved.
2014 :-P

i'll just say this applies everywhere

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.h:103: //
Notifies the Java LocationBar that suggestions were received based on the
s/LocationBar/AutocompleteController

[Maria, 2014-05-14 01:54:22]

PTAL. Made changes and added a question in the comments.

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

https://codereview.chromium.org/285633002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/05/14 00:49:23, Ted C wrote:
> 2014!

Done.

https://codereview.chromium.org/285633002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:22:
private long mNativeAutocompleteControllerAndroid; // Used by native code
On 2014/05/14 00:49:23, Ted C wrote:
> I think we can remove this trailing comment

Done.

https://codereview.chromium.org/285633002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:46:
* Resets the underlying autocomplete controller based on the specified profile .
On 2014/05/14 00:49:23, Ted C wrote:
> there is oddly an extra blank space before the .

Done.

https://codereview.chromium.org/285633002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteController.java:168:
public void onSuggestionsReceived(
On 2014/05/14 00:49:23, Ted C wrote:
> can this be protected instead of public (and really it should be private, so
> maybe @VisibleForTesting)?

Made it protected. This cannot be private not only because of testing, but it's
also overriden by a subclass.

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

https://codereview.chromium.org/285633002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:1:
// Copyright 2010 The Chromium Authors. All rights reserved.
On 2014/05/14 00:49:23, Ted C wrote:
> 2014

Done.

https://codereview.chromium.org/285633002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:65:
if (t.mNativeType == nativeType)
On 2014/05/14 00:49:23, Ted C wrote:
> make this a one liner

Done.

https://codereview.chromium.org/285633002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxSuggestion.java:155:
(mIsStarred ? 1 : 0) + (mIsDeletable ? 1 : 0);
FYI, I fixed up hashCode() method as part of this move, it didn't used to use
all the variables in equals()

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
File chrome/browser/android/omnibox/autocomplete_controller_android.cc (right):

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:54: const char
kChromeUINativeNewTabURL[] = "chrome-native://newtab/";
On 2014/05/14 00:49:23, Ted C wrote:
> this has in fact landed, so mind cleaning up the TODO here?

Done.

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:128: if
(autocomplete_controller_ == NULL)
On 2014/05/14 00:49:23, Ted C wrote:
> I think in chrome, it is now more common to see
> 
> if (!autocomplete_controller_)
>   return;
> 
> At least for pointers.

Done.

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:148: jobject
obj,
On 2014/05/14 00:49:23, Ted C wrote:
> indenting is off on these params (and else where below due to class renaming)

Done.

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:303:
"AutocompleteControllerAndroid",
Btw, I changed the key string here and I assume it's harmless since I couldn't
find any use of it, but wanted to check with you.

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:382: env,
match.type, match.relevance,
On 2014/05/14 00:49:23, Ted C wrote:
> I think each one of these needs to go on a new line since they don't all fit
on
> a single one.  Glancing up, I see it occurs a few other places in this file as
> well.

Done. For the record, I think it's crazy to be passing 1 param per line. But the
style guide does seem to imply this is the thing to do. Much sadness.

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:390: if
(profile_ == NULL) {
On 2014/05/14 00:49:23, Ted C wrote:
> no braces needed.

Done.

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
File chrome/browser/android/omnibox/autocomplete_controller_android.h (right):

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.h:1: // Copyright
2011 The Chromium Authors. All rights reserved.
On 2014/05/14 00:49:23, Ted C wrote:
> 2014 :-P
> 
> i'll just say this applies everywhere

Done.

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.h:103: //
Notifies the Java LocationBar that suggestions were received based on the
On 2014/05/14 00:49:23, Ted C wrote:
> s/LocationBar/AutocompleteController

Done.

[Ted C, 2014-05-15 18:10:52]

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
File chrome/browser/android/omnibox/autocomplete_controller_android.cc (right):

https://codereview.chromium.org/285633002/diff/20001/chrome/browser/android/o...
chrome/browser/android/omnibox/autocomplete_controller_android.cc:303:
"AutocompleteControllerAndroid",
On 2014/05/14 01:54:22, Maria wrote:
> Btw, I changed the key string here and I assume it's harmless since I couldn't
> find any use of it, but wanted to check with you.

Correct, you just want the name to be unique...and this certainly qualifies.

[Maria, 2014-05-15 20:17:42]

The CQ bit was checked by mariakhomenko@chromium.org

[commit-bot: I haz the power, 2014-05-15 20:18:13]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/mariakhomenko@chromium.org/285633002/4...

[commit-bot: I haz the power, 2014-05-15 23:52:22]

Change committed as 270861