707711 - No way to delete bookmarks

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Sam Tobin-Hochstadt [:samth]]

If a bookmarked page redirects to another url, it's very difficult to remove the bookmark.  You have to hit the "menu" button prior to the redirect/url change.

Comment 1

[Sam Tobin-Hochstadt [:samth]]

Here's a test case:  the HTC Sensation on T-Mobile ships with a default bookmark of http://home.web2go.com/ which redirects as described.  Unfortunately, this site only works on the T-Mobile cellular network, so it's not a general test case.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Let's add a context menu with a "Remove" action for the bookmark list

Comment 3

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Let's add a context menu with a "Remove" action for the bookmark list

Isn't the right fix to check original URL to decide whether the final URL is a bookmark or not?

Comment 5

[Sam Tobin-Hochstadt [:samth]]

mfinkle, as far as I can see, it's still possible to delete bookmarks in Aurora by going to the relevant page using the context menu, which has a button labeled "Remove".

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Sam Tobin-Hochstadt from comment #5)
> mfinkle, as far as I can see, it's still possible to delete bookmarks in
> Aurora by going to the relevant page using the context menu, which has a
> button labeled "Remove".

Correct, but this is really not an acceptable way to remove bookmarks. We need a "Remove" context menu on the Bookmark list.

(In reply to Lucas Rocha (:lucasr) from comment #3)
> (In reply to Mark Finkle (:mfinkle) from comment #2)
> > Let's add a context menu with a "Remove" action for the bookmark list
> 
> Isn't the right fix to check original URL to decide whether the final URL is
> a bookmark or not?

This would be an approach to _stop_ redirects from ending up in the bookmark list to begin with, but I don't know how feasible this approach would be to implement.

Comment 7

[Michael Kohler [:mkohler]]

Sriram,  are you working on a patch? I'd be happy to provide a patch.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Michael Kohler [:michaelkohler] from comment #7)
> Sriram,  are you working on a patch? I'd be happy to provide a patch.

Michael, a patch would be great. Thank you

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Michael

You should be able to remove a bookmark using:

BrowserDB.removeBookmark(resolver, getURL());

Like we do here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#437

Comment 10

[Michael Kohler [:mkohler]]

Attachment: Patch v1

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 588769 [details] [diff] [review]
Patch v1


>diff -r 823072af2430 mobile/android/base/resources/menu/awesomebar_contextmenu.xml

>+    <item android:id="@+id/remove_bookmark"
>+          android:title="@string/contextmenu_remove_bookmark"/>
> 
>     <item android:id="@+id/share"
>           android:title="@string/contextmenu_share"/>
> 
>     <item android:id="@+id/add_to_launcher"
>           android:title="@string/contextmenu_add_to_launcher"/>

Looks right to me. The only changes I want to make is to move "Share" above "Remove Bookmark", and to use "Remove" as the text. This matches the XUL design.

I can make those changes when I land this.

Thanks!

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1007541442b5

Comment 13

[Michael Kohler [:mkohler]]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1007541442b5

Mark, do we want this in Aurara too? (just thinking about string freeze) I think this is pretty low-risk.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Michael Kohler [:michaelkohler] from comment #13)
> (In reply to Mark Finkle (:mfinkle) from comment #12)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/1007541442b5
> 
> Mark, do we want this in Aurara too? (just thinking about string freeze) I
> think this is pretty low-risk.

Yes, I'd like it in Aurora. I was just waiting for it to land on m-c before requesting. As for the string issue, we'd like to get as many strings as possible landed by Jan 17th.

Comment 15

[Justin Wood (:Callek)]

https://hg.mozilla.org/mozilla-central/rev/1007541442b5

Comment 16

[Axel Hecht [:Pike]]

Should this request approval then?

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 588769 [details] [diff] [review]
Patch v1

[Triage Comment]
We want this on aurora, sooner rather than later for l10n. Its been on trunk for 4 days, no issues so far.

Comment 18

[Michael Kohler [:mkohler]]

(In reply to Brad Lassey [:blassey] from comment #17)
> Comment on attachment 588769 [details] [diff] [review]
> Patch v1
> 
> [Triage Comment]
> We want this on aurora, sooner rather than later for l10n. Its been on trunk
> for 4 days, no issues so far.

please keep in mind that the patch v1 was changed before checkin (see comment 11). I'm on mobile now, so I can't attach a correct patch. Mark, could you do that for me?

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Michael Kohler [:michaelkohler] from comment #18)
> (In reply to Brad Lassey [:blassey] from comment #17)
> > Comment on attachment 588769 [details] [diff] [review]
> > Patch v1
> > 
> > [Triage Comment]
> > We want this on aurora, sooner rather than later for l10n. Its been on trunk
> > for 4 days, no issues so far.
> 
> please keep in mind that the patch v1 was changed before checkin (see
> comment 11). I'm on mobile now, so I can't attach a correct patch. Mark,
> could you do that for me?

We'll use the code landed in HG, not the patch on bugzilla. Things should be fine.

Comment 20

[Paul Feher]

Verified fixed on:
Nightly Fennec 13.0a1 (2012-03-05)
Device: HTC Desire Z
OS: Android 2.3.3

Testcase added in litmus in BFT bookmarks
https://litmus.mozilla.org/show_test.cgi?id=50508