786029 - Fennec should integrate with global Android search

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sriram Ramasubramanian [:sriram]]

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

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Code Complete

Comment 2

[Sriram Ramasubramanian [:sriram]]

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.

Comment 3

[Sriram Ramasubramanian [:sriram]]

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

Comment 4

[Aaron Train [:aaronmt]]

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

Comment 5

[Sriram Ramasubramanian [:sriram]]

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

Comment 6

[Richard Newman [:rnewman]]

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 7

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 8

[Lucas Rocha (:lucasr)]

(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.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

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

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 11

[Richard Newman [:rnewman]]

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

Comment 12

[Lucas Rocha (:lucasr)]

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

Comment 13

[Sriram Ramasubramanian [:sriram]]

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

Comment 14

[Lucas Rocha (:lucasr)]

(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.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(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 16

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 17

[Lucas Rocha (:lucasr)]

(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 18

[Mark Finkle (:mfinkle) (use needinfo?)]

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"

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 20

[Lucas Rocha (:lucasr)]

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?

Comment 21

[Richard Newman [:rnewman]]

(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.

Comment 22

[Ian Barlow (:ibarlow)]

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

Comment 23

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c1d80269a13

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3c1d80269a13

Comment 26

[Aaron Train [:aaronmt]]

Attachment: Global search in action (Android 4.1.1)

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

Comment 27

[Cristian Nicolae (:xti)]

(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

Comment 28

[Aaron Train [:aaronmt]]

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?

Comment 29

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

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

Comment 30

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

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

Comment 31

[Gian-Carlo Pascutto [:gcp]]

See comment 24.

Comment 32

[Mark Goodwin [:mgoodwin]]

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).