Closed Bug 1040994 Opened 7 years ago Closed 7 years ago

Add LIMIT support for search history content provider

Categories

(Firefox for Android Graveyard :: Search Activity, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: eedens, Assigned: eedens)

References

Details

Attachments

(1 file, 2 obsolete files)

The search activity uses a CursorLoader to query the search history content provider. To support LIMIT, we'll need to add a URI parameter.
Attached patch bug-1040994-fix.patch (obsolete) — Splinter Review
Added LIMIT support through a Uri parameter. The patch includes an update to the content provider, plus an updated test case.

Try: https://tbpl.mozilla.org/?tree=Try&rev=0b0a5a446683
Attachment #8459358 - Flags: review?(rnewman)
Comment on attachment 8459358 [details] [diff] [review]
bug-1040994-fix.patch

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

What happens when the limit parameter is an empty string, or non-numeric, or so on?

Let's make sure we have tests for the failure cases, not just the success ones!

r+ with tests for those.

::: mobile/android/base/tests/testSearchHistoryProvider.java
@@ +127,5 @@
> +            }
> +
> +            final int limit = 5;
> +            final Uri uri = SearchHistory.CONTENT_URI.buildUpon()
> +                .appendQueryParameter(BrowserContract.PARAM_LIMIT, String.valueOf(limit)).build();

Prefer stacking for chains:


final Uri uri = SearchHistory.CONTENT_URI
                             .buildUpon()
		             .appendQueryParameter(BrowserContract.PARAM_LIMIT, String.valueOf(limit))
                             .build();
Attachment #8459358 - Flags: review?(rnewman) → review+
Updated unit tests: empty input, negative input, and non-numeric input.

Updated Try:  https://tbpl.mozilla.org/?tree=Try&rev=35081e5ccb9e
Attachment #8459864 - Attachment is obsolete: true
Attachment #8459864 - Attachment is obsolete: false
Attachment #8459358 - Attachment is obsolete: true
Comment on attachment 8459864 [details] [diff] [review]
bug-1040994-fix.v2.patch

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

Carrying r+ from rnewman's previous review.
Attachment #8459864 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/8a4691f767ea
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Eric, can you also file a follow-up to use this limit in the search history query we're making in the search activity?
https://hg.mozilla.org/mozilla-central/rev/8a4691f767ea
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Adding reference to bug for implementing in UI.
Blocks: 1042425
Let's try this again.  A nice easy one, margaret.
Attachment #8483801 - Flags: review?(margaret.leibovic)
Comment on attachment 8483801 [details] [diff] [review]
Part 6: Display device type icon. r=margaret

The code, I can manage.  Bugzilla?  Not so much.
Attachment #8483801 - Attachment is obsolete: true
Attachment #8483801 - Flags: review?(margaret.leibovic)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.