Last Comment Bug 701376 - show search engine(s) when there are no/few awesomebar results
: show search engine(s) when there are no/few awesomebar results
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P3 normal (vote)
: ---
Assigned To: Brian Nicholson (:bnicholson)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks: 708161
  Show dependency treegraph
 
Reported: 2011-11-10 07:52 PST by Madhava Enros [:madhava]
Modified: 2016-07-29 14:20 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
patch (24.45 KB, patch)
2011-12-01 16:54 PST, Brian Nicholson (:bnicholson)
lucasr.at.mozilla: feedback-
Details | Diff | Splinter Review
remove unnecessary argument from onUrlOpen (3.25 KB, patch)
2011-12-05 12:42 PST, Brian Nicholson (:bnicholson)
mark.finkle: review+
Details | Diff | Splinter Review
search engines patch v2 (27.36 KB, patch)
2011-12-05 12:43 PST, Brian Nicholson (:bnicholson)
mark.finkle: review+
lucasr.at.mozilla: feedback+
Details | Diff | Splinter Review
search engines patch v3 (27.35 KB, patch)
2011-12-06 18:34 PST, Brian Nicholson (:bnicholson)
bnicholson: review+
Details | Diff | Splinter Review

Description Madhava Enros [:madhava] 2011-11-10 07:52:39 PST
As in XUL Fennec, we should show make some search engines available for people to tap on when we run out of awesomesbar results to show them in the list below the entry field.

As a first set, we could use the ones that we use in XUL fennec: Google, Wikipedia, Amazon, Twitter.

In the design, we might want to indicate "default" next to Google (or whichever the user has chosen as the default in prefs). This default is the one that they'll also use if they just hit enter or tap the search icon in the field above.
Comment 1 Brian Nicholson (:bnicholson) 2011-12-01 16:54:11 PST
Created attachment 578442 [details] [diff] [review]
patch

This patch appends search engines to the AwesomeBar results.
Comment 2 Lucas Rocha (:lucasr) 2011-12-02 08:18:25 PST
Comment on attachment 578442 [details] [diff] [review]
patch

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

Looks generally good but needs some work before getting a r+ imo. I'd prefer mfinkle to review the js bits.

::: mobile/android/base/AwesomeBar.java
@@ +88,5 @@
>          mAwesomeTabs = (AwesomeBarTabs) findViewById(R.id.awesomebar_tabs);
>          mAwesomeTabs.setOnUrlOpenListener(new AwesomeBarTabs.OnUrlOpenListener() {
> +            public void onUrlOpen(String url) {
> +                openUrlAndFinish(url, null);
> +            }

Add empty between methods.

@@ +168,5 @@
>                  if (keyCode == KeyEvent.KEYCODE_ENTER) {
>                      if (event.getAction() != KeyEvent.ACTION_DOWN)
>                          return true;
>  
> +                    openUrlAndFinish(mText.getText().toString(), null);

I'd prefer if you add a method like startWebSearchAndFinish() that does the search. Adding an extra argument to openUrlAndFinish() that is null in almost every case feels a bit weird.

@@ +186,5 @@
>              }
>          });
> +
> +        GeckoAppShell.registerGeckoEventListener("SearchEngines:Data", this);
> +        GeckoAppShell.sendEventToGecko(new GeckoEvent("SearchEngines:Get", null));

This means users won't be able to use search engines until Gecko is loaded, right?

@@ +197,5 @@
> +            }
> +        } catch (Exception e) {
> +            // do nothing
> +            Log.i(LOGTAG, "handleMessage throws " + e + " for message: " + event);
> +        }

If you won't be getting list of engines anymore, simply disconnect the gecko event listener here instead of on destroy?

::: mobile/android/base/AwesomeBarTabs.java
@@ +95,2 @@
>  
> +    private AwesomeBarCursorAdapter mAllPagesAdapter;

AllPagesCursorAdapter for clarity?

@@ +102,5 @@
>      private static final int MAX_RESULTS = 100;
>  
>      public interface OnUrlOpenListener {
> +        public abstract void onUrlOpen(String url);
> +        public abstract void onSearch(String engine);

No need for the abstract here as this is an interface. I've done it wrong, don't follow it :-) Feel free to remove the 'abstract' from onUrlOpen().

Also, I noticed that you removed the first argument from onUrlOpen. Given that this is an unrelated change, I'd probably do it in separate micro-patch.

@@ +453,5 @@
> +            return resultCount + mSearchEngines.length();
> +        }
> +
> +        @Override
> +        public Object getItem(int position) {

Maybe add a comment about what you're doing here? It only adds search engines after the original filter results, right?

@@ +465,5 @@
> +                engine = mSearchEngines.getJSONObject(position - resultCount);
> +                engineName = engine.getString("name");
> +            } catch (JSONException e) {
> +                Log.e(LOGTAG, "error getting json arguments");
> +            }

Add empty line here.

@@ +472,5 @@
> +
> +        @Override
> +        public View getView(int position, View convertView, ViewGroup parent) {
> +            final int resultCount = super.getCount();
> +            if (position < resultCount)

Maybe factor out this code into a private method like "isSearchEnginePosition(position)" and reuse everywhere you need it?

@@ +474,5 @@
> +        public View getView(int position, View convertView, ViewGroup parent) {
> +            final int resultCount = super.getCount();
> +            if (position < resultCount)
> +                return super.getView(position, convertView, parent);
> +

I think on XUL Fennec we only show search engines when we should very few results for the search term. We can simply ignore search engines in case we're showing dozens of search results as they'll be buried at the end of the list anyway. I'd say, let's only show search engines if the search results only contain 2-4 items.

@@ +476,5 @@
> +            if (position < resultCount)
> +                return super.getView(position, convertView, parent);
> +
> +            final int index = position - resultCount;
> +            View v = mInflater.inflate(mLayout, parent, false);

You're not handling convertView correctly here. You have to check if convertView is null. If it is, you create a new view and populate it, if it's not null, you populate it with the new data and return it.

@@ +481,5 @@
> +            bindSearchEngineView(index, v);
> +            return v;
> +        }
> +
> +        private void bindSearchEngineView(int position, View view) {

Given that the list of available search engines is more or less constant, I think it would be better to inflate once and cache the views for each engine for later usage. This way you don't need to inflate a new view all the time.

@@ +498,5 @@
> +            TextView titleView = (TextView) view.findViewById(R.id.title);
> +            TextView urlView = (TextView) view.findViewById(R.id.url);
> +            ImageView faviconView = (ImageView) view.findViewById(R.id.favicon);
> +
> +            // convert data uri to image

Factor out this code to convert data uri to image into a separate private method? To keep bindSearchEngineView() simpler.

@@ +505,5 @@
> +            ByteArrayInputStream stream = new ByteArrayInputStream(bytes);
> +            Drawable engineIcon = (Drawable) Drawable.createFromStream(stream, "src");
> +
> +            titleView.setText(name);
> +            urlView.setText("Search for \"" + mSearchTerm + "\"");

Needs proper l10n support for this string.

@@ +684,5 @@
>  
>          String url = (String) historyItem.get(AwesomeBar.URL_KEY);
>  
>          if (mUrlOpenListener != null)
> +            mUrlOpenListener.onUrlOpen(url);

As I said before, this change is unrelated to what you're doing in this patch. Should go on a separate patch.

@@ +689,5 @@
>      }
>  
>      private void handleItemClick(ListView list, int position) {
> +        Object item = list.getItemAtPosition(position);
> +        if (item instanceof Cursor) {

Add a comment before this if explaining why you need to check the type of the item here.

@@ +738,5 @@
> +    public void setSearchEngines(JSONArray engines) {
> +        mSearchEngines = engines;
> +        GeckoAppShell.getMainHandler().post(new Runnable() {
> +            public void run() {
> +                mAllPagesAdapter.notifyDataSetChanged();

Maybe simply wait until the user filter again? Won't this cause a sudden re-scroll on the list view when Gecko finally returns the list of engines?
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-02 11:07:37 PST
Lucas has some good changes in here. My review is coming too, but you can start looking at some of his feedback.
Comment 4 Brian Nicholson (:bnicholson) 2011-12-02 13:09:26 PST
(In reply to Lucas Rocha (:lucasr) from comment #2)
> Comment on attachment 578442 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> Review of attachment 578442 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> @@ +186,5 @@
> >              }
> >          });
> > +
> > +        GeckoAppShell.registerGeckoEventListener("SearchEngines:Data", this);
> > +        GeckoAppShell.sendEventToGecko(new GeckoEvent("SearchEngines:Get", null));
> 
> This means users won't be able to use search engines until Gecko is loaded,
> right?

Correct.

> @@ +197,5 @@
> > +            }
> > +        } catch (Exception e) {
> > +            // do nothing
> > +            Log.i(LOGTAG, "handleMessage throws " + e + " for message: " + event);
> > +        }
> 
> If you won't be getting list of engines anymore, simply disconnect the gecko
> event listener here instead of on destroy?

This is an exceptional case--in fact, since we are providing the JSON ourselves, this should never happen.  I don't think it's worth adding code that provides an extremely minimal optimization for a case that should never happen in the first place.

> @@ +472,5 @@
> > +
> > +        @Override
> > +        public View getView(int position, View convertView, ViewGroup parent) {
> > +            final int resultCount = super.getCount();
> > +            if (position < resultCount)
> 
> Maybe factor out this code into a private method like
> "isSearchEnginePosition(position)" and reuse everywhere you need it?

I think it's overkill to factor out a single if statement condition (note that the resultCount is used elsewhere in the method, so we'd still need the super.getCount() here anyway).

> @@ +474,5 @@
> > +        public View getView(int position, View convertView, ViewGroup parent) {
> > +            final int resultCount = super.getCount();
> > +            if (position < resultCount)
> > +                return super.getView(position, convertView, parent);
> > +
> 
> I think on XUL Fennec we only show search engines when we should very few
> results for the search term. We can simply ignore search engines in case
> we're showing dozens of search results as they'll be buried at the end of
> the list anyway. I'd say, let's only show search engines if the search
> results only contain 2-4 items.

Rather than picking an arbitrary limit, why not just show them? As you said, they'll be buried in the list anyway.

> @@ +481,5 @@
> > +            bindSearchEngineView(index, v);
> > +            return v;
> > +        }
> > +
> > +        private void bindSearchEngineView(int position, View view) {
> 
> Given that the list of available search engines is more or less constant, I
> think it would be better to inflate once and cache the views for each engine
> for later usage. This way you don't need to inflate a new view all the time.

All of the hundreds of AwesomeBar results are dynamically inflated (from relatively slow SQL nonetheless), so adding a few more the same way shouldn't be a problem. There's a code complexity vs. performance tradeoff here, and caching the views, while not terribly complex, would still be more than a couple lines of extra code.

> @@ +738,5 @@
> > +    public void setSearchEngines(JSONArray engines) {
> > +        mSearchEngines = engines;
> > +        GeckoAppShell.getMainHandler().post(new Runnable() {
> > +            public void run() {
> > +                mAllPagesAdapter.notifyDataSetChanged();
> 
> Maybe simply wait until the user filter again? Won't this cause a sudden
> re-scroll on the list view when Gecko finally returns the list of engines?

No, the items are added to the list without altering the scroll position.

Outside of these things, I agree with your changes.
Comment 5 Lucas Rocha (:lucasr) 2011-12-05 02:17:26 PST
(In reply to Brian Nicholson (:bnicholson) from comment #4)
> > @@ +197,5 @@
> > > +            }
> > > +        } catch (Exception e) {
> > > +            // do nothing
> > > +            Log.i(LOGTAG, "handleMessage throws " + e + " for message: " + event);
> > > +        }
> > 
> > If you won't be getting list of engines anymore, simply disconnect the gecko
> > event listener here instead of on destroy?
> 
> This is an exceptional case--in fact, since we are providing the JSON
> ourselves, this should never happen.  I don't think it's worth adding code
> that provides an extremely minimal optimization for a case that should never
> happen in the first place.

I think I wasn't clear on my suggestion above. I was just suggesting to remove the event listener as soon as you get the list of engines as you won't need the listener anymore. How is this an optimization for a case that should never happen?
 
> > @@ +472,5 @@
> > > +
> > > +        @Override
> > > +        public View getView(int position, View convertView, ViewGroup parent) {
> > > +            final int resultCount = super.getCount();
> > > +            if (position < resultCount)
> > 
> > Maybe factor out this code into a private method like
> > "isSearchEnginePosition(position)" and reuse everywhere you need it?
> 
> I think it's overkill to factor out a single if statement condition (note
> that the resultCount is used elsewhere in the method, so we'd still need the
> super.getCount() here anyway).

Fair enough.

> > @@ +474,5 @@
> > > +        public View getView(int position, View convertView, ViewGroup parent) {
> > > +            final int resultCount = super.getCount();
> > > +            if (position < resultCount)
> > > +                return super.getView(position, convertView, parent);
> > > +
> > 
> > I think on XUL Fennec we only show search engines when we should very few
> > results for the search term. We can simply ignore search engines in case
> > we're showing dozens of search results as they'll be buried at the end of
> > the list anyway. I'd say, let's only show search engines if the search
> > results only contain 2-4 items.
> 
> Rather than picking an arbitrary limit, why not just show them? As you said,
> they'll be buried in the list anyway.

No big deal.

> > @@ +481,5 @@
> > > +            bindSearchEngineView(index, v);
> > > +            return v;
> > > +        }
> > > +
> > > +        private void bindSearchEngineView(int position, View view) {
> > 
> > Given that the list of available search engines is more or less constant, I
> > think it would be better to inflate once and cache the views for each engine
> > for later usage. This way you don't need to inflate a new view all the time.
> 
> All of the hundreds of AwesomeBar results are dynamically inflated (from
> relatively slow SQL nonetheless), so adding a few more the same way
> shouldn't be a problem. There's a code complexity vs. performance tradeoff
> here, and caching the views, while not terribly complex, would still be more
> than a couple lines of extra code.

This is entirely true. First of all, there aren't "hundreds" of results. We limit the results up to 100 exactly to avoid performance issues. Secondly, there's a reason Android's ListView has a mechanism to avoid unnecessarily inflating views for each item (the convertView argument I mentioned in my original review). Inflating is not that cheap and we should avoid doing it if we can.

I'm being picky with those micro-optimizations here for a simple reason: the AwesomeBar filtering *has to be* extremely fast. Anything we do to keep it as fast as possible is a worthy effort. If you're sure your implementation of extra engines items is not bringing any performance regression, I'm fine with it.

> > @@ +738,5 @@
> > > +    public void setSearchEngines(JSONArray engines) {
> > > +        mSearchEngines = engines;
> > > +        GeckoAppShell.getMainHandler().post(new Runnable() {
> > > +            public void run() {
> > > +                mAllPagesAdapter.notifyDataSetChanged();
> > 
> > Maybe simply wait until the user filter again? Won't this cause a sudden
> > re-scroll on the list view when Gecko finally returns the list of engines?
> 
> No, the items are added to the list without altering the scroll position.

Ok, cool.
Comment 6 Brian Nicholson (:bnicholson) 2011-12-05 12:42:25 PST
Created attachment 579136 [details] [diff] [review]
remove unnecessary argument from onUrlOpen
Comment 7 Brian Nicholson (:bnicholson) 2011-12-05 12:43:05 PST
Created attachment 579138 [details] [diff] [review]
search engines patch v2
Comment 8 Brian Nicholson (:bnicholson) 2011-12-05 12:43:54 PST
(In reply to Lucas Rocha (:lucasr) from comment #5)
> I think I wasn't clear on my suggestion above. I was just suggesting to
> remove the event listener as soon as you get the list of engines as you
> won't need the listener anymore. How is this an optimization for a case that
> should never happen?

Ah, I see. I interpreted your comment to mean to remove the listener in the catch block.  Removing it after receiving the message makes more sense.

I still didn't make this change, though, for two reasons. First, we'll still have to unregister the listener in onDestroy in case the message isn't received by the time we destroy the activity.  Secondly, the listeners are stored in a HashMap, which should be O(1) lookup regardless of the number of entries, so removing the listener earlier wouldn't improve performance anyway.  Let me know if you disagree.

> This is entirely true. First of all, there aren't "hundreds" of results. We
> limit the results up to 100 exactly to avoid performance issues. Secondly,
> there's a reason Android's ListView has a mechanism to avoid unnecessarily
> inflating views for each item (the convertView argument I mentioned in my
> original review). Inflating is not that cheap and we should avoid doing it
> if we can.
> 
> I'm being picky with those micro-optimizations here for a simple reason: the
> AwesomeBar filtering *has to be* extremely fast. Anything we do to keep it
> as fast as possible is a worthy effort. If you're sure your implementation
> of extra engines items is not bringing any performance regression, I'm fine
> with it.

You're correct about the inflations in my first patch. In response to your convertView comments, I changed the code there to use newView/convertView instead of creating another inflater, which (I think) should nullify this issue.
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-05 12:45:29 PST
(In reply to Lucas Rocha (:lucasr) from comment #5)
> (In reply to Brian Nicholson (:bnicholson) from comment #4)
> > > @@ +197,5 @@
> > > > +            }
> > > > +        } catch (Exception e) {
> > > > +            // do nothing
> > > > +            Log.i(LOGTAG, "handleMessage throws " + e + " for message: " + event);
> > > > +        }
> > > 
> > > If you won't be getting list of engines anymore, simply disconnect the gecko
> > > event listener here instead of on destroy?
> > 
> > This is an exceptional case--in fact, since we are providing the JSON
> > ourselves, this should never happen.  I don't think it's worth adding code
> > that provides an extremely minimal optimization for a case that should never
> > happen in the first place.
> 
> I think I wasn't clear on my suggestion above. I was just suggesting to
> remove the event listener as soon as you get the list of engines as you
> won't need the listener anymore. How is this an optimization for a case that
> should never happen?

One thing to keep in mind is that search engines can be added while Firefox is running, either as add-ons or as Open Search plugins. We might need to ping the UI to refresh the list.
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-05 12:46:17 PST
Comment on attachment 579136 [details] [diff] [review]
remove unnecessary argument from onUrlOpen

I assume you want this reviewed
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-05 12:48:35 PST
(In reply to Mark Finkle (:mfinkle) from comment #9)
> (In reply to Lucas Rocha (:lucasr) from comment #5)

> One thing to keep in mind is that search engines can be added while Firefox
> is running, either as add-ons or as Open Search plugins. We might need to
> ping the UI to refresh the list.

But I guess this isn't a big concern anyway, since we are only talking about the time the awesomebar activity is alive.
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-05 13:03:30 PST
Comment on attachment 579138 [details] [diff] [review]
search engines patch v2

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


>+    private class AwesomeBarCursorAdapter extends SimpleCursorAdapter {
>+
>+        private int mLayout;

nit: remove the blank line


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+  getSearchEngines: function() {

>+      };
>+    });
>+    sendMessageToJava({

Add a blank line before sendMessageToJava


>+  getSearchOrFixupURI: function(aData) {
>+    let args = JSON.parse(aData);
>+    let uri;
>+    if (args.engine) {
>+      let engine = Services.search.getEngineByName(args.engine);
>+      uri = engine.getSubmission(args.url).uri;
>+    } else
>+      uri = URIFixup.createFixupURI(args.url, Ci.nsIURIFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP);
>+    return uri ? uri.spec : args.url;
>+  },

It is possible that we might need to handle the postData, like we do here:
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/browser-ui.js#718

or here:
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/browser.js#642

But we need to add some code to allow the postData to be handled correctly. Let's do it in a followup patch.

>     } else if (aTopic == "Tab:Add") {
>-      let uri = URIFixup.createFixupURI(aData, Ci.nsIURIFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP);
>-      let newTab = this.addTab(uri ? uri.spec : aData);
>+      let newTab = this.addTab(this.getSearchOrFixupURI(aData));
>+      newTab.active = true;

| newTab.active = true | should not be needed anymore. We default to that for addTab

r+, but file the followup bug for postData handling.
Comment 13 Lucas Rocha (:lucasr) 2011-12-06 03:21:10 PST
Comment on attachment 579138 [details] [diff] [review]
search engines patch v2

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

Looks great.

::: mobile/android/base/AwesomeBar.java
@@ +248,5 @@
> +        Intent resultIntent = new Intent();
> +        resultIntent.putExtra(URL_KEY, url);
> +        resultIntent.putExtra(TYPE_KEY, mType);
> +        resultIntent.putExtra(SEARCH_KEY, engine);
> +        finishWithResult(resultIntent);

Awesome :-)
Comment 14 Brian Nicholson (:bnicholson) 2011-12-06 18:34:15 PST
Created attachment 579563 [details] [diff] [review]
search engines patch v3

fixed with mfinkle's changes
Comment 16 Aaron Train [:aaronmt] 2011-12-08 06:21:10 PST
Samsung Galaxy SII (Android 2.3.4)
20111208060054
http://hg.mozilla.org/integration/mozilla-inbound/rev/b0f8871174a5
Comment 17 Ed Morley [:emorley] 2011-12-08 08:40:34 PST
It's no problem for now, but for future landings on inbound, please leave the bug open until the changesets have merged to mozilla-central (https://wiki.mozilla.org/Tree_Rules/Inbound#Please_do_the_following_after_pushing_to_inbound). Thanks :-)

https://hg.mozilla.org/mozilla-central/rev/616da0ea7273
https://hg.mozilla.org/mozilla-central/rev/0d7b275a3d6e

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