Closed Bug 693891 Opened 13 years ago Closed 13 years ago

creating an app shortcut should use bookmark title, not page title

Categories

(Firefox for Android Graveyard :: Bookmarks, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 10

People

(Reporter: justdave, Assigned: justdave)

Details

Attachments

(1 file, 1 obsolete file)

Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111011 Firefox/10.0a1 Fennec/10.0a1

Steps to Reproduce:
1. Navigate to a page you want to put an icon for on your home screen.
2. Tap the star icon on a page to set a bookmark
3. Tap edit
4. Observe that the title is something obnoxiously long that starts with something that won't clue you in to what the icon is for.  Change it to something short that fits on an icon.
5. Click Done
6. Click on the star again
7. Click "Add to Home Screen"

Observed Results:

Icon is created on the home screen using the obnoxiously long title of the page itself.

Expected Results:

Icon should use the title you set on the bookmark before you created the home screen icon.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #566486 - Attachment description: p → patch v1
Attachment #566486 - Attachment is patch: true
Comment on attachment 566486 [details] [diff] [review]
patch v1

I'm not sure if this patch works -- I can't get a build to compile right now even without patching it, so no way to test yet.  Also I'm not familiar with the code.  But this kinda looks like the right thing to do.  Also not sure who should be flagged to review it.  Feel free to pass it to someone else if I picked the wrong person.
Attachment #566486 - Flags: review?(mark.finkle)
Comment on attachment 566486 [details] [diff] [review]
patch v1

This should work. I'll test on a phone
Attachment #566486 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #3)
> Comment on attachment 566486 [details] [diff] [review] [diff] [details] [review]
> patch v1
> 
> This should work. I'll test on a phone

Any luck?
Assignee: nobody → justdave
Although I technically have access to commit it's only because I'm a sysadmin, and I don't believe I have the proper vouchers in place to be allowed to under the current contributor guidelines.  (Aside from that, I actually have no clue how to commit, as I haven't done any development work using hg before).  Would someone like to commit this on my behalf, or walk me through how to do so?
I'm happy to land it for you, would you like me to walk you through sending to try, or is this something that you won't be doing again soon, so not worth learning?

Either way, if you wouldn't mind possibly setting up your .hgrc to add author info to the patch (http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed), and reattach an updated patch with author and commit message, that would be splendid (apparently splendid is the new awesome...) :-)
Status: NEW → ASSIGNED
OK, I got my dev environment working finally, and built it with this patch...  and it doesn't work.  It completely breaks the "Add to home screen"

Console says:

uncaught exception: [Exception... "Could not convert JavaScript argument arg 0 [nsINavBookmarksService.getBookmarkIdsForURI]" nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)" location: "JS frame :: resource://gre/modules/PlacesUtils.jsm :: PU_getMostRecentBookmarkForURI :: line 1083" data: no]

Don't know java well enough to know what this means :(
Comment on attachment 566486 [details] [diff] [review]
patch v1

Patch generates an exception in testing, functionality fails.
Attachment #566486 - Flags: review-
Attached patch patch v2Splinter Review
revised version that works (tested).  mfinkle gave me the fix on irc, so I'm assuming his r= carries over.
Attachment #566486 - Attachment is obsolete: true
Attachment #572074 - Flags: review?(mark.finkle)
Attachment #572074 - Flags: review?(mark.finkle) → review+
Attachment #572074 - Flags: checkin?
Keywords: checkin-needed
Target Milestone: --- → Firefox 10
https://hg.mozilla.org/mozilla-central/rev/b878590bf618
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: