removeAllPages should be replaced with History.jsm::clear (async), since the former is deprecated. Bookmarks should use async Bookmarks.jsm API too, but the old API has not been deprecated yet.

Thank you for the heads-up. The History.jsm (resource://gre/modules/History.jsm) change is clear, but which Bookmark code do you mean exactly? Everything in that module which handles bookmarks? https://dxr.mozilla.org/mozilla-central/source/testing/puppeteer/firefox/firefox_puppeteer/api/places.py

anything using bs and nsINavBookmarksService should insteaf use PlacesUtils.bookmarks.XXX where XXX is the APIs exposed by resource://gre/modules/Bookmarks.jsm Also History.jsm should be used through the PlacesUtils.history. accessor.

This patch exchanges all old and deprecated API calls with the ones in PlacesUtils. As a side-effect we can also condense some other code by using promises. Review commit: https://reviewboard.mozilla.org/r/38797/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38797/

Comment on attachment 8728114 [details] MozReview Request: Bug 1249259 - Update Firefox puppeteer for deprecated places API. r?mak Marco, it would be great if you can give feedback. Just in case I missed something or did something wrong.

Comment on attachment 8728114 [details] MozReview Request: Bug 1249259 - Update Firefox puppeteer for deprecated places API. r?mak https://reviewboard.mozilla.org/r/38797/#review35623 I don't feel comfortable reviewing this patch. Perhaps :mak should review rather than give feedback.

Comment on attachment 8728114 [details] MozReview Request: Bug 1249259 - Update Firefox puppeteer for deprecated places API. r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38797/diff/1-2/

https://reviewboard.mozilla.org/r/38797/#review35623 Good idea. I updated the mozreview request to make him the reviewer of this patch.

Comment on attachment 8728114 [details] MozReview Request: Bug 1249259 - Update Firefox puppeteer for deprecated places API. r?mak https://reviewboard.mozilla.org/r/38797/#review35901 I think you misunderstood my comment, likely cause I didn't explain it well. We have 2 bookmarking APIs: 1. a synchronous one using ids, nsINavBookmarksService, also exposed from PlacesUtils.bookmarks. 2. an asynchronous one using GUIDs, exposed from PlacesUtils.bookmarks. Both work, but we want internal code to use the new asynchronous API as far as possible and we'll migrate to that. The new API uses promises. For example in this case instead of getBookmarkIdsForURI and getFolderIdForItem you could just do (in a task, but you can use plain promises) let folderGuids = [] yield PlacesUtils.bookmarks.fetch({url: "your_url"}, bm => folderGuids.push(bm.parentGuid)); Atm, most of the new API documentation is directly in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm Now, I don't know if you can just move this code to use guids. So, for now it'd also be fine to take the patch as-is and do the bookmarking conversion work apart. The primary reason I filed this bug is to get rid of RemoveAllPages() from Places in this version Let me know how you intend to proceed, the patch looks globally good, the bookmarks work can happen separately. I'm fine either way. ::: testing/puppeteer/firefox/firefox_puppeteer/api/places.py:36 (Diff revision 2) > let uri = ios.newURI(url, null, null); You may want to import NetUtil and use NetUtil.newURI(url); If you'd use the new API, you could just pass a string or an URL object... ::: testing/puppeteer/firefox/firefox_puppeteer/api/places.py:106 (Diff revision 2) > options.resultType = options.RESULTS_AS_URI; this should be the default so likely you don't need to set it.

https://reviewboard.mozilla.org/r/38797/#review35901 > You may want to import NetUtil and use NetUtil.newURI(url); > If you'd use the new API, you could just pass a string or an URL object... Nice! Good to know that we do not need that anymore. > this should be the default so likely you don't need to set it. Indeed. I removed that line.

Comment on attachment 8728114 [details] MozReview Request: Bug 1249259 - Update Firefox puppeteer for deprecated places API. r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38797/diff/2-3/

Maja, in case I'm already out when this patch gets (hopefully) a r+, may you be able to land it for me? Thanks.

Comment on attachment 8728114 [details] MozReview Request: Bug 1249259 - Update Firefox puppeteer for deprecated places API. r?mak https://reviewboard.mozilla.org/r/38797/#review40051 nice! ::: testing/puppeteer/firefox/firefox_puppeteer/api/places.py:98 (Diff revision 3) > let urls = []; > > - let options = hs.getNewQueryOptions(); > - options.resultType = options.RESULTS_AS_URI; > + Components.utils.import("resource://gre/modules/PlacesUtils.jsm"); > + > + let options = PlacesUtils.history.getNewQueryOptions(); > + // options.resultType = options.RESULTS_AS_URI; I guess you wanted to remove this?

https://reviewboard.mozilla.org/r/38797/#review40051 > I guess you wanted to remove this? Totally! Will be fixed in the next version.

Comment on attachment 8728114 [details] MozReview Request: Bug 1249259 - Update Firefox puppeteer for deprecated places API. r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38797/diff/3-4/

Maja, Autoland should take of the landing now.