Quoting mak in bug 699850: « A first dependent bug may be to deprecate addVisit and addPageWithDetails, that are used in a LOT of tests, to avoid rewriting all the tests we can likely write an addVisit wrapper that uses updatePlaces async API and spin the events loop till we get the callback. We should push people toward updatePlaces API. » Let's do it.

http://mxr.mozilla.org/mozilla-central/search?string=addPageWithDetails mostly tests for this api. other consumers are IE, Opera and Safari migrators.

moved the tests stuff to bug 724475, so this tracks all the needed work.

Seems that addPageWithDetails is covered. I will take a (new) look at addVisit.

A quick search indicates that we use addVisit in a gazillion tests and exactly once in code, in Sync http://dxr.lanedo.com/mozilla-central/services/sync/tps/extensions/tps/modules/history.jsm.html#l106 . This occurrence looks easy to fix.

Comment on attachment 639293 [details] [diff] [review] Marking addVisit as deprecated Let's forget about this patch for the time being. I think I have improperly identified the implementation of addVisit.

Comment on attachment 639294 [details] [diff] [review] Removing client of addVisit in the code You should put this patch in a new bug in the proper component (Mozilla Services::Firefox Sync: Build, maybe?). AIUI this code is used for Sync's test harness (TPS), and :gps should probably look it over.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9) > Comment on attachment 639294 [details] [diff] [review] > Removing client of addVisit in the code > > You should put this patch in a new bug in the proper component (Mozilla > Services::Firefox Sync: Build, maybe?). AIUI this code is used for Sync's > test harness (TPS), and :gps should probably look it over. Done, thanks for the suggestion.

David, Paolo started doing a bunch of rewrites to remove AddVisit from tests, that partially overloads what you were planning to do, so I think would be cool if you could sync up and coordinate to bring on the effort. I honestly don't mind to much about test changes being reviewed by an old Places peer, since it's simple xpcshell tests mostly, so I'd also be fine if you'd want to review each other and I can do later checks.

Comment on attachment 639297 [details] [diff] [review] Marking addVisit as deprecated Review of attachment 639297 [details] [diff] [review]: ----------------------------------------------------------------- You need SR on the deprecation, I'm fine doing it asap, so there will be some time before we completely remove the method. ::: toolkit/components/places/nsNavHistory.cpp @@ +1289,5 @@ > + > + NS_WARNING("nsINavHistory::addVisit is deprecated. Please use rather mozIAsyncHistory::updatePlace"); > +#if defined(DEBUG) > + DumpJSStack(); > +#endif // defined(DEBUG) Unfortunately I really don't think any js consumer (like add-on devs) runs code in debug builds, they should ideally but let's stick to the reality. So personally I'd leave this additional output out and just do the deprecation marking in the idl. Notifying Jorge to send an e-mail to all add-on authors using this method would be largely more productive.

Comment on attachment 639297 [details] [diff] [review] Marking addVisit as deprecated Review of attachment 639297 [details] [diff] [review]: ----------------------------------------------------------------- ops, was meant to be a +, with removal of the debug warning ::: toolkit/components/places/nsNavHistory.cpp @@ +1289,5 @@ > + > + NS_WARNING("nsINavHistory::addVisit is deprecated. Please use rather mozIAsyncHistory::updatePlace"); > +#if defined(DEBUG) > + DumpJSStack(); > +#endif // defined(DEBUG) Unfortunately I really don't think any js consumer (like add-on devs) runs code in debug builds, they should ideally but let's stick to the reality. So personally I'd leave this additional output out and just do the deprecation marking in the idl. Notifying Jorge to send an e-mail to all add-on authors using this method would be largely more productive.

(In reply to Marco Bonardo [:mak] (Away 28 Jul - 12 Aug) from comment #11) > David, Paolo started doing a bunch of rewrites to remove AddVisit from > tests, that partially overloads what you were planning to do, so I think > would be cool if you could sync up and coordinate to bring on the effort. I > honestly don't mind to much about test changes being reviewed by an old > Places peer, since it's simple xpcshell tests mostly, so I'd also be fine if > you'd want to review each other and I can do later checks. I've almost finished to convert all the addVisit calls by now, only a few tests remain, which can be handled in a couple of hours. David, I think you'll be a good reviewer for the changes I made, as you're very familiar with the Promise module and should know the Task module. I'll redirect feedback requests to you, let me know if you think you can handle them.

After too much time, I gave up on reimplementing AddVisit in C++ and I just wrote a short layer in JS. Mak, would a quick sed to use this function be sufficient for the needs of this bug?

All call to addVisits should have been already removed from Places tests as part of the work on bug 775489, so, fortunately, we shouldn't need to spin the event loop for that anymore. Work in those bugs also fixed the rare test cases where the behavior of the asynchronous API is actually different, because the order of notifications is different or because errors are notified in a different moment.

I mean, calls to AddVisit have been replaced with the addVisits helper.

I suspected so, but I had this half-coded hack I wanted to finish, in case it could prove useful :)

Comment on attachment 650889 [details] [diff] [review] Dirty implementation of addVisit in JS clearing request until we figure if it's needed. provided Paolo's patches cover most/all uses we can probably skip this.

Sounds like a plan.

did this project hang? anything I can do to help it proceed? We definitely need to get rid of addvisit ASAP or we can't proceed with async history.

(In reply to Marco Bonardo [:mak] from comment #21) > did this project hang? anything I can do to help it proceed? We definitely > need to get rid of addvisit ASAP or we can't proceed with async history. It did hang actually, but is restarting right now. Expect updated patches for removing calls to addVisit from Places tests (dependencies of bug 775489) later this week! The work that remains to be done there is mainly a clean up, I expect that the currently attached patches will be similar to the updated version.

Since I'm not working on this, releasing the bug.

Thanks for your effort David, looks like in the end we are not that far from the target!

fixed by dependencies. Notice this fixed all of the in-tree callers, but the APIs have not been removed yet, they will be removed apart in another dependency of bug 834457.