Closed Bug 908344 Opened 6 years ago Closed 6 years ago

Bookmark keywords are broken

Categories

(Firefox for Android :: Awesomescreen, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 26
Tracking Status
firefox25 --- unaffected
firefox26 --- verified
fennec 26+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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...
Duplicate of this bug: 908518
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.
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
Attached patch patchSplinter Review
This is just a port of what used to be in AwesomeBar.java.

I filed bug 909455 about restoring the recordSearch functionality.
Attachment #795586 - Flags: review?(wjohnston)
Blocks: 909455
Attached patch testSplinter Review
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
Attachment #795625 - Flags: review?(wjohnston)
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);
Attachment #795586 - Flags: review?(wjohnston) → review+
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?)
Attachment #795625 - Flags: review?(wjohnston) → review+
(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.
Blocks: 912343
https://hg.mozilla.org/mozilla-central/rev/021cf0a2602a
https://hg.mozilla.org/mozilla-central/rev/c50dc9643c47
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Flags: in-moztrap?(fennec)
Keywords: verifyme
Verified fixed on:
Build: Firefox for Android 26.0a1 (2013-09-05)
Device: Samsung Galaxy Nexus
OS: Android 4.1.1
Flags: in-moztrap?(fennec) → in-moztrap+
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.