Closed Bug 1205149 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: ally, Assigned: sergej, Mentored)

References

Details

(Whiteboard: [good next bug])

Attachments

(1 file, 2 obsolete files)

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.
Mentor: ally, michael.l.comella
Depends on: 1200319
Whiteboard: [good next bug]
Attached patch suggestions.diff (obsolete) — Splinter Review
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.
Attachment #8663116 - Flags: review?(michael.l.comella)
Assignee: nobody → sergej
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?
Attachment #8663116 - Flags: review?(michael.l.comella) → feedback+
Attached patch suggestions2.diff (obsolete) — Splinter Review
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
Attachment #8664182 - Flags: review?(michael.l.comella)
(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?
Flags: needinfo?(sergej)
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 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?
Attachment #8664182 - Flags: review?(michael.l.comella) → review+
Attachment #8663116 - Attachment is obsolete: true
Flags: needinfo?(sergej)
(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?
Flags: needinfo?(michael.l.comella)
Never mind, I've found or to add
Flags: needinfo?(michael.l.comella)
Attachment #8664182 - Attachment is obsolete: true
Attachment #8665226 - Flags: review?(michael.l.comella)
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
Attachment #8665226 - Flags: review?(michael.l.comella) → review+
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?
Flags: needinfo?(michael.l.comella)
It's unclear when & how those will be fixed so let's just land this.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(sergej)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/35e6dbad591a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Depends on: 1209951
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.