Closed Bug 1189719 Opened 5 years ago Closed 5 years ago

Recall and display search history within main browser UI

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: krudnitski, Assigned: ally)

References

(Depends on 1 open bug)

Details

Attachments

(6 files, 6 obsolete files)

 **Requirements:**

- Be able to store local search queries   

- Be able to surface local search queries when typing in the URL bar  

- Be able to surface synced search queries when typing in the URL bar (if connected to a Firefox Account) and likely with no discernible difference that a query was local or synced  

**User Stories:**

- As a user, I type similar search queries over and over and want to 'pick' often-used queries without having to re-type the query to make my search experience faster.  

  - AC: Display oft-used search queries that were previously typed by the user (local and/or synced)  

- As a user, in order to pick up a research task more seamlessly, I want to leverage a previously-typed search query from any instance of Firefox and be able to choose it when starting to type the same query in the URL bar (ie 'travel to Barbados' on desktop can be chosen after starting to type 'travel t...' on mobile)  

  - AC: Display previously-typed search queries generated from any Firefox instance connected to a FxA
Darrin, Anthony - I'm not sure if we ended up having any potential designs on how to integrate the 'search query' into the awesomebar experience. Is it possible to dust anything off and re-spin (or start from somewhere)?

I'd like to see if we can continuously improve the search experience on mobile, and I see this one not only as a 'reducing typing' option but also as a 'task continuity' use case.
Severity: enhancement → normal
Flags: needinfo?(dhenein)
OS: Other → Android
Priority: P5 → --
Hardware: Other → All
Note that there's a SearchHistoryProvider that already records this stuff.

Syncing search history into the right place is Bug 1102924.
Component: General → Awesomescreen
Depends on: 1102924
Summary: Recall and display search history → Recall and display search history within main browser UI
Version: Firefox 41 → Trunk
Yeah, we have designs from the Search Activity we could try to integrate into our Awesomescreen. I'll post some designs I'm also looking at as a part of bug 1188179
Depends on: 1190967
Attached image prev_awes_searchhistory1.png (obsolete) —
This seems to me like the best place to start. We have this asset from our work in Search Activity and this just takes the lesser used "pills" and replaces them with search history terms.
^note, I've also removed the "gradient" that our current "pills" have.
Assignee: nobody → ally
No longer depends on: 1102924
Ally, I'm updating this with a simple change to "bold" parts of the suggestion. I'll also attach a spec
Attachment #8648884 - Attachment is obsolete: true
Specs
Flags: needinfo?(ally)
Depends on: 1102924
No longer depends on: 1190967
thanks
Flags: needinfo?(ally)
let me know if you got questions! or a build! :D
If a user has not turned on search suggestions, or has explicitly clicked "No" to turn off search suggestions, do we still want to show them their previous queries? Will they understand the difference or think its kinda creepy?

I'm inclined to show the previous searches if a user has not made an explicit choice, but I think it's rather rude to show them if the user has indicated he/she does not want search suggestions shown.
Flags: needinfo?(alam)
Fair question, I see these "pills" as all the same thing. So, the "opt in" would apply to everything I think.

Kar, just running this by you.
Flags: needinfo?(alam) → needinfo?(krudnitski)
(In reply to Anthony Lam (:antlam) from comment #11)
> Fair question, I see these "pills" as all the same thing. So, the "opt in"
> would apply to everything I think.
> 
> Kar, just running this by you.

I guess I see them being very different, as one is the user's own data & previous behavior, the other is supplied by commercial parties. People can feel very differently about the two, ownership and comfortable with the former, not so much the latter.
I think I'm mostly with Ally on this one.

From what I've seen, users mostly dislike search suggestions because they feel creepy about sending what they type to a search engine in real time.

I imagine users not wanting to show their search history for different reasons: either because it's not useful to them, or because they're embarrassed (same as for not wanting to show top sites or history).

I can easily imagine users who want search history but no suggestions (probably a large subset of our privacy-conscious no-keystrokes-leaking users), and even some who want suggestions but no search history.


Although I hate checkboxes breeding, I think in this case we have two very different parts of the experience, and they need different treatment.

IMO that's opt-in for search suggestions, as we have now, and opt-out for search history. We should also support the same kinds of control over search history items as for history (delete individual, clear all, don't show these things at all).


A knock-on from this division is that I think we need to be careful to differentiate your own history from suggestions.

Your mocks are close, Anthony, but as a user I would interpret these suggestions-with-clocks as *the search engine's* saved searches — Google saves your searches, and this is how I'd expect to see them! That leads to unhappiness for people: why is Yahoo saving my searches? and why aren't my Google saved searches showing up?

Elsewhere, Firefox's own data is always shown as a scrollable list (even in the search activity). I'm inclined to think that we should do something closer to that here.
For the sake of moving this forward, I will treat them as being one unit subject to the search suggestion setting (eg dont show anything until the user clicks yes, respect the no to mean all suggestions).
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #12)
> I guess I see them being very different, as one is the user's own data &
> previous behavior, the other is supplied by commercial parties. People can
> feel very differently about the two, ownership and comfortable with the
> former, not so much the latter.

I agree. Technically speaking, they are different. 

But I'm not convinced our users will identify with them the way we do (at least initially). Let me mull this over some more.

Since this is our initial go at this, I don't want to change too much of our UI/UX and get blocked. I have alternate mocks for this but I want to take an iterative approach here as we gauge feedback and collect some telemetry on usage as well.

(In reply to Richard Newman [:rnewman] from comment #13)
> I think I'm mostly with Ally on this one.
> 
> From what I've seen, users mostly dislike search suggestions because they
> feel creepy about sending what they type to a search engine in real time.
> 
> I imagine users not wanting to show their search history for different
> reasons: either because it's not useful to them, or because they're
> embarrassed (same as for not wanting to show top sites or history).

I can see this. That's why we allow for quick and easy "dismissing" in Firefox Search too. 

> I can easily imagine users who want search history but no suggestions
> (probably a large subset of our privacy-conscious no-keystrokes-leaking
> users), and even some who want suggestions but no search history.

Me too. 

Maybe we should separate out the "opt-in" UX for both. We have one in bug 1195614, maybe we need something else for "history".

> Although I hate checkboxes breeding, I think in this case we have two very
> different parts of the experience, and they need different treatment.
> 
> IMO that's opt-in for search suggestions, as we have now, and opt-out for
> search history. We should also support the same kinds of control over search
> history items as for history (delete individual, clear all, don't show these
> things at all).

As above ^ I'll work on UX-ing something there. But not in this bug.

> A knock-on from this division is that I think we need to be careful to
> differentiate your own history from suggestions.
> 
> Your mocks are close, Anthony, but as a user I would interpret these
> suggestions-with-clocks as *the search engine's* saved searches — Google
> saves your searches, and this is how I'd expect to see them! That leads to
> unhappiness for people: why is Yahoo saving my searches? and why aren't my
> Google saved searches showing up?
> 
> Elsewhere, Firefox's own data is always shown as a scrollable list (even in
> the search activity). I'm inclined to think that we should do something
> closer to that here.

Hm, perhaps. I can definitely see your point about visually distinguishing between engine specific saved searches and firefox saved searches but I'm not sure that's a pre-req for this bug. I'd also challenge this notion and question how much "distinction" needs to be made and what value it provides.

In the mean time, I think we should still try to incorporate and clean up the UI in the scope of this bug. And I'll work to improve the overall UX of the things we mentioned above. 

Ally - yes, it'd be great to proceed and get a build to test this out :)
Attached image device-2015-08-26-165405.png (obsolete) —
It's a wip, and has plenty of bugs, but the screenshot should warm some hearts.
Flags: needinfo?(alam)
Awesome! 

:) keep em comin'!
Flags: needinfo?(alam)
does not actually depend on search history syncing.
No longer depends on: 1102924
Depends on: 1199335
Blocks: 1199335
No longer depends on: 1199335
Attached image device-2015-08-27-104856.png (obsolete) —
Ahh, now with filtering! 

The clock icon in our code base is really light compared to the mocks (we only have one), so I'm going to tint it.
Attachment #8653207 - Attachment is obsolete: true
Attached image device-2015-08-27-111031.png (obsolete) —
Awww I can see the icon without my glasses. What do you think, bearing in mind that we have a polish bug for the search engine row itself?
Attachment #8653622 - Attachment is obsolete: true
Flags: needinfo?(alam)
awesome! yes tint it to our tabs tray icon grey :)
Flags: needinfo?(alam)
Attached patch showPreviousSearchTerms (obsolete) — Splinter Review
Bear in mind that 
- the ui changes to the search row are now in bug 1199335
- whilst we resolve this, the saved searches are shown if and only if the search engine suggestions are. Since that discussion is still on going, changing that will probably be a follow up bug if we decide that's what we want to do.

and while I'm at it, lets have a look for test bustages. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa937bcf973c
Attachment #8653702 - Flags: review?(michael.l.comella)
Comment on attachment 8653702 [details] [diff] [review]
showPreviousSearchTerms

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

Pressing and long-pressing the search results doesn't properly search.

Should we show search history if search suggestions are disabled/unset? In the literal sense, yes we should, but perhaps we should double-check with Anthony.

It doesn't appear that we limit the number of results from the DB for the search history – perhaps we should ask Anthony what a reasonable limit would be?

::: mobile/android/base/home/SearchEngineRow.java
@@ +142,5 @@
> +            suggestionText.setText(suggestion);
> +            setDescriptionOnSuggestion(suggestionText, suggestion);
> +
> +        } else {
> +            final TextView suggestionText = (TextView) v.findViewById(R.id.suggestion_text);

I know this is pre-existing so maybe followup but findViewById can be time consuming – it'd be good to store these values somehow.

@@ +181,5 @@
>      public void setOnEditSuggestionListener(OnEditSuggestionListener listener) {
>          mEditSuggestionListener = listener;
>      }
>  
> +    private void applyToView(String suggestion, boolean animate, int recycledSuggestionCount, Integer suggestionCounter, boolean isUserSavedSearch){

This name is confusing – what is being applied to the view?

Perhaps bindSuggestionView?

@@ +211,5 @@
> +        savedIcon.setVisibility(isUserSavedSearch ? View.VISIBLE: View.GONE);
> +        setSuggestionOnView(suggestionItem, suggestion, isUserSavedSearch);
> +
> +
> +        if (animate) {

nit: two lines of ws above

@@ +218,5 @@
> +            anim.setStartOffset(suggestionCounter * ANIMATION_DURATION);
> +            suggestionItem.startAnimation(anim);
> +        }
> +
> +    }

nit: rm ws above

@@ +222,5 @@
> +    }
> +
> +    private void updateFromSavedSearches(String searchTerm, boolean animate, int suggestionCounter, int recycledSuggestionCount) {
> +        ContentResolver cr = getContext().getContentResolver();
> +        final Cursor c = cr.query(SearchHistory.CONTENT_URI, null, null, null, null);

Don't forget to close the Cursor. Convention is [1] but Java 7 introduces a `try` statement [2] if you want to be fancy.

[1]: https://wiki.mozilla.org/Mobile/Fennec/Android#Closing_resources
[2]: https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

@@ +225,5 @@
> +        ContentResolver cr = getContext().getContentResolver();
> +        final Cursor c = cr.query(SearchHistory.CONTENT_URI, null, null, null, null);
> +        if (c != null && c.moveToFirst()) {
> +            do {
> +                final String savedSearch = c.getString(c.getColumnIndex("query"));

Use SearchHistory.QUERY

@@ +226,5 @@
> +        final Cursor c = cr.query(SearchHistory.CONTENT_URI, null, null, null, null);
> +        if (c != null && c.moveToFirst()) {
> +            do {
> +                final String savedSearch = c.getString(c.getColumnIndex("query"));
> +                if (savedSearch.startsWith(searchTerm)) {

If I'm not mistaken, you should be able to do this String matching in SQL which is faster in theory. You can pass in the SQL as args to cr.query.

@@ +234,5 @@
> +            } while (c.moveToNext());
> +        }
> +
> +        // Hide extra suggestions that have been recycled.
> +        for (int i = suggestionCounter + 1; i < recycledSuggestionCount; ++i) {

nit: I'd use a helper method here because the code is reused.

@@ +239,5 @@
> +            mSuggestionView.getChildAt(i).setVisibility(View.GONE);
> +        }
> +
> +    }
> +    private int updateFromSearchEngine(SearchEngine searchEngine, boolean animate, int recycledSuggestionCount) {

nit: ws above this line

@@ +241,5 @@
> +
> +    }
> +    private int updateFromSearchEngine(SearchEngine searchEngine, boolean animate, int recycledSuggestionCount) {
> +
> +        int suggestionCounter = 0;

afaict, this didn't need to move.

::: mobile/android/base/resources/layout/suggestion_item.xml
@@ +25,5 @@
>                android:textSize="14sp"
>                android:gravity="center_vertical"
>                android:layout_gravity="center_vertical"/>
>  
> +    <ImageView android:id="@+id/saved_search_icon"

I'd prefer to reuse the existing image and text views here, if possible, for perf reasons.

I see that the layout_width/height differ from the suggestion_magnifier – maybe the size of magnifier should be matched to the same size (check with antlam).

As we discussed irl, can you also file a followup for using compound drawables? Would be a good mentor bug.
Attachment #8653702 - Flags: review?(michael.l.comella) → feedback+
Sorry, missed this.

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #22)
> - the ui changes to the search row are now in bug 1199335

These being the specific style changes, I presume?

> - whilst we resolve this, the saved searches are shown if and only if the
> search engine suggestions are. Since that discussion is still on going,
> changing that will probably be a follow up bug if we decide that's what we
> want to do.

Fair enough! :)
Anthony, is there a desired max # of user's previous search history matches you'd like me to enforce? They are exact matches on the start of the text. 

I am hesitant to put one in because we'd could exclude something the user is looking for it and thinks we're broken.
Flags: needinfo?(alam)
Comment on attachment 8653702 [details] [diff] [review]
showPreviousSearchTerms

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

Some DB drive-by comments.

::: mobile/android/base/home/SearchEngineRow.java
@@ +144,5 @@
> +
> +        } else {
> +            final TextView suggestionText = (TextView) v.findViewById(R.id.suggestion_text);
> +            suggestionText.setText(suggestion);
> +            setDescriptionOnSuggestion(suggestionText, suggestion);

These two blocks are the same but for the ID. Smush.

@@ +222,5 @@
> +    }
> +
> +    private void updateFromSavedSearches(String searchTerm, boolean animate, int suggestionCounter, int recycledSuggestionCount) {
> +        ContentResolver cr = getContext().getContentResolver();
> +        final Cursor c = cr.query(SearchHistory.CONTENT_URI, null, null, null, null);

1. Specify the columns you want.

2. Make sure that the search history provider is running this query as DISTINCT.

3. As Michael notes, close the cursor. Something like:

  final ContentResolver cr = ...
  final Cursor c = ...
  if (c == null) {
      return;
  }
  try {
      if (!c.moveToFirst()) {
          return;
      }
      ...
  } finally {
      c.close();
  }

@@ +226,5 @@
> +        final Cursor c = cr.query(SearchHistory.CONTENT_URI, null, null, null, null);
> +        if (c != null && c.moveToFirst()) {
> +            do {
> +                final String savedSearch = c.getString(c.getColumnIndex("query"));
> +                if (savedSearch.startsWith(searchTerm)) {

Yes, this should definitely run against the DB, particularly if you're applying a limit (which is also a good idea). "terms LIKE '%?'", { searchTerm } is likely the approach to take.
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #25)
> Anthony, is there a desired max # of user's previous search history matches
> you'd like me to enforce? They are exact matches on the start of the text. 

Right now we do 4 pills max. So (looking at telemetry) I think a good place to start would just be to do 2 of each. That would replace the final 2 "suggestions" and doesn't change the total count of 4 so hopefully doesn't cause too many UI issues.

For tablets, since we have more room, perhaps we don't have to limit this number and just add as many as the user has to the 4 suggestions.

> I am hesitant to put one in because we'd could exclude something the user is
> looking for it and thinks we're broken.

Yeah, I have that concern too but the user can also just keep typing since they're already in that position. I'm also aware that our current UI isn't built to handle a lot of these "pills" so I'd like to table that discussion for now and possibly take a step back altogether (after v1).
Flags: needinfo?(alam) → needinfo?(ally)
(In reply to Anthony Lam (:antlam) from comment #27)
> (In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #25)
> > Anthony, is there a desired max # of user's previous search history matches
> > you'd like me to enforce? They are exact matches on the start of the text. 
> 
> Right now we do 4 pills max. So (looking at telemetry) I think a good place
> to start would just be to do 2 of each. That would replace the final 2
> "suggestions" and doesn't change the total count of 4 so hopefully doesn't
> cause too many UI issues.
> 
> For tablets, since we have more room, perhaps we don't have to limit this
> number and just add as many as the user has to the 4 suggestions.
> 
> > I am hesitant to put one in because we'd could exclude something the user is
> > looking for it and thinks we're broken.
> 
> Yeah, I have that concern too but the user can also just keep typing since
> they're already in that position. I'm also aware that our current UI isn't
> built to handle a lot of these "pills" so I'd like to table that discussion
> for now and possibly take a step back altogether (after v1).

Well not so fast. Now you need to decide how you'd like to eliminate other matches in saved searches. Do you want the two oldest? The two youngest?

The search engine suggestions arrive in ranked order, so presumably we'll take #1 and #2 for those. 

Since we are cutting suggestions from the search engine below the hardcoded minimum , we may need to do some checking to make sure we aren't accidentally breaking any promises to partners.
Flags: needinfo?(ally)
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #28)
> (In reply to Anthony Lam (:antlam) from comment #27)
> > (In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #25)
> > > Anthony, is there a desired max # of user's previous search history matches
> > > you'd like me to enforce? They are exact matches on the start of the text. 
> > 
> > Right now we do 4 pills max. So (looking at telemetry) I think a good place
> > to start would just be to do 2 of each. That would replace the final 2
> > "suggestions" and doesn't change the total count of 4 so hopefully doesn't
> > cause too many UI issues.
> > 
> > For tablets, since we have more room, perhaps we don't have to limit this
> > number and just add as many as the user has to the 4 suggestions.
> > 
> > > I am hesitant to put one in because we'd could exclude something the user is
> > > looking for it and thinks we're broken.
> > 
> > Yeah, I have that concern too but the user can also just keep typing since
> > they're already in that position. I'm also aware that our current UI isn't
> > built to handle a lot of these "pills" so I'd like to table that discussion
> > for now and possibly take a step back altogether (after v1).
> 
> Well not so fast. Now you need to decide how you'd like to eliminate other
> matches in saved searches. Do you want the two oldest? The two youngest?
> 
> The search engine suggestions arrive in ranked order, so presumably we'll
> take #1 and #2 for those. 
> 
> Since we are cutting suggestions from the search engine below the hardcoded
> minimum , we may need to do some checking to make sure we aren't
> accidentally breaking any promises to partners.

we (mcomella &I) there are enough open questions & implications with this to make it into a followup bug.

For this one, we'll go with max of 4, the 4 newest if there more than 4 saved searched. Suggestions from the engine will be left alone for now.
Blocks: 1200319
Blocks: 1200363
Blocks: 1200367
Blocks: 1200371
Blocks: 1200374
This bug grew larger by the comment, and mcomella suggested we triage it.

The results of triage are below. Please remember that we are shipping and iterating. :)

Triage of 1189719 (from Friday)
- Bug 1199335 - Polish the search engine row object UI 

- Bug 1200367 - Should saved/previous searches have their own pref or share the search suggestions pref?
-- NIs for kar & dhenein transferred to this bug. We still need your input!

- Bug 1200374 - Long taps on saved searches do not always fire properly.
- Bug 1200319 - How many, which ones of which type of pills should be displayed on phones or tablets for search suggestions
- Bug 1200363 - Rejigger SearchEngineRow.java to re-use ImageViews
- Bug 1200371 - ContentResolver.query()'s wildcard % not performing as expected

mcomella & I really look forward to rnewman's input on Bug 1200371.

- Test Coverage: suspicious test failures from try run 
- mobile/android/tests/browser/robocop/testAddSearchEngine.java 
			7105 1 INFO Passed: 21
			7106 2 INFO Failed: 0
			7107 3 INFO Todo: 0
- /mobile/android/tests/browser/robocop/testSessionOOMSave.java : fails on trunk
- /mobile/android/tests/browser/robocop/testDistribution.java 
	                7117 1 INFO Passed: 114
			7118 2 INFO Failed: 0
			7119 3 INFO Todo: 0
Flags: needinfo?(krudnitski)
Flags: needinfo?(dhenein)
Attached image pill max screenshot
Attachment #8653624 - Attachment is obsolete: true
Comment 31 contains the list of broken out bugs which are not in this patch, and the results of the possible test failures.

It should look pretty familiar.
Attachment #8653702 - Attachment is obsolete: true
Attachment #8655103 - Attachment is obsolete: true
Attachment #8655129 - Flags: review?(michael.l.comella)
Comment on attachment 8655129 [details] [diff] [review]
showPreviousSearchTerms

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

r+ w/ mostly style nits. :)

Don't forget to update your patch comment.

::: mobile/android/base/home/BrowserSearch.java
@@ +909,5 @@
>                  row.setOnEditSuggestionListener(mEditSuggestionListener);
>                  row.setSearchTerm(mSearchTerm);
>                  final SearchEngine engine = mSearchEngines.get(position);
>                  final boolean animate = (mAnimateSuggestions && engine.hasSuggestions());
> +                // bindView can be triggered before the opt-in prompt is shown or set

I had difficulty understanding the context here so I'd like a little elaboration: "bindView can be triggered before the opt-in prompt is shown or set, displaying search history results. We short circuit to prevent this."

I also think the comment makes more sense next to the if statement inside `updateSuggestions` because that's where the short-circuit actually happens.

::: mobile/android/base/home/SearchEngineRow.java
@@ +2,5 @@
>   * 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.home;
> +import org.mozilla.gecko.db.BrowserContract.SearchHistory;

nit: ws above this line.

@@ +13,5 @@
>  import org.mozilla.gecko.util.StringUtils;
>  import org.mozilla.gecko.widget.AnimatedHeightLayout;
>  import org.mozilla.gecko.widget.FaviconView;
>  import org.mozilla.gecko.widget.FlowLayout;
> +import android.database.Cursor;

nit: ws above this line - afaki conventionally imports are separated by 1st party/system/third party in Java, though (perhaps unintentionally) we're actually more aggressive and separate by entity (e.g. org.mozilla.gecko, android, org.json).

@@ +133,5 @@
>      private String getSuggestionTextFromView(View v) {
>          final TextView suggestionText = (TextView) v.findViewById(R.id.suggestion_text);
>          return suggestionText.getText().toString();
>      }
> +    private void setSuggestionOnView(View v, String suggestion, boolean isUserSavedSearch) {

nit: ws above this line

@@ +134,5 @@
>          final TextView suggestionText = (TextView) v.findViewById(R.id.suggestion_text);
>          return suggestionText.getText().toString();
>      }
> +    private void setSuggestionOnView(View v, String suggestion, boolean isUserSavedSearch) {
> +        final TextView suggestionText;

super-nit: You could remove the findViewById duplication by using:
  `final int suggestionTextId`

instead. Additionally, you could use the ternary operator for added brevity.

@@ +178,5 @@
>      }
>      public void setOnEditSuggestionListener(OnEditSuggestionListener listener) {
>          mEditSuggestionListener = listener;
>      }
> +    private void bindSuggestionView(String suggestion, boolean animate, int recycledSuggestionCount, Integer suggestionCounter, boolean isUserSavedSearch){

nit: ws above this line

@@ +183,5 @@
> +        final View suggestionItem;
> +
> +        // Reuse suggestion views from recycled view, if possible.
> +        if (suggestionCounter + 1 < recycledSuggestionCount) {
> +            suggestionItem = mSuggestionView.getChildAt(suggestionCounter + 1);

I think suggestionCounter can be better named in this context - previousSuggestionChildIndex?

@@ +216,5 @@
> +    }
> +
> +    private void hideRecycledSuggestions(int suggestionCounter, int recycledSuggestionCount) {
> +        // Hide extra suggestions that have been recycled.
> +        for (int i = suggestionCounter + 1; i < recycledSuggestionCount; ++i) {

Again, a better name for suggestionCounter in this context – lastVisibleChildIndex?

@@ +222,5 @@
> +        }
> +    }
> +
> +    private void updateFromSavedSearches(String searchTerm, boolean animate, int suggestionCounter, int recycledSuggestionCount) {
> +        ContentResolver cr = getContext().getContentResolver();

nit: `final`s in this function

@@ +235,5 @@
> +        try {
> +            if (c.moveToFirst()) {
> +                int counter = 0;
> +                do {
> +                    final String savedSearch = c.getString(c.getColumnIndex(SearchHistory.QUERY));

nit: -> c.getColumnIndexOrThrow will be more clear.

Also, afaik, this shouldn't change for each iteration so I'd get it for the first index only and re-use it in the loop.

@@ +243,5 @@
> +                    // Bug 1200371 will move the filtering/matching and limit into the sql query
> +                    if (savedSearch.startsWith(searchTerm)) {
> +                        bindSuggestionView(savedSearch, animate, recycledSuggestionCount, suggestionCounter, true);
> +                        ++suggestionCounter;
> +                        ++counter;

Two counters in a loop is probably a code smell but we'll overwrite one when we do it in SQL so wfm.

@@ +268,3 @@
>              ++suggestionCounter;
>          }
> +        hideRecycledSuggestions(suggestionCounter, recycledSuggestionCount);

Any reason not to do this in updateSuggestions? If both updateFromSearchEngine and updateFromSavedSearches ran independently, I can see the reason for this but the latter is dependent on the former because of the suggestionCounter.

@@ +276,5 @@
> +        return suggestionCounter;
> +    }
> +
> +    public void updateSuggestions(boolean suggestionsEnabled, SearchEngine searchEngine, String searchTerm, boolean animate) {
> +

nit: We don't skip lines after function headers (and if we did, this is inconsistent with this file) - rm the ws

@@ +279,5 @@
> +    public void updateSuggestions(boolean suggestionsEnabled, SearchEngine searchEngine, String searchTerm, boolean animate) {
> +
> +        if (suggestionsEnabled) {
> +            final int recycledSuggestionCount = mSuggestionView.getChildCount();
> +            int suggestionCounter = updateFromSearchEngine(searchEngine, animate, recycledSuggestionCount);

nit: `final int`
nit: `suggestionCounter` is a little unclear - `suggestionViewCount` or similar might be better

@@ +286,1 @@
>      }

nit: not you, but ws below this line

::: mobile/android/base/resources/layout/suggestion_item.xml
@@ +24,5 @@
>                android:textColor="@color/placeholder_active_grey"
>                android:textSize="14sp"
>                android:gravity="center_vertical"
>                android:layout_gravity="center_vertical"/>
> +    <ImageView android:id="@+id/saved_search_icon"

nit: ws above this line

@@ +30,5 @@
> +               android:src="@drawable/icon_most_recent_empty"
> +               android:tint="@color/tabs_tray_icon_grey"
> +               android:layout_marginRight="3dip"
> +               android:layout_width="18dip"
> +               android:layout_height="18dip"/>

nit: To be able to quickly skim the xml, I prefer the ordering of these xml attributes to be something like:
  android:id
  style
  (attributes affecting layout, e.g. layout_height, *gravity)
  (everything else - I have some preferences here but it's not so important)
Attachment #8655129 - Flags: review?(michael.l.comella) → review+
I have noticed that on some devices the search term does not make it to search query submitted to yahoo. Will be holding this to figure out that.
>Any reason not to do this in updateSuggestions? If both updateFromSearchEngine and updateFromSavedSearches ran independently, I can see the reason for this but the latter is dependent on the former because of the suggestionCounter.

Things don't hide that should. :)
the query is now always attached properly to the pill so that problem's fixed. That's the big thing.

All the new lines are where they are supposed to be (most predate my patch). I don't know why splinter isn't showing them correctly.

I did not re-order the existing xml to keep the changes minimal there now that I'm not adding entries. 

see above comment for why I didn't move hideRecycledViews() 

Anthony wanted to get rid of the magnifying glass, anyway, so I did that on my way to reusing the view for both types of pills.
Attachment #8655641 - Flags: review?(michael.l.comella)
Comment on attachment 8655641 [details] [diff] [review]
showPreviousSearchTerms

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

::: mobile/android/base/home/SearchEngineRow.java
@@ +123,5 @@
>          mUserEnteredView = (LinearLayout) findViewById(R.id.suggestion_user_entered);
>          mUserEnteredView.setOnClickListener(mClickListener);
>  
> +        final ImageView historyIcon = (ImageView) mUserEnteredView.findViewById(R.id.suggestion_item_icon);
> +        historyIcon.setVisibility(View.GONE);

You should declare this in XML.

@@ +140,5 @@
>      }
>  
> +    private void setSuggestionOnView(View v, String suggestion, boolean isUserSavedSearch) {
> +        final ImageView historyIcon = (ImageView) v.findViewById(R.id.suggestion_item_icon);
> +        historyIcon.setVisibility(isUserSavedSearch? View.VISIBLE: View.GONE);

nit: var ? View...

@@ +220,5 @@
> +    private void updateFromSavedSearches(String searchTerm, boolean animate, int suggestionCounter, int recycledSuggestionCount) {
> +        final ContentResolver cr = getContext().getContentResolver();
> +
> +        String[] columns = new String[] { SearchHistory.QUERY };
> +        String sortOrderAndLimit = SearchHistory.DATE +" DESC";

nit: .DATE + " ...

@@ +277,5 @@
> +        return suggestionCounter;
> +    }
> +
> +    public void updateSuggestions(boolean suggestionsEnabled, SearchEngine searchEngine, String searchTerm, boolean animate) {
> +    // bindView(calling function) can be triggered before the opt-in prompt is shown or set, displaying search history results

I think there's still a disconnect with this comment and the motivation for the statement below it.

Maybe "...displaying search history results which we don't want so only show the suggestions if enabled". Though in retrospect this is slightly redundant and maybe I should look more closely at the statement below and assume they're connected.

Do what you think is right.

nit: indent at the same level as `if`
Attachment #8655641 - Flags: review?(michael.l.comella) → review+
https://hg.mozilla.org/integration/fx-team/rev/5183c511ce583ee0ea00a6e057b2fa092a871290
Bug 1189719 - Recall and display search history within main browser UI.r=mcomella
https://hg.mozilla.org/mozilla-central/rev/5183c511ce58
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Tested using:
Device: Moto X (Android 4.4)
Build: Firefox for Android  43.0a1 (2015-09-06)

The clock icon is too light. I can barely see it. 
Is the color expected?
that should be addressed when ally lands bug 1199335 :)
Flags: needinfo?(alam)
We usually specify follow-up bugs to block the original bug, since technically they are things that are part of the original issue, but that just didn't get fixed in the original ticket. This also make bug dependency trees easier to follow :)
Depends on: 1203053
Depends on: 1215475
Did this already land in 43? It's still in Aha/44 and not in the rel notes for 44 beta nor 43 either.
Flags: needinfo?(margaret.leibovic)
(In reply to Barbara Bermes [:barbara] from comment #45)
> Did this already land in 43? It's still in Aha/44 and not in the rel notes
> for 44 beta nor 43 either.

I do not see this feature in Fx43.
Flags: needinfo?(margaret.leibovic)
(In reply to Mark Finkle (:mfinkle) from comment #46)
> (In reply to Barbara Bermes [:barbara] from comment #45)
> > Did this already land in 43? It's still in Aha/44 and not in the rel notes
> > for 44 beta nor 43 either.
> 
> I do not see this feature in Fx43.

For completeness, we landed this code in Fx43, but it was behind a NIGHTLY_BUILD flag. We polished the feature and let it ride the trains in Fx44.
Ok, so I will add it to the rel notes then for 44.
QA Contact: mihai.ninu
You need to log in before you can comment on or make changes to this bug.