Closed Bug 1480049 Opened 2 years ago Closed 2 years ago

Remove nsIAnnotationService::getPageAnnotation


(Toolkit :: Places, enhancement, P1)




Tracking Status
firefox63 --- fixed


(Reporter: standard8, Assigned: standard8)



(Whiteboard: [fxsearch])


(1 file)

We're now in the position where there's only a couple of production uses of nsIAnnotationService::getPageAnnotation, and the rest are test-only.

We can replace these with PlacesUtils.history.fetch(..., {includeAnnotations: true});
MozReview-Commit-ID: GwtZjIc7y5K
Comment on attachment 8996666 [details]
Bug 1480049 - Remove nsIAnnotationService::getPageAnnotation.

Marco Bonardo [::mak] (Away 9-26 Aug) has approved the revision.
Attachment #8996666 - Flags: review+
Mirroring my review comment manually to ask Marco for feedback :-)


Unfortunately, this is generally not viable as an approach. Race conditions are bound to happen as soon as you separate the decision of inserting or removing an element from the list and the moment when this is done. Regression tests are not great for checking for race conditions in general. Even if in this case something can probably be devised to make the current code create duplicate elements in the list, a new race condition can be added that isn't triggered by that particular test.

Also, the consumers of "onDownloadBatchStarting" and "onDownloadBatchEnded" expect the two calls to happen synchronously, together with all the addition and removal notifications. This is important for the Library view because the richlistbox is temporarily removed from the document. For this, you can probably add a regression test by triggering a batch, queuing a Promise microtask in "onDownloadBatchStarting", and then checking the microtask is still not executed in "onDownloadBatchEnded". This should fail with the current patch, but succeed with the previous state and the eventual solution.

The possibilities I see here are:
1. Change the Places API so we fetch the annotations along with the rest of the metadata of the Places node, and they are immediately available when we initialize each entry. This //may// have performance implications if we don't use JOIN but we fetch again for each record. This was the reason why gCachedPlacesMetaData exists, but in this case we couldn't use JOIN in the first place because we operated with a static Places API, while here we can make all the changes we weed. In fact, I seem to remember there are plans to change how the data is stored in the database, so this may be a better API for the long term.
2. Without changing the Places API, initialize gCachedPlacesMetaData fully during the asynchronous initialization in getList, and then keep it up to date ourselves for the lifetime of the application. I believe this is now possible since DownloadHistory is now the only module that ever changes the metadata. It's interesting that in at least one of the code paths where we call getPlacesMetaDataFor we have just stored the metadata in the database, so there is no need for a round trip.
3. Keep the current code structure and apply all the changes to the list immediately like we do now, but start tasks to fetch the metadata in the background asynchronously. This has the disadvantage that new additions will be followed by a change notification later, and may cause visible glitches in the user interface.
Flags: needinfo?(mak77)
I replied in Phabricator. I missed the fact onDownloadBatchEnded was considered synchronous. In general it's not a good idea to make notifications depending on being executed in the same tick, it sounds like a design flaw :(

Anyway, 1 is not feasible and 3 looks like more complicate. 2 looks like the best bet.
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] (Away 9-26 Aug) from comment #4)
> In general it's not a good idea to make
> notifications depending on being executed in the same tick

It's necessary for performance when making DOM updates...

Also, synchronous iteration, which is in practice what is happening here, also simplifies the code a lot, since every API consumer doesn't have to deal with race conditions independently.
Depends on: 1485113
Paolo, did you want to review this again, or shall I see if I can take just Marco's review from before?
Flags: needinfo?(paolo.mozmail)
I think Marco's review from before is enough, this is mostly Places code. Thanks!
Flags: needinfo?(paolo.mozmail)
Pushed by
Remove nsIAnnotationService::getPageAnnotation. r=mak
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.