1158275 - Remove old method of selecting non-primary search engines in BrowserSearch

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Follow-up to bug 1137483.

i.e. the list should only include the primary search engine, its suggestions, and awesomescreen results.

This should also remove the primary search engine from the quick search bar.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1158275_searchengines.patch

This patch will only add "getSuggestEngineCount()" search engines to the list (So theoretically there can be more than one). This patch does not yet remove the primary search engine(s) from the new SearchBar. I'll attach a second patch for that.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: search-empty-state.png

The initial state of the search view looks kinda empty now (see attached screenshot) - with suggestions not (yet) enabled and empty history.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1158275_searchengines_v2.patch

Updated patch that also handles the search bar.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd21a5dcd4ac

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Seems like testAddSearchEngine will need some love now.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8613829 [details] [diff] [review]
1158275_searchengines_v2.patch

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

Nice cleanup of ROW_SEARCH!

WFM!

::: mobile/android/base/home/BrowserSearch.java
@@ +348,5 @@
> +        mSearchEngineBar.setSearchEngines(
> +                mSearchEngines.size() > getSuggestEngineCount()
> +                ? mSearchEngines.subList(getSuggestEngineCount(), mSearchEngines.size())
> +                : new ArrayList<SearchEngine>()
> +        );

nit: I haven't seen the trailing paren on a newline by itself style before in our code, but it wfm.

@@ +587,5 @@
>              if (mAdapter != null) {
>                  mAdapter.notifyDataSetChanged();
>              }
>  
> +            mSearchEngineBar.setSearchEngines(

nit: Perhaps this should be a helper function.

@@ +881,5 @@
>              final int resultCount = super.getCount();
>  
>              // Don't show search engines or suggestions if search field is empty
>              if (TextUtils.isEmpty(mSearchTerm)) {
>                  return resultCount;

Some of the code in this file seems to be legacy - we never show the browser search screen if there is no search term entered. I wonder if we can't greatly simplify this file by removing these other cases (e.g. getSuggestEngineCount() returns a constant because we have that hard-coded atm) - file a followup?

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1158275_searchengines_v3.patch

This patch fixes a bug where selecting a search result with suggestions off might select the wrong row. In addition to that getSuggestEngineCount() is now gone and all the mSearchEngines size checking is now hidden behind getPrimaryEngineCount(). To avoid further confusion with patches that depend on each other in weird ways, this patch now includes the changes for bug 1158280 as well.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff07cb2652eb

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1158275_testAddSearchEngine.patch

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d42a927871b

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8614381 [details] [diff] [review]
1158275_searchengines_v3.patch

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

::: mobile/android/base/home/BrowserSearch.java
@@ +585,5 @@
>              if (mAdapter != null) {
>                  mAdapter.notifyDataSetChanged();
>              }
>  
> +            mSearchEngineBar.setSearchEngines(

nit: maybe this should be a helper function.

(But I see this was done in https://bugzilla.mozilla.org/show_bug.cgi?id=1170824#c1)

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8615555 [details] [diff] [review]
1158275_testAddSearchEngine.patch

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

::: mobile/android/tests/browser/robocop/testAddSearchEngine.java
@@ +145,5 @@
>          mActions.sendKeys(SEARCH_TEXT);
>          boolean correctNumSearchEnginesDisplayed = waitForCondition(new Condition() {
>              @Override
>              public boolean isSatisfied() {
> +                ListView searchResultList = findListViewWithTag(HomePager.LIST_TAG_BROWSER_SEARCH);

Nice rename. :) I like cleanup.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Nice job finding a low entropy way to make all the old search engine methodology go away!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8e5abcb55c35
https://hg.mozilla.org/integration/fx-team/rev/d7ed7208e7f4

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8e5abcb55c35
https://hg.mozilla.org/mozilla-central/rev/d7ed7208e7f4

Comment 16

[Flaviu Cos]

Verified as fixed in build 41.0a1 2015-06-15;
Device: Samsung Galaxy Tab 10.1 P7510 (Android 4.0.4).