Allow deleting partner bookmarks (partner bookmarks content provider)

RESOLVED FIXED in Firefox 49

Status

()

Firefox for Android
Distributions
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: sebastian, Assigned: sebastian)

Tracking

unspecified
Firefox 50
All
Android
Points:
---

Firefox Tracking Flags

(firefox48 wontfix, firefox49 fixed, firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

a year ago
We are reading bookmarks from the partner provider but currently they cannot be deleted.

* The partner bookmark provider, by default, does not allow deleting bookmarks
* We will need to add an overlay that filters deleted bookmarks (See bug 1286203)
* Folders can't be deleted in our UI but we should hide empty folders from the partner content provider
(Assignee)

Comment 1

a year ago
Created attachment 8771055 [details]
Bug 1286794 - Allow deleting of partner bookmarks by filtering content provider cursor.

Review commit: https://reviewboard.mozilla.org/r/64310/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64310/
Attachment #8771055 - Flags: review?(gkruglov)

Comment 2

a year ago
https://reviewboard.mozilla.org/r/64310/#review62348

I think it's possible to avoid using a CursorWrapper entirely. You should be able to do the filtering you want in the `getBookmarksInFolder` method as part of your selection criteria/args to the underlying content provider, which seems much cleaner? You will probably have to store a set of IDs in SharedPreferences instead of boolean values, which likely has a different (more complicated..?) concurrent access story than the current implementation... If that can be done cleanly, my vote would be to not use a wrapper.

::: mobile/android/base/java/org/mozilla/gecko/distribution/PartnerBookmarksProviderProxy.java:222
(Diff revision 1)
> +                .putBoolean(getDeletedPreferenceKey(bookmarkId), true)
> +                .apply();
> +    }
> +
> +    private static String getDeletedPreferenceKey(long bookmarkId) {
> +        return "distribution.partner.bookmark.deleted." + bookmarkId;

nit: Seems like this would make a good constant on top of the class

::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:452
(Diff revision 1)
>                      Snackbar.LENGTH_LONG);
>          }
>      }
> +
> +    private static class RemovePartnerBookmarkTask extends UIAsyncTask.WithoutParams<Void> {
> +        private Context context;

Would it make sense to use WeakReference here, to avoid potentially leaking context?
(Assignee)

Comment 3

a year ago
(In reply to :Grisha Kruglov from comment #2)
> https://reviewboard.mozilla.org/r/64310/#review62348
> 
> I think it's possible to avoid using a CursorWrapper entirely. You should be
> able to do the filtering you want in the `getBookmarksInFolder` method as
> part of your selection criteria/args to the underlying content provider,
> which seems much cleaner? You will probably have to store a set of IDs in
> SharedPreferences instead of boolean values, which likely has a different
> (more complicated..?) concurrent access story than the current
> implementation... If that can be done cleanly, my vote would be to not use a
> wrapper.

I can filter deleted bookmarks with a NOT IN(?). But I can't filter empty folders I think. This would require running a subquery but I can't do that because the ContentProvider does not reveal its underlying tables.
(Assignee)

Comment 4

a year ago
Comment on attachment 8771055 [details]
Bug 1286794 - Allow deleting of partner bookmarks by filtering content provider cursor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64310/diff/1-2/

Comment 5

a year ago
Comment on attachment 8771055 [details]
Bug 1286794 - Allow deleting of partner bookmarks by filtering content provider cursor.

https://reviewboard.mozilla.org/r/64310/#review63228

Looking good!
Attachment #8771055 - Flags: review?(gkruglov) → review+
(Assignee)

Comment 6

a year ago
https://hg.mozilla.org/integration/fx-team/rev/4abaf15d33453b20a721136f9f80454a7706df16
Bug 1286794 - Allow deleting of partner bookmarks by filtering content provider cursor. r=grisha
(Assignee)

Comment 7

a year ago
Comment on attachment 8771055 [details]
Bug 1286794 - Allow deleting of partner bookmarks by filtering content provider cursor.

(Bug 1286203 needs to be uplifted before this one)

Approval Request Comment

[Feature/regressing bug #]: Support for the deleting bookmarks from the partner bookmarks provider; introduced in bug 1283025 and uplifted to 48.0. (Partnership deal with 48.0 build)

[User impact if declined]: Without this patch users can't remove bookmarks from the partner bookmarks provider.

[Describe test coverage new/current, TreeHerder]: Local testing.

[Risks and why]: Low. This change only affects the partner code which is behind a pref and should not affect all other users.

[String/UUID change made/needed]: -
Attachment #8771055 - Flags: approval-mozilla-beta?
Attachment #8771055 - Flags: approval-mozilla-aurora?
Sebastian, the recent uplifts requests are very last minute (we ship in 8 days). Even if they are partner only, if there is any regressions, this is going to impact the product quality that we provide to our partners but also our users at the end. It might also increase the load on the various release teams in case we have to do a dot release for this partner.

Can it ride the train from 49? Happy to discuss by email if needed.
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4abaf15d3345
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
(Assignee)

Comment 10

a year ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #8)
> Can it ride the train from 49? Happy to discuss by email if needed.

Okay, let's go with 49. The icons are not that important I think. Not being able to remove the partner bookmarks is annoying but 49 is not far away.
Comment on attachment 8771055 [details]
Bug 1286794 - Allow deleting of partner bookmarks by filtering content provider cursor.

thanks, let's it ride the train then.
Attachment #8771055 - Flags: approval-mozilla-beta?
Attachment #8771055 - Flags: approval-mozilla-beta-
Attachment #8771055 - Flags: approval-mozilla-aurora?
Attachment #8771055 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 12

a year ago
Note: Bug 1286203 needs to be uplifted to Aurora before we can uplift this one here.

Comment 13

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/dd907ccf026d
status-firefox49: affected → fixed
it seems this broke aurora nightlys like https://treeherder.mozilla.org/logviewer.html#?job_id=3133326&repo=mozilla-aurora and so backed out
status-firefox49: fixed → affected
Flags: needinfo?(s.kaspari)
status-firefox48: affected → wontfix
(Assignee)

Comment 15

a year ago
Argh, the patch uses a new snackbar API that is not available on Aurora.
(Assignee)

Comment 16

a year ago
Created attachment 8775900 [details] [diff] [review]
1286794-AURORA.patch

Aurora version of the patch.
Flags: needinfo?(s.kaspari) → needinfo?(cbook)

Comment 17

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/587fb55f71b5
status-firefox49: affected → fixed
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.