Steps to reproduce - Tap the "Add a bookmark" tile - Type a URL Expected: - Hit enter to add a page to thumbnails Actual - There is a search button instead of an enter button, and tapping it only hides the keyboard. There is no way to actually add the link to your thumbnails.

Note to self: a fix for this will also require porting the changes from bug 858994 that got lost in the fig merge :(

Also, currently the "Edit" context menu item on pinned sites just launches an empty PinSiteDialog. Do we want to pre-filter that with the url for the thumbnail, as we did with the old about:home?

This adds logic to handle the user hitting enter with their search term, and restores the "user-entered" logic from bug 858994: https://hg.mozilla.org/mozilla-central/rev/b5b63beb5249 This would definitely be a good candidate for a robocop test... I'll try to work on that.

Comment on attachment 810140 [details] [diff] [review] Fix PinSiteDialog to handle pinning user-entered terms Review of attachment 810140 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/PinSiteDialog.java @@ +108,5 @@ > + > + // If the user manually entered a search term or URL, wrap the value in > + // a special URI until we can get a valid URL for this bookmark. > + final String text = mSearch.getText().toString(); > + final String url = TopSitesPage.encodeUserEnteredUrl(text); I like this a lot, but for some reason this method name confuses me. ::: mobile/android/base/home/TopSitesPage.java @@ +375,5 @@ > + } > + > + static String decodeUserEnteredUrl(String url) { > + Uri uri = Uri.parse(url); > + if ("user-entered".equals(uri.getScheme())) { I worry parsing the url overkill for something that's going to affect pageload speed. Can we just do a url.startsWith("user-entered") check?

(In reply to Wesley Johnston (:wesj) from comment #5) > ::: mobile/android/base/home/TopSitesPage.java > @@ +375,5 @@ > > + } > > + > > + static String decodeUserEnteredUrl(String url) { > > + Uri uri = Uri.parse(url); > > + if ("user-entered".equals(uri.getScheme())) { > > I worry parsing the url overkill for something that's going to affect > pageload speed. Can we just do a url.startsWith("user-entered") check? Looking at history, it looks like we decided to use the Uri methods to help us do escaping: https://bugzilla.mozilla.org/show_bug.cgi?id=858994#c18 So, I suppose we could use startsWith to check if we have a user-entered url, then do the Uri.parse inside the if statement. However, I'd prefer to keep our implementation the same as what's currently on beta/release if we're going to try to uplift this patch.

Sounds fine. Thanks for looking!

(In reply to :Margaret Leibovic from comment #3) > Also, currently the "Edit" context menu item on pinned sites just launches > an empty PinSiteDialog. Do we want to pre-filter that with the url for the > thumbnail, as we did with the old about:home? Split this off into bug 925082. https://hg.mozilla.org/integration/fx-team/rev/c96a2d66725c

Comment on attachment 810140 [details] [diff] [review] Fix PinSiteDialog to handle pinning user-entered terms [Approval Request Comment] Bug caused by (feature/regressing bug #): regression caused by bug 862793 and friends User impact if declined: can't pin a site that isn't already in history/bookmarks Testing completed (on m-c, etc.): landed on m-c 10/10 Risk to taking this patch (and alternatives if risky): low-risk, changes isolated to pin site dialog, restores code that was previously shipped String or IDL/UUID changes made by this patch: none