629539 - test that proper visit notifications are dispatched (test_async_history_api.js)

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Shawn Wilsher :sdwilsh]

Follow-up to bug 606966.  We need to test that proper visit notifications are dispatched.

Comment 1

[Shawn Wilsher :sdwilsh]

We already have onTitleChanged covered.  Happy days!

Comment 2

[Shawn Wilsher :sdwilsh]

onPageChanged cannot be tested due to bug 626836.

Comment 3

[Shawn Wilsher :sdwilsh]

This means we only need to test for:
onVisit
uri-visit-saved observer service topic

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 512644 [details] [diff] [review]
v1.0

>diff --git a/toolkit/components/places/tests/unit/test_async_history_api.js b/toolkit/components/places/tests/unit/test_async_history_api.js

> const TEST_DOMAIN = "http://mozilla.org/";
> const TOPIC_UPDATEPLACES_COMPLETE = "places-updatePlaces-complete";
>+const URI_VISIT_SAVED = "uri-visit-saved";

use TOPIC_ prefix coherently please

>+VisitObserver.prototype = {
>+  __proto__: NavHistoryObserver.prototype,
>+  onVisit: function(aURI,
>+                    aVisitId,
>+                    aTime,
>+                    aSessionId,
>+                    aReferringId,
>+                    aTransitionType)
>+  {
>+    do_log_info("onVisit(" + aURI.spec + ", " + aVisitId + ", " + aTime +
>+                ", " + aSessionId + ", " + aReferringId + ", " +
>+                aTransitionType + ")");
>+    if (!this.uri.equals(aURI)) {
>+      return;
>+    }
>+    this.callback(aURI, aTime, aTransitionType);

aURI in the callback seems completely useless, this listener is already bound to a certain uri (indeed you don't use aURI later)

>+  let observer = {
>+    observe: function(aSubject, aTopic, aData)
>+    {
>+      do_log_info("observe(" + aSubject + ", " + aTopic + ", " + aData + ")");
>+      do_check_true(aSubject instanceof Ci.nsIURI);
>+      do_check_true(aSubject.equals(place.uri));
>+
>+      Services.obs.removeObserver(observer, URI_VISIT_SAVED);
>+      finisher();
>+    },
>+  };
>+  Services.obs.addObserver(observer, URI_VISIT_SAVED, false);

you can use a function here rather than a object, saving some line of code

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> use TOPIC_ prefix coherently please
I would prefer to keep the CPP and JS constants named the same though.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #6)
> (In reply to comment #5)
> > use TOPIC_ prefix coherently please
> I would prefer to keep the CPP and JS constants named the same though.

ah ok, then I guess why the cpp constants don't follow the same schema, I think most of them have TOPIC somewhere in the name.
Btw, I'm fine both ways then.

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

For checkin.  This is a test-only change, so it doesn't need approval.

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Something just landed that bitrots this.  This fixes the bitrot.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 512902 [details] [diff] [review]
v1.2

not really...hg fail

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

OK, this is it for real this time.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/fc4b2389e865