1205149 - Update SearchEngineRow.java to use resource values.xml over private constants

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Allison Naaktgeboren :ally]

In Bug 1200319 we now have different behavior in the saved search & search engine pills based on form factor (tablet/not tablet). 

It would be most excellent if we stored the pill max value in values.xml and used android's resourcing system to make this code nicer and remove the isTablet checks.

The idea is that the behavior from Bug 1200319 would remain the same, making testing easier.

We think this would make an excellent good-next-bug for our recent bumper crop of new contributors.

Comment 1

[Sergej Kravcenko]

Attachment: suggestions.diff

Tablet check is still needed because empty search history is replaced by suggestions only on phones. There is possibility to add this flag to values.xml, but I think it's not a perfect solution.

Comment 2

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

Comment on attachment 8663116 [details] [diff] [review]
suggestions.diff

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

::: mobile/android/base/home/SearchEngineRow.java
@@ +240,5 @@
>          }
>          hideRecycledSuggestions(suggestionCounter, recycledSuggestionCount);
>      }
>  
> +    private Cursor getSavedSearches(String searchTerm, int limit) {

nit: limit -> suggestionLimit

@@ +256,4 @@
>          return cr.query(SearchHistory.CONTENT_URI, columns, actualQuery, queryArgs, sortOrderAndLimit);
>      }
>  
> +    private int updateFromSearchEngine(boolean animate, int recycledSuggestionCount, int savedCount, int limit) {

nit: limit -> searchEngineSuggestionLimit
nit: savedCount -> savedSuggestionCount

@@ +262,3 @@
>              // If there are less than max saved searches on phones, fill the space with more search engine suggestions
> +            if (!HardwareUtils.isTablet() && savedCount < limit) {
> +                limit += limit - savedCount;

I know you didn't write the style of this expression but I find it confusing. I think the confusion comes from the fact that the limit is for a single type of suggestion, not the limit of suggestions. Perhaps we could use another variable to clarify? e.g.

...updateFromSearchEngine(..., int savedSuggestionCount, int searchEngineSuggestionLimit) {
  ...
  final unusedSavedSuggestionViewCount = searchEngineSuggestionLimit - savedSuggestionCount;
  searchEngineSuggestionLimit += unusedSavedSuggestionViewCount;

::: mobile/android/base/resources/values-xlarge-v11/integers.xml
@@ +8,5 @@
>      <integer name="number_of_top_sites">9</integer>
>      <integer name="number_of_top_sites_cols">3</integer>
>      <integer name="panel_icon_grid_view_columns">6</integer>
>      <integer name="number_of_inline_share_devices">4</integer>
> +    <integer name="number_of_search_suggestions">4</integer>

This shouldn't be necessary – the resource system will lean on values-large-v11

::: mobile/android/base/resources/values/integers.xml
@@ +9,5 @@
>      <integer name="number_of_top_sites_cols">2</integer>
>      <integer name="max_icon_grid_columns">4</integer>
>      <integer name="panel_icon_grid_view_columns">3</integer>
>      <integer name="number_of_inline_share_devices">2</integer>
> +    <integer name="number_of_search_suggestions">2</integer>

This could be better named – this is the number of search suggestions for a single type of suggestion, right? Can you change the name to represent that?

Comment 3

[Sergej Kravcenko]

Attachment: suggestions2.diff

Added some optimisation, preload limits from resources.

There are still 2 problems, not sure if they must be fixed under this bug.
1. There is search suggestion limit in BrowserSearch.java line 628, it's hardcoded to 3. This leads to wrong number of search suggestions, 3 instead of 4 on tablets and when saved suggestions are empty on phones.

2. This one is more severe. Saved suggestions function does DB query on UI thread, this leads to StrictModeDiskReadViolation

Comment 4

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

(In reply to Sergej Kravcenko from comment #3)
> There are still 2 problems, not sure if they must be fixed under this bug.

They shouldn't be fixed under this bug – it's easier to abstract away issues & fixes that way. Can you file two separate bugs for these and CC me?

Comment 5

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

Also, don't forget to obsolete your old patch! You can do this by clicking "Details" next to it, clicking "edit details" near the top, checking "obsolete", and hitting "Submit"!

Comment 6

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

Comment on attachment 8664182 [details] [diff] [review]
suggestions2.diff

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

Looks good but doesn't apply locally – can you rebase this patch, Sergej?

Comment 7

[Sergej Kravcenko]

(In reply to Michael Comella (:mcomella) from comment #4)
> (In reply to Sergej Kravcenko from comment #3)
> > There are still 2 problems, not sure if they must be fixed under this bug.
> 
> They shouldn't be fixed under this bug – it's easier to abstract away issues
> & fixes that way. Can you file two separate bugs for these and CC me?

Do I need permission for that?

Comment 8

[Sergej Kravcenko]

Never mind, I've found or to add

Comment 9

[Sergej Kravcenko]

Attachment: suggestions_rebase

Comment 10

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

Comment on attachment 8665226 [details] [diff] [review]
suggestions_rebase

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

Applies & builds locally so carrying my r+ forward.

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

Comment 11

[Sergej Kravcenko]

This patch will be replaced by Bug 1207872 and possibly Bug 1207876, so I don't think it should be commited. What do you think?

Comment 12

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

It's unclear when & how those will be fixed so let's just land this.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/35e6dbad591a

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/35e6dbad591a