Closed Bug 1286794 Opened 3 years ago Closed 3 years ago

Allow deleting partner bookmarks (partner bookmarks content provider)

Categories

(Firefox for Android :: Android partner distribution, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox48 --- wontfix
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

Details

Attachments

(2 files)

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
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 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.
https://hg.mozilla.org/mozilla-central/rev/4abaf15d3345
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
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.
it seems this broke aurora nightlys like https://treeherder.mozilla.org/logviewer.html#?job_id=3133326&repo=mozilla-aurora and so backed out
Flags: needinfo?(s.kaspari)
Argh, the patch uses a new snackbar API that is not available on Aurora.
Aurora version of the patch.
Flags: needinfo?(s.kaspari) → needinfo?(cbook)
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.