Adding a URL to thumbnails is broken

VERIFIED FIXED in Firefox 26

Status

()

P1
normal
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: ibarlow, Assigned: Margaret)

Tracking

26 Branch
Firefox 27
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox26 fixed, firefox27 verified, fennec26+)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

5 years ago
tracking-fennec: --- → ?
(Reporter)

Updated

5 years ago
Duplicate of this bug: 917881
(Assignee)

Updated

5 years ago
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 2

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

Comment 3

5 years ago
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?
Flags: needinfo?(ibarlow)
(Assignee)

Comment 4

5 years ago
Created attachment 810140 [details] [diff] [review]
Fix PinSiteDialog to handle pinning user-entered terms

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.
Attachment #810140 - Flags: review?(wjohnston)
tracking-fennec: ? → 26+
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?
Attachment #810140 - Flags: review?(wjohnston) → review+
(Assignee)

Comment 6

5 years ago
(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!
(Assignee)

Comment 8

5 years ago
(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
Flags: needinfo?(ibarlow)
https://hg.mozilla.org/mozilla-central/rev/c96a2d66725c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27

Updated

5 years ago
Status: RESOLVED → VERIFIED
status-firefox26: --- → affected
status-firefox27: --- → verified
(Assignee)

Comment 10

5 years ago
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
Attachment #810140 - Flags: approval-mozilla-aurora?
Attachment #810140 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.