Just as we have Promise-based Bookmarks.jsm (proxified through PlacesUtils.jsm), let's get started with History.jsm.

As discussed over IRC, here's a skeleton History.jsm.

Comment on attachment 8495163 [details] [diff] [review] Introducing History.jsm Review of attachment 8495163 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/History.jsm @@ +4,5 @@ > + > +"use strict"; > + > +/** > + * This module provides an asynchronous API for managing history. you should also document how a place object is defined, which properties it has and what they contain. Ideally when a developer looks at the javadoc here, he should know everything about how to use this and what to expect. @@ +27,5 @@ > + > +let History = Object.freeze({ > + /** > + * Gets the available information for the given array of places, each > + * identified by either URL object or places GUID (string). what is an URL object? this is basically a shortcut to getPlacesInfo, so it shoudl accept a single guid, a single url, a single nsIURI, or an array of any of those (this will internally convert string url to nsIURIs before passing them to getPlacesInfo) @@ +32,5 @@ > + * > + * @param {Array<URI|string>} places An array of URLs and/or GUIDs. > + * @return {Promise<Array<VisitInfo>>} An array with the same size as `places`. If > + * a URL or GUID cannot be found, the item is `null`. Note that the VisitInfo > + * do NOT contain the visit data (i.e. `visits` is `null`). "The VisitInfo" is not documented fwiw here we need self-contained documentation, we should not refer to mozIAsyncHistory (I think we'll convert some more cpp code to js in future here) this should also specify a @throws (it will throw a js exception if input is invalid) @@ +40,5 @@ > + }, > + > + /** > + * Adds a set of visits for one or more PlaceInfo objects, and updates > + * the fields. should be a little more specific about which fields and what's the behavior (for example what happens when passing a null or empty title? Are all properties mandatory? which are?) @@ +42,5 @@ > + /** > + * Adds a set of visits for one or more PlaceInfo objects, and updates > + * the fields. > + * > + * @param {Array<PlaceInfo>} infos An array of places, as identified by this can either be a single place or an array of places. url of guid is fine, I think we should support URL, nsIURI or GUID, doing the url to nsIURI conversion internally before handling to updatePlaces. fwiw, if possible in this file I'd avoid calling these things "places" and I'd rather refer to page entries and visit entries, documenting above how these objects are defined. @@ +46,5 @@ > + * @param {Array<PlaceInfo>} infos An array of places, as identified by > + * their URL or GUID. All the fields specified are updated in the database. > + * @return {Promise<Array<PlaceInfo|null>>} An array of PlaceInfo objects, > + * containing the full information on the place, or `null` if the place > + * could not be found. needs a @throws as well It would also tell changes will notify through nsINavHistoryObserver (See how we documented that in Bookmarks.jsm) @@ +55,5 @@ > + > + /** > + * Remove visits from the database. > + * > + * @param {Array<URI|string>} places An array of URLs and/or GUIDs. should work as fetch: URL, GUID, nsIURI or an array of any of them. @@ +57,5 @@ > + * Remove visits from the database. > + * > + * @param {Array<URI|string>} places An array of URLs and/or GUIDs. > + * @return {Promise<Array<VisitInfo>>} An array with the same size as `places`. If > + * a URL or GUID cannot be found, the item is `null`. ditto (same as update) @@ +77,5 @@ > + * Information on a visit to a Place. > + * > + * @constructor > + */ > + VisitInfo: function() { we don't need to expose PlacesInfo or VisitInfo, they will just be plain js objects, whose properties are documented at the top of this module ::: toolkit/components/places/PlacesUtils.jsm @@ +1817,5 @@ > }); > > +XPCOMUtils.defineLazyGetter(PlacesUtils, "asyncHistory", () => { > + let bm = Cc["@mozilla.org/browser/history;1"] > + .getService(Ci.nsINavBookmarksService); you should proxy history, not asyncHistory... and you should not getService nsINavBookmarksService, rather nsINavHistoryService. (ps also bm was for bookmarks, so not great naming here.)

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #2) > ::: toolkit/components/places/History.jsm > @@ +4,5 @@ > > + > > +"use strict"; > > + > > +/** > > + * This module provides an asynchronous API for managing history. > > you should also document how a place object is defined, which properties it > has and what they contain. > > Ideally when a developer looks at the javadoc here, he should know > everything about how to use this and what to expect. Fair enough, I am missing the documentation on PlaceInfo and VisitInfo. > @@ +27,5 @@ > > + > > +let History = Object.freeze({ > > + /** > > + * Gets the available information for the given array of places, each > > + * identified by either URL object or places GUID (string). > > what is an URL object? https://developer.mozilla.org/en-US/docs/Web/API/URL.URL Wherever possible, we are trying to get rid of nsIURI in JS, in favor of the simpler and faster URL. > this is basically a shortcut to getPlacesInfo, so it shoudl accept a single > guid, a single url, a single nsIURI, or an array of any of those (this will > internally convert string url to nsIURIs before passing them to > getPlacesInfo) Frankly, I would prefer starting with the simplest/most orthogonal API. Turning single items into arrays doesn't simplify client code but it complicates documentation, implementation and testing. > this should also specify a @throws (it will throw a js exception if input is > invalid) ok > > @@ +40,5 @@ > > + }, > > + > > + /** > > + * Adds a set of visits for one or more PlaceInfo objects, and updates > > + * the fields. > > should be a little more specific about which fields and what's the behavior > (for example what happens when passing a null or empty title? Are all > properties mandatory? which are?) I am trying to reverse-engineer this from the code. I was planning to update this with more details in successive versions. > > @@ +42,5 @@ > > + /** > > + * Adds a set of visits for one or more PlaceInfo objects, and updates > > + * the fields. > > + * > > + * @param {Array<PlaceInfo>} infos An array of places, as identified by > > this can either be a single place or an array of places. url of guid is > fine, I think we should support URL, nsIURI or GUID, doing the url to nsIURI > conversion internally before handling to updatePlaces. I think we should strive to get rid of nsIURI in new JS APIs now that we have a better alternative. > fwiw, if possible in this file I'd avoid calling these things "places" and > I'd rather refer to page entries and visit entries, documenting above how > these objects are defined. Good idea. > @@ +46,5 @@ > > + * @param {Array<PlaceInfo>} infos An array of places, as identified by > > + * their URL or GUID. All the fields specified are updated in the database. > > + * @return {Promise<Array<PlaceInfo|null>>} An array of PlaceInfo objects, > > + * containing the full information on the place, or `null` if the place > > + * could not be found. > > needs a @throws as well > > It would also tell changes will notify through nsINavHistoryObserver (See > how we documented that in Bookmarks.jsm) > > @@ +55,5 @@ > > + > > + /** > > + * Remove visits from the database. > > + * > > + * @param {Array<URI|string>} places An array of URLs and/or GUIDs. > > should work as fetch: URL, GUID, nsIURI or an array of any of them. > > @@ +57,5 @@ > > + * Remove visits from the database. > > + * > > + * @param {Array<URI|string>} places An array of URLs and/or GUIDs. > > + * @return {Promise<Array<VisitInfo>>} An array with the same size as `places`. If > > + * a URL or GUID cannot be found, the item is `null`. > > ditto (same as update) > > @@ +77,5 @@ > > + * Information on a visit to a Place. > > + * > > + * @constructor > > + */ > > + VisitInfo: function() { > > we don't need to expose PlacesInfo or VisitInfo, they will just be plain js > objects, whose properties are documented at the top of this module My reasoning was that exposing PlacesInfo and VisitInfo: - provides a natural place to attach documentation; - makes it easier to document e.g. return values (`Promise<Array<VisitInfo>>` makes for something much clearer than `Promise<Array<Object>>`). > ::: toolkit/components/places/PlacesUtils.jsm > @@ +1817,5 @@ > > }); > > > > +XPCOMUtils.defineLazyGetter(PlacesUtils, "asyncHistory", () => { > > + let bm = Cc["@mozilla.org/browser/history;1"] > > + .getService(Ci.nsINavBookmarksService); > > you should proxy history, not asyncHistory... Oh, really? My bad. > and you should not getService nsINavBookmarksService, rather > nsINavHistoryService. > > (ps also bm was for bookmarks, so not great naming here.) Oops.

Does this match better what you had in mind?

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #3) > https://developer.mozilla.org/en-US/docs/Web/API/URL.URL > > Wherever possible, we are trying to get rid of nsIURI in JS, in favor of the > simpler and faster URL. nice, but we can't have half API using something and half something else and have to go through tens of conversions everywhere. If we want to add URL to the list of accepted inputs I'm fine with that, but we will keep needing nsIURI and url string for compatibility with the other Places APIs. > Frankly, I would prefer starting with the simplest/most orthogonal API. > Turning single items into arrays doesn't simplify client code but it > complicates documentation, implementation and testing. Unfortunately accepting both single item or array largely simplifies our internal code and tests, that's why it was done that way originally. Note that the current API already does that so I'm not asking you to change the behavior. > It largely simplifies our internal code already, from tests to views. > I think we should strive to get rid of nsIURI in new JS APIs now that we > have a better alternative. I surely agree, but here we need this API to speak with all of the others we have who speak either nsIURI or url string. In future when most APIs will accept url strings we can get rid of nsIURI support. > > we don't need to expose PlacesInfo or VisitInfo, they will just be plain js > > objects, whose properties are documented at the top of this module > > My reasoning was that exposing PlacesInfo and VisitInfo: > - provides a natural place to attach documentation; > - makes it easier to document e.g. return values > (`Promise<Array<VisitInfo>>` makes for something much clearer than > `Promise<Array<Object>>`). Yes but we don't need to enforce those objects to consumers, a js object is a js object, can be build in many ways (Object.create, Object.assign...) providing "constructors" will just make things harder for the consumer. If you want to structure the documentation as an object that's fine, provided it's clear it's not a real object one needs to work with the API (commented out object maybe?).

Comment on attachment 8495574 [details] [diff] [review] Introducing History.jsm, v2 Review of attachment 8495574 [details] [diff] [review]: ----------------------------------------------------------------- better, but not yet. ::: toolkit/components/places/History.jsm @@ +6,5 @@ > + > +/** > + * This module provides an asynchronous API for managing history. > + * > + * See the definition of VisitInfo and PageInfo for more details. you are not saying WHERE those definitions are. I'd love if they would be here, like: * These APIs work with PageInfo and VisitInfo objects. * * A PageInfo object has this structure: * PageInfo = { * guid: (string) * The globally unique indentifier of this page. * id: (number) * ... * uri: * ... * } * * A VisitInfo object has this structure: * ... @@ +36,5 @@ > + /** > + * Gets the available information for the given array of pages, each > + * identified by either DOM URL or GUID (string). > + * > + * @param {Array<URL|string>} pages An array of URLs and/or GUIDs. please make the required changes to support single item OR array and support nsIURIs @@ +41,5 @@ > + * @return {Promise<Array<PageInfo>|null>} An array with the same > + * size as `pages`. If a URL or GUID cannot be found, the > + * item is `null`. Note that the PageInfo item > + * does NOT contain the visit data (i.e. `visits` is `null`). > + * @throws {TypeError} if `infos` is not an Array<URL|string>. TypeError is a little bit special, meaning a recoverable error made by the developer, and not something to return from an high level API. I was just discussing this yesterday with Paolo, and he suggested that it's not a good idea to return TypeError from our APIs. It also has downsides, for example Task.jsm always dumps on TypeError(s) even if they are handled. Please just return Error, or just state this @throws a javascript exception. Fwiw, this should also throw if guid is invalid or if the url is malformed... @@ +43,5 @@ > + * item is `null`. Note that the PageInfo item > + * does NOT contain the visit data (i.e. `visits` is `null`). > + * @throws {TypeError} if `infos` is not an Array<URL|string>. > + */ > + fetch: function(pages) { nit: anonymous functions should be styled as function (), not function() @@ +53,5 @@ > + * any field of said objects. > + * > + * Any change may be observed through nsINavHistoryObserver > + * > + * @param {Array<PageInfo>} infos An array of pages, as identified by ditto @@ +59,5 @@ > + * in the database. > + * @return {Promise<Array<PageInfo|null>>} An array of PageInfo objects, > + * containing the full information on the page, or `null` if the > + * page could not be found. > + * @throws {TypeError} If `infos` is not an Array<PageInfo> or if trailing space @@ +71,5 @@ > + > + /** > + * Remove pages from the database. > + * > + * @param {Array<URL|string>} pages An array of page URLs and/or GUIDs. ditto @@ +74,5 @@ > + * > + * @param {Array<URL|string>} pages An array of page URLs and/or GUIDs. > + * @return {Promise<Array<bool>>} An array with the same size as `pages` > + * in which element `i` holds `true` if `pages[i]` was found or `false` > + * otherwise. I think we should return the info objects, updated just before the removal (since you need to fetch that data regardless to be able to notify, you have it for free), otherwise the consumer has to keep the array around just to be able later to reference it by index... @@ +75,5 @@ > + * @param {Array<URL|string>} pages An array of page URLs and/or GUIDs. > + * @return {Promise<Array<bool>>} An array with the same size as `pages` > + * in which element `i` holds `true` if `pages[i]` was found or `false` > + * otherwise. > + * @throws {TypeError} if `infos` is not an Array<URL|string>. or if a guid or uri is malformed @@ +79,5 @@ > + * @throws {TypeError} if `infos` is not an Array<URL|string>. > + */ > + remove: function(pages) { > + throw new Error("Method not implemented"); > + }, we should also add an isvisited API, the problem is that isURIVisited and isVisited names are taken... What about isPageVisited? @@ +116,5 @@ > + */ > + this.title = null; > + > + /** > + * The frecency of the place. s/place/page/ @@ +123,5 @@ > + */ > + this.frecency = null; > + > + /** > + * The visits to this place. ditto @@ +178,5 @@ > + /** > + * The user followed a link and got a new toplevel > + * window. > + */ > + LINK: Ci.nsINavHistoryService.TRANSITION_LINK, please just add the various TRANSITION_* constants to the History object, then we can just refer to them as History.TRANSITION_* rather than keeping around this enum object. ::: toolkit/components/places/PlacesUtils.jsm @@ +1809,5 @@ > }) > }; > > XPCOMUtils.defineLazyGetter(PlacesUtils, "history", function() { > + let component = Cc["@mozilla.org/browser/nav-history-service;1"] s/component/hs/ (just cause component is confusing and lowercase history is as well, in this context). @@ +1815,5 @@ > .QueryInterface(Ci.nsIBrowserHistory) > .QueryInterface(Ci.nsPIPlacesDatabase); > + return Object.freeze(new Proxy(component, { > + get: (target, name) => target.hasOwnProperty(name) ? target[name] > + : Bookmarks[name] looks like you don't like working on this bug :) this should be History, not Bookmarks

Marco, can you please give this a point estimate? It doesn't need QA, does it?

yep.

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #6) > > + * See the definition of VisitInfo and PageInfo for more details. > > you are not saying WHERE those definitions are. Well, they are in the same document, a bit lower. I'll move them up, to match the style of Bookmarks.jsm. > @@ +36,5 @@ > > + /** > > + * Gets the available information for the given array of pages, each > > + * identified by either DOM URL or GUID (string). > > + * > > + * @param {Array<URL|string>} pages An array of URLs and/or GUIDs. > > please make the required changes to support single item OR array and support > nsIURIs Not a big fan, but will do. > > @@ +41,5 @@ > > + * @return {Promise<Array<PageInfo>|null>} An array with the same > > + * size as `pages`. If a URL or GUID cannot be found, the > > + * item is `null`. Note that the PageInfo item > > + * does NOT contain the visit data (i.e. `visits` is `null`). > > + * @throws {TypeError} if `infos` is not an Array<URL|string>. > > TypeError is a little bit special, meaning a recoverable error made by the > developer, and not something to return from an high level API. I was just > discussing this yesterday with Paolo, and he suggested that it's not a good > idea to return TypeError from our APIs. > It also has downsides, for example Task.jsm always dumps on TypeError(s) > even if they are handled. > Please just return Error, or just state this @throws a javascript exception. Afaict, the errors you mention are exactly that, type errors. Marking them as generic `Error` makes them harder to catch, test, etc, and to recover from. So let's stick with `@throws if `infos` is not an Array<URL|string>` for the moment and have the same debate again soon :) > Fwiw, this should also throw if guid is invalid or if the url is malformed... Right. How do I check whether a string is a valid guid? Also, what's a malformed url if url is an instance of either URL or nsIURL? > > + fetch: function(pages) { > > nit: anonymous functions should be styled as function (), not function() ok. > @@ +74,5 @@ > > + * > > + * @param {Array<URL|string>} pages An array of page URLs and/or GUIDs. > > + * @return {Promise<Array<bool>>} An array with the same size as `pages` > > + * in which element `i` holds `true` if `pages[i]` was found or `false` > > + * otherwise. > > I think we should return the info objects, updated just before the removal > (since you need to fetch that data regardless to be able to notify, you have > it for free), otherwise the consumer has to keep the array around just to be > able later to reference it by index... Right. Do I understand correctly that we aim to return the PageInfo without VisitInfo? > @@ +79,5 @@ > > + * @throws {TypeError} if `infos` is not an Array<URL|string>. > > + */ > > + remove: function(pages) { > > + throw new Error("Method not implemented"); > > + }, > > we should also add an isvisited API, the problem is that isURIVisited and > isVisited names are taken... > What about isPageVisited? Ok. > @@ +1815,5 @@ > > .QueryInterface(Ci.nsIBrowserHistory) > > .QueryInterface(Ci.nsPIPlacesDatabase); > > + return Object.freeze(new Proxy(component, { > > + get: (target, name) => target.hasOwnProperty(name) ? target[name] > > + : Bookmarks[name] > > looks like you don't like working on this bug :) > this should be History, not Bookmarks Oops :)

> we should also add an isvisited API, the problem is that isURIVisited and isVisited names are taken... > What about isPageVisited? If, as I understand, you wish to minimize the number of API entry points, the difference between `isPageVisited` and `fetch` is pretty slim. We could simply add an option to `fetch` to determine whether we are interested in the `PageInfo`.

Here we go again.

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #9) > > > Fwiw, this should also throw if guid is invalid or if the url is malformed... > > Right. How do I check whether a string is a valid guid? copy this regex http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/head_common.js#673 > Also, what's a malformed url if url is an instance of either URL or nsIURL? url may also be a url string, so it may be malformed. > Right. Do I understand correctly that we aim to return the PageInfo without > VisitInfo? For now yes. We'll see if we need those when removing single visits, not important though.

Comment on attachment 8496465 [details] [diff] [review] Introducing History.jsm, v3 Review of attachment 8496465 [details] [diff] [review]: ----------------------------------------------------------------- once comments are addressed, please ask for SR to Gavin. ::: toolkit/components/places/History.jsm @@ +22,5 @@ > + * The title associated with the page. > + * - frecency: (number) > + * The frecency of the page. > + * See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Frecency_algorithm > + * - visits: Array<VisitInfo> fix indentation @@ +45,5 @@ > + * are required for `VisitInfo` arguments or returned for `VisitInfo` results. > + * > + * > +* > + * Each successful operation notifies through the nsINavHistoryObserver fix indentation on the above * @@ +102,5 @@ > + * If specified and "any", `visits` is an array > + * holding all information on a single visit. Use > + * this in particular if a client needs to know > + * whether a page has been visited at least once, > + * but does not care about the exact set of visits. Is this option needed for the implementation, or jut a nice to have? It's introducing complexity in the API and i'm not sure "any" is really something we need to expose externally. It might be useful internally, but it's confusing for consumers I'd rather prefer to expose lastVisitDate in PageInfo if that's the use-case. And make this a boolean includeVisits or includeVisitsData @@ +109,5 @@ > + * An array with the same size as `pages` (or 1 if the > + * argument was a single URL, nsIURI or string). If a > + * URL, nsIURI or GUID cannot be found, the item is > + * `null`. See argument `options` for more details on > + * the contents of this `PageInfo` item. If I pass a string/guid, I'm not expecting to get back an array... This is js, it's very easy to support mixed input/output and we should not be scared of using such capabilities, otherwise any consumer will have to let info = (yield PlacesUtils.history.fetch(url))[0] that quite sucks. In any case the consumer knows if he passed into an array or a single item. @@ +138,5 @@ > + * or (Array<PageInfo>) > + * An array of the above, to batch requests. > + * > + * @return (Promise<Array<PageInfo|null>>) > + * An array with the same size as `infos` (or 1 if the ditto @@ +144,5 @@ > + * found, or if the `uri` specified was for a protocol > + * that should not be stored (e.g. "chrome:", "mailbox:", > + * "about:", "imap:", "news:", "moz-anno:", "view-source:", > + * "resource:", "data:", "wyciwyg:", "javascript:", "blob:"), > + * the item is `null`. Note that the `PageInfo` item I think that for unsupported protocols we should instead throw immediately. not sure what you mean by "if a page cannot be found"... this can add pages... @@ +146,5 @@ > + * "about:", "imap:", "news:", "moz-anno:", "view-source:", > + * "resource:", "data:", "wyciwyg:", "javascript:", "blob:"), > + * the item is `null`. Note that the `PageInfo` item > + * does NOT contain the visit data (i.e. `visits` is `undefined`). > + * @throws If `infos` is typed incorrectly. "is typed" is confusing, "has unexpected type" @@ +152,5 @@ > + * @throws If a `guid` field provided is not a valid GUID. > + * @throws If a `PageInfo` does not have a `visits` field or if the > + * value of `visits` is ill-typed or is an empty array. > + * @throws If an element of `visits` is missing `visitDate` or if the > + * value of `visitDate` is invalid. I actually think we should make visitDate optional and default to now @@ +174,5 @@ > + * or (Array<URL|nsIURI|string>) > + * An array of the above, to batch requests. > + * > + * @return (Promise<Array<PageInfo|null>>) > + * An array with the same size as `pages` (or 1 if the argument ditto @@ +178,5 @@ > + * An array with the same size as `pages` (or 1 if the argument > + * was a single URL, nsIURI or string). If a URL, nsIURI or GUID > + * cannot be found, the item is `null`. Note that the `PageInfo` > + * item does NOT contain the visit data (i.e. `visits` is `undefined`). > + * @throws If `pages` is typed incorrectly or if a string provided "is typed incorrectly" is confusing, maybe "has an unexpected type" @@ +183,5 @@ > + * is not a valid GUID. > + */ > + remove: function (pages) { > + throw new Error("Method not implemented"); > + }, I think you forgot to add isPageVisited

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #13) > Comment on attachment 8496465 [details] [diff] [review] > Introducing History.jsm, v3 > > Review of attachment 8496465 [details] [diff] [review]: > ----------------------------------------------------------------- > > once comments are addressed, please ask for SR to Gavin. > > ::: toolkit/components/places/History.jsm > @@ +22,5 @@ > > + * The title associated with the page. > > + * - frecency: (number) > > + * The frecency of the page. > > + * See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Frecency_algorithm > > + * - visits: Array<VisitInfo> > > fix indentation > > @@ +45,5 @@ > > + * are required for `VisitInfo` arguments or returned for `VisitInfo` results. > > + * > > + * > > +* > > + * Each successful operation notifies through the nsINavHistoryObserver > > fix indentation on the above * > > @@ +102,5 @@ > > + * If specified and "any", `visits` is an array > > + * holding all information on a single visit. Use > > + * this in particular if a client needs to know > > + * whether a page has been visited at least once, > > + * but does not care about the exact set of visits. > > Is this option needed for the implementation, or jut a nice to have? > It's introducing complexity in the API and i'm not sure "any" is really > something we need to expose externally. It might be useful internally, but > it's confusing for consumers > I'd rather prefer to expose lastVisitDate in PageInfo if that's the > use-case. And make this a boolean includeVisits or includeVisitsData Given the initial requirements for the `remove` API, it was my understanding that you wanted to consolidate many API entry points into fewer, so this was an experiment in consolidating `isPageVisited` into `fetch`. I'm not really satisfied with the result, though, so I guess I'll go back to a separate method. Would `History.hasVisits` be a good name? > > @@ +109,5 @@ > > + * An array with the same size as `pages` (or 1 if the > > + * argument was a single URL, nsIURI or string). If a > > + * URL, nsIURI or GUID cannot be found, the item is > > + * `null`. See argument `options` for more details on > > + * the contents of this `PageInfo` item. > > If I pass a string/guid, I'm not expecting to get back an array... This is > js, it's very easy to support mixed input/output and we should not be scared > of using such capabilities, otherwise any consumer will have to > let info = (yield PlacesUtils.history.fetch(url))[0] > that quite sucks. > In any case the consumer knows if he passed into an array or a single item. I disagree. Changing the return type based on whether we have auto-converted some argument makes for an unreliable API. I would consider this acceptable in a strongly-typed language (say Haskell or Rust), because the compiler would catch the errors, but I believe it's a footgun in JS. If you wish to let the client decide, let's have `fetch` and `fetchMany`, etc. > @@ +144,5 @@ > I think that for unsupported protocols we should instead throw immediately. Sounds good to me. > not sure what you mean by "if a page cannot be found"... this can add > pages... Ah, you're right, of course. > @@ +152,5 @@ > > + * @throws If a `guid` field provided is not a valid GUID. > > + * @throws If a `PageInfo` does not have a `visits` field or if the > > + * value of `visits` is ill-typed or is an empty array. > > + * @throws If an element of `visits` is missing `visitDate` or if the > > + * value of `visitDate` is invalid. > > I actually think we should make visitDate optional and default to now Sounds good. > @@ +183,5 @@ > > + * is not a valid GUID. > > + */ > > + remove: function (pages) { > > + throw new Error("Method not implemented"); > > + }, > > I think you forgot to add isPageVisited Well, no, I replaced with with option "any" :)

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #14) > Would `History.hasVisits` be a good name? sure! > > @@ +109,5 @@ > > > + * An array with the same size as `pages` (or 1 if the > > > + * argument was a single URL, nsIURI or string). If a > > > + * URL, nsIURI or GUID cannot be found, the item is > > > + * `null`. See argument `options` for more details on > > > + * the contents of this `PageInfo` item. > > > > If I pass a string/guid, I'm not expecting to get back an array... This is > > js, it's very easy to support mixed input/output and we should not be scared > > of using such capabilities, otherwise any consumer will have to > > let info = (yield PlacesUtils.history.fetch(url))[0] > > that quite sucks. > > In any case the consumer knows if he passed into an array or a single item. > > I disagree. Changing the return type based on whether we have auto-converted > some argument makes for an unreliable API. The fact we auto-convert to an array is an internal implementation detail, to the consumer it will look like he passed into a single object, and got back a single object. Or he passed into an array, and got back an array. That's very consistent and natural. > If you wish to let the client decide, let's have `fetch` and `fetchMany`, > etc. Then the footgun will be that consumers will overlook the "many" API and use the single API in a loop, killing any perf win we could do internally. We may ask Mano what he thinks, I don't want to enforce my vision :)

I tend to agree with Marco. I don't find the "complex" output error prone and I prefer a single API. Having said that - again in line with what Marco said - I'd prefer fetchSingle rather over fetchMany.

I am trying very hard to make myself write that irregular API, but I have strong cognitive difficulties doing so :) What about starting with an API that has only arrays, and extending it later if necessary? If the only issue is in tests, I'm sure I can add the appropriate code in head_*.js to make my fellow Place-developers believe that I have actually written the irregular API :)

not just tests, we're trying to make this coherent with Bookmarks.jsm that acts on single items (for now, but in future may grow array support)

mak: That sounds promising. So what about a first version of History.jsm that acts on single items, then we'll sync with Bookmarks.jsm for however-we-decide-to-add-arrays? I realize that I'm blocking many things on this question, so I will eventually go with the overloaded single item/array if I can't manage to find another way out of this deadlock. But I just think that, while we're rebooting the API, we can do better than what I fear is a footgun.

the problem is that bookmarks and history have very different use-cases, in history it's more common to acts on batches and bookmarks go through transactions. So, unfortunately the answer is no.

My two cents: 1) I don't believe this is a problem at all. I just cannot imagine such an issue actually coming up later on. 2) I do, however, find the fact that |fetch| returns all items at once rather than incrementally, in an onRow-like callback, somewhat unfortunate. Think of a result-like tree view implementation that, rather than showing a rapidly growing list of entries, remains blank until the very last item information is available. While both options could be considered asynchronous, only one of them doesn't suck. And so as much as I like Promises, since |resolve| cannot be called multiple times, nor could it be called with multiple arguments (so one could do .then((...pages) => { for (page of pages) }), I find it unsuitable for incremental asynchronous operations. Even Promise-based Sqlite.jsm API provides a callback options for |execute|, which, interestingly, also enforces a different return value. Speaking of the return semantics of |execute|, they should also be on the table. We could go even further and make the array-case work only through a callback (with the promise resolving to null in that case, when the operation is complete), and leave the simple, fully-Promise-based route for the single input case (we can also consider disallowing a callback in that case, making the signatures difference even more sound).

good point about incremental flow, I think it's a good idea to make it work like execute, allowing to pass into an onrow handler. I'm not sure if it's a good idea to allow array results only with the callback though, for loops are much nicer than callbacks, and when you expect less than 10 pages, you shouldn't have to complicate your code due to an API limitation.

Very good point about the incremental flow. I opened bug 1074747 and https://etherpad.mozilla.org/AsyncStreams to try and come up with a nice non-callback-based API for this. I would like to try and implement a solution based on the "Co-inductive streams" solution on the Etherpad. Would that be ok with everyone?

Here is a new proposal, reworked for working with incremental results. This makes use of a trivial Stream object.

Let's just proceed with a callback, and modify later if we find this useful.

Comment on attachment 8497432 [details] [diff] [review] A variant of History.jsm using Stream (DO NOT LAND) We are not going to land this. Keeping the patch, though, as a basis for discussion of future evolutions of History.jsm.

Actually, now that we have Symbol.iterator, allowing a pretty implementation of the iterator protocol, this solution (assuming |next| returns a promise) seems very good to me.

(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #27) > Actually, now that we have Symbol.iterator, allowing a pretty implementation > of the iterator protocol, this solution (assuming |next| returns a promise) > seems very good to me. I have tried that already, and it doesn't work too nicely, I'm afraid.

So, for the sake or proceeding, let's go for the Sqlite.jsm::execute style. In future, once we'll have a good promise-streaming library, it will be very trivial to write a wrapper. Wrappers are easy to write and they can wrap anything we do here.

Agreed. That's mostly what I'm doing with v5. Since there was insistence that we really should simplify the case of returning a single value, the methods return a promise with the *last* value. This is by opposition to Sqlite.jsm:execute, which always returns an array, iirc. Tell me if you want me to move back to returning an array.

Sqlite.jsm doesn't always return an array, if you don't provide an onrow handler it returns an array with all of the rows, otherwise it returns an hasResult bool. And this is what I think we should do here, as well.

That sounds "fun".

Comment on attachment 8497451 [details] [diff] [review] Introducing History.jsm, v5 Review of attachment 8497451 [details] [diff] [review]: ----------------------------------------------------------------- ok to recap this is the final API as discussed over IRC: PageInfo = yield fetch(guid-nsIURI-URL-spec); bool hasResult = yield update(PageInfo OR Array<PageInfo>, optionalOnResultCallback); bool hasResult = yield remove(guid-nsIURI-URL-spec OR Array<guid-nsIURI-URL-spec>, optionalOnResultCallback); bool hasVisits = yield hasVisits(guid-nsIURI-URL-spec); Once this is done, we'll start implementing a removeRange to replace the various RemovePagesByXXX ::: toolkit/components/places/History.jsm @@ +14,5 @@ > + * following fields: > + * - guid: (string) > + * The globally unique id of the page. > + * - id: (string) > + * The machine-local (internal) id of the page. please add a comment specifying that usage of this field is deprecated and exists only until we complete moving the code to GUIDs, then it will be removed without notice. @@ +23,5 @@ > + * argument may hold `nsIURL` or `string` values for field `uri`, > + * but `PageInfo` objects returned by this module always hold `URL` > + * values. > + * - title: (string) > + * The title associated with the page. ", if any." that basically means we won't set this property if there's no title. @@ +25,5 @@ > + * values. > + * - title: (string) > + * The title associated with the page. > + * - frecency: (number) > + * The frecency of the page. ", if any." @@ +26,5 @@ > + * - title: (string) > + * The title associated with the page. > + * - frecency: (number) > + * The frecency of the page. > + * See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Frecency_algorithm please add a note that frecency is immutable, the only way to change it is by adding or removing visits. @@ +28,5 @@ > + * - frecency: (number) > + * The frecency of the page. > + * See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Frecency_algorithm > + * - visits: (Array<VisitInfo>) > + * All the visits for this page. ", if any." @@ +36,5 @@ > + * > + * A `VisitInfo` object is any object that contains A SUBSET of the following > + * fields: > + * - id: (string) > + * The machine-local (internal) id of the page. Is not this the id of the "visit" rather than of the page? Please remove this for now, I don't think we should expose the id of the visit for whatever reason, there should be no external use-cases (internal code is another story) @@ +38,5 @@ > + * fields: > + * - id: (string) > + * The machine-local (internal) id of the page. > + * - date: (Date) > + * The time the visit occurred. I will update Bookmarks.jsm to use Date objects as well. @@ +39,5 @@ > + * - id: (string) > + * The machine-local (internal) id of the page. > + * - date: (Date) > + * The time the visit occurred. > + * - transitionType: (number) what about just "transition"? @@ +79,5 @@ > + > + > +this.History = Object.freeze({ > + /** > + * Place-holder for a value that was requested but could not be found. As discussed over IRC, we'll remove NotFound and rather return a Map(by guid) when multiple results are expected. @@ +96,5 @@ > + > + /** > + * Gets the available information for one or more pages. > + * > + * @param pages: (URL or nsIURI) or string, since uri can be a string in the PageInfo object, this should accept a url string, internally we'll build a URL and eventually throw if the string is invalid (that means once we get a string we check if it's a valid guid, if it's not, we check if it's a valid URL, otherwise we throw) @@ +100,5 @@ > + * @param pages: (URL or nsIURI) > + * The full URI of the page. > + * or (string) > + * The PageInfo GUID of the page. > + * or (Array<URL|nsIURI|string>) Let's remove support for batched reads, we don't need it. What we need to batch is update and remove, not reads. so fetch will only fetch a single item, by URL, url string, nsIURI or guid. @@ +113,5 @@ > + * > + * @return (Promise<PageInfo|NotFound>) > + * A promise resolved once the request is complete, including > + * all calls to `onResult`. The Promise resolves to the latest > + * value passed to `onResult`. I'd prefer if you'd avoid using "value" and use "page" instead, it's more specific. also specify resolves to null if no item is found @@ +115,5 @@ > + * A promise resolved once the request is complete, including > + * all calls to `onResult`. The Promise resolves to the latest > + * value passed to `onResult`. > + * @throws If `pages` is typed incorrectly or if a string provided > + * is not a valid GUID. or not a valid url string. @@ +123,5 @@ > + }, > + > + /** > + * Adds a set of visits for one or more PageInfo objects, or update > + * some fields. note that passing a visit is mandatory (you cannot change the title unless you are adding a visit that has a new title), so I'd avoid the "update some fields" part. @@ +125,5 @@ > + /** > + * Adds a set of visits for one or more PageInfo objects, or update > + * some fields. > + * > + * This function recomputes the frecency of the page. I think this, at a maximum, should be set below everything as a @note. I'd also say that it can change the frecency of the page, not "recompute" @@ +133,5 @@ > + * Information on a page. This `PageInfo` MUST contain > + * - either a field `guid` or a field `uri`, as specified > + * by the definition of `PageInfo`; > + * - a field `visits`, as specified by the definition of > + * `PageInfo`. should specify that it should contain at least one visit. @@ +143,5 @@ > + * to now. > + * or (Array<PageInfo>) > + * An array of the above, to batch requests. > + * @param onResult: (function(PageInfo|NotFound), optional) > + * A callback invoked for each result. this will only be invoked for found items. Should also specify that if an onResult handler is provided, the method will resolve to the last found item or null, if nothing was found. @@ +150,5 @@ > + * contain the visit data (i.e. `visits` is > + * `undefined`). > + * - Otherwise, the argument is `NotFound`. > + * > + * @return (Promise<PageInfo|NotFound>) Should now document the fact if multiple items are expected and no onResult is provided, it will return a Map (keyed by input). Should also state that if no item is found, it resolves to null or to an empty Map if more than one result is expected. and that if onResult handler is provided, will resolve to either the last found result, or null if no result is found. @@ +163,5 @@ > + * @throws If a `PageInfo` has neither `guid` nor `uri`, > + * @throws If a `guid` field provided is not a valid GUID. > + * @throws If a `PageInfo` does not have a `visits` field or if the > + * value of `visits` is ill-typed or is an empty array. > + * @throws If an element of `visits` has an invalid `visitDate`. it's named date, not visitDate @@ +164,5 @@ > + * @throws If a `guid` field provided is not a valid GUID. > + * @throws If a `PageInfo` does not have a `visits` field or if the > + * value of `visits` is ill-typed or is an empty array. > + * @throws If an element of `visits` has an invalid `visitDate`. > + * @throws If an element of `visits` is missing `transitionType` or if I'd merge this to the above "invalid 'date' or an invalid "transition" @@ +226,5 @@ > + * value passed to `onResult`. > + * @throws If `pages` has an unexpected type or if a string provided > + * is not a valid GUID. > + */ > + hasVisits: function(pages, onResult) { hasVisits should also only accept a single item, like fetch.

Here we go.

Comment on attachment 8498195 [details] [diff] [review] Introducing History.jsm, v6 Review of attachment 8498195 [details] [diff] [review]: ----------------------------------------------------------------- ok, this is solid enough to build on top of it. please ask gavin for SR once comments are fixed. ::: toolkit/components/places/History.jsm @@ +26,5 @@ > + * - frecency: (number) > + * The frecency of the page, if any. > + * See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Frecency_algorithm > + * Note that frecency is recomputed internally, regardless of the value > + * of `frecency` fields passed as arguents. typo: arguents @@ +79,5 @@ > +this.History = Object.freeze({ > + /** > + * Fetch the available information for one page. > + * > + * @param page: (URL or nsIURI or uri as string) nit: we usually say "an url string" @@ +82,5 @@ > + * > + * @param page: (URL or nsIURI or uri as string) > + * The full URI of the page. > + * or (string) > + * The PageInfo GUID of the page. I'd prefer (URL or nsIURI) The full URI of the page or (string) The GUID or the url string of the page @@ +89,5 @@ > + * A promise resolved once the operation is complete. > + * @resolves (PageInfo | null) If the page could be found, the information > + * on that page. Note that this `PageInfo` does NOT contain the visit > + * data (i.e. `visits` is `undefined`). > + * trailing spaces @@ +94,5 @@ > + * @throws (Error) > + * If `page` does not have the expected type or if it is a string > + * that may be parsed neither as a valid URL nor as a valid GUID. > + */ > + fetch: function (pages) { s/pages/guidOrURI/ @@ +104,5 @@ > + * > + * Any change may be observed through nsINavHistoryObserver > + * > + * @note This function recomputes the frecency of the page automatically, > + * regardless of the value of field `frecency` passed as argument. s/field/property/ (everywhere) @@ +108,5 @@ > + * regardless of the value of field `frecency` passed as argument. > + * > + * @param infos: (PageInfo) > + * Information on a page. This `PageInfo` MUST contain > + * - either a field `guid` or a field `uri`, as specified "either a guid or uri property, ..." @@ +120,5 @@ > + * If the `visitDate` of a visit is not provided, it defaults > + * to now. > + * or (Array<PageInfo>) > + * An array of the above, to batch requests. > + * @param onResult: (function(PageInfo), optional) nit: I think we always used [optional]... there's no clear coding style for javadoc, some does @param {type} name [optional], others do @param name [optional] (type) or name (type) [optional] @@ +122,5 @@ > + * or (Array<PageInfo>) > + * An array of the above, to batch requests. > + * @param onResult: (function(PageInfo), optional) > + * A callback invoked for pages that were already present before > + * the call to `update`. The argument is a `PageInfo` what does this mean? it should also be invoked for additions, both additions and updates. @@ +123,5 @@ > + * An array of the above, to batch requests. > + * @param onResult: (function(PageInfo), optional) > + * A callback invoked for pages that were already present before > + * the call to `update`. The argument is a `PageInfo` > + * representing the previous information on the page. I'm not sure if it should be the previous or the newly updated information, off-hand I'd have said the latter. btw, we can also figure this later based on use-cases. @@ +140,5 @@ > + * by `infos` was present. > + * or (Map<PageInfo, PageInfo> otherwise) > + * A map containing the previous information on the pages passed > + * as `infos`. If a page was not previously present, this page > + * does not appear in the map. hm, we decided this should resolve to an hasResult bool in any case. like remove. @@ +175,5 @@ > + * > + * @param pages: (URL or nsIURI or string) > + * The full URI of the page. > + * or (string) > + * The PageInfo GUID of the page. ditto (group by argument type) @@ +196,5 @@ > + > + /** > + * Determine if a page has been visited. > + * > + * @param pages: (URL or nsIURI or string) s/pages/page/ @@ +198,5 @@ > + * Determine if a page has been visited. > + * > + * @param pages: (URL or nsIURI or string) > + * The full URI of the page. > + * or (string) ditto for grouping types @@ +209,5 @@ > + * @throws (Error) > + * If `pages` has an unexpected type or if a string provided > + * is neither not a valid GUID nor a valid URI. > + */ > + hasVisits: function(pages, onResult) { s/pages/page/ @@ +220,5 @@ > + */ > + > + /** > + * The user followed a link and got a new toplevel > + * window. some of these transition comments are wrapped too early, we should wrap at about 80 chars

Applied review, ready for SR.

note for SR: we are going to apply some of the decisions taken here to Bookmarks.jsm to keep the APIs consistent, for example the use of Date objects instead of microseconds and the support for URL objects

Comment on attachment 8498776 [details] [diff] [review] Introducing History.jsm, v7 >diff --git a/toolkit/components/places/History.jsm b/toolkit/components/places/History.jsm >+ * A `PageInfo` object is any object that contains A SUBSET of the >+ * following properties: >+ * - uri: (URL) >+ * or (nsIURL) >+ * or (string) >+ * The full URI of the page. Note that `PageInfo` values passed as >+ * argument may hold `nsIURL` or `string` values for property `uri`, >+ * but `PageInfo` objects returned by this module always hold `URL` >+ * values. Do you really mean nsIURL here? I imagine this should actually be nsIURI (and same for VisitInfo). >+ * - frecency: (number) >+ * The frecency of the page, if any. >+ * See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Frecency_algorithm >+ * Note that frecency is recomputed internally, regardless of the value >+ * of `frecency` properties passed as argument. This sentence could be clarified. I imagine this means that the "frecency" property of PageInfo objects passed into this API is never read? I.e. it's only useful as a read-only property on PageInfo results coming out of this API. In which case it would be clearer to say that explicitly: "frecency cannot be set via this API, it is effectively read-only from the consumer's perspective. This property is only relevant on PageInfo results, rather than PageInfo objects provided as arguments to e.g. update()." >+ update: function (infos, onResult) { >+ * @param infos: (PageInfo) >+ * If both a property `uri` and a property `guid` are provided, >+ * the GUID of the page is updated if necessary. This is a bit of an odd variant of this function. Can GUID updates be handled by a separate single-purpose method? >+ * @resolves (bool) >+ * `true` if at least one page entry was created, `false` otherwise >+ * (i.e. if page entries were updated but not created). This also seems a bit odd, but I assume it's designed this way for a common use case?

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #3) > https://developer.mozilla.org/en-US/docs/Web/API/URL.URL > > Wherever possible, we are trying to get rid of nsIURI in JS, in favor of the > simpler and faster URL. Curious about the specifics of "simpler and faster". But do URL objects exist in non-DOM contexts?

Simpler: compare the API of URL and that of NetUtil + nsIURL. Faster: webidl instead of xpidl. I'm almost sure I have used them already in Workers and in xpcshell.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #38) > >+ update: function (infos, onResult) { > > >+ * @param infos: (PageInfo) > > >+ * If both a property `uri` and a property `guid` are provided, > >+ * the GUID of the page is updated if necessary. > > This is a bit of an odd variant of this function. Can GUID updates be > handled by a separate single-purpose method? Ideally we would not even want consumers to be able to change GUIDs, so we are somehow "hiding" this feature here. The only reason this exists is the dumb behavior of Sync. It creates us more than one issue. So rather than having a separate method I'd prefer if we'd not even document this at all... > > >+ * @resolves (bool) > >+ * `true` if at least one page entry was created, `false` otherwise > >+ * (i.e. if page entries were updated but not created). > > This also seems a bit odd, but I assume it's designed this way for a common > use case? Actually, the problem is that we don't have a very valuable value to return...

btw, yes, it should be nsIURI, not nsIURL.

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #41) > Ideally we would not even want consumers to be able to change GUIDs, so we > are somehow "hiding" this feature here. > The only reason this exists is the dumb behavior of Sync. It creates us more > than one issue. So rather than having a separate method I'd prefer if we'd > not even document this at all... Does this mean that we want to update Sync to use History.jsm? Given the Sync codebase, this sounds a bit risky.

We MUST update Sync to use history.jsm cause we must remove the old synchronous API. Fwiw, only removals are missing, all the rest is already using async history. Btw, URL is not defined in xpcshell tests...

So, the fact xpcshell doesn't support URL, means anything starting from it doesn't support it as well, so modules using URL loaded from it won't support it as well. I fear we'll have to revert to url strings (and validate them through nsIURI). So input is either an url string or nsIURI, output is an url string. Thoughts?

Actually, you need to import the feature to use it in a .jsm or xpcshell test, but it is present: http://dxr.mozilla.org/mozilla-central/source/browser/components/loop/MozLoopService.jsm#32 http://dxr.mozilla.org/mozilla-central/source/js/xpconnect/tests/unit/test_url.js#8 By the way, just because something is not supported by xpcshell does not mean that .jsm imported by xpcshell cannot use it. For instance, ChromeWorker used to be supported in .jsm but not in xpcshell.

fair enough, I didn't know it was possible to import the symbol.

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #41) > > >+ * If both a property `uri` and a property `guid` are provided, > > >+ * the GUID of the page is updated if necessary. > The only reason this exists is the dumb behavior of Sync. It creates us more > than one issue. So rather than having a separate method I'd prefer if we'd > not even document this at all... Not documented and part of a general "update" method sounds worse - if we only need this for stupid Sync reasons, then we should make it its own function and call it dontCallThisUnlessYouAreStupidSync().

Ok. Let's not implement guid changing at all in history.jsm, Sync will keep using the unwrapped updatePlaces API. This wrapper should disallow changing guid.

Applied feedback, removed the weird GUID change stuff.

Please provide a Try link.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cc77c483118e

Good call, Ryan, there were obviously errors in the patch.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=557a9ef606f4

Comment on attachment 8501673 [details] [diff] [review] Introducing History.jsm, v9 Review of attachment 8501673 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/PlacesDBUtils.jsm @@ +29,5 @@ > + return Cc["@mozilla.org/browser/nav-history-service;1"] > + .getService(Ci.nsINavHistoryService) > + .QueryInterface(Ci.nsIBrowserHistory) > + .QueryInterface(Ci.nsPIPlacesDatabase) > + .DBConnection; Ideally this should just be PlacesUtils.history.DBConnection The old call should not break though, or we are going to break addons. Which error do you get if you do PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection ? We need it to keep working correctly. ::: toolkit/components/places/PlacesUtils.jsm @@ +1824,5 @@ > + property = History[name]; > + object = History; > + } > + if (typeof property == "function") { > + return property.bind(object); Is this needed for the module case, the xpcom component case, both? which error is this solving?

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #55) > Comment on attachment 8501673 [details] [diff] [review] > Introducing History.jsm, v9 > > Review of attachment 8501673 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/components/places/PlacesDBUtils.jsm > @@ +29,5 @@ > > + return Cc["@mozilla.org/browser/nav-history-service;1"] > > + .getService(Ci.nsINavHistoryService) > > + .QueryInterface(Ci.nsIBrowserHistory) > > + .QueryInterface(Ci.nsPIPlacesDatabase) > > + .DBConnection; > > Ideally this should just be PlacesUtils.history.DBConnection > > The old call should not break though, or we are going to break addons. > Which error do you get if you do > PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection ? We > need it to keep working correctly. Ah, right. This fixed some of the issues fixed by the other change, but that's not necessary anymore. > ::: toolkit/components/places/PlacesUtils.jsm > @@ +1824,5 @@ > > + property = History[name]; > > + object = History; > > + } > > + if (typeof property == "function") { > > + return property.bind(object); > > Is this needed for the module case, the xpcom component case, both? > which error is this solving? This is needed for the xpcom component case. If I remove this, it raises NS_ERROR_XPC_BAD_OP_ON_WN_PROTO whenever a method is invoked, which basically translates to "I have the method but not `this`". I have not checked whether this is also necessary for the module case, but that would make sense. Also, the use of `hasOwnProperty` instead of `in` didn't work, iirc because XPConnect translates `QueryInterface` to prototype chaining.

Same one minus, the PlacesDBUtils.jsm change. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=557a9ef606f4