1040994 - Add LIMIT support for search history content provider

Status: RESOLVED FIXED

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

Description

[Eric Edens [:eedens]]

The search activity uses a CursorLoader to query the search history content provider. To support LIMIT, we'll need to add a URI parameter.

Comment 1

[Eric Edens [:eedens]]

Attachment: bug-1040994-fix.patch

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

Comment 2

[Richard Newman [: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();

Comment 3

[Eric Edens [:eedens]]

Attachment: bug-1040994-fix.v2.patch

Updated unit tests: empty input, negative input, and non-numeric input.

Updated Try:  https://tbpl.mozilla.org/?tree=Try&rev=35081e5ccb9e

Comment 4

[Eric Edens [:eedens]]

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.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[:Margaret Leibovic]

Eric, can you also file a follow-up to use this limit in the search history query we're making in the search activity?

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/8a4691f767ea

Comment 8

[Eric Edens [:eedens]]

Adding reference to bug for implementing in UI.

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: Part 6: Display device type icon. r=margaret

Let's try this again.  A nice easy one, margaret.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8483801 [details] [diff] [review]
Part 6: Display device type icon. r=margaret

The code, I can manage.  Bugzilla?  Not so much.