Collect search history in Fennec

VERIFIED FIXED in Firefox 34

Status

()

Firefox for Android
Search Activity
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: eedens, Assigned: Margaret)

Tracking

unspecified
Firefox 33
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox34 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Prior to launching the search activity, we want to pre-load the search history by collecting items in Fennec.  So, when a user performs a search from Fennec's url bar, it should get added to the search history db. 

Content provider: https://lxr.mozilla.org/mozilla-central/source/mobile/android/base/db/SearchHistoryProvider.java
Unit tests and examples: https://lxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testSearchHistoryProvider.java
(Assignee)

Comment 1

4 years ago
Created attachment 8455809 [details] [diff] [review]
Record search history in Fennec

I put this in BrowserApp because I wasn't sure where else it should go, but I'm open to suggestions if you can think of a better place.
Attachment #8455809 - Flags: review?(rnewman)
Attachment #8455809 - Flags: review?(eedens)
Comment on attachment 8455809 [details] [diff] [review]
Record search history in Fennec

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

You missed a bit. Look for recordSearch in BrowserApp, which has two callers.

::: mobile/android/base/AndroidManifest.xml.in
@@ +391,5 @@
>                    android:permission="@ANDROID_PACKAGE_NAME@.permissions.BROWSER_PROVIDER"/>
>  
> +        <provider android:name="org.mozilla.gecko.db.SearchHistoryProvider"
> +                  android:authorities="@ANDROID_PACKAGE_NAME@.db.searchhistory"
> +                  android:permission="@ANDROID_PACKAGE_NAME@.permissions.BROWSER_PROVIDER"/>

Two points on this:

* Didn't eedens do this when he landed it? If not, oh well, oversight!
* Think about whether this permission is suitable, or if you want to introduce a new one.

::: mobile/android/base/BrowserApp.java
@@ +937,5 @@
>          EventDispatcher.getInstance().unregisterGeckoThreadListener((GeckoEventListener)this,
>              "Menu:Update",
>              "Reader:Added",
>              "Reader:FaviconRequest",
> +            "Search:Keyword",

N.B., BrowserHealthRecorder also subscribes to this event. When BrowserApp becomes a NativeEventListener for this event, you'll need to switch BHR as well to avoid a conflict -- EventDispatcher requires all consumers to be the same type.

@@ +1381,5 @@
>                  final String url = message.getString("url");
>                  handleReaderFaviconRequest(url);
> +            } else if (event.equals("Search:Keyword")) {
> +                final ContentValues values = new ContentValues();
> +                values.put(SearchHistory.QUERY, message.getString("query"));

Check for empty or missing value?
Attachment #8455809 - Flags: review?(rnewman) → feedback+
Depends on: 1030277
(Assignee)

Comment 3

4 years ago
Created attachment 8456274 [details] [diff] [review]
(v2) Record search history in Fennec

Updated to address feedback. As I mentioned on IRC, I don't think we should store bookmark keyword search terms, but yes, storing search suggestions the user tapped on sounds smart.
Attachment #8455809 - Attachment is obsolete: true
Attachment #8455809 - Flags: review?(eedens)
Attachment #8456274 - Flags: review?(rnewman)
Attachment #8456274 - Flags: review?(rnewman) → review+
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/e13b40fe4740
https://hg.mozilla.org/mozilla-central/rev/e13b40fe4740
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33

Comment 6

3 years ago
Verified as fixed in build 34.0a1 (2014-08-04);
Device: Samsung Galaxy Nexus (Android 4.2.1).

I see that the target milestone is Firefox 33;
Will this bug be uplifted to Aurora 33?
status-firefox34: --- → verified
(Assignee)

Updated

3 years ago
Depends on: 1048444

Comment 7

3 years ago
Marking the bug as verified based on comment 6.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.