1458865 - Stop using PlacesUtils.asyncHistory in comm-central

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Marco Bonardo [:mak]]

There are quite a few consumers that should be converted to the new promise based APIs in History.jsm (PlacesUtils.history.insert and friends).
See the entries in mail/ and suite/

https://dxr.mozilla.org/comm-central/search?q=asyncHistory&redirect=false

Comment 1

[Marco Bonardo [:mak]]

For now I'll leave the entry in PlacesUtils just for non-firefox consumers, but that should really be removed, when this is fixed, since it may break in the future.

Comment 2

[Marco Bonardo [:mak]]

Ni Jorg to triage this properly. (not sure if you're the right person)

Comment 3

[Richard Marti (:Paenglab)]

Attachment: asyncHistory.patch

This is a patch for TB only based on https://hg.mozilla.org/integration/autoland/rev/718d6eca69f2.

Marco, please can you check if my changes look correct?

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 8972994 [details] [diff] [review]
asyncHistory.patch

I'm really not a good reviewer for this. Maybe Marco will be as kind as to take the review. Looking at the patch from bug 1354531, this looks about right ;-)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cec8d9a15994
Stop using PlacesUtils.asyncHistory in TB. rs=jorgk,bustage-fix

Comment 6

[Jorg K (CEST = GMT+2)]

Looks like this needs to be a "post landing review" since the M-C patch has already landed.

Comment 7

[Jorg K (CEST = GMT+2)]

The push is green, BTW :-)

Comment 8

[Marco Bonardo [:mak]]

I made the patch in bug 1354531 a comm-central friendly one, it's checking for firefox and removing the PlacesUtils getter only there, though this is necessary to remove it completely.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8972994 [details] [diff] [review]
asyncHistory.patch

Review of attachment 8972994 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, while it works, this is not the right fix, you should really stop using updatePlaces, isURIVisited and asyncHistory in mail/ and suite/.
The reason is that asyncHistory and its methods will become internal Places details and in the future we'll start changing them for perf reasons. The external API will be the one exposed through PlacesUtils.history.

You can replace all of your calls to updatePlaces with PlacesUtils.history.insert. It's quite similar to updatePlaces but has a nicer js API. It's a promise-based API, described in History.jsm. It has a few subtle differences in the arguments, it takes a url property (string or URL or nsiURI) and a visits array, each visit has a transition (PlacesUtils.history.TRANSITIONS.*) and a date (Date object).

Comment 10

[Marco Bonardo [:mak]]

isURIVisited can be replaced by PlacesUtils.history.hasVisits

Comment 11

[Marco Bonardo [:mak]]

Also, should I file a separate bug for Seamonkey?

Comment 12

[Richard Marti (:Paenglab)]

(In reply to Marco Bonardo [::mak] from comment #9)
> Comment on attachment 8972994 [details] [diff] [review]
> asyncHistory.patch
> 
> Review of attachment 8972994 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You can replace all of your calls to updatePlaces with
> PlacesUtils.history.insert. It's quite similar to updatePlaces but has a
> nicer js API. It's a promise-based API, described in History.jsm. It has a
> few subtle differences in the arguments, it takes a url property (string or
> URL or nsiURI) and a visits array, each visit has a transition
> (PlacesUtils.history.TRANSITIONS.*) and a date (Date object).

My knowledge is far too low for promises. Maybe aceman can look into it.

(In reply to Marco Bonardo [::mak] from comment #11)
> Also, should I file a separate bug for Seamonkey?

That would be better as they use places also for their browser part and I don't know if they have forked it or use the m-c one.

Comment 13

[Jorg K (CEST = GMT+2)]

Thanks Marco, we'll do a follow-up.

As for SM, it's usually sufficient to NI them.

Comment 14

[Marco Bonardo [:mak]]

Fwiw, I'm available to review the patch.

Comment 15

[:aceman]

(In reply to Pulsebot from comment #5)
> Pushed by [e-mail]:
> https://hg.mozilla.org/comm-central/rev/cec8d9a15994
> Stop using PlacesUtils.asyncHistory in TB. rs=jorgk,bustage-fix

Some of the touched files do no import XPCOMUtils so the call of it causes errors. Please check them and add the missing imports.

Comment 16

[Jorg K (CEST = GMT+2)]

I'll do it.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/758455ef0d7c
Follow-up: Import XPCOMUtils.jsm. r=me DONTBUILD

Comment 18

[:aceman]

Attachment: 1458865.patch - use PlacesUtils.history.insert

Marco, did you mean this?

Comment 19

[:aceman]

Attachment: 1458865.patch - use PlacesUtils.history.insert v1.1

Removed now unneded import.

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8973484 [details] [diff] [review]
1458865.patch - use PlacesUtils.history.insert v1.1

Review of attachment 8973484 [details] [diff] [review]:
-----------------------------------------------------------------

Almost like that, there are a few details to change in all the files

::: mail/base/content/contentAreaClick.js
@@ +177,4 @@
>    // This can fail if there is a problem with the places database.
>    try {
> +    PlacesUtils.history.insert({
> +      url: url,

just "url,", in ES6 you can use shorthands like this instead fo repeating "url: url,"

@@ +178,5 @@
>    try {
> +    PlacesUtils.history.insert({
> +      url: url,
> +      visits: [{
> +        date: Date.now() * 1000,

just "date: new Date()," the neew API doesn't need PRTime microseconds

@@ +179,5 @@
> +    PlacesUtils.history.insert({
> +      url: url,
> +      visits: [{
> +        date: Date.now() * 1000,
> +        transition: PlacesUtils.history.TRANSITION_LINK

omit this, LINK is the default transition.

@@ +184,5 @@
>        }]
>      });
>    } catch (ex) {
>      Cu.reportError(ex);
>    }

This doesn't work as you expect, because insert is a promise-based API, you should instead

PlacesUtils.history.insert({
  ...
}).catch(Cu.reportError);

Comment 21

[:aceman]

Attachment: 1458865.patch - use PlacesUtils.history.insert v2

Thanks.

Comment 22

[Marco Bonardo [:mak]]

Comment on attachment 8973726 [details] [diff] [review]
1458865.patch - use PlacesUtils.history.insert v2

Review of attachment 8973726 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks!

I will file a separate bug for the suite/ part.

::: mail/base/content/contentAreaClick.js
@@ +179,5 @@
>      uri = Services.io.newURI(url);
>  
>    // This can fail if there is a problem with the places database.
> +  PlacesUtils.history.insert({
> +    url: uri.spec,

nit: just pass "url,"

Comment 23

[Frank-Rainer Grahl (:frg)]

> I will file a separate bug for the suite/ part.

Sorry was somewhat busy. I just ported the Fx Library over to 60. I still miss some fixes and will put this one in a followup patch. Tests in suite are generally and unfortunately broken and need someone who has much time on hand :( So if the code is only in tests don't bother. I didn't even look at them doing the last patches.

Comment 24

[:aceman]

(In reply to Marco Bonardo [::mak] from comment #22)
> ::: mail/base/content/contentAreaClick.js
> @@ +179,5 @@
> >      uri = Services.io.newURI(url);
> >  
> >    // This can fail if there is a problem with the places database.
> > +  PlacesUtils.history.insert({
> > +    url: uri.spec,
> 
> nit: just pass "url,"

In this function url is string or nsIURI. So it gets converted to nsIURI when needed and then I need uri.spec.

Comment 25

[Marco Bonardo [:mak]]

(In reply to :aceman from comment #24)
> In this function url is string or nsIURI. So it gets converted to nsIURI
> when needed and then I need uri.spec.

url is still a valid value, even after you call newURI on it, and since the API accepts anything, you can just pass url. It has no advantages apart from code compacting.

Comment 26

[Marco Bonardo [:mak]]

(In reply to Frank-Rainer Grahl (:frg) from comment #23)
> > I will file a separate bug for the suite/ part.
> 
> Sorry was somewhat busy. I just ported the Fx Library over to 60. I still
> miss some fixes and will put this one in a followup patch. Tests in suite
> are generally and unfortunately broken and need someone who has much time on
> hand :( So if the code is only in tests don't bother. I didn't even look at
> them doing the last patches.

no worries, I filed bug 1459855 for Seamonkey.

Comment 27

[:aceman]

How can the API accept anything? I do not understand. Where is an occurrence in m-c that would take a nsIURI for the url member?

Comment 28

[Frank-Rainer Grahl (:frg)]

> no worries, I filed bug 1459855 for Seamonkey.

Thanks. Seeing only 4 occurences left in test so no need to block.

Comment 29

[Marco Bonardo [:mak]]

(In reply to :aceman from comment #27)
> How can the API accept anything? I do not understand. Where is an occurrence
> in m-c that would take a nsIURI for the url member?

PlacesUtils.history.insert accepts either a string, an nsIURI or an URL, because it's not XPCOM, it's just javascript.
https://searchfox.org/mozilla-central/rev/f30847c12e5fb13791401ed4330ced3e1b9c8d81/toolkit/components/places/History.jsm#17

Comment 30

[Jorg K (CEST = GMT+2)]

Bystander's question: What's a URL in that context, I see: |filter.url instanceof URL|? That's an nsIURL?

Comment 31

[:aceman]

Attachment: 1458865.patch - use PlacesUtils.history.insert v2.1

OK, thanks.

Comment 32

[Marco Bonardo [:mak]]

https://developer.mozilla.org/en-US/docs/Web/API/URL/URL

Comment 33

[Jorg K (CEST = GMT+2)]

Try run? Ready to go?

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/28fb09316488
migrate from asyncHistory.updatePlaces to PlacesUtils.history.insert in Thunderbird. r=mak