Closed Bug 1489409 Opened 1 year ago Closed 1 year ago

Remove support for "annotations" options to PlacesTransactions.NewBookmark/NewFolder/NewLivemark


(Toolkit :: Places, enhancement, P2)




Tracking Status
firefox65 --- fixed


(Reporter: standard8, Assigned: kajalsah, Mentored)



(Whiteboard: [lang=js])


(1 file)

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.
Blocks: 1469698
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
Mentor: standard8
Whiteboard: [lang=js]
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 is a good place to look for items.

For instance, if you search for something like NewBookmark, then you'll get:

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.
Assignee: nobody → kajalksah07
Initial commit on removing annotations in the PlacesTransactions
Hi Kajal, did you see my latest comments on the patch?
Flags: needinfo?(kajalksah07)
Kajal, if you're still working on this please let me know ASAP, otherwise I'll probably look to get it landed somehow.
Flags: needinfo?(kajalksah07)
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
Attachment #9009795 - Attachment description: Removed annotations in PlacesTransactions → Bug 1489409 - Remove annotation options for PlacesTransactions.NewBookmark/NewFolder.
Blocks: 1509464
Pushed by
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
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.