[ChromeShell] Add suggestions for ChromeShell toolbar box.

[ChromeShell] Add suggestions for ChromeShell toolbar box.

BUG=

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

[aurimas (slooooooooow), 2014-05-20 16:51:05]

Hey Tommy,

PTAL.

Aurimas

[nyquist, 2014-05-21 18:53:07]

https://codereview.chromium.org/287293004/diff/40001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java
(right):

https://codereview.chromium.org/287293004/diff/40001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:34:
private Context mContext;
mContext, mUrlField and mToolbar should be final.

https://codereview.chromium.org/287293004/diff/40001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:39:
private AutocompleteController mAutocomplete;
final

Also, reorder all final members to be organized together so it is easy to see
what can change over the lifetime of the object.

https://codereview.chromium.org/287293004/diff/40001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:46:
public void afterTextChanged(final Editable editableText) {
Like we discussed offline, this could all move to onTextChanged, which I would
prefer. It just feels a little bit strange right now.

But I'm fine with it staying where it is now since the reasoning is to keep this
closer to how things are done downstream.

https://codereview.chromium.org/287293004/diff/40001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:73:
if (mRequestSuggestions != null) mRequestSuggestions = null;
Is this if-check really necessary? Seems like the outcome would be the same
regardless.

https://codereview.chromium.org/287293004/diff/40001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:81:
private class SuggestionArrayAdapter extends ArrayAdapter<OmniboxSuggestion> {
Could this class be final too?

https://codereview.chromium.org/287293004/diff/40001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:82:
private List<OmniboxSuggestion> mSuggestions;
final

https://codereview.chromium.org/287293004/diff/40001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:84:
public SuggestionArrayAdapter(Context context, int res, List<OmniboxSuggestion>
objects) {
s/objects/suggestions/

https://codereview.chromium.org/287293004/diff/40001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:111:
mContext = context;
Is it OK for this to only be an activity? I.e. will it ever be used outside of
the Activity lifespan? If we need it for more time, consider using
context.getApplicationContext() and rename member field to mApplicationContext.

[aurimas (slooooooooow), 2014-05-21 21:55:07]

https://chromiumcodereview.appspot.com/287293004/diff/40001/chrome/android/sh...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java
(right):

https://chromiumcodereview.appspot.com/287293004/diff/40001/chrome/android/sh...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:34:
private Context mContext;
On 2014/05/21 18:53:08, nyquist wrote:
> mContext, mUrlField and mToolbar should be final.

Done.

https://chromiumcodereview.appspot.com/287293004/diff/40001/chrome/android/sh...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:39:
private AutocompleteController mAutocomplete;
On 2014/05/21 18:53:08, nyquist wrote:
> final
> 
> Also, reorder all final members to be organized together so it is easy to see
> what can change over the lifetime of the object.

Done.

https://chromiumcodereview.appspot.com/287293004/diff/40001/chrome/android/sh...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:46:
public void afterTextChanged(final Editable editableText) {
On 2014/05/21 18:53:08, nyquist wrote:
> Like we discussed offline, this could all move to onTextChanged, which I would
> prefer. It just feels a little bit strange right now.
> 
> But I'm fine with it staying where it is now since the reasoning is to keep
this
> closer to how things are done downstream.

Done.

https://chromiumcodereview.appspot.com/287293004/diff/40001/chrome/android/sh...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:73:
if (mRequestSuggestions != null) mRequestSuggestions = null;
On 2014/05/21 18:53:08, nyquist wrote:
> Is this if-check really necessary? Seems like the outcome would be the same
> regardless.

Done.

https://chromiumcodereview.appspot.com/287293004/diff/40001/chrome/android/sh...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:81:
private class SuggestionArrayAdapter extends ArrayAdapter<OmniboxSuggestion> {
On 2014/05/21 18:53:08, nyquist wrote:
> Could this class be final too?

Done.

https://chromiumcodereview.appspot.com/287293004/diff/40001/chrome/android/sh...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:82:
private List<OmniboxSuggestion> mSuggestions;
On 2014/05/21 18:53:08, nyquist wrote:
> final

Done.

https://chromiumcodereview.appspot.com/287293004/diff/40001/chrome/android/sh...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:84:
public SuggestionArrayAdapter(Context context, int res, List<OmniboxSuggestion>
objects) {
On 2014/05/21 18:53:08, nyquist wrote:
> s/objects/suggestions/

Done.

https://chromiumcodereview.appspot.com/287293004/diff/40001/chrome/android/sh...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellSuggestionPopup.java:111:
mContext = context;
On 2014/05/21 18:53:08, nyquist wrote:
> Is it OK for this to only be an activity? I.e. will it ever be used outside of
> the Activity lifespan? If we need it for more time, consider using
> context.getApplicationContext() and rename member field to
mApplicationContext.

I don't think it will every outlive the activity. New toolbar will be created if
new activity is created.

[nyquist, 2014-05-21 22:05:46]

lgtm

[aurimas (slooooooooow), 2014-05-21 22:28:16]

mariakhomenko: PTAL

[Maria, 2014-05-21 22:55:20]

lgtm

https://chromiumcodereview.appspot.com/287293004/diff/60001/chrome/android/sh...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionArrayAdapter.java
(right):

https://chromiumcodereview.appspot.com/287293004/diff/60001/chrome/android/sh...
chrome/android/shell/java/src/org/chromium/chrome/shell/omnibox/SuggestionArrayAdapter.java:46:
t2.setText(mSuggestions.get(position).getUrl());
FYI, you may want to check suggestion type and not show the URL for search
suggestions? They are common and the URLs for them look very similar to each
other, so it looks weird.

[aurimas (slooooooooow), 2014-05-21 23:37:56]

The CQ bit was checked by aurimas@chromium.org

[commit-bot: I haz the power, 2014-05-21 23:42:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aurimas@chromium.org/287293004/60001

[commit-bot: I haz the power, 2014-05-22 08:54:27]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-22 08:57:06]

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

[commit-bot: I haz the power, 2014-05-22 08:57:06]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[aurimas (slooooooooow), 2014-05-22 17:58:16]

The CQ bit was checked by aurimas@chromium.org

[commit-bot: I haz the power, 2014-05-22 17:59:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aurimas@chromium.org/287293004/80001

[aurimas (slooooooooow), 2014-05-22 20:16:29]

The CQ bit was checked by aurimas@chromium.org

[commit-bot: I haz the power, 2014-05-22 20:18:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aurimas@chromium.org/287293004/100001

[commit-bot: I haz the power, 2014-05-22 23:28:32]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-23 00:42:38]

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

[commit-bot: I haz the power, 2014-05-23 00:42:38]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[aurimas (slooooooooow), 2014-05-23 01:12:26]

The CQ bit was checked by aurimas@chromium.org

[commit-bot: I haz the power, 2014-05-23 01:13:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aurimas@chromium.org/287293004/100001

[commit-bot: I haz the power, 2014-05-23 02:37:02]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-23 03:27:45]

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

[commit-bot: I haz the power, 2014-05-23 03:27:45]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[aurimas (slooooooooow), 2014-05-23 03:38:12]

The CQ bit was checked by aurimas@chromium.org

[commit-bot: I haz the power, 2014-05-23 03:38:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aurimas@chromium.org/287293004/100001

[commit-bot: I haz the power, 2014-05-23 04:45:02]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-23 06:07:53]

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

[commit-bot: I haz the power, 2014-05-23 06:07:54]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[aurimas (slooooooooow), 2014-05-23 12:06:07]

The CQ bit was checked by aurimas@chromium.org

[commit-bot: I haz the power, 2014-05-23 12:06:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aurimas@chromium.org/287293004/100001

[commit-bot: I haz the power, 2014-05-23 13:12:43]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-23 13:51:58]

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

[commit-bot: I haz the power, 2014-05-23 13:51:59]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[aurimas (slooooooooow), 2014-05-27 15:52:50]

The CQ bit was checked by aurimas@chromium.org

[commit-bot: I haz the power, 2014-05-27 15:53:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aurimas@chromium.org/287293004/100001

[commit-bot: I haz the power, 2014-05-27 17:26:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-27 18:24:16]

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

[commit-bot: I haz the power, 2014-05-27 18:24:18]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[aurimas (slooooooooow), 2014-05-27 18:26:42]

The CQ bit was checked by aurimas@chromium.org

[commit-bot: I haz the power, 2014-05-27 18:27:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aurimas@chromium.org/287293004/120001

[commit-bot: I haz the power, 2014-05-27 22:19:55]

Change committed as 273052