Stop using PlacesUtils.asyncHistory in comm-central

RESOLVED FIXED in Thunderbird 62.0

Status

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: mak, Assigned: Paenglab)

Tracking

unspecified
Thunderbird 62.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

Last year
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
Reporter

Comment 1

Last year
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.
Reporter

Comment 2

Last year
Ni Jorg to triage this properly. (not sure if you're the right person)
Flags: needinfo?(jorgk)
Reporter

Updated

Last year
No longer blocks: 1354531
Depends on: 1354531
Assignee

Comment 3

Last year
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?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8972994 - Flags: review?(jorgk)
Attachment #8972994 - Flags: feedback?(mak77)

Comment 4

Last year
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 ;-)
Attachment #8972994 - Flags: review?(mak77)
Attachment #8972994 - Flags: review?(jorgk)
Attachment #8972994 - Flags: feedback?(mak77)

Updated

Last year
Flags: needinfo?(jorgk)

Comment 5

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/cec8d9a15994
Stop using PlacesUtils.asyncHistory in TB. rs=jorgk,bustage-fix
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED

Comment 6

Last year
Looks like this needs to be a "post landing review" since the M-C patch has already landed.
Target Milestone: --- → Thunderbird 61.0

Comment 7

Last year
The push is green, BTW :-)
Reporter

Comment 8

Last year
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.
Reporter

Comment 9

Last year
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).
Attachment #8972994 - Flags: review?(mak77) → review-
Reporter

Updated

Last year
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter

Comment 10

Last year
isURIVisited can be replaced by PlacesUtils.history.hasVisits
Reporter

Comment 11

Last year
Also, should I file a separate bug for Seamonkey?
Assignee

Comment 12

Last year
(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.
Flags: needinfo?(acelists)
Thanks Marco, we'll do a follow-up.

As for SM, it's usually sufficient to NI them.
Flags: needinfo?(frgrahl)
Reporter

Comment 14

Last year
Fwiw, I'm available to review the patch.

Comment 15

Last year
(In reply to Pulsebot from comment #5)
> Pushed by mozilla@jorgk.com:
> 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.
Flags: needinfo?(acelists)
I'll do it.

Comment 17

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/758455ef0d7c
Follow-up: Import XPCOMUtils.jsm. r=me DONTBUILD
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED

Updated

Last year
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 18

Last year
Marco, did you mean this?
Attachment #8973473 - Flags: review?(mak77)

Comment 19

Last year
Removed now unneded import.
Attachment #8973473 - Attachment is obsolete: true
Attachment #8973473 - Flags: review?(mak77)
Attachment #8973484 - Flags: review?(mak77)
Reporter

Comment 20

Last year
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);
Attachment #8973484 - Flags: review?(mak77) → review-

Comment 21

Last year
Thanks.
Attachment #8973484 - Attachment is obsolete: true
Attachment #8973726 - Flags: review?(mak77)
Reporter

Comment 22

Last year
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,"
Attachment #8973726 - Flags: review?(mak77) → review+
Reporter

Updated

Last year
Blocks: 1459856
> 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

Last year
(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.
Reporter

Comment 25

Last year
(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.
Reporter

Comment 26

Last year
(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

Last year
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?
> no worries, I filed bug 1459855 for Seamonkey.

Thanks. Seeing only 4 occurences left in test so no need to block.
Flags: needinfo?(frgrahl)
Reporter

Comment 29

Last year
(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
Bystander's question: What's a URL in that context, I see: |filter.url instanceof URL|? That's an nsIURL?

Comment 31

Last year
OK, thanks.
Attachment #8973726 - Attachment is obsolete: true
Attachment #8973963 - Flags: review+
Try run? Ready to go?

Comment 34

Last year
Pushed by acelists@atlas.sk:
https://hg.mozilla.org/comm-central/rev/28fb09316488
migrate from asyncHistory.updatePlaces to PlacesUtils.history.insert in Thunderbird. r=mak
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED

Updated

Last year
Target Milestone: Thunderbird 61.0 → Thunderbird 62.0
You need to log in before you can comment on or make changes to this bug.