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)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 10
People
(Reporter: justdave, Assigned: justdave)
Details
Attachments
(1 file, 1 obsolete file)
1.00 KB,
patch
|
mfinkle
:
review+
emorley
:
checkin+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #566486 -
Attachment description: p → patch v1
Attachment #566486 -
Attachment is patch: true
Assignee | ||
Comment 2•13 years ago
|
||
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 3•13 years ago
|
||
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+
Assignee | ||
Comment 4•13 years ago
|
||
(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?
Updated•13 years ago
|
Keywords: checkin-needed
Updated•13 years ago
|
Assignee: nobody → justdave
Assignee | ||
Comment 5•13 years ago
|
||
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?
Comment 6•13 years ago
|
||
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
Assignee | ||
Comment 7•13 years ago
|
||
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 :(
Assignee | ||
Comment 8•13 years ago
|
||
Comment on attachment 566486 [details] [diff] [review]
patch v1
Patch generates an exception in testing, functionality fails.
Attachment #566486 -
Flags: review-
Assignee | ||
Comment 9•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Attachment #572074 -
Flags: review?(mark.finkle)
Updated•13 years ago
|
Attachment #572074 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Updated•13 years ago
|
Attachment #572074 -
Flags: checkin?
Comment 10•13 years ago
|
||
Comment on attachment 572074 [details] [diff] [review]
patch v2
https://hg.mozilla.org/integration/mozilla-inbound/rev/b878590bf618
Attachment #572074 -
Flags: checkin? → checkin+
Updated•13 years ago
|
Keywords: checkin-needed
Target Milestone: --- → Firefox 10
Comment 11•13 years ago
|
||
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.
Description
•