702810 - mozIAsyncHistory should expose an async isURIVisited method

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

We currently expose is-visited callbacks registration only through iHistory, it would be useful to js consumers as well, and as a replacement for nsIGlobalHistory2.isVisited()

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This uses the existing helper class that animates link coloring, I changed it so that providing an external handler (a callback) will notify it, rather than History.

Apart that, I slightly changed the visited status query, since after bug 692493 we can now rely on last_visit_date and avoid traversing the visits table. This should be a good perf win.

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1.1

forgot to remove a useless part of the test.

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 576919 [details] [diff] [review]
patch v1.1

Review of attachment 576919 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: toolkit/components/places/mozIAsyncHistory.idl
@@ +161,5 @@
> +[scriptable, function, uuid(994092bf-936f-449b-8dd6-0941e024360d)]
> +interface mozIVisitedStatusCallback : nsISupports
> +{
> +  /**
> +   * Notifies whether a certain uri has visits.

nit: s/uri/URI/

@@ +198,5 @@
>    void updatePlaces(in jsval aPlaceInfo,
>                      [optional] in mozIVisitInfoCallback aCallback);
>  
> +  /**
> +   * Checks if a given URI has visits.

nit: "has been visited"

@@ +201,5 @@
> +  /**
> +   * Checks if a given URI has visits.
> +   *
> +   * @param aURI
> +   *        The uri to check for.

nit: s/uri/URI/

::: toolkit/components/places/tests/unit/test_isURIVisited.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Tests functionality of the isURIVisited API.
> +
> +const SCHEMAS = {

typo nit: SCHEMES

(and where used below)

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.2

Sorry Gavin for the second SR request in the day, trying to change longstanding issues is a pain, requesting quick API changes too :(

Btw, some note:
- I didn't use isVisited cause I don't want to clash with nsIBrowserHistory
- I was recently thinking if I should rather make a generic mozIPlacesBoolCallback to be reused in any API that just need to return a bool. But it's usually dangerous to name things before needing them, so I decided for a more specific mozIVisitedStatusCallback.
- these APIs are all marked experimental since we are still iterating over the whole Places thing and we don't want implementers to consider them immutable like the old ones, they may change quickly. It's likely we can remove that once we have also a decent shape for mozIAsyncBookmarks and we figure out they cover well enough our needs.

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.3

Gavin suggested to add the URI we are notifying about in the callback, that is a good suggestion for when you want to reuse the same handler. So I added it here.

Comment 6

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bdf6415e2db

Comment 7

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/9bdf6415e2db

Comment 8

[Mark Banner (:standard8)]

I checked in a unit-test only bustage fix for apps that implement the imap protocol, with r=mak over irc:

https://hg.mozilla.org/mozilla-central/rev/e68aa21fd927

Comment 9

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/mozIVisitStatusCallback
https://developer.mozilla.org/en/XPCOM_Interface_Reference/mozIAsyncHistory#isURIVisited%28%29

And mentioned on Firefox 11 for developers.