Closed Bug 1249259 Opened 8 years ago Closed 8 years ago

Puppeteer is using deprecated Places API

Categories

(Testing :: Firefox UI Tests, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mak, Assigned: whimboo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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/
Attachment #8728114 - Flags: review?(mjzffr)
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.
Attachment #8728114 - Flags: feedback?(mak77)
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Blocks: 1254923
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/
Attachment #8728114 - Attachment description: MozReview Request: Bug 1249259 - Update Firefox puppeteer for deprecated places API. r?maja_zf → MozReview Request: Bug 1249259 - Update Firefox puppeteer for deprecated places API. r?mak
Attachment #8728114 - Flags: feedback?(mak77) → review?(mak77)
https://reviewboard.mozilla.org/r/38797/#review35623

Good idea. I updated the mozreview request to make him the reviewer of this patch.
Attachment #8728114 - Flags: review?(mak77)
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/
Attachment #8728114 - Flags: review?(mak77)
Maja, in case I'm already out when this patch gets (hopefully) a r+, may you be able to land it for me? Thanks.
Flags: needinfo?(mjzffr)
Attachment #8728114 - Flags: review?(mak77) → review+
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.
Flags: needinfo?(mjzffr)
https://hg.mozilla.org/mozilla-central/rev/02ad453e3bfc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.