Last Comment Bug 586885 - show search suggestions when entering text in awesome bar
: show search suggestions when entering text in awesome bar
Status: VERIFIED FIXED
: privacy-review-needed, uiwanted
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: P3 enhancement (vote)
: Firefox 16
Assigned To: Brian Nicholson (:bnicholson)
: Xiao Meng Wei :xwei
Mentors:
Depends on: 776781 739364 760572 762064 762068 762968 763184 764699 767541 769145 770886 774517 775374 820576
Blocks: 773788
  Show dependency treegraph
 
Reported: 2010-08-12 23:21 PDT by Brad Lassey [:blassey] (use needinfo?)
Modified: 2016-07-29 14:19 PDT (History)
21 users (show)
xwei: in‑moztrap+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified
-
15+


Attachments
WIP (6.55 KB, patch)
2010-09-25 20:20 PDT, Wesley Johnston (:wesj)
21: feedback+
Details | Diff | Splinter Review
patch (3.20 KB, patch)
2011-08-22 14:57 PDT, Brad Lassey [:blassey] (use needinfo?)
mark.finkle: review-
Details | Diff | Splinter Review
Form History patch (5.59 KB, patch)
2011-09-09 11:12 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Form History fix v1 (10.98 KB, patch)
2011-09-21 16:35 PDT, Wesley Johnston (:wesj)
gavin.sharp: feedback+
Details | Diff | Splinter Review
Part 1: Use AwesomeBarItem to handle click callbacks (5.54 KB, patch)
2012-05-11 16:34 PDT, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
Part 1: Use AwesomeBarItem to handle click callbacks (6.15 KB, patch)
2012-05-11 16:39 PDT, Brian Nicholson (:bnicholson)
mark.finkle: review+
Details | Diff | Splinter Review
Part 2: Add search suggestions to AwesomeBar (33.75 KB, patch)
2012-05-11 17:04 PDT, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
Part 3: Add search suggestion preference (3.01 KB, patch)
2012-05-11 17:08 PDT, Brian Nicholson (:bnicholson)
mark.finkle: review+
ibarlow: review+
Details | Diff | Splinter Review
suggestions screen shot (122.20 KB, image/png)
2012-05-11 17:33 PDT, Brian Nicholson (:bnicholson)
no flags Details
Multi-row search suggest mockup (328.07 KB, image/png)
2012-05-15 07:54 PDT, Ian Barlow (:ibarlow)
no flags Details
Part 2 (v2): Add search suggestions to AwesomeBar (47.09 KB, patch)
2012-05-15 22:35 PDT, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
Part 2 (v3): Add search suggestions to AwesomeBar (47.40 KB, patch)
2012-05-15 23:38 PDT, Brian Nicholson (:bnicholson)
mark.finkle: review+
Details | Diff | Splinter Review
Part 2a: Add FlowLayout for floating elements (4.46 KB, patch)
2012-05-21 17:13 PDT, Brian Nicholson (:bnicholson)
mark.finkle: review+
Details | Diff | Splinter Review
Part 2b: Add search suggestions to AwesomeBar (42.67 KB, patch)
2012-05-21 17:15 PDT, Brian Nicholson (:bnicholson)
lucasr.at.mozilla: review-
bnicholson: review+
Details | Diff | Splinter Review
Part 2b: Add search suggestions to AwesomeBar (v2) (47.49 KB, patch)
2012-05-23 17:12 PDT, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
Part 2b: Add search suggestions to AwesomeBar (v2) (47.48 KB, patch)
2012-05-23 17:24 PDT, Brian Nicholson (:bnicholson)
mark.finkle: review+
lucasr.at.mozilla: review+
Details | Diff | Splinter Review
Test case part 1: Add raw host to robotium.config (4.66 KB, patch)
2012-05-24 16:02 PDT, Brian Nicholson (:bnicholson)
jmaher: review+
Details | Diff | Splinter Review
Test case part 2: Search suggestions test case (8.13 KB, patch)
2012-05-24 16:05 PDT, Brian Nicholson (:bnicholson)
gbrown: review+
Details | Diff | Splinter Review
talos only: Add rawhost to robotium.config (1.30 KB, patch)
2012-06-01 08:48 PDT, Brian Nicholson (:bnicholson)
jmaher: review+
Details | Diff | Splinter Review
adjust talos.json to reference the new talos.zip file (1.0) (561 bytes, patch)
2012-06-01 10:34 PDT, Joel Maher ( :jmaher)
armenzg: review+
Details | Diff | Splinter Review
search suggestions single patch (70.55 KB, patch)
2012-06-10 22:45 PDT, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
search suggestions single patch, v2 (72.15 KB, patch)
2012-07-14 01:15 PDT, Brian Nicholson (:bnicholson)
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
search suggestions follow-up patches (27.02 KB, patch)
2012-07-14 01:21 PDT, Brian Nicholson (:bnicholson)
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Brad Lassey [:blassey] (use needinfo?) 2010-08-12 23:21:41 PDT
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.
Comment 1 Doug Turner (:dougt) 2010-09-16 11:23:22 PDT
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?
Comment 2 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2010-09-16 11:29:49 PDT
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?
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2010-09-23 11:43:59 PDT
Would be nice to have, but we have no design and no patch
Comment 4 Wesley Johnston (:wesj) 2010-09-25 20:20:33 PDT
Created attachment 478610 [details] [diff] [review]
WIP

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.
Comment 5 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2010-09-28 09:38:18 PDT
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.
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2010-09-28 12:41:30 PDT
(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.
Comment 7 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-28 20:30:07 PDT
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.
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2010-09-28 20:59:06 PDT
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.
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-11 07:21:17 PDT
Madhava - Thoughts on this idea?
Comment 10 Brad Lassey [:blassey] (use needinfo?) 2011-08-22 14:57:48 PDT
Created attachment 554974 [details] [diff] [review]
patch

this is wes's patch updated to tip. Not sure why its sat for so long
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-22 21:32:14 PDT
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
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-22 21:50:51 PDT
Hmm, I do see that the patch does actually return search suggestions, even with the errors. I wonder how we can squelch the error.
Comment 13 Wesley Johnston (:wesj) 2011-08-23 07:34:25 PDT
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.
Comment 14 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-23 08:17:12 PDT
(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.
Comment 15 Wesley Johnston (:wesj) 2011-09-09 11:12:54 PDT
Created attachment 559518 [details] [diff] [review]
Form History patch

Patch to remove the form history errors. Seems fine on desktop and mobile, but I need to write tests for this.
Comment 16 Wesley Johnston (:wesj) 2011-09-21 16:35:07 PDT
Created attachment 561610 [details] [diff] [review]
Form History fix v1

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.
Comment 17 Wesley Johnston (:wesj) 2011-09-21 16:48:32 PDT
> 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 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-09 15:31:48 PST
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.
Comment 19 Aaron Train [:aaronmt] 2012-01-09 15:43:57 PST
I wonder if Native Fennec want's this.
Comment 20 Brian Nicholson (:bnicholson) 2012-05-11 16:34:27 PDT
Created attachment 623359 [details] [diff] [review]
Part 1: Use AwesomeBarItem to handle click callbacks

Uses a more object-oriented approach for handling AwesomeBar clicks.
Comment 21 Brian Nicholson (:bnicholson) 2012-05-11 16:39:16 PDT
Created attachment 623360 [details] [diff] [review]
Part 1: Use AwesomeBarItem to handle click callbacks

Missed some lines refreshing the patch.
Comment 22 Brian Nicholson (:bnicholson) 2012-05-11 17:04:52 PDT
Created attachment 623367 [details] [diff] [review]
Part 2: Add search suggestions to AwesomeBar

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.
Comment 23 Brian Nicholson (:bnicholson) 2012-05-11 17:08:26 PDT
Created attachment 623368 [details] [diff] [review]
Part 3: Add search suggestion preference

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.
Comment 24 Brian Nicholson (:bnicholson) 2012-05-11 17:33:37 PDT
Created attachment 623372 [details]
suggestions screen shot

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.
Comment 25 Kevin Brosnan [:kbrosnan] 2012-05-11 17:42:02 PDT
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.
Comment 26 Brian Nicholson (:bnicholson) 2012-05-11 18:06:18 PDT
(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.
Comment 27 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-14 06:37:06 PDT
Brian - Can you post a build for UX to play with?
Comment 28 Ian Barlow (:ibarlow) 2012-05-14 06:55:37 PDT
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
Comment 29 Brian Nicholson (:bnicholson) 2012-05-14 10:33:51 PDT
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.
Comment 30 Ian Barlow (:ibarlow) 2012-05-15 07:54:45 PDT
Created attachment 624041 [details]
Multi-row search suggest mockup

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.
Comment 31 Ian Barlow (:ibarlow) 2012-05-15 08:13:55 PDT
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?
Comment 32 Brian Nicholson (:bnicholson) 2012-05-15 10:30:39 PDT
(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?
Comment 33 Brian Nicholson (:bnicholson) 2012-05-15 22:35:38 PDT
Created attachment 624302 [details] [diff] [review]
Part 2 (v2): Add search suggestions to AwesomeBar

Updated to use new flow UI.
Comment 34 Brian Nicholson (:bnicholson) 2012-05-15 22:44:07 PDT
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 35 Brian Nicholson (:bnicholson) 2012-05-15 23:35:43 PDT
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 = [];
>
Comment 36 Brian Nicholson (:bnicholson) 2012-05-15 23:38:01 PDT
Created attachment 624309 [details] [diff] [review]
Part 2 (v3): Add search suggestions to AwesomeBar

Fixed a few mistakes.
Comment 37 Brian Nicholson (:bnicholson) 2012-05-16 09:53:52 PDT
Whoa, just noticed that it showed the entire patch when I changed the name in comment 35. Sorry about that.
Comment 38 :Margaret Leibovic 2012-05-16 10:09:40 PDT
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
Comment 39 :Margaret Leibovic 2012-05-16 10:10:48 PDT
(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|
Comment 40 Brian Nicholson (:bnicholson) 2012-05-16 10:31:47 PDT
(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.
Comment 41 Joe Drew (not getting mail) 2012-05-16 11:33:45 PDT
New feature, hence minus for 1.0.
Comment 42 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-16 15:01:07 PDT
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.
Comment 43 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-18 21:28:14 PDT
>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 44 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-21 10:42:31 PDT
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
Comment 45 Gervase Markham [:gerv] 2012-05-21 10:50:27 PDT
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
Comment 46 Brian Nicholson (:bnicholson) 2012-05-21 17:10:23 PDT
(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.
Comment 47 Brian Nicholson (:bnicholson) 2012-05-21 17:13:24 PDT
Created attachment 625828 [details] [diff] [review]
Part 2a: Add FlowLayout for floating elements

New FlowLayout implementation, built from scratch.
Comment 48 Brian Nicholson (:bnicholson) 2012-05-21 17:15:10 PDT
Created attachment 625829 [details] [diff] [review]
Part 2b: Add search suggestions to AwesomeBar

New patch with Mark's changes and FlowLayout moved to patch 2a.
Comment 49 Brian Nicholson (:bnicholson) 2012-05-21 17:15:40 PDT
Comment on attachment 625829 [details] [diff] [review]
Part 2b: Add search suggestions to AwesomeBar

Carrying over r=mfinkle
Comment 50 Brian Nicholson (:bnicholson) 2012-05-21 17:21:37 PDT
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;
-    }
+    });
Comment 51 Brian Nicholson (:bnicholson) 2012-05-21 17:31:55 PDT
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 52 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-21 20:36:55 PDT
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.
Comment 53 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-21 20:38:01 PDT
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 54 Ian Barlow (:ibarlow) 2012-05-22 06:09:01 PDT
Comment on attachment 623368 [details] [diff] [review]
Part 3: Add search suggestion preference

"Show search suggestions" works for me
Comment 55 Lucas Rocha (:lucasr) 2012-05-23 03:43:37 PDT
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.
Comment 56 Brian Nicholson (:bnicholson) 2012-05-23 15:29:57 PDT
(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.
Comment 57 Brian Nicholson (:bnicholson) 2012-05-23 17:12:12 PDT
Created attachment 626639 [details] [diff] [review]
Part 2b: Add search suggestions to AwesomeBar (v2)

(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.
Comment 58 Brian Nicholson (:bnicholson) 2012-05-23 17:24:42 PDT
Created attachment 626647 [details] [diff] [review]
Part 2b: Add search suggestions to AwesomeBar (v2)

Minor change - forgot to refresh a small part of the patch.
Comment 59 Lucas Rocha (:lucasr) 2012-05-24 02:15:50 PDT
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?
Comment 60 Brian Nicholson (:bnicholson) 2012-05-24 16:02:11 PDT
Created attachment 627000 [details] [diff] [review]
Test case part 1: Add raw host to robotium.config

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.
Comment 61 Brian Nicholson (:bnicholson) 2012-05-24 16:05:03 PDT
Created attachment 627001 [details] [diff] [review]
Test case part 2: Search suggestions test case

This patch uses a local suggestion server that returns suggestions for the query "foo bar".
Comment 62 Joel Maher ( :jmaher) 2012-05-24 18:27:42 PDT
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).
Comment 63 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-25 06:12:05 PDT
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
Comment 64 Geoff Brown [:gbrown] 2012-05-25 11:11:36 PDT
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.
Comment 65 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-31 22:22:16 PDT
What's the ETA on landing this?
Comment 66 Brian Nicholson (:bnicholson) 2012-06-01 08:46:01 PDT
(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.
Comment 67 Brian Nicholson (:bnicholson) 2012-06-01 08:48:07 PDT
Created attachment 629209 [details] [diff] [review]
talos only: Add rawhost to robotium.config

I haven't been able to run talos tests on my machine, so this patch hasn't been tested.
Comment 68 Joel Maher ( :jmaher) 2012-06-01 08:52:42 PDT
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.
Comment 69 Joel Maher ( :jmaher) 2012-06-01 09:51:25 PDT
modified talos patch landed:
http://hg.mozilla.org/build/talos/rev/aec9ddd2a04c
Comment 70 Joel Maher ( :jmaher) 2012-06-01 10:34:41 PDT
Created attachment 629239 [details] [diff] [review]
adjust talos.json to reference the new talos.zip file (1.0)

Armen if you can quick review and verify the path to talos.zip exists.
Comment 71 Armen Zambrano [:armenzg] (EDT/UTC-4) 2012-06-01 10:35:14 PDT
Comment on attachment 629239 [details] [diff] [review]
adjust talos.json to reference the new talos.zip file (1.0)

The URL is correct.
Comment 72 Brian Nicholson (:bnicholson) 2012-06-01 11:18:08 PDT
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9453edbf76e2
Comment 73 Brian Nicholson (:bnicholson) 2012-06-01 12:44:15 PDT
(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.
Comment 74 Brian Nicholson (:bnicholson) 2012-06-04 14:22:25 PDT
Trying again with the patch from comment 70:
https://tbpl.mozilla.org/?tree=Try&rev=1fbd4b3c2a01
Comment 77 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-06 11:18:58 PDT
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.
Comment 78 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-06-06 14:23:44 PDT
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.
Comment 79 Brian Nicholson (:bnicholson) 2012-06-10 22:45:45 PDT
Created attachment 631804 [details] [diff] [review]
search suggestions single patch

[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
Comment 80 Alex Keybl [:akeybl] 2012-06-11 13:05:38 PDT
(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 81 Brian Nicholson (:bnicholson) 2012-06-11 13:15:54 PDT
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.
Comment 82 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-11 13:30:14 PDT
(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.
Comment 83 Matt Brubeck (:mbrubeck) 2012-07-12 17:31:31 PDT
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.
Comment 84 Brian Nicholson (:bnicholson) 2012-07-14 01:15:26 PDT
Created attachment 642190 [details] [diff] [review]
search suggestions single patch, v2

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
Comment 85 Brian Nicholson (:bnicholson) 2012-07-14 01:21:46 PDT
Created attachment 642193 [details] [diff] [review]
search suggestions follow-up patches

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
Comment 86 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-14 19:11:20 PDT
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
Comment 87 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-14 21:03:59 PDT
I tried landing these two patches, but HG is not liking them very much. Says both patches are empty.
Comment 88 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-14 21:26:44 PDT
(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.
Comment 89 Ryan VanderMeulen [:RyanVM] 2012-07-15 05:54:41 PDT
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
Comment 90 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-15 06:15:54 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/5a158c8f7684
https://hg.mozilla.org/releases/mozilla-aurora/rev/eecb9bc9cf03

and then disabled the robocop tests:
https://hg.mozilla.org/releases/mozilla-aurora/rev/cc642945416c

Since the feature is disabled by default, we need to add flip the pref for the test.
Comment 91 Stefan Arentz [:st3fan] 2012-07-21 17:52:08 PDT
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?
Comment 92 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-07-23 09:48:35 PDT
Please file a new bug for that.

Note You need to log in before you can comment on or make changes to this bug.