implement History.hasVisits as a wrapper of asyncHistory.isURIVisited

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mak, Assigned: meet1995, Mentored)

Tracking

({good-first-bug})

55 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment)

Reporter

Description

2 years ago
The current API signature in History.jsm has an onResult callback that is not needed, this should just return a promise that resolves to a bool.
it should have a test_hasVisits.js test in toolkit/components/places/tests/history.

all direct use of isURIVisited should be converted to the new API, apart from test_isURIVisited.js
promiseIsURIVisited should as well be removed and all of its usage replaced with the new API.
Assignee

Comment 1

2 years ago
Hi Marco,
I'd like to take this up as my first bug.. It would be great if you could mentor to solve this bug.

Thank you.
Reporter

Comment 2

2 years ago
(In reply to meet1995 from comment #1)
> Hi Marco,
> I'd like to take this up as my first bug.. It would be great if you could
> mentor to solve this bug.

Sure, have you gone through https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction already? Do you have a local working build?
Assignee

Comment 3

2 years ago
Thanks. 
I have installed VS 2015 , I should have a local working build by tomorrow.
Assignee

Comment 4

2 years ago
(In reply to Marco Bonardo [::mak] from comment #2)
> (In reply to meet1995 from comment #1)
> > Hi Marco,
> > I'd like to take this up as my first bug.. It would be great if you could
> > mentor to solve this bug.
> 
> Sure, have you gone through
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Introduction already? Do you have a local working build?

I was able to build mozilla-central in artifact mode, do I need to build it in full mode ?
Reporter

Comment 5

2 years ago
artifact mode should be fine since all of the involved code is javascript.
Assignee

Comment 6

2 years ago
(In reply to Marco Bonardo [::mak] from comment #5)
> artifact mode should be fine since all of the involved code is javascript.

Thanks, I'll in to the code and try to understand the bug and get back to you in case of queries.
Assignee

Comment 7

2 years ago
(In reply to meet1995 from comment #6)
> (In reply to Marco Bonardo [::mak] from comment #5)
> > artifact mode should be fine since all of the involved code is javascript.
> 
> Thanks, I'll in to the code and try to understand the bug and get back to
> you in case of queries.

*Thanks, I'll look in to the code and try to understand the bug and get back to you in case of queries.
Assignee

Comment 8

2 years ago
Hi Marco, I tried understanding the code and was able come up with this https://gist.github.com/xtroncode/78de9a9cf5f6d23cbf9542e16a424874 . I am still struggling to understand how to use this function, I have used `PlacesUtils.history.hasVisits()` by looking at other tests but the function is defined in History.jsm . It would be great if you could point me to some resources where I can understand the pattern ( like what is .jsm etc). Besides this there are around 132 references of promiseIsURIVisited (including definitions) which need to be replaced with `PlacesUtils.history.hasVisits` also any direct reference to isURIVisited needs to be replaced by the new api, right?
Assignee

Comment 9

2 years ago
Hi Marco, I have been reading JavaScript code modules documentation on MDN and have a bit more clarity around how things integrate. Did you get a chance to look at the gist ?

Thanks.
Reporter

Comment 10

2 years ago
Please set the needinfo flag (bottom of the bug report) when you have questions or need help, we get hundreds of bugmails every day and simple comments may end up being unnoticed.

Your function should be implemented inside the History.jsm file that is in the toolkit/components/places/ folder of the src dir.
There is already a placeholder there and you can look at how are implemented. You can likely use an async function.
The old isURIVisited API is documented here: http://searchfox.org/mozilla-central/source/toolkit/components/places/mozIAsyncHistory.idl#182 it just directly takes a function callback, you don't need an object.

The test should go to toolkit/components/places/tests/history/

(In reply to meet1995 from comment #8)

> Besides this there are around 132 references of promiseIsURIVisited
> (including definitions) which need to be replaced with
> `PlacesUtils.history.hasVisits`

yes, this could happen in a separate changeset from the implementation.
Ideally I'd say one changeset to implement the API and one to fix old consumers.
In most cases hasVisits will be a drop-in replacement to promiseIsURIVisited, so it will end up being mostly a search&replace task.

> also any direct reference to isURIVisited
> needs to be replaced by the new api, right?

http://searchfox.org/mozilla-central/search?q=.isURIVisited
Everywhere BUT toolkit/components/places/tests/unit/test_isURIVisited.js that is directly testing the API and will continue doing so.
Assignee

Comment 11

2 years ago
Thanks. Can you assign this bug to me ?
Flags: needinfo?(mak77)
Reporter

Comment 12

2 years ago
we usually assign on first patch attachment, but if it helps you tracking your work this won't hurt.
Assignee: nobody → meet1995
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Reporter

Comment 15

2 years ago
mozreview-review
Comment on attachment 8872421 [details]
Bug 1366231 - Implement History.hasVisits as a wrapper of asyncHistory.isURIVisited;

https://reviewboard.mozilla.org/r/143926/#review148692

::: toolkit/components/places/History.jsm:531
(Diff revision 2)
>     * @throws (Error)
>     *      If `pages` has an unexpected type or if a string provided
>     *      is neither not a valid GUID nor a valid URI.
>     */
> -  hasVisits(page, onResult) {
> -    throw new Error("Method not implemented");
> +  hasVisits(page) {
> +    let uri = PlacesUtils.normalizeToURLOrGUID(page);

This will also allow a guid, that's a good idea. let's change the argument to be named guidOrURI and also update the javadoc accordingly. see fetch()

::: toolkit/components/places/History.jsm:533
(Diff revision 2)
>     *      is neither not a valid GUID nor a valid URI.
>     */
> -  hasVisits(page, onResult) {
> -    throw new Error("Method not implemented");
> +  hasVisits(page) {
> +    let uri = PlacesUtils.normalizeToURLOrGUID(page);
> +    return PlacesUtils.withConnectionWrapper("History.jsm: hasVisits",
> +     db => hasVisits(db,uri)

This is not needed, since the implementation here is very simple you can just inline it here, the need for "inner" implementations is so that the API part is shorter, but it doesn't matter for very small implementations.

You also don't need withConnectionWrapper because isURIVisited uses its own connection handler.
here you just need to return new Promise...

::: toolkit/components/places/History.jsm:1265
(Diff revision 2)
> +// Inner implementation of History.hasVisits.
> +var hasVisits = async function(db, uri) {
> +  return new Promise((resolve, reject) => {
> +     PlacesUtils.asyncHistory.isURIVisited(uri,{
> +          isVisited:(aURI,aIsVisited)=>{
> +            resolve(aIsVisited);

please, always indent by 2 spaces, and add whitespaces before braces, after : and around =>
Attachment #8872421 - Flags: review?(mak77)
Comment hidden (mozreview-request)
Reporter

Comment 17

2 years ago
mozreview-review
Comment on attachment 8872421 [details]
Bug 1366231 - Implement History.hasVisits as a wrapper of asyncHistory.isURIVisited;

https://reviewboard.mozilla.org/r/143926/#review149694

Almost there, still some minor details to fix.

::: toolkit/components/places/History.jsm:530
(Diff revision 3)
>     *      is neither not a valid GUID nor a valid URI.
>     */
> -  hasVisits(page, onResult) {
> -    throw new Error("Method not implemented");
> +  hasVisits(guidOrURI) {
> +    guidOrURI = PlacesUtils.normalizeToURLOrGUID(guidOrURI);
> +
> +    return new Promise((resolve, reject) => {

don't pass reject, since we don't use it. just resolve => {

::: toolkit/components/places/History.jsm:532
(Diff revision 3)
> -  hasVisits(page, onResult) {
> -    throw new Error("Method not implemented");
> +  hasVisits(guidOrURI) {
> +    guidOrURI = PlacesUtils.normalizeToURLOrGUID(guidOrURI);
> +
> +    return new Promise((resolve, reject) => {
> +      PlacesUtils.asyncHistory.isURIVisited(guidOrURI, {
> +          isVisited: (aURI, aIsVisited) => {

the indentation here is still wrong (4 spaces instead of 2), and you still don't need to pass an object, just pass an arrow function callback.

::: toolkit/components/places/tests/history/test_hasVisits.js:27
(Diff revision 3)
> +    "passing an invalid (not of type URI or nsIURI) object to History.hasVisits should throw a TypeError"
> +  );
> +});
> +
> +add_task(async function test_history_has_visits(){
> +    const TEST_URL = "http://mozilla.com/";

wrong indentation in the whole function, should be 2 spaces

::: toolkit/components/places/tests/history/test_hasVisits.js:29
(Diff revision 3)
> +});
> +
> +add_task(async function test_history_has_visits(){
> +    const TEST_URL = "http://mozilla.com/";
> +    await PlacesTestUtils.clearHistory();
> +    Assert.equal(await PlacesUtils.history.hasVisits(TEST_URL),false,"Test Url should not be in history.")

please always put a space after commas
And try to wrap lines at about 80 chars, for example in this case:
Assert.equal(await PlacesUtils.history.hasVisits(TEST_URL), false,
             "Test Url should not be in history.");
             
(this line is also missing a semicolon)
Attachment #8872421 - Flags: review?(mak77)
Comment hidden (mozreview-request)
Reporter

Updated

2 years ago
Blocks: 1370881
Reporter

Comment 19

2 years ago
mozreview-review
Comment on attachment 8872421 [details]
Bug 1366231 - Implement History.hasVisits as a wrapper of asyncHistory.isURIVisited;

https://reviewboard.mozilla.org/r/143926/#review150746

LGTM, thank you.
I'm starting a Try run to quickly check tests, then will land.

Note I filed bug 1370881 to replace consumers, since this is ready there's no reason to delay its landing.
Attachment #8872421 - Flags: review?(mak77) → review+

Comment 20

2 years ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/aa3d11c78ac9
Implement History.hasVisits as a wrapper of asyncHistory.isURIVisited; r=mak
Sorry, this had to be backed out for linting failures, e.g. in History.jsm:

https://hg.mozilla.org/integration/autoland/rev/525fb3e1314b9da3d86c0f68c7bb76301eaae1bb

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=aa3d11c78ac9a7ee493b595e91bf49f4c77e50bc&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=105219545&repo=autoland

>[task 2017-06-07T16:07:31.460230Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/places/History.jsm:1258:3 | Newline required at end of file but not found. (eol-last)
Just add a linebreak at the end of the file.

>[task 2017-06-07T16:07:31.460362Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/places/tests/history/test_hasVisits.js:1:52 | Expected linebreaks to be 'LF' but found 'CRLF'. (linebreak-style)
Please convert these Windows linebreaks to Unix ones.

See http://gecko.readthedocs.io/en/latest/tools/lint/usage.html?highlight=eslint for how to run the linting locally.

Please fix the issues and submit an updated patch. Thank you.
Flags: needinfo?(meet1995)
Comment hidden (mozreview-request)
Assignee

Comment 23

2 years ago
I have submitted an updated patch for review.
Flags: needinfo?(meet1995) → needinfo?(mak77)
Reporter

Comment 24

2 years ago
I've applied the patch locally and ran mach eslint, it seems ok now. It's unfortunate mozreview doesn't run it, and the Try run requesting xpcshell tests should also have run it...

Btw, once you setup your text editor to satisfy newline at end of file and unix newlines you should be fine for a while :)
Flags: needinfo?(mak77)

Comment 25

2 years ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/90d3eb2f3d00
Implement History.hasVisits as a wrapper of asyncHistory.isURIVisited; r=mak

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/90d3eb2f3d00
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee

Updated

2 years ago
Keywords: checkin-needed
Reporter

Comment 27

2 years ago
this has already landed, it doesn't need further checkin-needed :)
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.