Closed Bug 702810 Opened 8 years ago Closed 8 years ago

mozIAsyncHistory should expose an async isURIVisited method

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

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()
Blocks: livemarksIO
Flags: in-testsuite+
Summary: mozIAsyncHistory should expose an isVisited method → mozIAsyncHistory should expose an async isURIVisited method
Attached patch patch v1.0 (obsolete) — Splinter Review
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.
Attachment #576917 - Flags: review?(dietrich)
Attached patch patch v1.1 (obsolete) — Splinter Review
forgot to remove a useless part of the test.
Attachment #576917 - Attachment is obsolete: true
Attachment #576917 - Flags: review?(dietrich)
Attachment #576919 - Flags: review?(dietrich)
Keywords: dev-doc-needed
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)
Attachment #576919 - Flags: review?(dietrich) → review+
Attached patch patch v1.2 (obsolete) — Splinter Review
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.
Attachment #576919 - Attachment is obsolete: true
Attachment #577436 - Flags: superreview?(gavin.sharp)
Attached patch patch v1.3Splinter Review
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.
Attachment #577436 - Attachment is obsolete: true
Attachment #577436 - Flags: superreview?(gavin.sharp)
Attachment #577576 - Flags: superreview?(gavin.sharp)
Attachment #577576 - Flags: superreview?(gavin.sharp) → superreview+
https://hg.mozilla.org/mozilla-central/rev/9bdf6415e2db
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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
Blocks: 484928
You need to log in before you can comment on or make changes to this bug.