Allow deleting partner bookmarks (partner bookmarks content provider)

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sebastian, Assigned: sebastian)

Tracking

unspecified
Firefox 50
All
Android
Points:
---

Firefox Tracking Flags

(firefox48 wontfix, firefox49 fixed, firefox50 fixed)

Details

Attachments

(2 attachments)

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
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

3 years 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?
(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.
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

3 years 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+
https://hg.mozilla.org/integration/fx-team/rev/4abaf15d33453b20a721136f9f80454a7706df16
Bug 1286794 - Allow deleting of partner bookmarks by filtering content provider cursor. r=grisha
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4abaf15d3345
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
(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+
Note: Bug 1286203 needs to be uplifted to Aurora before we can uplift this one here.

Comment 13

3 years 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
Argh, the patch uses a new snackbar API that is not available on Aurora.
Created attachment 8775900 [details] [diff] [review]
1286794-AURORA.patch

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

Comment 17

3 years 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.