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

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P2)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Mark Banner (:standard8)]

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

Comment 2

[Kajal Kumari Sah (:kajalsah)]

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?

Comment 3

[Mark Banner (:standard8)]

Hi Kajal, yes that's fine. We only generally assign bugs once the first patch is up though.

Comment 4

[Kajal Kumari Sah (:kajalsah)]

@Mark Can I get some more hints on this? The files where I can find these objects being used?

Comment 5

[Mark Banner (:standard8)]

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

Comment 6

[Kajal Kumari Sah (:kajalsah)]

Attachment: Bug 1489409 - Remove annotation options for PlacesTransactions.NewBookmark/NewFolder.

Initial commit on removing annotations in the PlacesTransactions

Comment 7

[Mark Banner (:standard8)]

Hi Kajal, did you see my latest comments on the patch? https://phabricator.services.mozilla.com/D6093#142067

Comment 8

[Mark Banner (:standard8)]

Kajal, if you're still working on this please let me know ASAP, otherwise I'll probably look to get it landed somehow.

Comment 9

[Mark Banner (:standard8)]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd2ec1be4ead
Remove annotation options for PlacesTransactions.NewBookmark/NewFolder. r=mak

Comment 11

[Mark Banner (:standard8)]

Assigning to Kajal for the record as they did most of the work here.

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/bd2ec1be4ead