615992 - IsBookmarked can lock for each onVisit call

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

onVisit the bookmarks service checks if the visit is for a bookmark. We should make this check async and eventually on the cloned connection.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 499213 [details] [diff] [review]
patch v1.0

>+  struct ItemChangeData {
>+    PRInt64 itemId;
>+    nsCOMPtr<nsIURI> uri;
>+    nsCString property;
>+    PRBool isAnnotation;
nit: bool

>+++ b/toolkit/components/places/tests/bookmarks/test_async_observers.js
>+  onItemChanged: function(aItemId, aProperty, aIsAnnotation, aNewValue,
>+                          aLastModified, aItemType)
>+  {
>+    print("Check that we got the correct change information.");
nit: please use do_log_info (https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/head_common.js#600) so we get the proper TEST-INFO bits

>+  onItemVisited: function(aItemId, aVisitId, aTime)
>+  {
>+    print("Check that we got the correct visit information.");
ditto

>+  QueryInterface: XPCOMUtils.generateQI([
>+    Ci.nsINavBookmarkObserver
nit: trailing comma

>+let tests = [
nit: use gTests

>+    PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
>+                                         uri("http://book.ma.rk/"),
global nit: I'd prefer us to stop using this helper method so that we can link to test code when people ask for samples.  NetUtil.newURI isn't too terribly long

>+function run_next_test()
>+{
>+  if (tests.length == 0) {
>+    PlacesUtils.bookmarks.removeObserver(observer, false);
>+    do_test_finished();
>+    return;
>+  }
>+
>+  observer.reset();
>+  (tests.shift())();
>+}
I'd really prefer to use the global run_next_test method which is being added in bug 606966 part 2 (which can land independently of that bug).

r=sdwilsh

Comment 3

[Marco Bonardo [:mak]]

(In reply to comment #2)
> Comment on attachment 499213 [details] [diff] [review]
> patch v1.0
> 
> >+  struct ItemChangeData {
> >+    PRInt64 itemId;
> >+    nsCOMPtr<nsIURI> uri;
> >+    nsCString property;
> >+    PRBool isAnnotation;
> nit: bool

this is not for internal use, I get it as a PRBool and I pass it to a method taking PRBool, so it's just easier to retain it as a PRBool

> 
> >+  QueryInterface: XPCOMUtils.generateQI([
> >+    Ci.nsINavBookmarkObserver
> nit: trailing comma

as it is I can just use a prefix comma to add further rows, that's how we did usually afaict

Comment 4

[Shawn Wilsher :sdwilsh]

(In reply to comment #3)
> as it is I can just use a prefix comma to add further rows, that's how we did
> usually afaict
That's how we do it in c++ land where trailing commas are illegal, but in js land, we have done the trailing comma.

Comment 5

[Marco Bonardo [:mak]]

(In reply to comment #2)
> I'd really prefer to use the global run_next_test method which is being added
> in bug 606966 part 2 (which can land independently of that bug).

Actually, that code has some glitch. You call do_test_pending, execute the test and immediately call do_test_finished, even if actually an async test would have not yet finished. That could cause us to skip or execute the last test at the wrong moment.
Plus, if the test calls do_test_pending before run_next_test, everything hangs because run_next_test does not expect nor closes an external pending.
Can we avoid managing pending and finished in run_next_next and let the test manage them?

Comment 6

[Marco Bonardo [:mak]]

otherwise we could do

  function _run_next_test() {
    if (gTestIndex > 0) {
      do_test_finished();
    }
    if (gTestIndex < gTests.length) {

so we finish on next run_next_test call skipping the first call.
This won't solve the external pending call, but in such a case we could just add a final test that calls it.

Comment 7

[Marco Bonardo [:mak]]

hm, nvm comment 6, we would end up executing only the first test and then the first do_test_finished would think all test is finished...
This async handling does not pertain to this helper imo.

Comment 8

[Marco Bonardo [:mak]]

so, if you really want to move those in the helper we could do

  function _run_next_test() {
    if (gTestIndex < gTests.length) {
      if (gTestIndex == 0) {
        do_test_pending();
      }
      /* run test */
    }
    else {
      do_test_finished();
    }
  }

this will just wrap everything once. if the test calls pending before, it will have to call finished in the last test.

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> Actually, that code has some glitch. You call do_test_pending, execute the test
> and immediately call do_test_finished, even if actually an async test would
> have not yet finished. That could cause us to skip or execute the last test at
> the wrong moment.
> Plus, if the test calls do_test_pending before run_next_test, everything hangs
> because run_next_test does not expect nor closes an external pending.
> Can we avoid managing pending and finished in run_next_next and let the test
> manage them?
This was copied out of the storage test harness, which assumes that the test will be calling do_test_pending, and do_test_finished itself.  There was a reason for also having it in run_next_test, but I cannot recall what that was anymore :/

Comment 10

[Marco Bonardo [:mak]]

hm, I don't see any valid reason to have both, either we require the test to call them always (And sounds a bit crazy for sync tests), or we do like in comment 8, imo.

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Could you please recheck the changes to tests?

Actually what you did (and what storage does) looks useless because do_execute_soon already wraps the call in a pending/finished, so your additional calls are just wrapping these calls (and interleaving with them).
And both calls happen before the function has really finished (thus the need for storage tests to have a external do_test_pending

I think that with my changes we can avoid the external pending call and still be sure both sync and async tests are correctly handled. Then if we want to add a do_test_pending call to the test just to make clear it's an async test, we can do it and then we just have to add a do_test_finished before/after the last run_next_test.

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch v1.2

ehr, I'm not sure if I had qrefreshed

Comment 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 501135 [details] [diff] [review]
patch v1.2

looks good to me

r=sdwilsh

Comment 14

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/093bd23aff20

Comment 15

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/093bd23aff20