Sriram Ramasubramanian [:sriram] Assignee
	Description • 12 years ago

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

	Sriram Ramasubramanian [:sriram] Assignee
	Comment 2 • 12 years ago

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.

	Sriram Ramasubramanian [:sriram] Assignee
	Comment 3 • 12 years ago

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

	Aaron Train [:aaronmt]
	Comment 4 • 12 years ago

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

	Sriram Ramasubramanian [:sriram] Assignee
	Comment 5 • 12 years ago

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

	Richard Newman [:rnewman]
	Comment 6 • 12 years ago

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.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 7 • 12 years ago

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

	Lucas Rocha (:lucasr)
	Comment 8 • 12 years ago

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

	Sriram Ramasubramanian [:sriram] Assignee
	Comment 9 • 12 years ago

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

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 10 • 12 years ago

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

	Richard Newman [:rnewman]
	Comment 11 • 12 years ago

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

	Lucas Rocha (:lucasr)
	Comment 12 • 12 years ago

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

	Sriram Ramasubramanian [:sriram] Assignee
	Comment 13 • 12 years ago

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

	Lucas Rocha (:lucasr)
	Comment 14 • 12 years ago

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

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 15 • 12 years ago

(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

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 16 • 12 years ago

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?

	Lucas Rocha (:lucasr)
	Comment 17 • 12 years ago

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

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 18 • 12 years ago

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"

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 19 • 12 years ago

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

	Lucas Rocha (:lucasr)
	Comment 20 • 12 years ago

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?

	Richard Newman [:rnewman]
	Comment 21 • 12 years ago

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

	Ian Barlow (:ibarlow)
	Comment 22 • 12 years ago

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

	Sriram Ramasubramanian [:sriram] Assignee
	Comment 23 • 12 years ago

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

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 24 • 12 years ago

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.

	Ed Morley [:emorley]
	Comment 25 • 12 years ago

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

	Aaron Train [:aaronmt]
	Comment 26 • 12 years ago

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

	Cristian Nicolae (:xti)
	Comment 27 • 12 years ago

(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

	Aaron Train [:aaronmt]
	Comment 28 • 12 years ago

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?

	Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]
	Comment 29 • 12 years ago

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

	Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]
	Comment 30 • 12 years ago

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

	Gian-Carlo Pascutto [:gcp]
	Comment 31 • 12 years ago

See comment 24.

	Mark Goodwin [:mgoodwin]
	Comment 32 • 11 years ago

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