Closed Bug 615992 Opened 10 years ago Closed 10 years ago

IsBookmarked can lock for each onVisit call

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b10
Tracking Status
blocking2.0 --- final+

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [fixed-in-places][softblocker])

Attachments

(1 file, 2 obsolete files)

onVisit the bookmarks service checks if the visit is for a bookmark. We should make this check async and eventually on the cloned connection.
blocking2.0: --- → final+
blocking2.0: final+ → -
status2.0: --- → wanted
blocking2.0: - → final+
status2.0: wanted → ---
Attached patch patch v1.0 (obsolete) — Splinter Review
Attachment #499213 - Flags: review?(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
Attachment #499213 - Flags: review?(sdwilsh) → review+
Whiteboard: [has review][needs new patch]
(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
(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.
(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?
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.
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.
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.
(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 :/
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.
Attached patch patch v1.1 (obsolete) — Splinter Review
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.
Attachment #499213 - Attachment is obsolete: true
Attachment #501134 - Flags: review?(sdwilsh)
Attached patch patch v1.2Splinter Review
ehr, I'm not sure if I had qrefreshed
Attachment #501134 - Attachment is obsolete: true
Attachment #501135 - Flags: review?(sdwilsh)
Attachment #501134 - Flags: review?(sdwilsh)
Comment on attachment 501135 [details] [diff] [review]
patch v1.2

looks good to me

r=sdwilsh
Attachment #501135 - Flags: review?(sdwilsh) → review+
Whiteboard: [has review][needs new patch] → [has review][can land]
http://hg.mozilla.org/projects/places/rev/093bd23aff20
Flags: in-testsuite+
Whiteboard: [has review][can land] → [fixed-in-places]
Whiteboard: [fixed-in-places] → [fixed-in-places][soft blocker]
Whiteboard: [fixed-in-places][soft blocker] → [fixed-in-places][softblocker]
http://hg.mozilla.org/mozilla-central/rev/093bd23aff20
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
Depends on: 629285
You need to log in before you can comment on or make changes to this bug.