908344 - Bookmark keywords are broken

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[:Margaret Leibovic]

Typing a bookmark keyword will just perform a search, instead of navigating to that bookmarked page.

I confirmed this isn't a problem on Aurora. Setting tracking-fennec:26+ since this is a regression.

If I'm ambitious, maybe I'll write a robocop test for this...

Comment 2

[Dave Townsend [:mossop]]

I also downgraded to a nightly from 17th and it worked there so it's definitely regressed in the past few days and the new UI work seems the most likely candidate.

Comment 3

[:Margaret Leibovic]

Yup, looks like this logic never made its way into the new edit mode logic:
https://hg.mozilla.org/mozilla-central/annotate/a3cc1c802366/mobile/android/base/AwesomeBar.java#l394

Comment 4

[:Margaret Leibovic]

Attachment: patch

This is just a port of what used to be in AwesomeBar.java.

I filed bug 909455 about restoring the recordSearch functionality.

Comment 5

[:Margaret Leibovic]

Attachment: test

I wrote a test for this! The updateBookmark method is kinda gross, but I couldn't come up with a better way to do this that wouldn't involve either updating directly to the content provider (feels fragile) or modifying LocalBrowserDB (maybe we should actually do this, but I don't want to risk a regression). I think/hope bug 907734 will help make this better.

Verified the test fails before my patch and passes afterwards. Here's try push:
https://tbpl.mozilla.org/?tree=Try&rev=7cf7a91e52d6

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 795586 [details] [diff] [review]
patch

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

::: mobile/android/base/BrowserApp.java
@@ +1444,5 @@
> +
> +                final String keywordUrl = BrowserDB.getUrlForKeyword(getContentResolver(), keyword);
> +                final String searchUrl = (keywordUrl != null)
> +                                       ? keywordUrl.replace("%s", URLEncoder.encode(keywordSearch))
> +                                       : url;

I don't really like this ternary. it make this hard to read. Can we do something to make it clearer that we're ignoring the keyword stuff if we don't fine one? i.e.

String searchUrl = url;
if (!TextUtils.isEmpty(keywordUrl)) {
    searchUrl = keywordUrl.replace("%s", URLEncoder.encode(keywordSearch));
}

or even:

if (!TextUtils.isEmpty(keywordUrl)) {
    Tabs.getInstance().loadUrl(keywordUrl.replace("%s", URLEncoder.encode(keywordSearch)),
                               Tabs.LOADURL_USER_ENTERED);
    return;
}
Tabs.getInstance().loadUrl(url, Tabs.LOADURL_USER_ENTERED);

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 795625 [details] [diff] [review]
test

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

Nice. It would be pretty easy to extend this to support passing the search terms in as well... ( hint :) )

::: mobile/android/base/tests/AboutHomeTest.java.in
@@ +150,5 @@
> +        try {
> +            ContentResolver resolver = getActivity().getContentResolver();
> +            ClassLoader classLoader = getActivity().getClassLoader();
> +            Class browserDB = classLoader.loadClass("org.mozilla.gecko.db.BrowserDB");
> +            Method getBookmarkForUrl = browserDB.getMethod("getBookmarkForUrl", ContentResolver.class, String.class);

Some of this makes you realize we just need to fixup our BrowserDB API's. Like BrowserDB.addBookmark should return the id of the created bookmark. It should probably also support a version with a keyword URL, so that we don't have to do this "add, then update" juggling.

I'm not sure if you want to do that here or file a follow up bug to tackle some of this (mentor bug?)

Comment 8

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #7)
> Comment on attachment 795625 [details] [diff] [review]
> test
> 
> Review of attachment 795625 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice. It would be pretty easy to extend this to support passing the search
> terms in as well... ( hint :) )

Maybe that can be a mentor bug :)

> ::: mobile/android/base/tests/AboutHomeTest.java.in
> @@ +150,5 @@
> > +        try {
> > +            ContentResolver resolver = getActivity().getContentResolver();
> > +            ClassLoader classLoader = getActivity().getClassLoader();
> > +            Class browserDB = classLoader.loadClass("org.mozilla.gecko.db.BrowserDB");
> > +            Method getBookmarkForUrl = browserDB.getMethod("getBookmarkForUrl", ContentResolver.class, String.class);
> 
> Some of this makes you realize we just need to fixup our BrowserDB API's.
> Like BrowserDB.addBookmark should return the id of the created bookmark. It
> should probably also support a version with a keyword URL, so that we don't
> have to do this "add, then update" juggling.
> 
> I'm not sure if you want to do that here or file a follow up bug to tackle
> some of this (mentor bug?)

Yeah, I was torn about whether or not it would be worthwhile to update LocalBrowserDB. I think that it's okay to land this for now, and maybe we can take a higher level look at how to improve the LocalBrowserDB for testing in some of the other bugs we have open to improve the DB testing APIs.

Comment 9

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/c50dc9643c47
https://hg.mozilla.org/integration/fx-team/rev/021cf0a2602a

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/021cf0a2602a
https://hg.mozilla.org/mozilla-central/rev/c50dc9643c47

Comment 11

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 26.0a1 (2013-09-05)
Device: Samsung Galaxy Nexus
OS: Android 4.1.1