Closed
Bug 1458865
Opened 7 years ago
Closed 7 years ago
Stop using PlacesUtils.asyncHistory in comm-central
Categories
(Thunderbird :: General, defect)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 62.0
People
(Reporter: mak, Assigned: Paenglab)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
mak
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
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•7 years ago
|
||
Ni Jorg to triage this properly. (not sure if you're the right person)
Flags: needinfo?(jorgk)
Reporter | ||
Updated•7 years ago
|
Assignee | ||
Comment 3•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
Flags: needinfo?(jorgk)
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: 7 years ago
Resolution: --- → FIXED
Comment 6•7 years ago
|
||
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•7 years ago
|
||
The push is green, BTW :-)
Reporter | ||
Comment 8•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 10•7 years ago
|
||
isURIVisited can be replaced by PlacesUtils.history.hasVisits
Reporter | ||
Comment 11•7 years ago
|
||
Also, should I file a separate bug for Seamonkey?
Assignee | ||
Comment 12•7 years ago
|
||
(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)
Comment 13•7 years ago
|
||
Thanks Marco, we'll do a follow-up.
As for SM, it's usually sufficient to NI them.
Flags: needinfo?(frgrahl)
Reporter | ||
Comment 14•7 years ago
|
||
Fwiw, I'm available to review the patch.
Comment 15•7 years ago
|
||
(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)
Comment 16•7 years ago
|
||
I'll do it.
Comment 17•7 years ago
|
||
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: 7 years ago → 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 19•7 years ago
|
||
Removed now unneded import.
Attachment #8973473 -
Attachment is obsolete: true
Attachment #8973473 -
Flags: review?(mak77)
Attachment #8973484 -
Flags: review?(mak77)
Reporter | ||
Comment 20•7 years ago
|
||
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•7 years ago
|
||
Thanks.
Attachment #8973484 -
Attachment is obsolete: true
Attachment #8973726 -
Flags: review?(mak77)
Reporter | ||
Comment 22•7 years ago
|
||
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+
Comment 23•7 years ago
|
||
> 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•7 years ago
|
||
(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•7 years ago
|
||
(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•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
> 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•7 years ago
|
||
(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•7 years ago
|
||
Bystander's question: What's a URL in that context, I see: |filter.url instanceof URL|? That's an nsIURL?
Comment 31•7 years ago
|
||
OK, thanks.
Attachment #8973726 -
Attachment is obsolete: true
Attachment #8973963 -
Flags: review+
Reporter | ||
Comment 32•7 years ago
|
||
Comment 33•7 years ago
|
||
Try run? Ready to go?
Comment 34•7 years ago
|
||
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: 7 years ago → 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Target Milestone: Thunderbird 61.0 → Thunderbird 62.0
You need to log in
before you can comment on or make changes to this bug.
Description
•