Fennec should integrate with global Android search so that users can search for bookmarks and history right from the Google search widget.

Comment on attachment 655745 [details] [diff] [review] WIP: Code Complete This adds all the code required (plumbing) for add search results to Android search. However, it doesn't use the "awesomebar" logic to return the results. (It's too complex for a non-SQL guy to understand and use it :( ) But for that, everything else works fine.

It would look like this: http://cl.ly/image/2g272H1W1q2x (Drag & Drop was easier).

How does this work if there is no first-launch profile? Does this require waiting for Gecko?

Search wouldn't provide any results from that app (Fennec).

Comment on attachment 655745 [details] [diff] [review] WIP: Code Complete Review of attachment 655745 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by! ::: mobile/android/base/AndroidManifest.xml.in @@ +208,5 @@ > android:excludeFromRecents="true"/> > > <provider android:name="@ANDROID_PACKAGE_NAME@.db.BrowserProvider" > + android:authorities="@ANDROID_PACKAGE_NAME@.db.browser"> > + android:permission="@ANDROID_PACKAGE_NAME@.permissions.BROWSER_PROVIDER"> That doesn't look like the right number of ">". ::: mobile/android/base/GeckoApp.java @@ +1930,5 @@ > } > + else if (Intent.ACTION_SEARCH.equals(action)) { > + String uri = getURIFromIntent(intent); > + GeckoAppShell.sendEventToGecko(GeckoEvent.createURILoadEvent(uri)); > + Log.i(LOGTAG,"Intent : SEARCH SUGGEST - " + uri); Space between arguments. Also, INFO seems too verbose for this. Is 'uri' ever user input or user browsing history? If so, don't log it at all by default. ::: mobile/android/base/db/BrowserProvider.java.in @@ +1799,5 @@ > } > > + case SEARCH_SUGGEST: { > + debug("Query is on search suggest: " + uri); > + selection = DBUtils.concatenateWhere(selection, "(" + Combined.URL + " LIKE ? OR " + Trailing whitespace. @@ +1803,5 @@ > + selection = DBUtils.concatenateWhere(selection, "(" + Combined.URL + " LIKE ? OR " + > + Combined.TITLE + " LIKE ?)"); > + selectionArgs = DBUtils.appendSelectionArgs(selectionArgs, > + new String[] { "%" + uri.getLastPathSegment() + "%", > + "%" + uri.getLastPathSegment() + "%" }); getLastPathSegment() can return null. ::: mobile/android/base/strings.xml.in @@ +10,5 @@ > #includesubst @SYNCSTRINGSPATH@ > ]> > #includesubst @BOOKMARKSPATH@ > <resources> > + <string name="application_name">@MOZ_APP_DISPLAYNAME@</string> Can we just call this "moz_app_displayname"? That seems like it'll save someone some time in `grep` in the future.

Comment on attachment 655745 [details] [diff] [review] WIP: Code Complete * I agree with Richard's drive-by comments. * We should get buy-in from UX about using (or not using) the frecency algorithm. * I'm fine with starting small and adding more results later. Example: WebApps

(In reply to Sriram Ramasubramanian [:sriram] from comment #2) > Comment on attachment 655745 [details] [diff] [review] > WIP: Code Complete > > This adds all the code required (plumbing) for add search results to Android > search. > > However, it doesn't use the "awesomebar" logic to return the results. (It's > too complex for a non-SQL guy to understand and use it :( ) But for that, > everything else works fine. Drive-by: add unit tests for the new bits you added in BrowserProvider.

Added changes as per rnewman's comments. What's the description we need to give for this entry? I've given "Bookmarks and History"

(In reply to Sriram Ramasubramanian [:sriram] from comment #9) > What's the description we need to give for this entry? I've given "Bookmarks > and History" I think Ian needs to give feedback for the description. "Bookmarks and History" works for now.

Take a look at the Android style guide. It'll probably suggest sentence case, not title case.

Just a reminder: add unit tests for the new stuff in BrowserProvider.

Unit tests like? This doesn't do anything and it's integrating with Android's.

(In reply to Sriram Ramasubramanian [:sriram] from comment #13) > Unit tests like? This doesn't do anything and it's integrating with > Android's. Unit tests exercising the new SEARCH_SUGGEST uri. Should be simple to write a test case that adds some bookmarks and history entries and use the SEARCH_SUGGEST query to return a known result set.

(In reply to Lucas Rocha (:lucasr) from comment #14) > (In reply to Sriram Ramasubramanian [:sriram] from comment #13) > > Unit tests like? This doesn't do anything and it's integrating with > > Android's. > > Unit tests exercising the new SEARCH_SUGGEST uri. Should be simple to write > a test case that adds some bookmarks and history entries and use the > SEARCH_SUGGEST query to return a known result set. Good idea

Comment on attachment 663491 [details] [diff] [review] Patch Passing review to Lucas. I want him to stay in the loop on DB provider changes. I am fine with a followup patch for the tests, but we should add them. Lucas - Can you point Sriram to some examples of provider tests?

(In reply to Mark Finkle (:mfinkle) from comment #16) > Comment on attachment 663491 [details] [diff] [review] > Patch > > Passing review to Lucas. I want him to stay in the loop on DB provider > changes. I am fine with a followup patch for the tests, but we should add > them. > > Lucas - Can you point Sriram to some examples of provider tests? The test for the SEARCH_SUGGEST stuff will probably be very similar to TestCombinedView: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testBrowserProvider.java.in#1317 i.e. add a couple of history and bookmark entries, then query using a URI based on SearchManagerSearchManager.SUGGEST_URI_PATH_QUERY or something. Check if number of results is as expected and if the rows have the expected values. You can probably just add a new Runnable test to testBrowserProvider.

Comment on attachment 663491 [details] [diff] [review] Patch >diff --git a/mobile/android/base/db/BrowserProvider.java.in b/mobile/android/base/db/BrowserProvider.java.in >+ case SEARCH_SUGGEST: { >+ qb.setProjectionMap(SEARCH_SUGGEST_PROJECTION_MAP); >+ qb.setTables(VIEW_COMBINED_WITH_IMAGES); We don't return any images, so let's avoid using the combined view with images. It will be slower. I see that we could return icons/images to the search system, but I don't think stock does that and it would make us slower. >diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd >+<!ENTITY searchable_description "Bookmarks and History"> Seems like the guidelines want us to use sentence case: "Bookmarks and history"

Android's guide for Search: http://developer.android.com/guide/topics/search/index.html

Comment on attachment 663491 [details] [diff] [review] Patch Review of attachment 663491 [details] [diff] [review]: ----------------------------------------------------------------- I'm inclined to block this feature until we have tests for it. If this really needs to land asap (which I think is the case), please file a follow-up AND post a patch for it as soon as you can. As I said, adding tests for BrowserProvider is super simple, there's no reason to not write them as you go, especially for small features like this one. ::: mobile/android/base/AndroidManifest.xml.in @@ +222,5 @@ > android:authorities="@ANDROID_PACKAGE_NAME@.db.browser" > + android:permission="@ANDROID_PACKAGE_NAME@.permissions.BROWSER_PROVIDER"> > + > + <path-permission android:pathPrefix="/search_suggest_query" > + android:readPermission="android.permission.GLOBAL_SEARCH" /> I wonder if this has any privacy implications... ::: mobile/android/base/db/BrowserProvider.java.in @@ +1894,5 @@ > + new String[] { "%" + keyword + "%", > + "%" + keyword + "%" }); > + > + if (TextUtils.isEmpty(sortOrder)) > + sortOrder = DEFAULT_HISTORY_SORT_ORDER; rnewman is possible moving the sort order bits used by the awesomebar into BrowserProvider. If/when this happens, we should probably use the same sort order used in awesomebar here. Please file a follow-up. ::: mobile/android/base/locales/en-US/android_strings.dtd @@ +228,5 @@ > <!ENTITY bookmarkhistory_import_wait "Please wait..."> > > <!ENTITY webapp_generic_name "App"> > > +<!ENTITY searchable_description "Bookmarks and History"> I assume design team (ibarlow or madhava) has approved this string?

(In reply to Lucas Rocha (:lucasr) from comment #20) > > +<!ENTITY searchable_description "Bookmarks and History"> > > I assume design team (ibarlow or madhava) has approved this string? Probably not: needs to be sentence case, at the very least.

I think "Bookmarks and history" is fine, can I see a screenshot with this string in context? Thanks.

Mark - We should do a privacy/security check on this feature. We need to make sure how accessible the Firefox history data becomes. I think it is only local to the device, but it would be nice to verify. Also, users do have the ability to turn access to Firefox on and off from the Global Search settings. We also need to make sure that no data is leaked, even locally, during Private Browsing. This looks like it will be working correctly when Private Browsing lands.

Note for testers: in-case anyone wants to see this in action; see screenshot

(In reply to Aaron Train [:aaronmt] from comment #26) > Created attachment 668827 [details] > Global search in action (Android 4.1.1) > > Note for testers: in-case anyone wants to see this in action; see screenshot Thanks Aaron. After I enabled the Nightly app from Google Search settings, I was able to verify this bug and it seems that everything works fine. Nightly's bookmarks and history results were displayed as expected. Closing bug as verified fixed on: Firefox 18.0a1 (2012-10-08) Device: Galaxy Note OS: Android 4.0.4

https://hg.mozilla.org/mozilla-central/diff/3c1d80269a13/mobile/android/base/AndroidManifest.xml.in Can we confirm if the readPermission value added wont show up as another 'scary' permission in appInfo or on the Store?

Please update the information in this wiki page for the privacy review, if you have questions about what is needed or how to do it please contact me. https://wiki.mozilla.org/Privacy/Reviews/_Fennec_integration_with_global_Android_search

gcp: can you explain why a privacy-review is needed?

See comment 24.

I'm satisfied from testing that this is working as intended; I see no reference to local search data leaving the device in the relevant documentation (that's not to say this won't change, of course).