Closed
Bug 1286794
Opened 8 years ago
Closed 8 years ago
Allow deleting partner bookmarks (partner bookmarks content provider)
Categories
(Firefox for Android Graveyard :: Android partner distribution, defect)
Firefox for Android Graveyard
Android partner distribution
All
Android
Tracking
(firefox48 wontfix, firefox49 fixed, firefox50 fixed)
RESOLVED
FIXED
Firefox 50
People
(Reporter: sebastian, Assigned: sebastian)
Details
Attachments
(2 files)
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta-
|
Details |
17.60 KB,
patch
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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•8 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?
Assignee | ||
Comment 3•8 years 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•8 years 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•8 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+
Assignee | ||
Comment 6•8 years 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•8 years 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?
Comment 8•8 years ago
|
||
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.
Updated•8 years ago
|
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4abaf15d3345
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Assignee | ||
Comment 10•8 years 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 11•8 years ago
|
||
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•8 years ago
|
||
Note: Bug 1286203 needs to be uplifted to Aurora before we can uplift this one here.
Comment 13•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/dd907ccf026d
Comment 14•8 years ago
|
||
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)
Updated•8 years ago
|
Assignee | ||
Comment 15•8 years ago
|
||
Argh, the patch uses a new snackbar API that is not available on Aurora.
Assignee | ||
Comment 16•8 years ago
|
||
Aurora version of the patch.
Flags: needinfo?(s.kaspari) → needinfo?(cbook)
Comment 17•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/587fb55f71b5
Updated•8 years ago
|
Flags: needinfo?(cbook)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•