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

RESOLVED FIXED in Firefox 39

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Rolcol, Assigned: Henry Addo, Mentored)

Tracking

34 Branch
Firefox 39
All
Android
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [good first bug][lang=java])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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")
Thanks for the report. I see this trying this out, very edge casey.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: reproducible
OS: Windows 7 → Android
Hardware: x86_64 → ARM
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);
Mentor: margaret.leibovic
Hardware: ARM → All
Summary: Keyword bookmarks navigate to literally "%S" without text substitution → Keyword bookmarks navigate to literal "%S" without text substitution
Whiteboard: [good first bug][lang=java]

Comment 3

4 years ago
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

4 years ago
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!
Assignee: nobody → apbarros17
(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

4 years ago
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

4 years ago
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

4 years ago
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.
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

3 years ago
There hasn't been an update on this bug in a while. Would it be ok if I take a stab at this?
(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!
Assignee: apbarros17 → nobody

Comment 12

3 years ago
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?
Flags: needinfo?(margaret.leibovic)
(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!
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 14

3 years ago
Created attachment 8567450 [details] [diff] [review]
Fix for bug 1084663

Made replace function to replace both lower and uppercase placeholders
Attachment #8567450 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 15

3 years ago
Created attachment 8567452 [details] [diff] [review]
Make replace call match for both upper and lower case
Attachment #8567452 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 16

3 years ago
(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

Updated

3 years ago
Assignee: nobody → henry

Comment 17

3 years ago
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" :)
Attachment #8567450 - Attachment is obsolete: true
Attachment #8567450 - Flags: review?(margaret.leibovic)

Comment 18

3 years ago
(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

3 years ago
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.
Attachment #8567452 - Flags: review?(margaret.leibovic) → review-

Comment 20

3 years ago
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.
(Assignee)

Comment 21

3 years ago
Created attachment 8568287 [details] [diff] [review]
fixes-bug-1084663.patch
Attachment #8567452 - Attachment is obsolete: true
Attachment #8568287 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 22

3 years ago
(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/`
(Reporter)

Comment 23

3 years ago
By the look of it, that patch doesn't distinguish between an uppercase %S and lowercase %s.
(Assignee)

Comment 24

3 years ago
(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?
(Assignee)

Comment 25

3 years ago
Created attachment 8568336 [details] [diff] [review]
fixes-bug-1084663.patch

Don't escape upper case %S
Attachment #8568287 - Attachment is obsolete: true
Attachment #8568287 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 26

3 years ago
(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.
(Assignee)

Updated

3 years ago
Attachment #8568336 - Flags: review?(margaret.leibovic)

Comment 27

3 years ago
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?
Attachment #8568336 - Flags: review?(margaret.leibovic) → feedback+
(Assignee)

Comment 28

3 years ago
Created attachment 8573195 [details] [diff] [review]
Fix for bug 1084663

Should be the right patch this time.
Attachment #8568336 - Attachment is obsolete: true
Attachment #8573195 - Flags: review?(margaret.leibovic)

Comment 29

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

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

Looks good, thanks!
Attachment #8573195 - Flags: review?(margaret.leibovic) → review+

Comment 30

3 years ago
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.
(Assignee)

Comment 31

3 years ago
Great. Thanks for the quick review. Keeping an eye on the try run.
(Assignee)

Updated

3 years ago
Keywords: reproducible → checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/676f1ad2ac2b
Keywords: checkin-needed
Whiteboard: [good first bug][lang=java] → [good first bug][lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/676f1ad2ac2b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=java][fixed-in-fx-team] → [good first bug][lang=java]
Target Milestone: --- → Firefox 39
You need to log in before you can comment on or make changes to this bug.