All users were logged out of Bugzilla on October 13th, 2018

IsBookmarked can lock for each onVisit call

RESOLVED FIXED in mozilla2.0b10

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
mozilla2.0b10
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Updated

8 years ago
blocking2.0: --- → final+

Updated

8 years ago
blocking2.0: final+ → -
status2.0: --- → wanted

Updated

8 years ago
blocking2.0: - → final+
status2.0: wanted → ---
(Assignee)

Comment 1

8 years ago
Created attachment 499213 [details] [diff] [review]
patch v1.0
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]
(Assignee)

Comment 3

8 years ago
(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.
(Assignee)

Comment 5

8 years ago
(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?
(Assignee)

Comment 6

8 years ago
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.
(Assignee)

Comment 7

8 years ago
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.
(Assignee)

Comment 8

8 years ago
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 :/
(Assignee)

Comment 10

8 years ago
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.
(Assignee)

Comment 11

8 years ago
Created attachment 501134 [details] [diff] [review]
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.
Attachment #499213 - Attachment is obsolete: true
Attachment #501134 - Flags: review?(sdwilsh)
(Assignee)

Comment 12

8 years ago
Created attachment 501135 [details] [diff] [review]
patch v1.2

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+

Updated

8 years ago
Whiteboard: [has review][needs new patch] → [has review][can land]
(Assignee)

Comment 14

8 years ago
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
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
(Assignee)

Updated

8 years ago
Depends on: 629285
You need to log in before you can comment on or make changes to this bug.