to properly remove batches and handle cases like bug 913160 we want batch notifications that allow views to make smarter choices.

Ignore the comments completely (I just copied over onDeleteURI's documentation) and the unintended whitespace changes.

And, of course, I'll need to add onDeletePages to all those js-observers out there.

Also ignore that |dump| I left there.

Comment on attachment 8359726 [details] [diff] [review] wip Review of attachment 8359726 [details] [diff] [review]: ----------------------------------------------------------------- Yes, this is more or less the idea, the patch clearly needs cleanup, is not much readable in the current state. You should also handle any other callpoints invoking onDeleteURIs, like for example NotifyOnPageExpired ::: toolkit/components/places/nsINavHistoryService.idl @@ +701,5 @@ > * The unique ID associated with the page. > * @param aReason > * Indicates the reason for the removal. see REASON_* constants. > */ > + void onDeletePages([array, size_is(aLength)] in mozIPlaceInfo aPages, in unsigned long aLength, in unsigned short aReason); once you write the documentation, we should add a very visible warning that consumers should not handle both this and onDeleteURIs, or they will get duplicated notifications. and we want to add @deprecated to onDeleteURI (actually we may want a separate bug for this, since it would be nice first to notify about the change, and then to add a console warning when we call consumers of it, so I'd leave it for a follow-up) ::: toolkit/components/places/nsNavHistory.cpp @@ +2355,5 @@ > + NS_ENSURE_TRUE(infos, NS_ERROR_OUT_OF_MEMORY); > + > + for (int32_t i = 0; i < deleteURIsInfos.Count(); i ++) { > + (infos[i]) = deleteURIsInfos[i]; > + NS_ADDREF(infos[i]); don't we have a better auto array that addrefs its contents, rather than manually allocating and addrefing? This sounds a bit strange...

Marco, do you agree to remove placeId from mozIPlaceInfo? It forces either an API complication (returning -1 if it's not available) or a performance issue (I don't have it handy in NotifyOnPageExpired). It's only used in tests, and to be frank, it should have never found its way to the interface given that places-ids are completely internal.

Actually... the same complication/performance issue applies to the title property. Nevertheless, I think placeId should be removed.

For title, I'll just add that to nsPIPlacesHistoryListenersNotifier::NotifyOnPageExpired. It's an internal interface so I don't think that's a problem (yes, I realize the same argument could be applied to placeId).

(In reply to Mano from comment #6) > Marco, do you agree to remove placeId from mozIPlaceInfo? It forces either > an API complication (returning -1 if it's not available) or a performance > issue (I don't have it handy in NotifyOnPageExpired). > > It's only used in tests, and to be frank, it should have never found its way > to the interface given that places-ids are completely internal. which kind of testing do we make with it? I'm fine with killing it and replacing "good" testing using it with url matching (select by url). by good testing I mean something checking more than "this is is a valid number"

So here is what we have. In test_async_history_api.js: 1. a test that ensures updatePlaces ignores placeId as as an input parameter (by checking that the "returned" placeId is different). 2. a test that ensures that "returned" placeId is set to a valid number (> 0) 3. a test that ensures placeId is set to 0 for EMBED visits 3.1. a test that checks the other way around (> 0 for non-embed). In test_getPlacesInfo.js: 1. a test that checks all properties, including placeId, are set to the same values whether you use uri or guid as the input for getPlacesInfo Nothing here seems particularly interesting, imo. _____ Tough life, comment 8 is wrong: places expiration doesn't join with moz_places in its queries, so the title is not available free of charge... What should I do?

> In test_async_history_api.js: > 1. a test that ensures updatePlaces ignores placeId as as an input parameter > (by checking that the "returned" placeId is different). can skip > 2. a test that ensures that "returned" placeId is set to a valid number (> 0) can skip > 3. a test that ensures placeId is set to 0 for EMBED visits we may replace this by checking the page url doesn't appear in the DB (there should be an helper already in head_common > 3.1. a test that checks the other way around (> 0 for non-embed). ditto > In test_getPlacesInfo.js: > 1. a test that checks all properties, including placeId, are set to the same > values whether you use uri or guid as the input for getPlacesInfo can skip > Tough life, comment 8 is wrong: places expiration doesn't join with > moz_places in its queries, so the title is not available free of charge... > What should I do? may you better explain me what's the title for?

> may you better explain me what's the title for? We "return" a place info object, and that interface exposes a "title" attribute. The title is available for us in all cases but the expiration-notification case. We could make "title" behave like visits and frecency, which are sometimes unavailable, but I would really like to avoid that.

I think the use cases for needing the title when you are notified a page is going away are so limited that we may make the title "unavailable" in this case. For how much that sucks, it doesn't break important use-cases.

Ok. I'll do that so we can move on. I should probably file a bug on mozIPlaceInfo design in general given how it gets a bit more stink every time it's adopted. Basically, every mozIPlaceInfo occurrence in the API has its own semantics. I think it wouldn't be as bad if it was a plain js object (in which case one could do something like Object.hasOwnProperty(info, "title"/"frecency"/"visits")... just as it is now when it's used as the input for updatePlaces.

While this works and the various tests pass, and while it's probably worth landing anyway, for the sake of having the API in place, this really isn't what we hoped for. Here's the short story: Apart removals done by synchronous removePages, which is going to be deprecated pretty soon, batch-removals will notify as multiple calls to onDeletePages with a single page. That's because both History.jsm and expiration use the notifyOnPageExpired "private" API for notifying on removals. As it name suggests, this method works with one page at a time. We cannot just modify it to take a place-infos array, because of the wholeEntry argument (which controls whether it's onDeletePages or onDeleteVisits that's to be called). I think that as we just did (or about to do) with bookmarks, the solution here is to introduce history.observers, and duplicate notifyOnPageExpired's logic in History.jsm, and probably in the expiration component too. However, to make things even more complicated, notifyOnPageExpired also invalidates mDaysOfHistory... so we need a way to tell the cpp History component to do that. All of the above is quite a bit of work and this patch is getting pretty large already. While not so helpful as it is, I think it's worth landing as a good first step. Consumers (including addons) can adopt it already. They will just be notified with a single page most of the time, until we fix that.

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #11) > > 3. a test that ensures placeId is set to 0 for EMBED visits > > we may replace this by checking the page url doesn't appear in the DB (there > should be an helper already in head_common > > > 3.1. a test that checks the other way around (> 0 for non-embed). > > ditto Didn't do that, because the tests add the same url in other transaction types (so page_in_database is going to return true anyway). However, the remaining null-guid check should be enough, IMO.

(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #15) > I think that as we just did (or about to do) with bookmarks, the solution > here is to introduce history.observers, and duplicate notifyOnPageExpired's > logic in History.jsm, and probably in the expiration component too. Bug 1089332.

*** If Marco agrees that the patch is worth landing in the current situation as detailed above. I should file the following bugs (both are to be dependencies of bug 1089332): * History.jsm's remove API for multiple pages doesn't batch onDeletePages notifications (onDeletePages is called for each page) * Same goes for the nsPlacesExpiration case. I should also file a bug on finding a solution for mDaysOfHistory invalidation on "external" removal (that's currently done by NotifyOnPageExpired). *** One thing that worries me is that onDeletePages (like onDeleteURI) is supposed to be called only for unbookmarked pages (and soon, untagged pages). For a bookmarked-page, onDeleteVisits, a single-page API, is supposed to be called. This means that, if removePages is called with 4 pages, 2 of which are bookmarked and 2 which are not, we'd notify a single onDeletePages notification (for the two unbookmarked pages) and two onDeleteVisits notifications (for the bookmarked pages). This is somewhat ugly. I wonder if we could somehow take care of onDeleteVisits in onDeletePages.

(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #18) > I should also file a bug on finding a solution for mDaysOfHistory > invalidation on "external" removal (that's currently done by > NotifyOnPageExpired). it's sort of crazy, but we could create a trigger on moz_historyvisits that calls a custom function that dispatches a navhistory method on the main-thread to invalidate the cache. one day hasHistoryEntries and queries will be async and the cache could be removed or simplified.

Comment on attachment 8511616 [details] [diff] [review] disappointing patch clearing request based on IRC discussions We will try to coalesce notifications into onDeletePages to simplify their usage

I'm going to finish this bug in the next couple of days, now that bug 1089332 is fixed we'll actually have a final implementation here rather than a "prepare the ground" sort-of patch.

I made a mistake here. We keep carrying this over for no good reason. That is, it's carried over because almost all Places work that's done at the moment is more important than this nice-to-have API... and I keep adding additional bugs, silently ignoring this bug. So, while I'm pretty sure I'm going to get this done backloged-or-not before the end of this year, I suggest to drop this from the backlog for the next iteration to reflect reality. If the dialog patch is done by mid-iteration, and no major work comes up, I may retake this.

Removed from IT 37.1

We should rather do bug 1340498