1084663 - Keyword bookmarks navigate to literal "%S" without text substitution

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Rolcol]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141014134955

Steps to reproduce:

Imported bookmarks from Desktop (Windows 7), one of which has a keyword associated with it.  For example, my issue is with a bookmark to: "https://www.reddit.com/r/%S".  I have the "sr" keyword associated with this bookmark so that I can navigate to a subreddit by simply typing "sr firefox".  Firefox on Desktop correctly replaces "%S" with "firefox", resulting in the "https://www.reddit.com/r/firefox" URL.  This is correct behavior.


Actual results:

On Android, typing "sr firefox" takes me to literally "https://www.reddit.com/r/%S", resulting in a 404 page.  There is no URL text substitution.


Expected results:

Firefox for Android should behave like Firefox on Desktop and correctly replace "%S".  Note: I am not making a typo by capitalizing the "%S".  Lowercase "%s" works correctly, but capital "%S" is meant to ignore HTML URL encoding (such as when using "sr firefox/new" and going to "https://www.reddit.com/r/firefox/new" instead of "https://www.reddit.com/r/firefox%2Fnew")

Comment 1

[Aaron Train [:aaronmt]]

Thanks for the report. I see this trying this out, very edge casey.

Comment 2

[Richard Newman [:rnewman]]

BrowserApp:
                // Otherwise, construct a search query from the bookmark keyword.
                final String searchUrl = keywordUrl.replace("%s", URLEncoder.encode(keywordSearch));
                Tabs.getInstance().loadUrl(searchUrl, Tabs.LOADURL_USER_ENTERED);

Comment 3

[Alex Barros]

Hi, I would like to try this bug. Its my very first bug, so how can start?

If this is the code:
 // Otherwise, construct a search query from the bookmark keyword.
   final String searchUrl = keywordUrl.replace("%s", URLEncoder.encode(keywordSearch));
   Tabs.getInstance().loadUrl(searchUrl, Tabs.LOADURL_USER_ENTERED);

this solution can be applied?
 // Otherwise, construct a search query from the bookmark keyword.
   String temp = keywordUrl.replace("%S", URLEncoder.encode(keywordSearch));
   final String searchUrl = temp.replace("%s", URLEncoder.encode(keywordSearch));
   Tabs.getInstance().loadUrl(searchUrl, Tabs.LOADURL_USER_ENTERED);

Comment 4

[:Margaret Leibovic]

Welcome, Alex! The first thing you should do is get a Fennec build set up. You should follow the instructions here:
https://wiki.mozilla.org/Mobile/Fennec/Android

After that, you can find this piece of code in BrowserApp.java:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1968

Your suggested solution sounds like it's on the right track, but I think it would be better to only have one replace call, and use a regular expression to match both %s and %S. 

Let me know if you need more help working on a patch!

Comment 5

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #4)
> ... and use a regular expression
> to match both %s and %S. 

I think they're supposed to do different things, so we'd need one escaped and one unescaped.

Comment 6

[Patrick Aupperle]

This bug look easy to solve, yet there hasn't been an update in about 20 days it seems. 

Are you still working on this, Alex Barros?

Would it be wrong for me to attempt a fix for this and submit a patch (my first contribution to opensource software)?

Comment 7

[Alex Barros]

Sorry for my absence, I'm kind of busy with some academic projects, so I will not to be able to fix this bugs in next days.Patrick feel free to take this bug.

Comment 8

[Patrick Aupperle]

Sorry, it looks like I spoke too soon. I can't seem to get my build of fennec running (it crashes in the emulator and for some reason fails to build at API level 20, which my phone is at). I could go ahead and try to fix the bug (it really does seem trivial), but I would be unable to test it, so that seems like a bad idea. I'll try a few more times to get my build working, but the future looks grim.

Comment 9

[Richard Newman [:rnewman]]

The emulator must have hardware accel support, so likely won't work without some effort.

You should be building with target = 9, using SDK 23 and build-tools 21. Make sure your mozconfig reflects the tools you have installed.

Comment 10

[James Timmerman]

There hasn't been an update on this bug in a while. Would it be ok if I take a stab at this?

Comment 11

[Richard Newman [:rnewman]]

(In reply to James Timmerman from comment #10)
> There hasn't been an update on this bug in a while. Would it be ok if I take
> a stab at this?

Go ahead!

Comment 12

[James Timmerman]

In BrowserApp.java I updated the line:

// Old:
final String searchUrl = keywordUrl.replace("%s", URLEncoder.encode(keywordSearch));

// New:
final String searchUrl = keywordUrl.replace("%s", URLEncoder.encode(keywordSearch)).replace("%S", keywordSearch);

Margaret Leibovic, you mentioned using a single replace call, but I'm not sure how I would go about doing that given that %S and %s should behave differently.

I built and ran my rendition on my nexus 7 using Rolcol's test cases:

Using bookmark "http://www.reddit.com/r/%S" with keyword set to "sr"

    address: "sr firefox/new" becomes "http://www.reddit.com/r/firefox/new"

Using the same bookmark except with a lowercase "%s"

    address: "sr firefox/new" becomes "http://www.reddit.com/r/firefox%2Fnew"


It seems to work just like the desktop version of Firefox. Would my next step be to write tests for the code or to submit it to mozreview?

Comment 13

[Richard Newman [:rnewman]]

(In reply to James Timmerman from comment #12)

> Margaret Leibovic, you mentioned using a single replace call, but I'm not
> sure how I would go about doing that given that %S and %s should behave
> differently.

See comment 5; I think you can disregard Margaret's comment, unless you want to get really creative with regex functions, which is unlikely to be worthwhile here.


> It seems to work just like the desktop version of Firefox. Would my next
> step be to write tests for the code or to submit it to mozreview?

Tests would be much appreciated!

Comment 14

[Henry Addo]

Attachment: Fix for bug 1084663

Made replace function to replace both lower and uppercase placeholders

Comment 15

[Henry Addo]

Attachment: Make replace call match for both upper and lower case

Comment 16

[Henry Addo]

(In reply to henry from comment #14)
> Created attachment 8567450 [details] [diff] [review]
> Fix for bug 1084663
> 
> Made replace function to replace both lower and uppercase placeholders

Ignore this. I submitted the wrong patch file

Comment 17

[:Margaret Leibovic]

Comment on attachment 8567450 [details] [diff] [review]
Fix for bug 1084663

Thanks for jumping in with a patch!

Tip for the future: In the "edit details" view for attachments, you can mark them as "obsolete" :)

Comment 18

[:Margaret Leibovic]

(In reply to James Timmerman from comment #12)
> In BrowserApp.java I updated the line:
> 
> // Old:
> final String searchUrl = keywordUrl.replace("%s",
> URLEncoder.encode(keywordSearch));
> 
> // New:
> final String searchUrl = keywordUrl.replace("%s",
> URLEncoder.encode(keywordSearch)).replace("%S", keywordSearch);
> 
> Margaret Leibovic, you mentioned using a single replace call, but I'm not
> sure how I would go about doing that given that %S and %s should behave
> differently.
> 
> I built and ran my rendition on my nexus 7 using Rolcol's test cases:
> 
> Using bookmark "http://www.reddit.com/r/%S" with keyword set to "sr"
> 
>     address: "sr firefox/new" becomes "http://www.reddit.com/r/firefox/new"
> 
> Using the same bookmark except with a lowercase "%s"
> 
>     address: "sr firefox/new" becomes "http://www.reddit.com/r/firefox%2Fnew"
> 
> 
> It seems to work just like the desktop version of Firefox. Would my next
> step be to write tests for the code or to submit it to mozreview?

Hey James, it looks like Henry beat you to writing a patch here :(

If you're interested in helping test this patch and give feedback, that would definitely be appreciated, but hopefully you can also find another bug to work on. I'm sorry about that!

Comment 19

[:Margaret Leibovic]

Comment on attachment 8567452 [details] [diff] [review]
Make replace call match for both upper and lower case

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

::: mobile/android/base/BrowserApp.java
@@ +2272,5 @@
>  
>                  recordSearch(null, "barkeyword");
>  
>                  // Otherwise, construct a search query from the bookmark keyword.
> +                final String searchUrl = keywordUrl.replace("(%s|%S)", URLEncoder.encode(keywordSearch));

Given the discussion in comments 12 and 13, I don't think this is the right approach here. We want %s and %S to do different things.

It looks like James was on the right track in comment 12, you should take a look at his proposed changes.

Comment 20

[James Timmerman]

No problem. I'm still not sure how to officially submit a patch. 

I did manage to get the robocop test framework working. All existing tests pass on my nexus 7 except the ones for Adobe Flash (which makes sense since I don't have the plugin installed). I also just managed to get the project working in Android Studio... which makes life much easier.

I'll still tinker with writing tests for this though. Perhaps I'll ask about how to submit the tests on IRC when I get there.

Comment 21

[Henry Addo]

Attachment: fixes-bug-1084663.patch

Comment 22

[Henry Addo]

(In reply to henry from comment #21)
> Created attachment 8568287 [details] [diff] [review]
> fixes-bug-1084663.patch

Is there a test case for `BrowserApp` that I can hook into to test the fix? I can't seem to find it in the `mobile/android/tests/`

Comment 23

[Rolcol]

By the look of it, that patch doesn't distinguish between an uppercase %S and lowercase %s.

Comment 24

[Henry Addo]

(In reply to Richard Newman [:rnewman] from comment #5)
> (In reply to :Margaret Leibovic from comment #4)
> > ... and use a regular expression
> > to match both %s and %S. 
> 
> I think they're supposed to do different things, so we'd need one escaped
> and one unescaped.

Just noticed this comment. Hence why I'm escaping for both. Any reason why we want the upper case unescaped? So it matches with the desktop version?

Comment 25

[Henry Addo]

Attachment: fixes-bug-1084663.patch

Don't escape upper case %S

Comment 26

[Henry Addo]

(In reply to Rolcol from comment #23)
> By the look of it, that patch doesn't distinguish between an uppercase %S
> and lowercase %s.

I didn't see comment #5. I updated the patch for a fix.

Comment 27

[:Margaret Leibovic]

Comment on attachment 8568336 [details] [diff] [review]
fixes-bug-1084663.patch

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

Sorry for the slow review! This is looking good, but it looks like this patch builds on top of an old patch, instead of applying directly to mozilla-central or fx-team. You'll need to update that before we can land it.

::: mobile/android/base/BrowserApp.java
@@ +2272,5 @@
>  
>                  recordSearch(null, "barkeyword");
>  
>                  // Otherwise, construct a search query from the bookmark keyword.
> +                final String searchUrl = keywordUrl.replace("%s", URLEncoder.encode(keywordSearch)).replace("%S", keywordSearch);

Please also add a comment explaining the difference between the %s and the %S situations, so future developers understand what's going on here.

@@ -2272,5 @@
>  
>                  recordSearch(null, "barkeyword");
>  
>                  // Otherwise, construct a search query from the bookmark keyword.
> -                final String searchUrl = keywordUrl.replace("(%s|%S)", URLEncoder.encode(keywordSearch));

It looks like this patch builds on top of your previous patch. Could you please update it to create a patch that applies directly on top of a recent mozilla-central or fx-team build?

Comment 28

[Henry Addo]

Attachment: Fix for bug 1084663

Should be the right patch this time.

Comment 29

[:Margaret Leibovic]

Comment on attachment 8573195 [details] [diff] [review]
Fix for bug 1084663

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

Looks good, thanks!

Comment 30

[:Margaret Leibovic]

Here's a try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42a2323ae628

If this is all green, you can set the checkin-needed keyword on this bug, and someone will check this code in for you.

Comment 31

[Henry Addo]

Great. Thanks for the quick review. Keeping an eye on the try run.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/676f1ad2ac2b

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/676f1ad2ac2b