722184 - Add keyword support to AwesomeBar searches

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Dão Gottwald [:dao]]

On desktop Firefox, I created a bunch of bookmark keywords over the years, such as "dict" for translating something or "imdb" for looking up movies. I synced them over to my phone, where they proved exceedingly useful. Ever since Firefox got updated to the native Android builds, my keywords stopped working.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: (1/2) add keyword support to databases

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: (2/2) awesomebar keyword searches

Comment 3

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

Comment on attachment 594381 [details] [diff] [review]
(1/2) add keyword support to databases

Looks good to me, but let's get another set of eyes

Comment 4

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

Comment on attachment 594382 [details] [diff] [review]
(2/2) awesomebar keyword searches


>diff --git a/mobile/android/base/AwesomeBar.java b/mobile/android/base/AwesomeBar.java

>     private void openUrlAndFinish(String url) {
>+        url = url.replaceAll(" +$", "");

trim() is cleaner, if we are ok with removing leading spaces too. I think we'd be OK. If it needs to be a trailing trim, add a comment for dummies like me (// Trim trailing spaces)

          url = url.trim()

and make sure you do some good manual testing on this. try keywords and urls and searches to makes sure they all work.

Comment 5

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

Comment on attachment 594382 [details] [diff] [review]
(2/2) awesomebar keyword searches

Actually, let's not bother to trim

Comment 6

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Comment on attachment 594381 [details] [diff] [review]
> (1/2) add keyword support to databases
> 
> Looks good to me, but let's get another set of eyes

Aw, man, I'm gonna need to update my patch in bug 716918 to deal with this.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 594381 [details] [diff] [review]
(1/2) add keyword support to databases

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

I'm not quite sure why we need some of this updateBookmark stuff and not using our own ContentProvider. We should be able to do something like:

mAppContext.getContentResolver().update(Bookmarks.CONTENT_URI, mProjection, ...)

on our own, right? That should do the work of abstracting to the correct provider for this android version and locale vs. android database.

Maybe this is much simpler or better for some reason though?

::: mobile/android/base/db/AndroidBrowserDB.java
@@ +282,5 @@
> +
> +        int updated = cr.update(Browser.BOOKMARKS_URI,
> +                                values,
> +                                Browser.BookmarkColumns.URL + " = ?",
> +                                new String[] { oldUri });

updated isn't needed here, although most other update implmentations do return it.

::: mobile/android/base/db/BrowserDB.java
@@ +85,5 @@
>          public void addBookmark(ContentResolver cr, String title, String uri);
>  
>          public void removeBookmark(ContentResolver cr, String uri);
>  
> +        public void updateBookmark(ContentResolver cr, String oldUri, String uri, String title, String keyword);

I wonder if we should just pass in a ContentValues (or some other hashtable/map thing) here instead of a bunch of strings.

Comment 8

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #7)
> Comment on attachment 594381 [details] [diff] [review]
> (1/2) add keyword support to databases
> 
> Review of attachment 594381 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not quite sure why we need some of this updateBookmark stuff and not
> using our own ContentProvider. We should be able to do something like:
> 
> mAppContext.getContentResolver().update(Bookmarks.CONTENT_URI, mProjection,
> ...)
> 
> on our own, right? That should do the work of abstracting to the correct
> provider for this android version and locale vs. android database.
> 
> Maybe this is much simpler or better for some reason though?

The problem is that the URI isn't consistent between Android and local DB - it can be either Bookmarks.CONTENT_URI, Browser.BOOKMARKS_URI, or BOOKMARKS_CONTENT_URI_POST_11. Frankly, the DB stuff is kind of a mess now, and I think we should do a complete fixup in another bug. The methods I added are consistent with everything else we do in BrowserDB (for now).

> ::: mobile/android/base/db/BrowserDB.java
> @@ +85,5 @@
> >          public void addBookmark(ContentResolver cr, String title, String uri);
> >  
> >          public void removeBookmark(ContentResolver cr, String uri);
> >  
> > +        public void updateBookmark(ContentResolver cr, String oldUri, String uri, String title, String keyword);
> 
> I wonder if we should just pass in a ContentValues (or some other
> hashtable/map thing) here instead of a bunch of strings.

I was also just trying to be consistent with the current API here, but yeah, this is something we should consider for a DB API cleanup.

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 594381 [details] [diff] [review]
(1/2) add keyword support to databases

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

Makes sense. I think this is probably fine. I worry a bit that we're not checking many of the values passed in. It seems like the Android DB apis are designed to handle a lot of null's, and I guess we'll just have to be careful.

Comment 10

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3dab1c85daf9
http://hg.mozilla.org/integration/mozilla-inbound/rev/1e26b9a3011e
http://hg.mozilla.org/integration/mozilla-inbound/rev/6dda2ea147cb

I removed the trim() calls in the AwesomeBar patch. I also changed the String.format() to replace() in case the URL contains multiple %s instances (and we only support %s substitution anyway).

I also landed a follow-up DB patch since I forgot to remove one of the unused "updated" variables.

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: (1/2) add keyword support to databases (follow-up patch)

Comment 12

[Brian Nicholson (:bnicholson)]

Comment on attachment 594381 [details] [diff] [review]
(1/2) add keyword support to databases

[Approval Request Comment]
Allows us to use keyword in the database. Low risk.

Comment 13

[Brian Nicholson (:bnicholson)]

Comment on attachment 594382 [details] [diff] [review]
(2/2) awesomebar keyword searches

[Approval Request Comment]
Adds keyword support to AwesomeScreen searches. Low risk.

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 595299 [details] [diff] [review]
(1/2) add keyword support to databases (follow-up patch)

[Approval Request Comment]
Follow-up patch to remove unused variable. Very low risk.

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6dda2ea147cb

Comment 16

[Ed Morley [:emorley]]

Sorry, and:
https://hg.mozilla.org/mozilla-central/rev/3dab1c85daf9
https://hg.mozilla.org/mozilla-central/rev/1e26b9a3011e

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 594381 [details] [diff] [review]
(1/2) add keyword support to databases

[Triage Comment]
Mobile only - approved for Aurora 12 and Beta 11.

Comment 18

[Justin Wood (:Callek)]

Patch 2 here failed to apply:

Justin@ORION /d/sources/mozilla-beta
$ hg qpush
applying keywords_awesomebar.patch
patching file mobile/android/base/AwesomeBar.java
Hunk #4 FAILED at 478
1 out of 4 hunks FAILED -- saving rejects to file mobile/android/base/AwesomeBar.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh keywords_awesomebar.patch

Comment 19

[Cristian Nicolae (:xti)]

I am able to reproduce this issue on the latest Nightly build. There are no results in AwesomeBar if searching for a saved bookmark keyword. Reopening bug

--
Firefox 13.0a1 (2012-03-05)
Device: HTC Desire
OS: Android 2.2

Comment 20

[Aaron Train [:aaronmt]]

Please search next time and don't reopen bugs that dont apply to the issue.

* bug 721731, and bug 732794