Closed Bug 1084663 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

34 Branch
All
Android
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: rolyc5, Assigned: henry, Mentored)

Details

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

Attachments

(1 file, 4 obsolete files)

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]
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);
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.
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)?
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.
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.
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
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)
Attached patch Fix for bug 1084663 (obsolete) — Splinter Review
Made replace function to replace both lower and uppercase placeholders
Attachment #8567450 - Flags: review?(margaret.leibovic)
Attachment #8567452 - Flags: review?(margaret.leibovic)
(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
Assignee: nobody → henry
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)
(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 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-
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.
Attached patch fixes-bug-1084663.patch (obsolete) — Splinter Review
Attachment #8567452 - Attachment is obsolete: true
Attachment #8568287 - Flags: review?(margaret.leibovic)
(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/`
By the look of it, that patch doesn't distinguish between an uppercase %S and lowercase %s.
(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?
Attached patch fixes-bug-1084663.patch (obsolete) — Splinter Review
Don't escape upper case %S
Attachment #8568287 - Attachment is obsolete: true
Attachment #8568287 - Flags: review?(margaret.leibovic)
(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.
Attachment #8568336 - Flags: review?(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?
Attachment #8568336 - Flags: review?(margaret.leibovic) → feedback+
Should be the right patch this time.
Attachment #8568336 - Attachment is obsolete: true
Attachment #8573195 - Flags: review?(margaret.leibovic)
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+
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.
Great. Thanks for the quick review. Keeping an eye on the try run.
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
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=java][fixed-in-fx-team] → [good first bug][lang=java]
Target Milestone: --- → Firefox 39
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.