700250 - Deprecate synchronous methods to add a visit (addVisit, addPageWithDetails, addURI)

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 1

[Dietrich Ayala (:dietrich)]

http://mxr.mozilla.org/mozilla-central/search?string=addPageWithDetails

mostly tests for this api. other consumers are IE, Opera and Safari migrators.

Comment 2

[Marco Bonardo [:mak]]

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

Comment 3

[David Teller [:Yoric] - still alive but not very active]

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

Comment 4

[David Teller [:Yoric] - still alive but not very active]

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 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Marking addVisit as deprecated

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Removing client of addVisit in the code

Comment 7

[David Teller [:Yoric] - still alive but not very active]

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 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Marking addVisit as deprecated

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 11

[Marco Bonardo [:mak]]

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 12

[Marco Bonardo [:mak]]

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 13

[Marco Bonardo [:mak]]

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.

Comment 14

[:Paolo Amadini]

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

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Attachment: Dirty implementation of addVisit in JS

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?

Comment 16

[:Paolo Amadini]

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.

Comment 17

[:Paolo Amadini]

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

Comment 18

[David Teller [:Yoric] - still alive but not very active]

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

Comment 19

[Marco Bonardo [:mak]]

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.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Sounds like a plan.

Comment 21

[Marco Bonardo [:mak]]

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.

Comment 22

[:Paolo Amadini]

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

Comment 23

[David Teller [:Yoric] - still alive but not very active]

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

Comment 24

[Marco Bonardo [:mak]]

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

Comment 25

[Marco Bonardo [:mak]]

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.