Closed Bug 1489409 Opened 1 year ago Closed 1 year ago
Remove support for "annotations" options to Places
Transactions .New Bookmark/New Folder/New Livemark
46 bytes, text/x-phabricator-request
|Details | Review|
The PlacesTransactions objects, NewBookmark, NewFolder and NewLivemark, can currently received "annotations" or "annotation" input properties. However, these properties are never used, and annotations are going away, so we can remove them now. We should remove "annotations" from the transaction definition, as well as the code to handle inserting them. We should also remove the defineInputProps/defineArrayInputProp and the associated functions. Also the documentation needs the references to annotations removing. Note: we should not remove "excludingAnnotations"/"excludingAnnotation" from other places in the file at this time, as they are still required.
I'm happy to mentor this bug. Note that the file being referenced here is PlacesTransactions.jsm. You should also ensure the tests still pass, by running a minimum of: ./mach xpcshell-test toolkit/components/places but this would be useful to check as well: ./mach mochitest browser/components/places
Hello Mark, I am new to open source and looking forward to Outreachy Program. I want to start with this bug. Can I take this issue if this is available to a new user?
Hi Kajal, yes that's fine. We only generally assign bugs once the first patch is up though.
@Mark Can I get some more hints on this? The files where I can find these objects being used?
(In reply to Kajal Kumari Sah from comment #4) > @Mark Can I get some more hints on this? The files where I can find these > objects being used? Sure. Using https://searchfox.org/ is a good place to look for items. For instance, if you search for something like NewBookmark, then you'll get: https://searchfox.org/mozilla-central/search?q=NewBookmark Generally, the objects in PlacesTransactions are referenced by PT.NewBookmark or PlacesTransactions.NewBookmark. I've already been through the places that use these and checked that annotations are no longer passed to them, though you are welcome to check as well. I think all the changes that are required are in PlacesTranscations.jsm or test_async_transactions.js - the related test file.
Initial commit on removing annotations in the PlacesTransactions
Hi Kajal, did you see my latest comments on the patch? https://phabricator.services.mozilla.com/D6093#142067
Kajal, if you're still working on this please let me know ASAP, otherwise I'll probably look to get it landed somehow.
Unfortunately I've not heard from Kajal. As we'd like to move forward with this soonish, I'll update the patch and add the minor test changes so it can land.
Assignee: kajalksah07 → standard8
Status: NEW → ASSIGNED
Attachment #9009795 - Attachment description: Removed annotations in PlacesTransactions → Bug 1489409 - Remove annotation options for PlacesTransactions.NewBookmark/NewFolder.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/bd2ec1be4ead Remove annotation options for PlacesTransactions.NewBookmark/NewFolder. r=mak
Assigning to Kajal for the record as they did most of the work here.
Assignee: standard8 → kajalksah07
You need to log in before you can comment on or make changes to this bug.