Closed Bug 586885 Opened 14 years ago Closed 12 years ago

show search suggestions when entering text in awesome bar

Categories

(Firefox for Android Graveyard :: General, enhancement, P3)

enhancement

Tracking

(firefox15 verified, firefox16 verified, firefox17 verified, blocking-fennec1.0 -, fennec15+)

VERIFIED FIXED
Firefox 16
Tracking Status
firefox15 --- verified
firefox16 --- verified
firefox17 --- verified
blocking-fennec1.0 --- -
fennec 15+ ---

People

(Reporter: blassey, Assigned: bnicholson)

References

Details

(Keywords: privacy-review-needed, uiwanted)

Attachments

(12 files, 11 obsolete files)

10.98 KB, patch
Gavin
: feedback+
Details | Diff | Splinter Review
6.15 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
3.01 KB, patch
mfinkle
: review+
ibarlow
: review+
Details | Diff | Splinter Review
328.07 KB, image/png
Details
4.46 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
47.48 KB, patch
mfinkle
: review+
lucasr
: review+
Details | Diff | Splinter Review
4.66 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
8.13 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
1.30 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
561 bytes, patch
armenzg
: review+
Details | Diff | Splinter Review
72.15 KB, patch
Details | Diff | Splinter Review
27.02 KB, patch
Details | Diff | Splinter Review
One of Fennec's goals has been to limit the amount of typing a user has to do, but we don't utilize search suggestions like firefox does. I have no idea how we can integrate search suggestions with awesomebar suggestions, except for fully switching to search suggestions when the space bar is hit. But hopefully Madhava has been thinking about it. Another consideration is combining suggestions from multiple search providers.
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0b2+
do we really want to send every user key press to google?  sounds like we need a discussion about this.  part of me LOVES google suggestions, part of me doesn't like doing this for everything we type in our ui.  mike, ideas?
I thought to have seen IE9 screenshots (yesterday) asking to the user (by a direct link into the search results) if he wants to use bing's suggestions. We could potentially do that with google's suggestions?
tracking-fennec: 2.0b2+ → 2.0-
Would be nice to have, but we have no design and no patch
Attached patch WIP (obsolete) — Splinter Review
Here's a WIP patch that gets this up and running the easiest way possible. It uses the FF search-suggestion component. Ideally, I think:

1.) it would only check for results if we didn't have some minimum number of history results to show.
2.) it would also not throw errors when it can't find form history stuff.
3.) we'd provide some way to determine who was being used for the search (we could pretty easily change the search provider to whoever you did your last search with)

It would be easy to move the Firefox component over... or maybe to write an autocomplete controller that queried both history and the search service in one. Wanted some feedback before I moved on.
Assignee: nobody → wjohnston
Attachment #478610 - Flags: feedback?(mark.finkle)
Comment on attachment 478610 [details] [diff] [review]
WIP

I like the feature and the simple way it is added.


I'm a bit worried about the performance impact of it thought (but it looks fast on Desktop). We need to be sure it does not regress typing and awesome bar display on devices.

I'm seeing this error a lot while typing:
 * Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "formHistoryResult.wrappedJSObject is undefined" {file: "file:///home/steakdepapillon/Devel/mozilla-fennec/build/dist/bin/components/nsInputListAutoComplete.js" line: 58}]' when calling method: [nsIInputListAutoComplete::autoCompleteSearch]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///home/steakdepapillon/Devel/mozilla-fennec/build/dist/bin/components/nsSearchSuggestions.js :: SAC_SHSearch :: line 258"  data: yes]
There is also a bug when there is no results, the search suggestion icon can appear on the right side (you need to reset it into this function http://mxr.mozilla.org/mobile-browser/source/chrome/content/bindings.xml#309)

If we decide to go into this direction we need to file a bug to be able to change the default search engine as suggested by Wes.
Attachment #478610 - Flags: feedback+
(In reply to comment #4)
> Created attachment 478610 [details] [diff] [review]

> It would be easy to move the Firefox component over... or maybe to write an
> autocomplete controller that queried both history and the search service in
> one. Wanted some feedback before I moved on.

We do already have an AutoCompleteCache.js component that you could add this to.
I don't think we should be doing this for Fennec 2.0, myself. Feels like it adds a bunch of new things to test and a new feature at a time when we're looking to converge and close off.

There's also no design document for this (that I know of) and indicating which items are suggestions (and indeed, how many suggestions to show) would require a pretty big reworking of the awesomescreen.

I'd move this for post-2.0.
Yep, it's not blocking 2.0; I think an add-on might be interesting, if we have enough hooks to make it happen.

I don't expect a design for this in the 2.0 timeframe, so we should not expect this to land in 2.0 final.
Attachment #478610 - Flags: feedback?(mark.finkle)
Whiteboard: awesomepanel → [fennec-4.1?]
Madhava - Thoughts on this idea?
tracking-fennec: - → 7+
Keywords: uiwanted
Whiteboard: [fennec-4.1?]
tracking-fennec: 7+ → +
Attached patch patch (obsolete) — Splinter Review
this is wes's patch updated to tip. Not sure why its sat for so long
Attachment #478610 - Attachment is obsolete: true
Attachment #554974 - Flags: review?(mark.finkle)
I see the same error as Vivien does in comment 5:

Error: formHistoryResult.wrappedJSObject is undefined
Source File: file:///home/mfinkle/source/mozilla-trunk/mobile-debug/dist/bin/components/nsInputListAutoComplete.js
Line: 58

I think we need to move away from the "simple" approach and try to merge this into AutoCompleteCache.js
Attachment #554974 - Flags: review?(mark.finkle) → review-
Hmm, I do see that the patch does actually return search suggestions, even with the errors. I wonder how we can squelch the error.
I think all we need to remove the error is a method/pref to turn off form history results, which I imagine are failing because they don't currently exist in this context:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchSuggestions.js#162

However, it would probably also be nice to save previous searches like desktop does. Those will likely return quickly enough we would have to start worrying about the ordering of them and places results.
(In reply to Wesley Johnston (:wesj) from comment #13)
> I think all we need to remove the error is a method/pref to turn off form
> history results, which I imagine are failing because they don't currently
> exist in this context:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/
> nsSearchSuggestions.js#162

Good. Looks like we'll need to add a way to flip that variable.

> However, it would probably also be nice to save previous searches like
> desktop does. Those will likely return quickly enough we would have to start
> worrying about the ordering of them and places results.

How are searches saved? I wonder how useful a saved search will be, considering the amount of space and memory it will take up.
Attached patch Form History patch (obsolete) — Splinter Review
Patch to remove the form history errors. Seems fine on desktop and mobile, but I need to write tests for this.
Attachment #554974 - Attachment is obsolete: true
This fixes the errors we are getting with form history in Fennec, and gives us a pref to disable searching the form history when using the search suggestions provider. Adds a test for the new pref.

The real problem seems to be that nsInputListAutoComplete.js expects to always receive FormAutoCompleteResult objects which it can call wrappedJSObject on and then play with willy nilly. In Fennec, it winds up getting real nsIAutoCompleteResult object and has problems. I adjusted it to use nsIAutoCompleteResult.getValueAt(), which I hope is similar to what it was already trying to do. I'm hoping you'll know if thats ok gavin?

nsFormAutocompleteResult also tries to use wrappedJSObject to copy prevEntries passed into it. For some reason, it stores those values in an "entries" object, which it never seems to use? The only time it is used is in nsInputListAutoComplete, and I just removed that use... so I am bit confused and leery about this. I just made it skip around that bit if wrappedJSObject is null.

I updated the search suggestions provider to use Services.jsm on the way, and fixed a bug that came up when you had disabled both search suggestions and form history suggestions.
Attachment #559518 - Attachment is obsolete: true
Attachment #561610 - Flags: review?
Attachment #561610 - Flags: review? → review?(gavin.sharp)
> How are searches saved? I wonder how useful a saved search will be,
> considering the amount of space and memory it will take up.

This is just sorta form autocomplete. We already save it in a lot of other places. I could rework this to try and make those appear as well, but we'd probably have to edit form autocomplete not to save urls (since places already is).

I think I would really like to (instead) fix bug 623925 so that you could have a textbox with autocomplete="form-history search-providers search-suggestions history" etc. rather than writing providers that try to do everything.
Comment on attachment 561610 [details] [diff] [review]
Form History fix v1

Sorry this lingered for so long - it looks fine, but has bit rotten slightly. I'd name the pref browser.search.suggest.formhistory, and then add a single observer to browser.search.suggest, though. Could probably also inline _loadFormHistoryPref/_loadSuggestPref since they're very small.
Attachment #561610 - Flags: review?(gavin.sharp) → feedback+
I wonder if Native Fennec want's this.
Assignee: wjohnston → nobody
Product: Fennec → Fennec Native
tracking-fennec: + → ?
tracking-fennec: ? → 12+
Assignee: nobody → madhava
Priority: -- → P3
Assignee: madhava → bnicholson
Target Milestone: --- → Firefox 15
Uses a more object-oriented approach for handling AwesomeBar clicks.
Attachment #623359 - Flags: review?(mark.finkle)
Missed some lines refreshing the patch.
Attachment #623359 - Attachment is obsolete: true
Attachment #623360 - Flags: review?(mark.finkle)
Attachment #623359 - Flags: review?(mark.finkle)
Implements search suggestions in Java rather than using Gecko autocomplete components. Using Gecko would result in more latency for the results, especially when Gecko is busy.

Unfortunately, we still rely on Gecko for getting the URL/engine for the search. This means we can still have delayed results - especially if the user tries to search immediately after opening Fennec (and before Gecko has loaded). To fix this, the search engine preferences are copied to the SharedPreferences when fetched. When the AwesomeBar is shown, it pulls the values directly from the SharedPreferences without having to message Gecko. Gecko still sends this information in case it has changed (where it is then updated accordingly).

The approach I used for getting the suggestion URL is a bit hacky, but I couldn't think of any better ways to do it. Search engines in Gecko don't directly expose the search URL template - they simply return a search URL when getSubmission() is called. Even if we looked into the underlying engine data's wrappedJSObject to get the suggestion template, we'd get something like this:

  http://suggestqueries.google.com/complete/search?output=firefox&client=firefox&hl={moz:locale}&q={searchTerms}

where we need to do additional substitions (like {moz:locale}). So instead, I just called getSubmission("__searchTerms__"), with the "__searchTerms__" placeholder string, that will produce a URL like this:

  http://suggestqueries.google.com/complete/search?output=firefox&client=firefox&hl=en-US&q=__searchTerms__

This URL is then passed to Java, where Java replaces the "__searchTerms__" in this URL with the query.
Attachment #623367 - Flags: review?(mark.finkle)
Simply adds the search suggestion preference to the Java UI. This pref is already checked in patch 2, where we pass a null suggestionTemplate if it's disabled, so we don't need to do any additional handling to make this it work.
Attachment #623368 - Flags: review?(mark.finkle)
Attached image suggestions screen shot (obsolete) —
I haven't seen any mockups for this feature, so here's what I came up with.

I assumed we wanted the suggestion results to appear before the standard results, or else they wouldn't be very useful. We get back 10 results when we do the query, but I truncated the results so they don't completely hide the other results. Four suggestions takes about half of the remaining screen space (keyboard included).

I also added long-press listeners to suggestions so that they can be easily edited. Long-pressing a suggestion replaces the text in the textbox with the suggested text, moves the cursor to the end, and shows the keyboard.
Attachment #623372 - Flags: feedback?(madhava)
Attachment #623372 - Flags: feedback?(ibarlow)
This is going to depend on the device screen size. We support some low resolution devices such as the droid pro. I suspect 4 items will fill its awesome screen.
(In reply to Kevin Brosnan [:kbrosnan] from comment #25)
> This is going to depend on the device screen size. We support some low
> resolution devices such as the droid pro. I suspect 4 items will fill its
> awesome screen.

Yeah, smaller resolution screens could be a problem, though the Droid Pro should be fine since it doesn't have an on-screen keyboard.
Brian - Can you post a build for UX to play with?
Brian this is awesome, it's great to see work happening on Search Suggest. As it turns out, we actually *do* have some UI designed for this feature -- I blogged about it here http://ianbarlow.wordpress.com/2012/03/09/enhanced-search-in-the-firefox-awesomebar/ and filed bug 739369 here for the tablet version. I just hadn't gotten around to filing a bug for phones yet. 

As Mark said it would be great to get a build to play with, and see what we can do to adapt the design to some of the mockups in the above links. 

A few notes based on the screenshot I see above

1. We'll want to try placing search suggestions in a single horizontal row of "tiles", in order to keep more Awesomebar results above the keyboard, and make better use of the (small amount of) horizontal space we have.

2. Add the search provider's icon on the far left of the search suggest row, so people know where their search is going

3. As we run out of Awesomebar results, let's add suggest links from other search providers as well, like this http://ianbarlow.files.wordpress.com/2012/03/search4.png
Ian, thanks for the feedback. Here's a build with the existing implementation:
http://dl.dropbox.com/u/35559547/fennec-suggestions.apk

It looks like the horizontal arrangement could work well on tablets, though I'm not sure how we could pull it off on phones. When I type a relatively simple query on my phone - like "mozilla firefox vs" - the suggested results nearly fill up the entire row. I've actually seen a number of suggestions that are so long that they require two rows. This is on a Droid RAZR, which has a higher resolution than many other phones.

Can you elaborate on what you mean by adding suggestion links from other providers? Looking at that screenshot, it doesn't look like the other providers have suggestions next to them.
Thanks for the build, Brian, this is fantastic, adding search suggest gives such a massive boost to the awesomebar UX :)

Having played with this a bit, I see your point about longer strings probably not working very well in a single row of tiles on the phone browser. It would be interesting to try adapting the design I put forward to allow for two (or possibly even 3) rows of tiles -- see the attached mockup. It's still a more efficent use of space compared to one row per search, and also gives room for longer search strings to wrap if necessary. 

As for suggest links from other providers, I think we need to explore either
1. Showing only one suggested topic
2. or showing a list of multiple suggestions as with google. The mockup from above only shows one, because I wonder if it might be difficult to read multiple clusters of tiles across multiple search providers. We should try it though to see.
Brian, search suggest can also be quite slow to engage, particularly on slower phones like the htc desire s where I can be typing for several seconds before seeing any suggestions. 

Any ideas on how we can make this quicker?
(In reply to Ian Barlow (:ibarlow) from comment #31)
> Brian, search suggest can also be quite slow to engage, particularly on
> slower phones like the htc desire s where I can be typing for several
> seconds before seeing any suggestions. 
> 
> Any ideas on how we can make this quicker?

I'm not sure if the slowness you're experiencing is due to the build or the network you're using. The attachment in comment 29 is a debug build, so it's possible that a standard build could speed things up. But if the problem is the network, I don't think there's much we can do - this patch already includes an optimization to reuse previous suggestions to reduce network overhead.

Is the stock browser faster?
Updated to use new flow UI.
Attachment #623367 - Attachment is obsolete: true
Attachment #624302 - Flags: review?(mark.finkle)
Attachment #623367 - Flags: review?(mark.finkle)
Here's a build containing the new changes:
http://dl.dropbox.com/u/35559547/fennec-suggestions2.apk

Since this uses the actual search engine icon for the suggestion row, this means the icon won't show up before Gecko has loaded or if Gecko is busy (as described in comment 22). We can fix this by also saving the engine icon to the Android shared prefs, if necessary.
Comment on attachment 624302 [details] [diff] [review]
Part 2 (v2): Add search suggestions to AwesomeBar

># HG changeset patch
># Parent 55864e0bea5c3a9f5c2ce18afbae09980c8f0132
># User Brian Nicholson <bnicholson@mozilla.com>
>Bug 586885 - Add search suggestions to AwesomeBar
>
>diff --git a/mobile/android/base/AwesomeBar.java b/mobile/android/base/AwesomeBar.java
>--- a/mobile/android/base/AwesomeBar.java
>+++ b/mobile/android/base/AwesomeBar.java
>@@ -40,21 +40,23 @@
> package org.mozilla.gecko;
> 
> import android.app.Activity;
> import android.app.AlertDialog;
> import android.content.DialogInterface;
> import android.content.Intent;
> import android.content.ContentResolver;
> import android.content.Context;
>+import android.content.SharedPreferences;
> import android.content.res.Resources;
> import android.content.res.Configuration;
> import android.database.Cursor;
> import android.graphics.Bitmap;
> import android.graphics.BitmapFactory;
>+import android.os.AsyncTask;
> import android.os.Bundle;
> import android.text.Editable;
> import android.text.Spanned;
> import android.text.TextUtils;
> import android.text.TextWatcher;
> import android.util.AttributeSet;
> import android.util.Log;
> import android.view.ContextMenu;
>@@ -71,41 +73,49 @@ import android.widget.Button;
> import android.widget.EditText;
> import android.widget.ExpandableListView;
> import android.widget.ImageButton;
> import android.widget.ListView;
> import android.widget.TabWidget;
> import android.widget.Toast;
> 
> import java.net.URLEncoder;
>+import java.util.ArrayList;
> import java.util.Map;
> 
> import org.mozilla.gecko.db.BrowserContract.Bookmarks;
> import org.mozilla.gecko.db.BrowserContract.Combined;
> import org.mozilla.gecko.db.BrowserDB.URLColumns;
> import org.mozilla.gecko.db.BrowserDB;
> 
> import org.json.JSONObject;
> 
> public class AwesomeBar extends GeckoActivity implements GeckoEventListener {
>     private static final String LOGTAG = "GeckoAwesomeBar";
>+    private static final int SUGGESTION_TIMEOUT = 2000;
>+    private static final int SUGGESTION_MAX = 3;
> 
>     static final String URL_KEY = "url";
>     static final String CURRENT_URL_KEY = "currenturl";
>     static final String TYPE_KEY = "type";
>     static final String SEARCH_KEY = "search";
>     static final String USER_ENTERED_KEY = "user_entered";
>     static enum Type { ADD, EDIT };
> 
>     private String mType;
>     private AwesomeBarTabs mAwesomeTabs;
>     private AwesomeBarEditText mText;
>     private ImageButton mGoButton;
>     private ContentResolver mResolver;
>     private ContextMenuSubject mContextMenuSubject;
>+    private SuggestClient mSuggestClient;
>+    private AsyncTask<String, Void, ArrayList<String>> mSuggestTask;
>+
>+    private static String sSuggestEngine;
>+    private static String sSuggestTemplate;
> 
>     @Override
>     public void onCreate(Bundle savedInstanceState) {
>         super.onCreate(savedInstanceState);
> 
>         Log.d(LOGTAG, "creating awesomebar");
> 
>         mResolver = Tabs.getInstance().getContentResolver();
>@@ -120,18 +130,30 @@ public class AwesomeBar extends GeckoAct
>         tabWidget.setDividerDrawable(null);
> 
>         mAwesomeTabs = (AwesomeBarTabs) findViewById(R.id.awesomebar_tabs);
>         mAwesomeTabs.setOnUrlOpenListener(new AwesomeBarTabs.OnUrlOpenListener() {
>             public void onUrlOpen(String url) {
>                 openUrlAndFinish(url);
>             }
> 
>-            public void onSearch(String engine) {
>-                openSearchAndFinish(mText.getText().toString(), engine);
>+            public void onSearch(String engine, String text) {
>+                openSearchAndFinish(text, engine);
>+            }
>+
>+            public void onLongClick(final String text) {
>+                GeckoApp.mAppContext.mMainHandler.post(new Runnable() {
>+                    public void run() {
>+                        mText.setText(text);
>+                        mText.setSelection(mText.getText().length());
>+                        mText.requestFocus();
>+                        InputMethodManager imm = (InputMethodManager) getSystemService(Context.INPUT_METHOD_SERVICE);
>+                        imm.showSoftInput(mText, InputMethodManager.SHOW_IMPLICIT);
>+                    }
>+                });
>             }
>         });
> 
>         mGoButton.setOnClickListener(new Button.OnClickListener() {
>             public void onClick(View v) {
>                 openUserEnteredAndFinish(mText.getText().toString());
>             }
>         });
>@@ -204,16 +226,36 @@ public class AwesomeBar extends GeckoAct
>                 // If the AwesomeBar has a composition string, don't call updateGoButton().
>                 // That method resets IME and composition state will be broken.
>                 if (hasCompositionString(s)) {
>                     return;
>                 }
> 
>                 // no composition string. It is safe to update IME flags.
>                 updateGoButton(text);
>+
>+                // cancel previous query
>+                if (mSuggestTask != null && mSuggestTask.getStatus() == AsyncTask.Status.RUNNING) {
>+                    mSuggestTask.cancel(true);
>+                }
>+
>+                if (mSuggestClient != null) {
>+                    mSuggestTask = new AsyncTask<String, Void, ArrayList<String>>() {
>+                         protected ArrayList<String> doInBackground(String... query) {
>+                             return mSuggestClient.query(query[0]);
>+                         }
>+
>+                         protected void onProgressUpdate(Void... progress) { }
>+
>+                         protected void onPostExecute(ArrayList<String> suggestions) {
>+                             mAwesomeTabs.setSuggestions(suggestions);
>+                         }
>+                    };
>+                    mSuggestTask.execute(text);
>+                }
>             }
> 
>             public void beforeTextChanged(CharSequence s, int start, int count,
>                                           int after) {
>                 // do nothing
>             }
> 
>             public void onTextChanged(CharSequence s, int start, int before,
>@@ -244,24 +286,58 @@ public class AwesomeBar extends GeckoAct
>                 }
>             }
>         });
> 
>         registerForContextMenu(mAwesomeTabs.findViewById(R.id.all_pages_list));
>         registerForContextMenu(mAwesomeTabs.findViewById(R.id.bookmarks_list));
>         registerForContextMenu(mAwesomeTabs.findViewById(R.id.history_list));
> 
>+        if (sSuggestTemplate == null) {
>+            GeckoAppShell.getHandler().post(new Runnable() {
>+                public void run() {
>+                    SharedPreferences prefs = GeckoApp.mAppContext.getSharedPreferences();
>+                    sSuggestEngine = prefs.getString("suggestEngine", null);
>+                    sSuggestTemplate = prefs.getString("suggestTemplate", null);
>+                    if (sSuggestTemplate != null) {
>+                        mSuggestClient = new SuggestClient(GeckoApp.mAppContext, sSuggestTemplate, SUGGESTION_TIMEOUT, SUGGESTION_MAX);
>+                        mAwesomeTabs.setSuggestEngine(sSuggestEngine, null);
>+                    }
>+                }
>+            });
>+        } else {
>+            mSuggestClient = new SuggestClient(GeckoApp.mAppContext, sSuggestTemplate, SUGGESTION_TIMEOUT, SUGGESTION_MAX);
>+        }
>+
>         GeckoAppShell.registerGeckoEventListener("SearchEngines:Data", this);
>         GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("SearchEngines:Get", null));
>     }
> 
>     public void handleMessage(String event, JSONObject message) {
>         try {
>             if (event.equals("SearchEngines:Data")) {
>-                mAwesomeTabs.setSearchEngines(message.getJSONArray("searchEngines"));
>+                // store the template for search suggestions in android prefs
>+                final String suggestEngine = message.isNull("suggestEngine") ? null : message.getString("suggestEngine");
>+                final String suggestTemplate = message.isNull("suggestTemplate") ? null : message.getString("suggestTemplate");
>+                if (!TextUtils.equals(suggestTemplate, sSuggestTemplate)) {
>+                    GeckoAppShell.getHandler().post(new Runnable() {
>+                        public void run() {
>+                            SharedPreferences prefs = GeckoApp.mAppContext.getSharedPreferences();
>+                            SharedPreferences.Editor editor = prefs.edit();
>+                            editor.putString("suggestEngine", suggestEngine);
>+                            editor.putString("suggestTemplate", suggestTemplate);
>+                            editor.commit();
>+                        }
>+                    });
>+                    sSuggestEngine = suggestEngine;
>+                    sSuggestTemplate = suggestTemplate;
>+                    mSuggestClient = new SuggestClient(GeckoApp.mAppContext, suggestTemplate, SUGGESTION_TIMEOUT, SUGGESTION_MAX);
>+                }
>+
>+                mAwesomeTabs.setSearchEngines(suggestEngine, message.getJSONArray("searchEngines"));
>             }
>         } catch (Exception e) {
>             // do nothing
>             Log.i(LOGTAG, "handleMessage throws " + e + " for message: " + event);
>         }
>     }
> 
>     @Override
>diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java
>--- a/mobile/android/base/AwesomeBarTabs.java
>+++ b/mobile/android/base/AwesomeBarTabs.java
>@@ -67,16 +67,17 @@ import android.widget.LinearLayout;
> import android.widget.ListView;
> import android.widget.SimpleCursorAdapter;
> import android.widget.SimpleExpandableListAdapter;
> import android.widget.TabHost;
> import android.widget.TextView;
> 
> import java.io.ByteArrayInputStream;
> import java.io.IOException;
>+import java.util.ArrayList;
> import java.util.Date;
> import java.util.HashMap;
> import java.util.LinkedList;
> import java.util.List;
> import java.util.Map;
> 
> import org.json.JSONArray;
> import org.json.JSONException;
>@@ -96,34 +97,36 @@ public class AwesomeBarTabs extends TabH
> 
>     private static enum HistorySection { TODAY, YESTERDAY, WEEK, OLDER };
> 
>     private Context mContext;
>     private boolean mInflated;
>     private LayoutInflater mInflater;
>     private OnUrlOpenListener mUrlOpenListener;
>     private View.OnTouchListener mListTouchListener;
>-    private JSONArray mSearchEngines;
>     private ContentResolver mContentResolver;
>     private ContentObserver mContentObserver;
>+    private SearchEngine mSuggestEngine;
>+    private ArrayList<SearchEngine> mSearchEngines;
> 
>     private BookmarksQueryTask mBookmarksQueryTask;
>     private HistoryQueryTask mHistoryQueryTask;
>     
>     private AwesomeBarCursorAdapter mAllPagesCursorAdapter;
>     private BookmarksListAdapter mBookmarksAdapter;
>     private SimpleExpandableListAdapter mHistoryAdapter;
> 
>     // FIXME: This value should probably come from a
>     // prefs key (just like XUL-based fennec)
>     private static final int MAX_RESULTS = 100;
> 
>     public interface OnUrlOpenListener {
>         public void onUrlOpen(String url);
>-        public void onSearch(String engine);
>+        public void onSearch(String engine, String text);
>+        public void onLongClick(String suggestion);
>     }
> 
>     private class ViewHolder {
>         public TextView titleView;
>         public TextView urlView;
>         public ImageView faviconView;
>         public ImageView starView;
>     }
>@@ -626,166 +629,221 @@ public class AwesomeBarTabs extends TabH
>             mHistoryQueryTask = null;
>         }
>     }
> 
>     private interface AwesomeBarItem {
>         public void onClick();
>     }
> 
>-    private class AwesomeBarCursorItem implements AwesomeBarItem {
>-        private Cursor mCursor;
>+    private class AwesomeBarCursorAdapter extends SimpleCursorAdapter {
>+        private String mSearchTerm;
> 
>-        public AwesomeBarCursorItem(Cursor cursor) {
>-            mCursor = cursor;
>+        private static final int ROW_SEARCH = 0;
>+        private static final int ROW_STANDARD = 1;
>+
>+        private class AwesomeBarCursorItem implements AwesomeBarItem {
>+            private Cursor mCursor;
>+
>+            public AwesomeBarCursorItem(Cursor cursor) {
>+                mCursor = cursor;
>+            }
>+
>+            public void onClick() {
>+                String url = mCursor.getString(mCursor.getColumnIndexOrThrow(URLColumns.URL));
>+                if (mUrlOpenListener != null)
>+                    mUrlOpenListener.onUrlOpen(url);
>+            }
>         }
> 
>-        public void onClick() {
>-            String url = mCursor.getString(mCursor.getColumnIndexOrThrow(URLColumns.URL));
>-            if (mUrlOpenListener != null)
>-                mUrlOpenListener.onUrlOpen(url);
>+        private class AwesomeBarSearchEngineItem implements AwesomeBarItem {
>+            private String mSearchEngine;
>+
>+            public AwesomeBarSearchEngineItem(String searchEngine) {
>+                mSearchEngine = searchEngine;
>+            }
>+
>+            public void onClick() {
>+                if (mUrlOpenListener != null)
>+                    mUrlOpenListener.onSearch(mSearchEngine, mSearchTerm);
>+            }
>         }
>-    }
>-
>-    private class AwesomeBarSearchEngineItem implements AwesomeBarItem {
>-        private String mSearchEngine;
>-
>-        public AwesomeBarSearchEngineItem(String searchEngine) {
>-            mSearchEngine = searchEngine;
>-        }
>-
>-        public void onClick() {
>-            if (mUrlOpenListener != null)
>-                mUrlOpenListener.onSearch(mSearchEngine);
>-        }
>-    }
>-
>-    private class AwesomeBarCursorAdapter extends SimpleCursorAdapter {
>-        private String mSearchTerm;
> 
>         public AwesomeBarCursorAdapter(Context context) {
>             super(context, -1, null, new String[] {}, new int[] {});
>             mSearchTerm = "";
>         }
> 
>         public void filter(String searchTerm) {
>             mSearchTerm = searchTerm;
>             getFilter().filter(searchTerm);
>         }
> 
>+        private int getSuggestionCount() {
>+            return (mSearchTerm.length() == 0 || mSuggestEngine == null) ? 0 : 1;
>+        }
>+
>         // Add the search engines to the number of reported results.
>         @Override
>         public int getCount() {
>             final int resultCount = super.getCount();
> 
>-            // don't show additional search engines if search field is empty
>+            // don't show search engines or suggestions if search field is empty
>             if (mSearchTerm.length() == 0)
>                 return resultCount;
> 
>-            return resultCount + mSearchEngines.length();
>+            return resultCount + mSearchEngines.size() + getSuggestionCount();
>         }
> 
>         // If an item is part of the cursor result set, return that entry.
>         // Otherwise, return the search engine data.
>         @Override
>         public Object getItem(int position) {
>-            final int resultCount = super.getCount();
>-            if (position < resultCount)
>+            int engineIndex = getEngineIndex(position);
>+
>+            if (engineIndex == -1) {
>+                // return awesomebar result
>+                position -= getSuggestionCount();
>                 return new AwesomeBarCursorItem((Cursor) super.getItem(position));
>-
>-            JSONObject engine;
>-            String engineName = null;
>-            try {
>-                engine = mSearchEngines.getJSONObject(position - resultCount);
>-                engineName = engine.getString("name");
>-            } catch (JSONException e) {
>-                Log.e(LOGTAG, "Error getting search engine JSON", e);
>             }
> 
>-            return new AwesomeBarSearchEngineItem(engineName);
>+            // return search engine
>+            return new AwesomeBarSearchEngineItem(getEngine(engineIndex).name);
>+        }
>+
>+        private SearchEngine getEngine(int index) {
>+            final int suggestionCount = getSuggestionCount();
>+            if (index < suggestionCount)
>+                return mSuggestEngine;
>+            return mSearchEngines.get(index - suggestionCount);
>+        }
>+
>+        private int getEngineIndex(int position) {
>+            final int resultCount = super.getCount();
>+            final int suggestionCount = getSuggestionCount();
>+
>+            // return suggestion engine index
>+            if (position < suggestionCount)
>+                return 0;
>+
>+            // not an engine
>+            position -= suggestionCount;
>+            if (position < resultCount)
>+                return -1;
>+
>+            // return search engine index
>+            return position - resultCount + 1;
>+        }
>+
>+        @Override
>+        public int getItemViewType(int position) {
>+            return getEngineIndex(position) == -1 ? ROW_STANDARD : ROW_SEARCH;
>+        }
>+
>+        @Override
>+        public int getViewTypeCount() {
>+            // view can be either a standard awesomebar row or a search engine row
>+            return 2;
>+        }
>+
>+        @Override
>+        public boolean isEnabled(int position) {
>+            int index = getEngineIndex(position);
>+            if (index != -1) {
>+                return getEngine(index).suggestions.isEmpty();
>+            }
>+            return true;
>         }
> 
>         @Override
>         public View getView(int position, View convertView, ViewGroup parent) {
>             ViewHolder viewHolder = null;
> 
>-            if (convertView == null) {
>-                convertView = mInflater.inflate(R.layout.awesomebar_row, null);
>+            if (getItemViewType(position) == ROW_SEARCH) {
>+                if (convertView == null) {
>+                    convertView = mInflater.inflate(R.layout.awesomebar_suggestion_row, null);
>+                } else {
>+                    ((FlowLayout) convertView.findViewById(R.id.suggestion_layout)).removeAllViews();
>+                }
> 
>-                viewHolder = new ViewHolder();
>-                viewHolder.titleView = (TextView) convertView.findViewById(R.id.title);
>-                viewHolder.urlView = (TextView) convertView.findViewById(R.id.url);
>-                viewHolder.faviconView = (ImageView) convertView.findViewById(R.id.favicon);
>-                viewHolder.starView = (ImageView) convertView.findViewById(R.id.bookmark_star);
>+                bindSearchEngineView(getEngine(getEngineIndex(position)), convertView);
>+            } else {
>+                if (convertView == null) {
>+                    viewHolder = new ViewHolder();
> 
>-                convertView.setTag(viewHolder);
>-            } else {
>-                viewHolder = (ViewHolder) convertView.getTag();
>-            }
>+                    convertView = mInflater.inflate(R.layout.awesomebar_row, null);
>+                    viewHolder.titleView = (TextView) convertView.findViewById(R.id.title);
>+                    viewHolder.urlView = (TextView) convertView.findViewById(R.id.url);
>+                    viewHolder.faviconView = (ImageView) convertView.findViewById(R.id.favicon);
>+                    viewHolder.starView = (ImageView) convertView.findViewById(R.id.bookmark_star);
> 
>-            final int resultCount = super.getCount();
>-            if (position < resultCount) {
>+                    convertView.setTag(viewHolder);
>+                } else {
>+                    viewHolder = (ViewHolder) convertView.getTag();
>+                }
>+
>+                position -= getSuggestionCount();
>                 Cursor cursor = getCursor();
>                 if (!cursor.moveToPosition(position))
>                     throw new IllegalStateException("Couldn't move cursor to position " + position);
> 
>                 updateTitle(viewHolder.titleView, cursor);
>                 updateUrl(viewHolder.urlView, cursor);
>                 updateFavicon(viewHolder.faviconView, cursor);
>                 updateBookmarkStar(viewHolder.starView, cursor);
>-            } else {
>-                bindSearchEngineView(position - resultCount, viewHolder);
>             }
> 
>             return convertView;
>         }
> 
>-        private Drawable getDrawableFromDataURI(String dataURI) {
>-            String base64 = dataURI.substring(dataURI.indexOf(',') + 1);
>-            Drawable drawable = null;
>-            try {
>-                byte[] bytes = GeckoAppShell.decodeBase64(base64, GeckoAppShell.BASE64_DEFAULT);
>-                ByteArrayInputStream stream = new ByteArrayInputStream(bytes);
>-                drawable = Drawable.createFromStream(stream, "src");
>-                stream.close();
>-            } catch (IllegalArgumentException e) {
>-                Log.i(LOGTAG, "exception while decoding drawable: " + base64, e);
>-            } catch (IOException e) { }
>-            return drawable;
>-        }
>+        private void bindSearchEngineView(SearchEngine engine, View convertView) {
>+            FlowLayout layout = (FlowLayout) convertView.findViewById(R.id.suggestion_layout);
> 
>-        private void bindSearchEngineView(int position, ViewHolder viewHolder) {
>-            String name;
>-            String iconURI;
>-            String searchText = getResources().getString(R.string.awesomebar_search_engine, mSearchTerm);
>-            try {
>-                JSONObject searchEngine = mSearchEngines.getJSONObject(position);
>-                name = searchEngine.getString("name");
>-                iconURI = searchEngine.getString("iconURI");
>-            } catch (JSONException e) {
>-                Log.e(LOGTAG, "Error getting search engine JSON", e);
>-                return;
>+            View searchItem = mInflater.inflate(R.layout.awesomebar_suggestion_item, null);
>+            ((TextView) searchItem.findViewById(R.id.suggestion_text)).setText(mSearchTerm);
>+            layout.addView(searchItem);
>+
>+            ImageView iconView = (ImageView) convertView.findViewById(R.id.suggestion_icon);
>+            iconView.setImageDrawable(engine.icon);
>+
>+            for (final String suggestion : engine.suggestions) {
>+                View suggestionItem = mInflater.inflate(R.layout.awesomebar_suggestion_item, null);
>+                ((TextView) suggestionItem.findViewById(R.id.suggestion_text)).setText(suggestion);
>+                ((ImageView) suggestionItem.findViewById(R.id.suggestion_magnifier)).setVisibility(View.GONE);
>+                layout.addView(suggestionItem);
>+                suggestionItem.setOnClickListener(new OnClickListener() {
>+                    @Override
>+                    public void onClick(View v) {
>+                        if (mUrlOpenListener != null) {
>+                            mUrlOpenListener.onSearch(mSuggestEngine.name, suggestion);
>+                        }
>+                    }
>+                });
>+                suggestionItem.setOnLongClickListener(new OnLongClickListener() {
>+                    @Override
>+                    public boolean onLongClick(View v) {
>+                        if (mUrlOpenListener != null) {
>+                            mUrlOpenListener.onLongClick(suggestion);
>+                            return true;
>+                        }
>+                        return false;
>+                    }
>+                });
>             }
>-
>-            viewHolder.titleView.setText(name);
>-            viewHolder.urlView.setText(searchText);
>-            Drawable drawable = getDrawableFromDataURI(iconURI);
>-            viewHolder.faviconView.setImageDrawable(drawable);
>-            viewHolder.starView.setVisibility(View.GONE);
>         }
>     };
> 
>     public AwesomeBarTabs(Context context, AttributeSet attrs) {
>         super(context, attrs);
> 
>         Log.d(LOGTAG, "Creating AwesomeBarTabs");
> 
>         mContext = context;
>         mInflated = false;
>-        mSearchEngines = new JSONArray();
>+        mSearchEngines = new ArrayList<SearchEngine>();
>         mContentResolver = context.getContentResolver();
>         mContentObserver = null;
>         mInflater = (LayoutInflater) mContext.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
>     }
> 
>     @Override
>     protected void onFinishInflate() {
>         super.onFinishInflate();
>@@ -799,17 +857,18 @@ public class AwesomeBarTabs extends TabH
>         mInflated = true;
> 
>         // This should be called before adding any tabs
>         // to the TabHost.
>         setup();
> 
>         mListTouchListener = new View.OnTouchListener() {
>             public boolean onTouch(View view, MotionEvent event) {
>-                hideSoftInput(view);
>+                if (event.getAction() == MotionEvent.ACTION_DOWN)
>+                    hideSoftInput(view);
>                 return false;
>             }
>         };
> 
>         addAllPagesTab();
>         addBookmarksTab();
>         addHistoryTab();
> 
>@@ -1048,17 +1107,89 @@ public class AwesomeBarTabs extends TabH
>         // The tabs should only be visible if there's no on-going search
>         int tabsVisibility = (searchTerm.length() == 0 ? View.VISIBLE : View.GONE);
>         getTabWidget().setVisibility(tabsVisibility);
> 
>         // Perform the actual search
>         mAllPagesCursorAdapter.filter(searchTerm);
>     }
> 
>-    public void setSearchEngines(final JSONArray engines) {
>+    private Drawable getDrawableFromDataURI(String dataURI) {
>+        String base64 = dataURI.substring(dataURI.indexOf(',') + 1);
>+        Drawable drawable = null;
>+        try {
>+            byte[] bytes = GeckoAppShell.decodeBase64(base64, GeckoAppShell.BASE64_DEFAULT);
>+            ByteArrayInputStream stream = new ByteArrayInputStream(bytes);
>+            drawable = Drawable.createFromStream(stream, "src");
>+            stream.close();
>+        } catch (IllegalArgumentException e) {
>+            Log.i(LOGTAG, "exception while decoding drawable: " + base64, e);
>+        } catch (IOException e) { }
>+        return drawable;
>+    }
>+
>+    private class SearchEngine {
>+        public String name;
>+        public Drawable icon;
>+        public ArrayList<String> suggestions;
>+
>+        public SearchEngine(String name) {
>+            this(name, null);
>+        }
>+
>+        public SearchEngine(String name, Drawable icon) {
>+            this.name = name;
>+            this.icon = icon;
>+            this.suggestions = new ArrayList<String>();
>+        }
>+    };
>+
>+    public void setSuggestEngine(String name, Drawable icon) {
>+        final SearchEngine suggestEngine = new SearchEngine(name, icon);
>+        if (mSuggestEngine != null)
>+            suggestEngine.suggestions = mSuggestEngine.suggestions;
>+
>         GeckoAppShell.getMainHandler().post(new Runnable() {
>             public void run() {
>-                mSearchEngines = engines;
>+                mSuggestEngine = suggestEngine;
>+                mAllPagesCursorAdapter.notifyDataSetChanged();
>+            }
>+        });
>+    }
>+
>+    public void setSuggestions(final ArrayList<String> suggestions) {
>+        if (mSuggestEngine != null) {
>+            GeckoAppShell.getMainHandler().post(new Runnable() {
>+                public void run() {
>+                    mSuggestEngine.suggestions = suggestions;
>+                    mAllPagesCursorAdapter.notifyDataSetChanged();
>+                }
>+            });
>+        }
>+    }
>+
>+    public void setSearchEngines(String suggestEngine, JSONArray engines) {
>+        final ArrayList<SearchEngine> searchEngines = new ArrayList<SearchEngine>();
>+        for (int i = 0; i < engines.length(); i++) {
>+            try {
>+                JSONObject engineJSON = engines.getJSONObject(i);
>+                String name = engineJSON.getString("name");
>+                String iconURI = engineJSON.getString("iconURI");
>+                Drawable icon = getDrawableFromDataURI(iconURI);
>+                if (name.equals(suggestEngine)) {
>+                    setSuggestEngine(name, icon);
>+                } else {
>+                    searchEngines.add(new SearchEngine(name, icon));
>+                }
>+            } catch (JSONException e) {
>+                Log.e(LOGTAG, "Error getting search engine JSON", e);
>+                return;
>+            }
>+        }
>+
>+        GeckoAppShell.getMainHandler().post(new Runnable() {
>+            public void run() {
>+                mSearchEngines = searchEngines;
>                 mAllPagesCursorAdapter.notifyDataSetChanged();
>             }
>         });
>     }
> }
>diff --git a/mobile/android/base/FlowLayout.java b/mobile/android/base/FlowLayout.java
>new file mode 100644
>--- /dev/null
>+++ b/mobile/android/base/FlowLayout.java
>@@ -0,0 +1,99 @@
>+/**
>+ * Code licensed under CC-by-SA
>+ *  
>+ * @author Henrik Gustafsson
>+ * @see http://stackoverflow.com/questions/549451/line-breaking-widget-layout-for-android
>+ * @license http://creativecommons.org/licenses/by-sa/2.5/
>+ */
>+
>+/* This code has been modified for use with Mozilla Firefox. */
>+
>+package org.mozilla.gecko;
>+
>+import android.content.Context;
>+import android.util.AttributeSet;
>+import android.util.Log;
>+import android.view.View;
>+import android.view.ViewGroup;
>+
>+public class FlowLayout extends ViewGroup {
>+
>+    private int mVerticalSpacing;
>+    private int mHorizontalSpacing;
>+
>+    public FlowLayout(Context context) {
>+        super(context);
>+    }
>+
>+    public FlowLayout(Context context, AttributeSet attrs) {
>+        super(context, attrs);
>+        mHorizontalSpacing = attrs.getAttributeIntValue(null, "horizontal_spacing", 0);
>+        mVerticalSpacing = attrs.getAttributeIntValue(null, "vertical_spacing", 0);
>+    }
>+
>+    @Override
>+    protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
>+        assert(MeasureSpec.getMode(widthMeasureSpec) != MeasureSpec.UNSPECIFIED);
>+
>+        final int width = MeasureSpec.getSize(widthMeasureSpec) - getPaddingLeft() - getPaddingRight();
>+        int height = MeasureSpec.getSize(heightMeasureSpec) - getPaddingTop() - getPaddingBottom();
>+        final int count = getChildCount();
>+
>+        int xpos = getPaddingLeft();
>+        int ypos = getPaddingTop();
>+        boolean firstChild = true;
>+
>+        for (int i = 0; i < count; i++) {
>+            final View child = getChildAt(i);
>+            if (child.getVisibility() != GONE) {
>+                child.measure(MeasureSpec.makeMeasureSpec(width, MeasureSpec.AT_MOST),
>+                              MeasureSpec.makeMeasureSpec(height, MeasureSpec.getMode(heightMeasureSpec)));
>+
>+                final int childw = child.getMeasuredWidth();
>+
>+                if (firstChild || xpos + childw > width) {
>+                    xpos = getPaddingLeft();
>+                    ypos += child.getMeasuredHeight();
>+                    if (!firstChild)
>+                        ypos += mVerticalSpacing;
>+                    firstChild = false;
>+                }
>+
>+                xpos += childw + mHorizontalSpacing;
>+            }
>+        }
>+
>+        if (MeasureSpec.getMode(heightMeasureSpec) == MeasureSpec.UNSPECIFIED) {
>+            height = ypos;
>+        } else if (MeasureSpec.getMode(heightMeasureSpec) == MeasureSpec.AT_MOST) {
>+            if (ypos < height) {
>+                height = ypos;
>+            }
>+        }
>+        setMeasuredDimension(width, height);
>+    }
>+
>+    @Override
>+    protected void onLayout(boolean changed, int l, int t, int r, int b) {
>+        final int count = getChildCount();
>+        final int width = r - l;
>+        int xpos = getPaddingLeft();
>+        int ypos = getPaddingTop();
>+        int lastChildH = 0;
>+
>+        for (int i = 0; i < count; i++) {
>+            final View child = getChildAt(i);
>+            if (child.getVisibility() != GONE) {
>+                final int childw = child.getMeasuredWidth();
>+                final int childh = child.getMeasuredHeight();
>+                if (xpos + childw > width) {
>+                    xpos = getPaddingLeft();
>+                    ypos += lastChildH + mVerticalSpacing;
>+                }
>+                lastChildH = childh;
>+                child.layout(xpos, ypos, xpos + childw, ypos + childh);
>+                xpos += childw + mHorizontalSpacing;
>+            }
>+        }
>+    }
>+}
>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
>--- a/mobile/android/base/GeckoApp.java
>+++ b/mobile/android/base/GeckoApp.java
>@@ -2825,9 +2825,13 @@ abstract public class GeckoApp
>                 return true;
>             }
>         });
>     }
> 
>     public boolean linkerExtract() {
>         return false;
>     }
>+
>+    public SharedPreferences getSharedPreferences() {
>+        return getSharedPreferences("shared.prefs", MODE_PRIVATE);
>+    }
> }
>diff --git a/mobile/android/base/Makefile.in b/mobile/android/base/Makefile.in
>--- a/mobile/android/base/Makefile.in
>+++ b/mobile/android/base/Makefile.in
>@@ -87,16 +87,17 @@ FENNEC_JAVA_FILES = \
>   db/AndroidBrowserDB.java \
>   db/BrowserDB.java \
>   db/LocalBrowserDB.java \
>   db/DBUtils.java \
>   DoorHanger.java \
>   DoorHangerPopup.java \
>   Favicons.java \
>   FloatUtils.java \
>+  FlowLayout.java \
>   FormAssistPopup.java \
>   GeckoActionBar.java \
>   GeckoApplication.java \
>   GeckoApp.java \
>   GeckoAppShell.java \
>   GeckoAsyncTask.java \
>   GeckoBatteryManager.java \
>   GeckoBackgroundThread.java \
>@@ -121,16 +122,17 @@ FENNEC_JAVA_FILES = \
>   PromptService.java \
>   sqlite/ByteBufferInputStream.java \
>   sqlite/MatrixBlobCursor.java \
>   sqlite/SQLiteBridge.java \
>   sqlite/SQLiteBridgeException.java \
>   RemoteTabs.java \
>   SetupScreen.java \
>   SiteIdentityPopup.java \
>+  SuggestClient.java \
>   SurfaceBits.java \
>   Tab.java \
>   Tabs.java \
>   TabsTray.java \
>   TabsAccessor.java \
>   Telemetry.java \
>   gfx/BitmapUtils.java \
>   gfx/BufferedCairoImage.java \
>@@ -274,16 +276,18 @@ RES_LAYOUT = \
>   $(SYNC_RES_LAYOUT) \
>   res/layout/autocomplete_list.xml \
>   res/layout/autocomplete_list_item.xml \
>   res/layout/awesomebar.xml \
>   res/layout/awesomebar_actionbar.xml \
>   res/layout/awesomebar_folder_row.xml \
>   res/layout/awesomebar_header_row.xml \
>   res/layout/awesomebar_row.xml \
>+  res/layout/awesomebar_suggestion_item.xml \
>+  res/layout/awesomebar_suggestion_row.xml \
>   res/layout/awesomebar_search.xml \
>   res/layout/awesomebar_tab_indicator.xml \
>   res/layout/awesomebar_tabs.xml \
>   res/layout/bookmark_edit.xml \
>   res/layout/browser_toolbar.xml \
>   res/layout/doorhangerpopup.xml \
>   res/layout/doorhanger.xml \
>   res/layout/gecko_app.xml \
>@@ -629,16 +633,17 @@ MOZ_ANDROID_DRAWABLES += \
>   mobile/android/base/resources/drawable/progress_spinner_14.png                \
>   mobile/android/base/resources/drawable/progress_spinner_15.png                \
>   mobile/android/base/resources/drawable/progress_spinner_16.png                \
>   mobile/android/base/resources/drawable/progress_spinner_17.png                \
>   mobile/android/base/resources/drawable/progress_spinner_18.png                \
>   mobile/android/base/resources/drawable/remote_tabs_group_bg_repeat.xml        \
>   mobile/android/base/resources/drawable/start.png                              \
>   mobile/android/base/resources/drawable/site_security_level.xml                \
>+  mobile/android/base/resources/drawable/suggestion_selector.xml                \
>   mobile/android/base/resources/drawable/tabs_button.xml                        \
>   mobile/android/base/resources/drawable/tabs_button_tail.xml                   \
>   mobile/android/base/resources/drawable/tabs_level.xml                         \
>   mobile/android/base/resources/drawable/tabs_tray_bg_repeat.xml                \
>   mobile/android/base/resources/drawable/tabs_tray_pressed_bg_repeat.xml        \
>   mobile/android/base/resources/drawable/tabs_tray_close_button.xml             \
>   mobile/android/base/resources/drawable/tabs_tray_list_divider.xml             \
>   mobile/android/base/resources/drawable/tabs_tray_list_selector.xml            \
>diff --git a/mobile/android/base/SuggestClient.java b/mobile/android/base/SuggestClient.java
>new file mode 100644
>--- /dev/null
>+++ b/mobile/android/base/SuggestClient.java
>@@ -0,0 +1,166 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+package org.mozilla.gecko;
>+
>+import org.json.JSONArray;
>+
>+import android.content.Context;
>+import android.net.ConnectivityManager;
>+import android.net.NetworkInfo;
>+import android.text.TextUtils;
>+import android.util.Log;
>+
>+import java.io.BufferedInputStream;
>+import java.io.InputStream;
>+import java.io.InterruptedIOException;
>+import java.net.HttpURLConnection;
>+import java.net.URL;
>+import java.net.URLEncoder;
>+import java.util.ArrayList;
>+
>+/**
>+ * Use network-based search suggestions.
>+ */
>+public class SuggestClient {
>+    private static final String LOGTAG = "GeckoSuggestClient";
>+    private static final String USER_AGENT = GeckoApp.mAppContext.getDefaultUAString();
>+
>+    private final Context mContext;
>+    private final int mTimeout;
>+
>+    // should contain the string "__searchTerms__", which is replaced with the query
>+    private final String mSuggestTemplate;
>+
>+    // the maximum number of suggestions to return
>+    private final int mMaxResults;
>+
>+    // previous suggestions can be reused to reduce network I/O
>+    private String mPrevQuery;
>+    private ArrayList<String> mPrevSuggestions;
>+
>+    public SuggestClient(Context context, String suggestTemplate, int timeout, int maxResults) {
>+        mContext = context;
>+        mMaxResults = maxResults;
>+        mSuggestTemplate = suggestTemplate;
>+        mPrevQuery = "";
>+        mPrevSuggestions = new ArrayList<String>();
>+        mTimeout = timeout;
>+    }
>+
>+    public SuggestClient(Context context, String suggestTemplate, int timeout) {
>+        this(context, suggestTemplate, timeout, Integer.MAX_VALUE);
>+    }
>+
>+    /**
>+     * Queries for a given search term and returns an ArrayList of suggestions.
>+     */
>+    public ArrayList<String> query(String query) {
>+        ArrayList<String> suggestions = new ArrayList<String>();
>+        if (TextUtils.isEmpty(mSuggestTemplate) || TextUtils.isEmpty(query)) {
>+            return suggestions;
>+        }
>+
>+        // reuse previous suggestions if possible
>+        if (query.startsWith(mPrevQuery) && !TextUtils.isEmpty(mPrevQuery)) {
>+            ArrayList<String> filtered = filterPrevSuggestions(query);
>+            int minResults = Math.min(mMaxResults, mPrevSuggestions.size());
>+            if (filtered != null && filtered.size() >= minResults) {
>+                Log.d(LOGTAG, "Reusing '" + mPrevQuery + "' suggestions for query '" + query + "'");
>+                mPrevQuery = query;
>+                mPrevSuggestions = filtered;
>+                for (int i = 0; i < minResults; i++)
>+                    suggestions.add(filtered.get(i));
>+                return suggestions;
>+            }
>+        }
>+
>+        if (!isNetworkConnected()) {
>+            Log.i(LOGTAG, "Not connected to network");
>+            return suggestions;
>+        }
>+
>+        try {
>+            String encoded = URLEncoder.encode(query, "UTF-8");
>+            String suggestUri = mSuggestTemplate.replace("__searchTerms__", encoded);
>+
>+            URL url = new URL(suggestUri);
>+            String json = null;
>+            HttpURLConnection urlConnection = (HttpURLConnection) url.openConnection();
>+            try {
>+                urlConnection.setConnectTimeout(mTimeout);
>+                urlConnection.setRequestProperty("User-Agent", USER_AGENT);
>+                InputStream in = new BufferedInputStream(urlConnection.getInputStream());
>+                json = convertStreamToString(in);
>+            } finally {
>+                urlConnection.disconnect();
>+            }
>+
>+            if (json != null) {
>+                /*
>+                 * Sample result:
>+                 * ["foo",["food network","foothill college","foot locker",...]]
>+                 */
>+                JSONArray results = new JSONArray(json);
>+                JSONArray jsonSuggestions = results.getJSONArray(1);
>+                
>+                mPrevSuggestions = new ArrayList<String>();
>+                mPrevQuery = query;
>+                int added = 0;
>+                for (int i = 0; i < jsonSuggestions.length(); i++) {
>+                    String suggestion = jsonSuggestions.getString(i);
>+                    if (suggestion.equalsIgnoreCase(query))
>+                        continue;
>+
>+                    if (added++ < mMaxResults)
>+                        suggestions.add(suggestion);
>+                    mPrevSuggestions.add(suggestion);
>+                }
>+            } else {
>+                Log.d(LOGTAG, "Suggestion query failed");
>+            }
>+        } catch (InterruptedIOException e) {
>+            Log.d(LOGTAG, "Suggestion query interrupted");
>+        } catch (Exception e) {
>+            Log.w(LOGTAG, "Error", e);
>+        }
>+        return suggestions;
>+    }
>+
>+    private boolean isNetworkConnected() {
>+        NetworkInfo networkInfo = getActiveNetworkInfo();
>+        return networkInfo != null && networkInfo.isConnected();
>+    }
>+
>+    private NetworkInfo getActiveNetworkInfo() {
>+        ConnectivityManager connectivity = (ConnectivityManager) mContext
>+                .getSystemService(Context.CONNECTIVITY_SERVICE);
>+        if (connectivity == null)
>+            return null;
>+        return connectivity.getActiveNetworkInfo();
>+    }
>+
>+    private String convertStreamToString(java.io.InputStream is) {
>+        try {
>+            return new java.util.Scanner(is).useDelimiter("\\A").next();
>+        } catch (java.util.NoSuchElementException e) {
>+            return "";
>+        }
>+    }
>+
>+    private ArrayList<String> filterPrevSuggestions(String query) {
>+        ArrayList<String> filtered = new ArrayList<String>();
>+        int queryLength = query.length();
>+        int prevQueryLength = mPrevQuery.length();
>+        String queryOffset = query.substring(prevQueryLength, queryLength);
>+        for (String suggestion : mPrevSuggestions) {
>+            if (queryLength < suggestion.length()) {
>+                String offset = suggestion.substring(prevQueryLength, queryLength);
>+                if (queryOffset.equalsIgnoreCase(offset))
>+                    filtered.add(suggestion);
>+            }
>+        }
>+        return filtered;
>+    }
>+}
>diff --git a/mobile/android/base/resources/drawable/suggestion_selector.xml b/mobile/android/base/resources/drawable/suggestion_selector.xml
>new file mode 100644
>--- /dev/null
>+++ b/mobile/android/base/resources/drawable/suggestion_selector.xml
>@@ -0,0 +1,36 @@
>+<?xml version="1.0" encoding="utf-8"?>
>+<selector xmlns:android="http://schemas.android.com/apk/res/android">
>+
>+    <item android:state_pressed="true">
>+        <shape>
>+            <solid android:color="#bbb" />
>+
>+            <padding android:left="7dp"
>+                     android:top="7dp"
>+                     android:right="7dp"
>+                     android:bottom="7dp" /> 
>+           
>+            <corners android:bottomRightRadius="4dp"
>+                     android:bottomLeftRadius="4dp"
>+                     android:topLeftRadius="4dp"
>+                     android:topRightRadius="4dp"/> 
>+        </shape>
>+    </item>
>+
>+    <item android:state_enabled="true">
>+        <shape>
>+            <solid android:color="#ddd" />
>+
>+            <padding android:left="7dp"
>+                     android:top="7dp"
>+                     android:right="7dp"
>+                     android:bottom="7dp" /> 
>+           
>+            <corners android:bottomRightRadius="4dp"
>+                     android:bottomLeftRadius="4dp"
>+                     android:topLeftRadius="4dp"
>+                     android:topRightRadius="4dp"/> 
>+        </shape>
>+    </item>
>+
>+</selector>
>diff --git a/mobile/android/base/resources/layout/awesomebar_suggestion_item.xml b/mobile/android/base/resources/layout/awesomebar_suggestion_item.xml
>new file mode 100644
>--- /dev/null
>+++ b/mobile/android/base/resources/layout/awesomebar_suggestion_item.xml
>@@ -0,0 +1,22 @@
>+<?xml version="1.0" encoding="utf-8"?>
>+<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
>+              android:layout_width="wrap_content"
>+              android:layout_height="wrap_content"
>+              android:background="@drawable/suggestion_selector"
>+              android:gravity="center_vertical"
>+              android:orientation="horizontal">
>+
>+    <ImageView xmlns:android="http://schemas.android.com/apk/res/android"  
>+               android:id="@+id/suggestion_magnifier"
>+               android:src="@drawable/ic_awesomebar_search"
>+               android:layout_marginRight="3dip"
>+               android:layout_width="16dip"
>+               android:layout_height="16dip" />
>+
>+    <TextView xmlns:android="http://schemas.android.com/apk/res/android"  
>+              android:id="@+id/suggestion_text"
>+              android:layout_width="wrap_content"
>+              android:layout_height="wrap_content"
>+              android:textColor="?android:attr/textColorPrimary"/>
>+
>+</LinearLayout>
>diff --git a/mobile/android/base/resources/layout/awesomebar_suggestion_row.xml b/mobile/android/base/resources/layout/awesomebar_suggestion_row.xml
>new file mode 100644
>--- /dev/null
>+++ b/mobile/android/base/resources/layout/awesomebar_suggestion_row.xml
>@@ -0,0 +1,21 @@
>+<?xml version="1.0" encoding="utf-8"?>
>+<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
>+                android:layout_width="fill_parent"
>+                android:layout_height="wrap_content"
>+                android:padding="6dip">
>+
>+    <ImageView android:id="@+id/suggestion_icon"
>+               android:layout_width="32dip"
>+               android:layout_height="32dip"
>+               android:layout_marginRight="10dip"
>+               android:minWidth="32dip"
>+               android:minHeight="32dip"
>+               android:scaleType="fitCenter"/>
>+
>+    <org.mozilla.gecko.FlowLayout android:id="@+id/suggestion_layout"
>+                                  android:layout_toRightOf="@id/suggestion_icon"
>+                                  android:layout_width="wrap_content"
>+                                  android:layout_height="wrap_content"
>+                                  horizontal_spacing="8"
>+                                  vertical_spacing="8"/>
>+</RelativeLayout>
>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>--- a/mobile/android/chrome/content/browser.js
>+++ b/mobile/android/chrome/content/browser.js
>@@ -4849,25 +4849,50 @@ var SearchEngines = {
>       let engineData = Services.search.getVisibleEngines({});
>       let searchEngines = engineData.map(function (engine) {
>         return {
>           name: engine.name,
>           iconURI: (engine.iconURI ? engine.iconURI.spec : null)
>         };
>       });
> 
>+      let suggestTemplate = null;
>+      let suggestEngine = null;
>+      if (Services.prefs.getBoolPref("browser.search.suggest.enabled")) {
>+        let engine = this.getSuggestionEngine();
>+        if (engine != null) {
>+          suggestEngine = engine.name;
>+          suggestTemplate = engine.getSubmission("__searchTerms__", "application/x-suggestions+json").uri.spec;
>+        }
>+      }
>+
>       sendMessageToJava({
>         gecko: {
>           type: "SearchEngines:Data",
>-          searchEngines: searchEngines
>+          searchEngines: searchEngines,
>+          suggestEngine: suggestEngine,
>+          suggestTemplate: suggestTemplate
>         }
>       });
>     }
>   },
> 
>+  getSuggestionEngine: function () {
>+    let engines = [ Services.search.currentEngine,
>+                    Services.search.defaultEngine,
>+                    Services.search.originalDefaultEngine ];
>+
>+    for each (let engine in engines) {
>+      if (engine && engine.supportsResponseType("application/x-suggestions+json"))
>+        return engine;
>+    }
>+
>+    return null;
>+  },
>+
>   addEngine: function addEngine(aElement) {
>     let form = aElement.form;
>     let charset = aElement.ownerDocument.characterSet;
>     let docURI = Services.io.newURI(aElement.ownerDocument.URL, charset, null);
>     let formURL = Services.io.newURI(form.getAttribute("action"), charset, docURI).spec;
>     let method = form.method.toUpperCase();
>     let formData = [];
>
Attachment #624302 - Attachment description: Part 2: Add search suggestions to AwesomeBar → Part 2 (v2): Add search suggestions to AwesomeBar
Fixed a few mistakes.
Attachment #624302 - Attachment is obsolete: true
Attachment #624309 - Flags: review?(mark.finkle)
Attachment #624302 - Flags: review?(mark.finkle)
blocking-fennec1.0: --- → ?
Whoa, just noticed that it showed the entire patch when I changed the name in comment 35. Sorry about that.
Comment on attachment 624309 [details] [diff] [review]
Part 2 (v3): Add search suggestions to AwesomeBar

Review of attachment 624309 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ +4882,5 @@
> +
> +    for each (let engine in engines) {
> +      if (engine && engine.supportsResponseType("application/x-suggestions+json"))
> +        return engine;
> +    }

Drive-by: You're not supposed to use |for each...of| on arrays. You can use the hot new |for...of| though: https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...of
(In reply to Margaret Leibovic [:margaret] from comment #38)

> Drive-by: You're not supposed to use |for each...of| ...

Gr, I meant |for each...in|
(In reply to Margaret Leibovic [:margaret] from comment #38)
> Comment on attachment 624309 [details] [diff] [review]
> Part 2 (v3): Add search suggestions to AwesomeBar
> 
> Review of attachment 624309 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +4882,5 @@
> > +
> > +    for each (let engine in engines) {
> > +      if (engine && engine.supportsResponseType("application/x-suggestions+json"))
> > +        return engine;
> > +    }
> 
> Drive-by: You're not supposed to use |for each...of| on arrays. You can use
> the hot new |for...of| though:
> https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...of

Thanks, good catch.
New feature, hence minus for 1.0.
tracking-fennec: 12+ → 15+
blocking-fennec1.0: ? → -
Attachment #623360 - Flags: review?(mark.finkle) → review+
Comment on attachment 623368 [details] [diff] [review]
Part 3: Add search suggestion preference

I am fine with adding this pref and it's location. Some people might not want Google (or any provider) to get their search queries.

Let's get UX signoff too. The string "Show search suggestions" is the same as the one used in desktop. If we get into a string length issue we could try variants.
Attachment #623368 - Flags: review?(mark.finkle)
Attachment #623368 - Flags: review?(ibarlow)
Attachment #623368 - Flags: review+
Attachment #623372 - Attachment is obsolete: true
Attachment #623372 - Flags: feedback?(madhava)
Attachment #623372 - Flags: feedback?(ibarlow)
>diff --git a/mobile/android/base/FlowLayout.java b/mobile/android/base/FlowLayout.java
>+/**
>+ * Code licensed under CC-by-SA
>+ *  
>+ * @author Henrik Gustafsson
>+ * @see http://stackoverflow.com/questions/549451/line-breaking-widget-layout-for-android
>+ * @license http://creativecommons.org/licenses/by-sa/2.5/
>+ */
>+
>+/* This code has been modified for use with Mozilla Firefox. */

We need to verify that this license is OK for adding to Mozilla.
Comment on attachment 624309 [details] [diff] [review]
Part 2 (v3): Add search suggestions to AwesomeBar

>diff --git a/mobile/android/base/AwesomeBar.java b/mobile/android/base/AwesomeBar.java
> public class AwesomeBar extends GeckoActivity implements GeckoEventListener {
>     private static final String LOGTAG = "GeckoAwesomeBar";
>+    private static final int SUGGESTION_TIMEOUT = 2000;
>+    private static final int SUGGESTION_MAX = 3;

nit: add a line break to separate them from LOGTAG

>+    private static String sSuggestEngine;
>+    private static String sSuggestTemplate;

Is it necessary to make these static?

>+        if (sSuggestTemplate == null) {
>+            GeckoAppShell.getHandler().post(new Runnable() {
>+                public void run() {
>+                    SharedPreferences prefs = GeckoApp.mAppContext.getSharedPreferences();
>+                    sSuggestEngine = prefs.getString("suggestEngine", null);
>+                    sSuggestTemplate = prefs.getString("suggestTemplate", null);
>+                    if (sSuggestTemplate != null) {
>+                        mSuggestClient = new SuggestClient(GeckoApp.mAppContext, sSuggestTemplate, SUGGESTION_TIMEOUT, SUGGESTION_MAX);
>+                        mAwesomeTabs.setSuggestEngine(sSuggestEngine, null);
>+                    }
>+                }
>+            });

I didn't look too deeply, but what happens if this activity (AwesomeBar) is closed before the runnable fires?

>diff --git a/mobile/android/base/FlowLayout.java b/mobile/android/base/FlowLayout.java

>+/**
>+ * Code licensed under CC-by-SA
>+ *  
>+ * @author Henrik Gustafsson
>+ * @see http://stackoverflow.com/questions/549451/line-breaking-widget-layout-for-android
>+ * @license http://creativecommons.org/licenses/by-sa/2.5/
>+ */

We need an "OK" to use this file

>+/* This code has been modified for use with Mozilla Firefox. */

What changes did you make?

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+    public SharedPreferences getSharedPreferences() {
>+        return getSharedPreferences("shared.prefs", MODE_PRIVATE);
>+    }

Do we really want this in GeckoApp? I don't want GeckoApp to acquire too many util methods. This seems like it could be used in Awesomebar.java directly.

>diff --git a/mobile/android/base/SuggestClient.java b/mobile/android/base/SuggestClient.java
>diff --git a/mobile/android/base/resources/drawable/suggestion_selector.xml b/mobile/android/base/resources/drawable/suggestion_selector.xml

>+        <shape>
>+            <solid android:color="#bbb" />

>+        <shape>
>+            <solid android:color="#ddd" />

Do we use colors.xml for these types of colors? I don't know if hardcoding them is what we want to do.


r+ with those nits and questions answered

I do think we want to put suggestions on a separate row from the provider, but we can do this in a followup.

I want Lucas to look at this too, mainly for the AwesomebarTabs changes
Attachment #624309 - Flags: review?(mark.finkle)
Attachment #624309 - Flags: review?(lucasr.at.mozilla)
Attachment #624309 - Flags: review+
I'm afraid CC-BY-SA is not compatible with Mozilla's licensing policy:
http://www.mozilla.org/MPL/license-policy.html
(see under "Other Information").

Sorry :-|

Gerv
(In reply to Mark Finkle (:mfinkle) from comment #44)
> >+    private static String sSuggestEngine;
> >+    private static String sSuggestTemplate;
> 
> Is it necessary to make these static?

It's not necessary, but I did this as an optimization. Getting the engine name/template from the shared preferences requires reading from disk, so by making them persist, this only has to be done once.

> 
> >+        if (sSuggestTemplate == null) {
> >+            GeckoAppShell.getHandler().post(new Runnable() {
> >+                public void run() {
> >+                    SharedPreferences prefs = GeckoApp.mAppContext.getSharedPreferences();
> >+                    sSuggestEngine = prefs.getString("suggestEngine", null);
> >+                    sSuggestTemplate = prefs.getString("suggestTemplate", null);
> >+                    if (sSuggestTemplate != null) {
> >+                        mSuggestClient = new SuggestClient(GeckoApp.mAppContext, sSuggestTemplate, SUGGESTION_TIMEOUT, SUGGESTION_MAX);
> >+                        mAwesomeTabs.setSuggestEngine(sSuggestEngine, null);
> >+                    }
> >+                }
> >+            });
> 
> I didn't look too deeply, but what happens if this activity (AwesomeBar) is
> closed before the runnable fires?

Nothing too terrible - the static pref variables still get updated with the shared pref values, the client gets created (which will be be destroyed immediately after), and the search suggestions will be set on mAwesomeTabs (which will also be destroyed).

> 
> >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> 
> >+    public SharedPreferences getSharedPreferences() {
> >+        return getSharedPreferences("shared.prefs", MODE_PRIVATE);
> >+    }
> 
> Do we really want this in GeckoApp? I don't want GeckoApp to acquire too
> many util methods. This seems like it could be used in Awesomebar.java
> directly.

Moved to AwesomeBar.

> 
> >diff --git a/mobile/android/base/SuggestClient.java b/mobile/android/base/SuggestClient.java
> >diff --git a/mobile/android/base/resources/drawable/suggestion_selector.xml b/mobile/android/base/resources/drawable/suggestion_selector.xml
> 
> >+        <shape>
> >+            <solid android:color="#bbb" />
> 
> >+        <shape>
> >+            <solid android:color="#ddd" />
> 
> Do we use colors.xml for these types of colors? I don't know if hardcoding
> them is what we want to do.

Moved these to colors.xml.
New FlowLayout implementation, built from scratch.
Attachment #625828 - Flags: review?(mark.finkle)
New patch with Mark's changes and FlowLayout moved to patch 2a.
Attachment #624309 - Attachment is obsolete: true
Attachment #624309 - Flags: review?(lucasr.at.mozilla)
Attachment #625829 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 625829 [details] [diff] [review]
Part 2b: Add search suggestions to AwesomeBar

Carrying over r=mfinkle
Attachment #625829 - Flags: review+
Forgot Margaret's change from comment 38. Just made this change:

-    for each (let engine in engines) {
+    engines.forEach(function (engine) {
       if (engine && engine.supportsResponseType("application/x-suggestions+json"))
         return engine;
-    }
+    });
One other change I made in the SuggestClient that I forgot to mention:

I removed the optimization that reused previous results. I created this with the assumption that the search query will be a prefix of all result strings, but that isn't the case. For example, searching "mozila" returns results containing "mozilla". Because of the way suggestions were filtered, this resulted in suggestions not appearing for a number of queries.
Comment on attachment 625828 [details] [diff] [review]
Part 2a: Add FlowLayout for floating elements

Looks OK to me. Think about whether we can test this using robocop.
Attachment #625828 - Flags: review?(mark.finkle) → review+
In general, think about how to test the SuggestClient. Maybe we can make a simple mochitest endpoint that could return different hardcoded JSON sets for given input strings.
Comment on attachment 623368 [details] [diff] [review]
Part 3: Add search suggestion preference

"Show search suggestions" works for me
Attachment #623368 - Flags: review?(ibarlow) → review+
Comment on attachment 625829 [details] [diff] [review]
Part 2b: Add search suggestions to AwesomeBar

Review of attachment 625829 [details] [diff] [review]:
-----------------------------------------------------------------

The explosion in complexity of AwesomeBar/AwesomeBarTabs is starting to worry me a bit. We'll have to refactor a lot of this code very soon. The changes to AwesomeBar look generally good but I think the inflation/recycling of suggestion items needs work to avoid performance problems in the list view.

::: mobile/android/base/AwesomeBar.java
@@ +139,5 @@
> +            public void onSearch(String engine, String text) {
> +                openSearchAndFinish(text, engine);
> +            }
> +
> +            public void onLongClick(final String text) {

This new API looks a bit too generic. onSuggestionClicked (or something similar) would be more direct and obvious.

@@ +233,5 @@
>                  // no composition string. It is safe to update IME flags.
>                  updateGoButton(text);
> +
> +                // cancel previous query
> +                if (mSuggestTask != null && mSuggestTask.getStatus() == AsyncTask.Status.RUNNING) {

Is this status check actually necessary? And it's racy anyway as the status might change after the 'if' condition runs.

@@ +238,5 @@
> +                    mSuggestTask.cancel(true);
> +                }
> +
> +                if (mSuggestClient != null) {
> +                    mSuggestTask = new AsyncTask<String, Void, ArrayList<String>>() {

Maybe just name declare this async task in a class so that this code just does something like "mSuggestTask = new QuerySuggestionsTask()". Not a big deal but it would make the code a bit cleaner.

@@ +243,5 @@
> +                         protected ArrayList<String> doInBackground(String... query) {
> +                             return mSuggestClient.query(query[0]);
> +                         }
> +
> +                         protected void onProgressUpdate(Void... progress) { }

Unused? Just removed it.

@@ +291,5 @@
>          registerForContextMenu(mAwesomeTabs.findViewById(R.id.all_pages_list));
>          registerForContextMenu(mAwesomeTabs.findViewById(R.id.bookmarks_list));
>          registerForContextMenu(mAwesomeTabs.findViewById(R.id.history_list));
>  
> +        if (sSuggestTemplate == null) {

Factor all this code to load mSuggestClient into a separate "loadSuggestClient" method. onCreate() has already got too chunky...

@@ +315,5 @@
>      public void handleMessage(String event, JSONObject message) {
>          try {
>              if (event.equals("SearchEngines:Data")) {
> +                // store the template for search suggestions in android prefs
> +                final String suggestEngine = message.isNull("suggestEngine") ? null : message.getString("suggestEngine");

You can use message.optString() instead.

@@ +316,5 @@
>          try {
>              if (event.equals("SearchEngines:Data")) {
> +                // store the template for search suggestions in android prefs
> +                final String suggestEngine = message.isNull("suggestEngine") ? null : message.getString("suggestEngine");
> +                final String suggestTemplate = message.isNull("suggestTemplate") ? null : message.getString("suggestTemplate");

Same here.

@@ +318,5 @@
> +                // store the template for search suggestions in android prefs
> +                final String suggestEngine = message.isNull("suggestEngine") ? null : message.getString("suggestEngine");
> +                final String suggestTemplate = message.isNull("suggestTemplate") ? null : message.getString("suggestTemplate");
> +                if (!TextUtils.equals(suggestTemplate, sSuggestTemplate)) {
> +                    GeckoAppShell.getHandler().post(new Runnable() {

Move that code into a saveSuggestEngineData method?

::: mobile/android/base/AwesomeBarTabs.java
@@ +759,5 @@
> +            if (getItemViewType(position) == ROW_SEARCH) {
> +                if (convertView == null) {
> +                    convertView = mInflater.inflate(R.layout.awesomebar_suggestion_row, null);
> +                } else {
> +                    ((FlowLayout) convertView.findViewById(R.id.suggestion_layout)).removeAllViews();

This will slightly break the View Holder pattern and findViewById will be called more often that it should. You have to apply View Holder pattern for the ROW_SEARCH type of items too.

@@ +765,5 @@
>  
> +                bindSearchEngineView(getEngine(getEngineIndex(position)), convertView);
> +            } else {
> +                if (convertView == null) {
> +                    viewHolder = new ViewHolder();

Why flipping the order of those calls? Seems unnecessary.

@@ +793,5 @@
>              return convertView;
>          }
>  
> +        private void bindSearchEngineView(final SearchEngine engine, View convertView) {
> +            FlowLayout layout = (FlowLayout) convertView.findViewById(R.id.suggestion_layout);

This change has two problems:
1. It will potentially inflate several views on each getView() call for ROW_SEARCH type
2. It will cause several findViewById calls on each getView() call

This will negatively impact scrolling performance in the awesomebar list view (especially when scrolling ROW_SEARCH type of rows). Could you please post a screenshot of what you're implementing here just so I can maybe suggest alternative ways to do it? Maybe create pool of stub views for the suggestions that can be recycled instead of inflating all of them of each getView call?

@@ +814,5 @@
> +                View suggestionItem = mInflater.inflate(R.layout.awesomebar_suggestion_item, null);
> +                ((TextView) suggestionItem.findViewById(R.id.suggestion_text)).setText(suggestion);
> +                ((ImageView) suggestionItem.findViewById(R.id.suggestion_magnifier)).setVisibility(View.GONE);
> +                layout.addView(suggestionItem);
> +                suggestionItem.setOnClickListener(new OnClickListener() {

You can probably instantiate the click listener only once and use it for all items. No need to create one listener for each.

@@ +822,5 @@
> +                            mUrlOpenListener.onSearch(engine.name, suggestion);
> +                        }
> +                    }
> +                });
> +                suggestionItem.setOnLongClickListener(new OnLongClickListener() {

Same here.
Attachment #625829 - Flags: review?(lucasr.at.mozilla) → review-
(In reply to Lucas Rocha (:lucasr) from comment #55)
> 
> This change has two problems:
> 1. It will potentially inflate several views on each getView() call for
> ROW_SEARCH type
> 2. It will cause several findViewById calls on each getView() call
> 
> This will negatively impact scrolling performance in the awesomebar list
> view (especially when scrolling ROW_SEARCH type of rows). Could you please
> post a screenshot of what you're implementing here just so I can maybe
> suggest alternative ways to do it? Maybe create pool of stub views for the
> suggestions that can be recycled instead of inflating all of them of each
> getView call?

I've been thinking about how to improve this, and there are a few options:

1) Since we currently set the number of max suggestions to 3 (plus one more for the user-entered query), we could simply add 4 static awesomebar_suggestion_items to each suggestion_layout. This way, we always know there will be exactly 4 children, and any unused items could be set to View.GONE. This would be the easiest to implement, though I don't like scattering the assumption that the max suggestions will be 3 throughout the code.

2) Instead of calling removeAllViews() on the converted view, we could count the number N of child views it contained. If it's less than the number M of suggestions we want to show, we could inflate M-N views. If M<N, we could remove (or hide) the extra views.

3) Implement a pool of views as you suggested, and reuse ones that get recycled. I think this would involve adding a RecyclerListener to the ListView, seeing if it's a search view, iterating its suggestion_item children, and adding each to a cache, where they're popped when needed. This also sounds rather expensive, but maybe there's an easier way to do it.

It might also be fine to just land this and file a follow-up bug for improving this (if necessary). There's no question that inflation is relatively expensive, but "relative" here still means just a few ms. There's only 4 search engines (by default), so the performance hit really won't be perceptible.

Regarding a screenshot, see images 2 and 3 at http://ianbarlow.wordpress.com/2012/03/09/enhanced-search-in-the-firefox-awesomebar/ (it's a mockup, but essentially the same). If you'd like, you can also try out the build in comment 34.
(In reply to Lucas Rocha (:lucasr) from comment #55)
> 
> ::: mobile/android/base/AwesomeBar.java
> @@ +139,5 @@
> > +            public void onSearch(String engine, String text) {
> > +                openSearchAndFinish(text, engine);
> > +            }
> > +
> > +            public void onLongClick(final String text) {
> 
> This new API looks a bit too generic. onSuggestionClicked (or something
> similar) would be more direct and obvious.

onSuggestionClicked probably isn't the best name since this is the callback for a suggestion *long* click (a normal suggestion click just does a search). I went with onEditSuggestion, which describes what the user wants to do, not what the user is doing (like onUrlOpen and onSearch).

> 
> @@ +793,5 @@
> >              return convertView;
> >          }
> >  
> > +        private void bindSearchEngineView(final SearchEngine engine, View convertView) {
> > +            FlowLayout layout = (FlowLayout) convertView.findViewById(R.id.suggestion_layout);
> 
> This change has two problems:
> 1. It will potentially inflate several views on each getView() call for
> ROW_SEARCH type
> 2. It will cause several findViewById calls on each getView() call
> 
> This will negatively impact scrolling performance in the awesomebar list
> view (especially when scrolling ROW_SEARCH type of rows). Could you please
> post a screenshot of what you're implementing here just so I can maybe
> suggest alternative ways to do it? Maybe create pool of stub views for the
> suggestions that can be recycled instead of inflating all of them of each
> getView call?

I went with option #2 in comment 56. It reuses the suggestion items, and it was fairly straightforward to implement.
Attachment #625829 - Attachment is obsolete: true
Attachment #626639 - Flags: review?(mark.finkle)
Attachment #626639 - Flags: review?(lucasr.at.mozilla)
Minor change - forgot to refresh a small part of the patch.
Attachment #626639 - Attachment is obsolete: true
Attachment #626639 - Flags: review?(mark.finkle)
Attachment #626639 - Flags: review?(lucasr.at.mozilla)
Attachment #626647 - Flags: review?(mark.finkle)
Attachment #626647 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 626647 [details] [diff] [review]
Part 2b: Add search suggestions to AwesomeBar (v2)

Review of attachment 626647 [details] [diff] [review]:
-----------------------------------------------------------------

Not that I'm giving r+ only to the changes in AwesomeBar and AwesomeBarTabs. I'm assuming that mfinkle is reviewing the other parts of the patch.

::: mobile/android/base/AwesomeBar.java
@@ +259,5 @@
> +        if (sSuggestTemplate == null) {
> +            loadSuggestClientFromPrefs();
> +        } else {
> +            loadSuggestClient();
> +        }

Much cleaner, great.

::: mobile/android/base/AwesomeBarTabs.java
@@ +87,5 @@
>  
>      public interface OnUrlOpenListener {
>          public void onUrlOpen(String url);
> +        public void onSearch(String engine, String text);
> +        public void onEditSuggestion(String suggestion);

Nice. "onEditSuggestion" is a much better name :-)

@@ +97,5 @@
>          public ImageView faviconView;
>          public ImageView starView;
>      }
>  
> +    private class SearchViewHolder {

SearchEntryViewHolder for consistency?

@@ +651,5 @@
>              getFilter().filter(searchTerm);
>          }
>  
> +        private int getSuggestionCount() {
> +            return (mSearchTerm.length() == 0 || mSuggestEngine == null) ? 0 : 1;

Just checking: is mSearchTerm guaranteed to be not null?

@@ +664,4 @@
>              if (mSearchTerm.length() == 0)
>                  return resultCount;
>  
> +            return resultCount + mSearchEngines.size() + getSuggestionCount();

Just checking: is mSearchEngines guaranteed to be not null at any time getCount() gets called?

@@ +682,5 @@
> +            // return search engine
> +            return new AwesomeBarSearchEngineItem(getEngine(engineIndex).name);
> +        }
> +
> +        private SearchEngine getEngine(int index) {

getEngineAtIndex() for clarity? Not a big deal.

@@ +717,5 @@
> +            return 2;
> +        }
> +
> +        @Override
> +        public boolean isEnabled(int position) {

Wondering: why do you need to implement this? Is it because We might show an empty/non-clickable suggestions row in the results?

@@ +780,5 @@
> +        private void bindSearchEngineView(final SearchEngine engine, SearchViewHolder viewHolder) {
> +            OnClickListener clickListener = new OnClickListener() {
> +                public void onClick(View v) {
> +                    if (mUrlOpenListener != null) {
> +                        String suggestion = ((TextView) v.findViewById(R.id.suggestion_text)).getText().toString();

You could probably do something like:

SearchEntryViewHolder viewHolder = v.getTag();
String suggestion = viewHolder.userEnteredTextView.getText().toString();
mUrlOpenListener.onSearch(engine.name, suggestion);

Won't this code break

@@ +789,5 @@
>  
> +            OnLongClickListener longClickListener = new OnLongClickListener() {
> +                public boolean onLongClick(View v) {
> +                    if (mUrlOpenListener != null) {
> +                        String suggestion = ((TextView) v.findViewById(R.id.suggestion_text)).getText().toString();

Same here.

@@ +818,5 @@
> +                    suggestionItem = suggestionView.getChildAt(i+1);
> +                    suggestionItem.setVisibility(View.VISIBLE);
> +                } else {
> +                    suggestionItem = mInflater.inflate(R.layout.awesomebar_suggestion_item, null);
> +                    ((ImageView) suggestionItem.findViewById(R.id.suggestion_magnifier)).setVisibility(View.GONE);

If the magnifier icon is only shown for the first suggestion, maybe it should only be added there? You're creating a few ImageViews that will never be used. I won't r- because of this but maybe file a follow-up?
Attachment #626647 - Flags: review?(lucasr.at.mozilla) → review+
The SuggestClient makes its own HTTP requests outside of Gecko, so it is unable to resolve http://mochi.test URLs. This patch adds the IP of the remote machine to the robotium.config.
Attachment #627000 - Flags: review?(jmaher)
This patch uses a local suggestion server that returns suggestions for the query "foo bar".
Attachment #627001 - Flags: review?(gbrown)
Comment on attachment 627000 [details] [diff] [review]
Test case part 1: Add raw host to robotium.config

Review of attachment 627000 [details] [diff] [review]:
-----------------------------------------------------------------

This looks just fine.  One thing is we generate a robotium.config in talos (hg.mozilla.org/build/talos), so we should make sure we update talos as well (or ensure we handle the scenario where the rawurl doesn't exist).
Attachment #627000 - Flags: review?(jmaher) → review+
Comment on attachment 626647 [details] [diff] [review]
Part 2b: Add search suggestions to AwesomeBar (v2)

>diff --git a/mobile/android/base/AwesomeBar.java b/mobile/android/base/AwesomeBar.java

>+        if (sSuggestTemplate == null) {
>+            loadSuggestClientFromPrefs();
>+        } else {
>+            loadSuggestClient();
>+        }

{} not needed for one-liners
Attachment #626647 - Flags: review?(mark.finkle) → review+
Comment on attachment 627001 [details] [diff] [review]
Test case part 2: Search suggestions test case

Review of attachment 627001 [details] [diff] [review]:
-----------------------------------------------------------------

Looks just fine and runs reliably for me.
Attachment #627001 - Flags: review?(gbrown) → review+
What's the ETA on landing this?
(In reply to Mark Finkle (:mfinkle) from comment #65)
> What's the ETA on landing this?

This has failed try for a couple of reasons. It failed at first because the test phones aren't connected to a network, making the SuggestClient return immediately. To fix it, I added an mCheckNetwork boolean to the SuggestClient used exclusively by the robotium and modified via reflection.

All the mochitests now pass, but several of the talos tests (rck, rck2, rp) are failing. This is probably because the rawhost config parameter doesn't exist for talos tests, causing breakage as Joel said in comment 62. I've been trying to get talos tests running on my machine, but I think I'll just give up for now and use try.

Hopefully, making this change to talos will fix those errors. I'll push to try again once the change is made to talos, and I'll land after that.
I haven't been able to run talos tests on my machine, so this patch hasn't been tested.
Attachment #629209 - Flags: review?(jmaher)
Comment on attachment 629209 [details] [diff] [review]
talos only: Add rawhost to robotium.config

Review of attachment 629209 [details] [diff] [review]:
-----------------------------------------------------------------

This is great.  I can test this locally when my tegra frees up.
Attachment #629209 - Flags: review?(jmaher) → review+
Depends on: 760572
Armen if you can quick review and verify the path to talos.zip exists.
Attachment #629239 - Flags: review?(armenzg)
Comment on attachment 629239 [details] [diff] [review]
adjust talos.json to reference the new talos.zip file (1.0)

The URL is correct.
Attachment #629239 - Flags: review?(armenzg) → review+
(In reply to Lucas Rocha (:lucasr) from comment #59)
> Comment on attachment 626647 [details] [diff] [review]
> Part 2b: Add search suggestions to AwesomeBar (v2)
>
> > +        private int getSuggestionCount() {
> > +            return (mSearchTerm.length() == 0 || mSuggestEngine == null) ? 0 : 1;
> 
> Just checking: is mSearchTerm guaranteed to be not null?

Yeah, mSearchTerm is initialized to an empty string, so it will never be null after that (since getText() doesn't return null).

> > +            return resultCount + mSearchEngines.size() + getSuggestionCount();
> 
> Just checking: is mSearchEngines guaranteed to be not null at any time
> getCount() gets called?

Yep, mSearchEngines is initialized to an empty ArrayList.

> > +        @Override
> > +        public boolean isEnabled(int position) {
> 
> Wondering: why do you need to implement this? Is it because We might show an
> empty/non-clickable suggestions row in the results?

If a row only shows what the user typed without any additional suggestions, I figured the whole row should be clickable since it only contains one entry. But if it contains multiple suggestions, clicking the row itself does nothing (I'll add a comment for that).

> 
> @@ +780,5 @@
> > +        private void bindSearchEngineView(final SearchEngine engine, SearchViewHolder viewHolder) {
> > +            OnClickListener clickListener = new OnClickListener() {
> > +                public void onClick(View v) {
> > +                    if (mUrlOpenListener != null) {
> > +                        String suggestion = ((TextView) v.findViewById(R.id.suggestion_text)).getText().toString();
> 
> You could probably do something like:
> 
> SearchEntryViewHolder viewHolder = v.getTag();
> String suggestion = viewHolder.userEnteredTextView.getText().toString();
> mUrlOpenListener.onSearch(engine.name, suggestion);

That won't work since these listeners are associated with the individual suggestion Views (and not the entire awesomebar_suggestion_row), so they don't have view holders.

> If the magnifier icon is only shown for the first suggestion, maybe it
> should only be added there? You're creating a few ImageViews that will never
> be used. I won't r- because of this but maybe file a follow-up?

Perhaps there should be separate XML layouts defined for "suggestion views" vs "user-entered views"? Filed bug 760634.
Flags: in-moztrap?(fennec)
Depends on: 762064
Depends on: 762068
Security: This feature sends anything typed into the awesomebar to a search provider (Google by default) so we can get real-time suggestions.

This is similar to the search box in desktop Firefox, but it happens all the time.
sec triage: in looking at this we think this needs a privacy review over a sec review.
From a sec perspective this should have a pref that the user can set to turn this off.
Depends on: 762968
Depends on: 755944
Attached patch search suggestions single patch (obsolete) — Splinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: no search suggestions in awesomebar
Testing completed (on m-c, etc.): 5 days in m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none
Attachment #631804 - Flags: approval-mozilla-aurora?
(In reply to Brian Nicholson (:bnicholson) from comment #79)
> User impact if declined: no search suggestions in awesomebar

Was this approval requested by the product team?
Comment on attachment 631804 [details] [diff] [review]
search suggestions single patch

No, though I do want to address the followup bugs before this is considered.
Attachment #631804 - Flags: approval-mozilla-aurora?
No longer depends on: 755944
(In reply to Alex Keybl [:akeybl] from comment #80)
> (In reply to Brian Nicholson (:bnicholson) from comment #79)
> > User impact if declined: no search suggestions in awesomebar
> 
> Was this approval requested by the product team?

This feature is on the existing roadmap for Fx15. I agree about the followup bugs though. Let's get this cleaned up a bit.
Depends on: 763184
Depends on: 764699
Depends on: 767541
Depends on: 769145
Some parts of bug 739364 were not able to land on Firefox 15 because they change the styling of search suggestions.  If search suggestions end up landing on Firefox 15, let's make sure to also land those missing parts from bug 739364.
Depends on: 739364
Blocks: 773788
Rebased with suggestions disabled by default.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: No suggestions in AwesomeScreen
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk, especially since its disabled by default
String or UUID changes made by this patch: 1 string: "Show search suggestions" preference
Attachment #631804 - Attachment is obsolete: true
Attachment #642190 - Flags: approval-mozilla-aurora?
This is a single patch of all necessary follow-ups, including bug 762064, bug 762068, bug 764699, bug 767541, bug 762968, bug 739364, and bug 770886.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this bug
User impact if declined: misc problems with suggestions
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none
Attachment #642193 - Flags: approval-mozilla-aurora?
Comment on attachment 642193 [details] [diff] [review]
search suggestions follow-up patches

Alex, Madhava and I talked in email and all thought this was a good plan, so I am a+ the rollup patch
Attachment #642193 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #642190 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I tried landing these two patches, but HG is not liking them very much. Says both patches are empty.
(In reply to Mark Finkle (:mfinkle) from comment #87)
> I tried landing these two patches, but HG is not liking them very much. Says
> both patches are empty.

I figured it out and got the patches to apply. Doing a test build now.
Well, you must have managed to land something on aurora, because you've had robocop perma-orange ever since.

https://tbpl.mozilla.org/php/getParsedLog.php?id=13546703&tree=Mozilla-Aurora
2 INFO TEST-UNEXPECTED-FAIL | testSearchSuggestions | Results for query 'f' matched expected suggestions - got false, expected true
Depends on: 774517
QA Contact: xwei
Flags: in-moztrap?(fennec) → in-moztrap?(xwei)
Depends on: 775374
I noticed that the SuggestClient does not use Fennec's proxy settings. It looks like the HttpURLConnection that is used has no knowledge about Fennec's proxy settings. Should I file a separate bug for that?
Please file a new bug for that.
Status: RESOLVED → VERIFIED
Flags: in-moztrap?(xwei) → in-moztrap+
Depends on: 820576
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.