Last Comment Bug 636603 - mozIAsyncHistory: only call mozIVisitInfoCallback on failure
: mozIAsyncHistory: only call mozIVisitInfoCallback on failure
Status: RESOLVED FIXED
[places-next-wanted][fixed in service...
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Allison Naaktgeboren :ally
:
Mentors:
Depends on: 678129 678259
Blocks: 636304 676110
  Show dependency treegraph
 
Reported: 2011-02-24 15:39 PST by Philipp von Weitershausen [:philikon]
Modified: 2011-10-17 10:44 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed changes to the idl file (3.42 KB, patch)
2011-07-22 16:09 PDT, Allison Naaktgeboren :ally
sdwilsh: review+
Details | Diff | Splinter Review
api change for super review, with comment & name nit addressed (3.48 KB, patch)
2011-07-25 12:06 PDT, Allison Naaktgeboren :ally
no flags Details | Diff | Splinter Review
patch for superreview (3.45 KB, patch)
2011-07-25 12:17 PDT, Allison Naaktgeboren :ally
robert.strong.bugs: superreview+
Details | Diff | Splinter Review
wip patch, code complete, but not test complete (39.66 KB, patch)
2011-07-26 21:56 PDT, Allison Naaktgeboren :ally
philipp: feedback+
mak77: feedback+
Details | Diff | Splinter Review
tested completed, but a few are hanging, wip patch for help (48.76 KB, patch)
2011-07-28 16:25 PDT, Allison Naaktgeboren :ally
no flags Details | Diff | Splinter Review
version of 1 api,implementation, & tests (47.21 KB, patch)
2011-07-29 09:47 PDT, Allison Naaktgeboren :ally
no flags Details | Diff | Splinter Review
version 1 of the sync engine change to use api changes (2.06 KB, patch)
2011-08-03 11:00 PDT, Allison Naaktgeboren :ally
no flags Details | Diff | Splinter Review
Part 2 (v2): Make Sync use new API (2.30 KB, patch)
2011-08-03 16:07 PDT, Allison Naaktgeboren :ally
philipp: review+
Details | Diff | Splinter Review
Part 1 (v3) (35.71 KB, patch)
2011-08-03 16:11 PDT, Allison Naaktgeboren :ally
no flags Details | Diff | Splinter Review
Part 1 (v3) : API change, implementation (32.02 KB, patch)
2011-08-03 20:30 PDT, Allison Naaktgeboren :ally
mak77: review-
Details | Diff | Splinter Review
Part 1 (v4) : API change, implementation (31.91 KB, patch)
2011-08-04 10:27 PDT, Allison Naaktgeboren :ally
mak77: review+
Details | Diff | Splinter Review
Part 1 (v5) : API change, implementation (31.47 KB, patch)
2011-08-04 16:41 PDT, Allison Naaktgeboren :ally
no flags Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2011-02-24 15:39:36 PST
We need to reduce the GC churn during a history sync. Sync does not need the placeInfo object if the record has been stored successfully. So either we should only call the mozIVisitInfoCallback on failure or have a separate callback just for errors.
Comment 1 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-02-24 16:18:29 PST
Provided this can be opted in and out, maybe  adding a callbackMode (fancy name) property that can be set to ALWAYS, ONERROR, ... This way we could preserve all uses of a general purpose API (I'm starting thinking we should have created a mozIPlacesSynchronitazion specialized interface for Sync (or other synchronization backends) rather than trying to come with both a specialized-general-purpose interface).
Comment 2 Philipp von Weitershausen [:philikon] 2011-02-24 16:19:40 PST
That works for me. Anything that gets us out of unnecessary churn. 99.99...% of the time we don't care about the callback!
Comment 3 Shawn Wilsher :sdwilsh 2011-02-24 16:21:12 PST
(In reply to comment #1)
> Provided this can be opted in and out, maybe  adding a callbackMode (fancy
> name) property that can be set to ALWAYS, ONERROR, ... This way we could
> preserve all uses of a general purpose API (I'm starting thinking we should
> have created a mozIPlacesSynchronitazion specialized interface for Sync (or
> other synchronization backends) rather than trying to come with both a
> specialized-general-purpose interface).
We were originally talking about passing in an object instead of just a function, and if it didn't have a property, we wouldn't call it.
Comment 4 Allison Naaktgeboren :ally 2011-07-22 16:09:56 PDT
Created attachment 547836 [details] [diff] [review]
proposed changes to the idl file

Going with sdwilsh's suggestion of passing in an object with callback functions on it. these are the idl changes. If these are okay, we can proceed with that.  Philikon points that this means we can remove the places-updateplaces-complete observer in the future and add a handleCompletion() callback.
Comment 5 Shawn Wilsher :sdwilsh 2011-07-25 10:32:23 PDT
Comment on attachment 547836 [details] [diff] [review]
proposed changes to the idl file

>+   * @param [optional] aCallbacks
nit: this should still be `aCallback`

r=sdwilsh
Comment 6 Philipp von Weitershausen [:philikon] 2011-07-25 10:37:38 PDT
Comment on attachment 547836 [details] [diff] [review]
proposed changes to the idl file

One thought crossed my mind just now: Since IDL doesn't allow us to specify that handleError and handleResult are optional methods on the mozIVisitInfoCallbacks object, we might want to explain that in the comment.
Comment 7 Allison Naaktgeboren :ally 2011-07-25 11:29:42 PDT
nit addressed, comments added, but I will not upload a new patch for that. Thanks sdwilsh!

sdwilsh, I've changed it, but could you please explain what convention/rules governs your nit so that I will know in the future? It an argument object meant to contain 1 or more callbacks so I don't understand why it should be singular and not plural.
Comment 8 Allison Naaktgeboren :ally 2011-07-25 12:06:54 PDT
Created attachment 548244 [details] [diff] [review]
api change for super review, with comment & name nit addressed
Comment 9 Allison Naaktgeboren :ally 2011-07-25 12:17:14 PDT
Created attachment 548247 [details] [diff] [review]
patch for superreview
Comment 10 Allison Naaktgeboren :ally 2011-07-26 21:56:45 PDT
Created attachment 548689 [details] [diff] [review]
wip patch, code complete, but not test complete

wip in progress patch.
--assumes api awaiting superreview is unchanged
--implementation code complete
-unwritten:
--old tests are partially updated for new api, any below '//ally' have not been updated
--new tests (since both callbacks are optional)
---with no callbacks defined
---with success callback but not error callback
---with error callback but not success callback
---with both defined

any and all feedback welcome at this stage, but please dont focus too much on the nits, there will be another round for that.
Comment 11 Philipp von Weitershausen [:philikon] 2011-07-26 22:26:10 PDT
Comment on attachment 548689 [details] [diff] [review]
wip patch, code complete, but not test complete

>Bug 636603 - mozIAsyncHistory: only call mozIVisitInfoCallback on failure
>
>part 1: idl changes

You kinda lumped the IDL changes in the implementation here, so you probably want to split those into 2-3 patches (I typically do IDL, implementation, tests, especially when the tests contain lots of stupid mechanical changes like in this case.) Alternatively you could get rid of that line in the checkin comment that alludes to multiple parts ;)

> /**
>  * Notifies a callback object about completion.
>  */
> class NotifyCompletion : public nsRunnable
> {
>+//ally-note to be the only call to onComplete

Yes, because much of the other stuff in this file runs on the I/O thread and the callbacks always need to be run on the main thread. NotifyCompletion takes care of that (the other components dispatch it to the main thread).

>+    if(mResult == NS_OK) {
>+      (void)mCallback->HandleResult(place);
>+    }
>+    else {
>+      (void)mCallback->HandleError(mResult, place);
>+    }
> 
>-    (void)mCallback->OnComplete(mResult, place);
>     return NS_OK;
...
>+// ally-todo if handleError is defined when there is an error, call it
>+// ally-todo if handleResult is defined when there is a success, call it

I believe you have already addressed those with your changes to NotifyCompletion. We simply call mCallback->HandleResult. This method will exist in C++ land. If there's no underlying JS method on that object, XPCOM will return NS_ERROR_NO_METHOD. We're fine with that. In fact, we're fine with any return value -- hence the cast to void :)

 NS_ERROR_NO_METHOD

>+// ally-todo unbork compiler
>+// ally-todo CanAddURI will trigger callback as well ? 

Yes, but everything that triggers the callback does so via NotifyCompletion which you touched, so I think you're good here.


Looks good! Nearly there, just mechanical changes to the tests and a few extra test cases to go! :)
Comment 12 Richard Newman [:rnewman] 2011-07-26 23:57:57 PDT
Comment on attachment 548689 [details] [diff] [review]
wip patch, code complete, but not test complete

Review of attachment 548689 [details] [diff] [review]:
-----------------------------------------------------------------

Further to Philipp's comments, as we race on review...

Leaving flag, as this is a WIP.

::: toolkit/components/places/History.cpp
@@ +538,3 @@
>  public:
> +  
> +  NotifyCompletion(mozIVisitInfoCallbacks* aCallback,

Nit: would prefer "aCallbacks" to agree with the type. (Same elsewhere, of course.)

::: toolkit/components/places/mozIAsyncHistory.idl
@@ +139,5 @@
> +
> +  /**
> +   * Called for each visit added, title change, or guid change when passed to
> +   * mozIAsyncHistory::updatePlaces.  If more than one operation is done for 
> +   * a given visit, only one callback will be given (i.e. title change and 

e.g., not i.e.?

http://public.wsu.edu/~brians/errors/e.g.html

@@ +163,5 @@
>     * @param aPlaceInfo
>     *        The mozIPlaceInfo object[s] containing the information to store or
>     *        update.  This can be a single object, or an array of objects.
>     * @param [optional] aCallback
> +   *        A mozIVisitInfoCallbacks object which contains callbacks to be 

s/contains/consists of/

::: toolkit/components/places/tests/unit/test_async_history_api.js
@@ +422,5 @@
>    });
>  
>    let callbackCount = 0;
> +  gHistory.updatePlaces(places, {
> +    handleError: function handleError(aResultCode, aPlaceInfo) {

Y'know, you could phrase these in terms of a couple of helpers like:

  function positive(f) {
    return {
      handleError: function handleError(aResultCode, aPlaceInfo) {
        do_throw("Unexpected error, success expected.");
      },
      handleResult: f
    };
  }

and the inverse. Usage:

  gHistory.updatePlaces(places, negative(function ...))

which would drop a lot of these diffs down to one changed line.
Comment 13 Philipp von Weitershausen [:philikon] 2011-07-27 00:02:29 PDT
(In reply to comment #12)
> Leaving flag, as this is a WIP.

You should probably clear the r? flag ;)

> > +  NotifyCompletion(mozIVisitInfoCallbacks* aCallback,
> 
> Nit: would prefer "aCallbacks" to agree with the type. (Same elsewhere, of
> course.)

See sdwilsh's comment 5.

> > +  /**
> > +   * Called for each visit added, title change, or guid change when passed to
> > +   * mozIAsyncHistory::updatePlaces.  If more than one operation is done for 
> > +   * a given visit, only one callback will be given (i.e. title change and 
> 
> e.g., not i.e.?
> 
> http://public.wsu.edu/~brians/errors/e.g.html

Pretty sure we're enumerating here, not listing examples. (The i.e. was in sdwilsh's original comment, Ally merely preserved it.)

> Y'know, you could phrase these in terms of a couple of helpers like:
> 
>   function positive(f) {
>     return {
>       handleError: function handleError(aResultCode, aPlaceInfo) {
>         do_throw("Unexpected error, success expected.");
>       },
>       handleResult: f
>     };
>   }
> 
> and the inverse. Usage:
> 
>   gHistory.updatePlaces(places, negative(function ...))
> 
> which would drop a lot of these diffs down to one changed line.

I agree (and already suggested it to Ally in person). Except "positive" and "negative" are poor names for functions, even when they're helpers. expectSuccess and expectFailure or similar would be more suitable.
Comment 14 Richard Newman [:rnewman] 2011-07-27 00:05:34 PDT
Comment on attachment 548689 [details] [diff] [review]
wip patch, code complete, but not test complete

(In reply to comment #13)

> See sdwilsh's comment 5.

Heh. I disagree, but I'll leave it up to the C++ guy :)

> I agree (and already suggested it to Ally in person). Except "positive" and
> "negative" are poor names for functions, even when they're helpers.
> expectSuccess and expectFailure or similar would be more suitable.

Sure. Direction not recommendation :)  Leave it up to the dev to think of the right name!
Comment 15 Allison Naaktgeboren :ally 2011-07-27 12:34:21 PDT
(In reply to comment #11)
> Comment on attachment 548689 [details] [diff] [review] [review]
> wip patch, code complete, but not test complete
> 
> >Bug 636603 - mozIAsyncHistory: only call mozIVisitInfoCallback on failure
> >
> >part 1: idl changes
> 
> You kinda lumped the IDL changes in the implementation here, so you probably
> want to split those into 2-3 patches (I typically do IDL, implementation,
> tests, especially when the tests contain lots of stupid mechanical changes
> like in this case.) Alternatively you could get rid of that line in the
> checkin comment that alludes to multiple parts ;)

How do I change the checkin comment?
Comment 16 Philipp von Weitershausen [:philikon] 2011-07-27 12:38:19 PDT
(In reply to comment #15)
> How do I change the checkin comment?

hg qrefresh -e

https://developer.mozilla.org/en/Mercurial_queues#The_life_of_a_patch is your friend.
Comment 17 Allison Naaktgeboren :ally 2011-07-27 12:45:34 PDT
> 
> ::: toolkit/components/places/tests/unit/test_async_history_api.js
> @@ +422,5 @@
> >    });
> >  
> >    let callbackCount = 0;
> > +  gHistory.updatePlaces(places, {
> > +    handleError: function handleError(aResultCode, aPlaceInfo) {
> 
> Y'know, you could phrase these in terms of a couple of helpers like:
> 
>   function positive(f) {
>     return {
>       handleError: function handleError(aResultCode, aPlaceInfo) {
>         do_throw("Unexpected error, success expected.");
>       },
>       handleResult: f
>     };
>   }
> 
> and the inverse. Usage:
> 
>   gHistory.updatePlaces(places, negative(function ...))
> 
> which would drop a lot of these diffs down to one changed line.

Sounds great, but my JavaScript fu must be weak, I don't understand (philikon, I don't remember this conversation?).   sdwilsh, mak, would that change in the test style/structure be acceptable?
Comment 18 Philipp von Weitershausen [:philikon] 2011-07-27 12:51:40 PDT
(In reply to comment #17)
> Sounds great, but my JavaScript fu must be weak, I don't understand
> (philikon, I don't remember this conversation?).   sdwilsh, mak, would that
> change in the test style/structure be acceptable?


function expectResult(handleResult) {
  return {
    handleResult: handleResult,
    handleError: function handleError(aResultCode, aPlaceInfo) {
      do_throw("Unexpected error: " + aResultCode);
    }
  };
}

and invert for expectError. Use like so:

  gHistory.updatePlaces(places, expectResult(function handleResult(aPlaceInfo) {
    // same stuff here, no reindention
  }));
Comment 19 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-07-27 15:50:33 PDT
I'd be fine with the helpers, provided they have better naming than positive and negative or angel and devil :)
Comment 20 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-07-27 15:57:39 PDT
A couple notes on the idl:
1. Personally I'd have retained the original mozIVisitInfoCallback name, rather than mozIVisitInfoCallbacks. The interface describes one single object, the fact it has multiple methods is a fact of poor interest, and would be a simpler mind association to the existing interfaces like mozIStorageStatementCallback.
2. We should implement handleCompletion before next merge, so we coalesce interface changes in a single release rather than spreading to multiple releases. Like we did for observers.
Comment 21 Philipp von Weitershausen [:philikon] 2011-07-27 16:52:40 PDT
(In reply to comment #20)
> A couple notes on the idl:
> 1. Personally I'd have retained the original mozIVisitInfoCallback name,
> rather than mozIVisitInfoCallbacks. The interface describes one single
> object, the fact it has multiple methods is a fact of poor interest, and
> would be a simpler mind association to the existing interfaces like
> mozIStorageStatementCallback.

Oh hrm, didn't realize mozIStorageStatementCallback was singular too. I agree with you then. Fortunately a simple fix :)

> 2. We should implement handleCompletion before next merge, so we coalesce
> interface changes in a single release rather than spreading to multiple
> releases. Like we did for observers.

I'm not opposed to this (e.g. making it part 2 of this bug), though I'm not sure it's really necessary. We clearly marked mozIAsyncHistory experimental so we could rev it quickly as we need it...
Comment 22 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-07-27 17:04:05 PDT
So, I'd like to see a patch with mozIVisitInfoCallback name restored (apart from previous reasons, that will reduce number of changes) and without spurious comments.
Apart thism the global approach and changes seem fine, but I can't review the current patch in its status.
Comment 23 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-07-27 17:07:11 PDT
(In reply to comment #21)
> I'm not opposed to this (e.g. making it part 2 of this bug), though I'm not
> sure it's really necessary. We clearly marked mozIAsyncHistory experimental
> so we could rev it quickly as we need it...

Can be done in a follow-up, I mostly meant "If we could fix that follow-up before next version, you'd get bonus points, otherwise we'll survive still"
Comment 24 Allison Naaktgeboren :ally 2011-07-28 16:25:28 PDT
Created attachment 549264 [details] [diff] [review]
tested completed, but a few are hanging, wip patch for help

several tests are hanging. this contains cpp debug and js debug output and should only execute one of the hanging tests.
Comment 25 Richard Newman [:rnewman] 2011-07-28 16:26:53 PDT
Comment on attachment 549264 [details] [diff] [review]
tested completed, but a few are hanging, wip patch for help

Looking.
Comment 26 Philipp von Weitershausen [:philikon] 2011-07-28 16:44:26 PDT
Comment on attachment 549264 [details] [diff] [review]
tested completed, but a few are hanging, wip patch for help

Not a final patch, removing review flags.
Comment 27 Richard Newman [:rnewman] 2011-07-28 17:16:08 PDT
This changed version works for me:

http://rnewman.pastebin.mozilla.org/1284568

(Yes, pastebin. You're going to update the patch :D)
Comment 28 Allison Naaktgeboren :ally 2011-07-29 09:47:54 PDT
Created attachment 549406 [details] [diff] [review]
version of 1 api,implementation, & tests

Marco, this is missing the change back to singular name and it could use some serious nit picking before going for final review.

thanks
Comment 29 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-01 15:42:20 PDT
Comment on attachment 549406 [details] [diff] [review]
version of 1 api,implementation, & tests

Review of attachment 549406 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/mozIAsyncHistory.idl
@@ +127,4 @@
>  {
>    /**
> +   * Called when the given mozIPlaceInfo object could not be processed.
> +   * It is an optional method. 

how can this be an optional method? won't xpcom loudly complain about the missing part of the interface? I think we have similar issues with nsITimer interfaces missing Notify for example. Sure the test won't fail and this will work, but it's going to make noise afaict. I think it's fine to request implementers to add empty functions if they don't care, but we don't have a clean way to annotate optional methods in idl.

@@ +131,3 @@
>     *
>     * @param aResultCode
> +   *        nsresult indicating failure reason.

"the failure reason"

@@ +140,5 @@
> +  /**
> +   * Called for each visit added, title change, or guid change when passed to
> +   * mozIAsyncHistory::updatePlaces.  If more than one operation is done for 
> +   * a given visit, only one callback will be given (i.e. title change and 
> +   * add visit). It is an optional method.

ditto on optional method.

@@ +152,5 @@
>  
>  /**
>   * @status EXPERIMENTAL
>   */
> +[scriptable, uuid(e47ecb0f-0740-41fb-bd43-821a607a0287)]

clearly this change on mozIAsyncHistory won't be needed once you restore the original mozIVisitInfoCallback name

::: toolkit/components/places/tests/unit/test_async_history_api.js
@@ +454,5 @@
>    });
>  
>    let callbackCount = 0;
> +  gHistory.updatePlaces(places, expectHandleError(
> +    function (aResultCode, aPlaceInfo) {

if you oneline these 2 lines, and avoid one indentation, the patch will be largely smaller and easier to review. Most of the patch is indeed made up of indentation changes.

@@ +464,5 @@
> +      if (++callbackCount == places.length) {
> +        waitForAsyncUpdates(run_next_test);
> +      }
> +    })
> +  );

clearly these 2 would end up being onelined as well

@@ +1316,3 @@
>    });
> +  
> +  gHistory.updatePlaces(places, {} );

I don't think we should advertize something like this, a null callback is fine, a broken one should be considered bad coding practice.
Comment 30 Philipp von Weitershausen [:philikon] 2011-08-01 16:00:54 PDT
(In reply to comment #29)
> >    /**
> > +   * Called when the given mozIPlaceInfo object could not be processed.
> > +   * It is an optional method. 
> 
> how can this be an optional method? won't xpcom loudly complain about the
> missing part of the interface?

Nope. See the tests which prove that it works. If the object doesn't have the given method, XPConnect will return NS_ERROR_XPC_CANT_GET_METHOD_INFO or something like that. The call will fail, for sure, but we don't care about the return value anyway.

Pretty sure this is how mozIStorageCallback does it too.

> I think we have similar issues with nsITimer
> interfaces missing Notify for example. Sure the test won't fail and this
> will work, but it's going to make noise afaict.

What noise?

> I think it's fine to request
> implementers to add empty functions if they don't care, but we don't have a
> clean way to annotate optional methods in idl.

Not having a function there *at all* is the point of this patch. Crossing XPCOM borders, even for an empty function, is expensive and it creates GC-able objects that we desperately want to avoid in Sync.

> >    });
> > +  
> > +  gHistory.updatePlaces(places, {} );
> 
> I don't think we should advertize something like this, a null callback is
> fine, a broken one should be considered bad coding practice.

I'm not sure what you mean. This is a valid use of the interface, even if it's a bit nonsensical. The point is to test that not passing either callback method doesn't break anything.
Comment 31 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-01 16:28:52 PDT
Sorry I was confusing with the various bugs we have about NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED exceptions, but there errors are actually handled and returned, while we ignore them.

So, yeah it's fine, sorry for alarm, feel free to ignore my comments on that.
Just please move those optional comments to a @note

A dumb question, if you define the method as a function() without arguments the cost is exactly the same as defining them or is there any sort of lazy construction? From your comment looks like cost is exactly the same?
Comment 32 Robert Strong [:rstrong] (use needinfo to contact me) 2011-08-01 16:35:38 PDT
Comment on attachment 548247 [details] [diff] [review]
patch for superreview

>diff --git a/toolkit/components/places/mozIAsyncHistory.idl b/toolkit/components/places/mozIAsyncHistory.idl
>--- a/toolkit/components/places/mozIAsyncHistory.idl
>+++ b/toolkit/components/places/mozIAsyncHistory.idl
>@@ -117,57 +117,68 @@
>    */
>   [implicit_jscontext]
>   readonly attribute jsval visits;
> };
> 
> /**
>  * @status EXPERIMENTAL
>  */
>-[scriptable,function, uuid(3b97ca3c-5ea8-424f-b429-797477c52302)]
>-interface mozIVisitInfoCallback : nsISupports
>+[scriptable, uuid(eb0b406f-8f57-4f2b-b0da-8883684b138a)]
>+interface mozIVisitInfoCallbacks : nsISupports
> {
>   /**
>-   * Called for each visit added, title change, or guid change when passed to
>-   * mozIAsyncHistory::updatePlaces.
>+   * Called when the given mozIPlaceInfo object could not be processed.
>+   * It is an optional method. 
That it is optional doesn't seem relevant here and below since it is actually an optional param for the updatePlaces method.

sr=rs with that fixed.
Comment 33 Philipp von Weitershausen [:philikon] 2011-08-01 20:56:52 PDT
(In reply to comment #32)
> >   /**
> >-   * Called for each visit added, title change, or guid change when passed to
> >-   * mozIAsyncHistory::updatePlaces.
> >+   * Called when the given mozIPlaceInfo object could not be processed.
> >+   * It is an optional method. 
> That it is optional doesn't seem relevant here and below since it is
> actually an optional param for the updatePlaces method.

Yes, the whole callback object is optional. That still separate from being ok with the individual methods gone AWOL (which is what we want). In other words, any of these are ok:

updatePlaces(places);
udpatePlaces(places, {});
updatePlaces(places, {handleError: ...});
updatePlaces(places, {handleSuccess: ...});
updatePlaces(places, {handleSuccess: ..., handleError: ...});
Comment 34 Philipp von Weitershausen [:philikon] 2011-08-01 21:01:50 PDT
(In reply to comment #31)
> A dumb question, if you define the method as a function() without arguments
> the cost is exactly the same as defining them or is there any sort of lazy
> construction? From your comment looks like cost is exactly the same?

Sorry, I'm having trouble parsing that sentence. Having XPConnect detect that the function is missing from the callback object altogether certainly perform better than invoking an empty function () {};. Yes, some things like 'arguments' are created lazily, but stuff like the closure scope aren't (though I'm not a Tracemonkey expert, so don't take my word for it.) In any case it's almost trivial to make those callbacks optional, and Sync only cares about handleError, so it seems like a no-brainer to me...
Comment 35 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-02 01:47:44 PDT
I was mostly curious if in case you don't consume the arguments object the cost is high as consuming it, I thought most of the problems were with GC of the places objects we pass to the callback, not with the closure scope. I'm fine with this, but having measures of the effects of having the callback, an empty function() {} or no function may be generally useful for future.
Comment 36 Philipp von Weitershausen [:philikon] 2011-08-02 10:37:03 PDT
(In reply to comment #35)
> I was mostly curious if in case you don't consume the arguments object the
> cost is high as consuming it, I thought most of the problems were with GC of
> the places objects we pass to the callback, not with the closure scope.

Yes, in a follow-up to this we could even save creating the PlaceInfo object ahead of time.

> I'm
> fine with this, but having measures of the effects of having the callback,
> an empty function() {} or no function may be generally useful for future.

Agreed.
Comment 37 Allison Naaktgeboren :ally 2011-08-02 15:34:37 PDT
(In reply to comment #32)
> Comment on attachment 548247 [details] [diff] [review] [diff] [details] [review]
> patch for superreview
> 
> >diff --git a/toolkit/components/places/mozIAsyncHistory.idl b/toolkit/components/places/mozIAsyncHistory.idl
> >--- a/toolkit/components/places/mozIAsyncHistory.idl
> >+++ b/toolkit/components/places/mozIAsyncHistory.idl
> >@@ -117,57 +117,68 @@
> >    */
> >   [implicit_jscontext]
> >   readonly attribute jsval visits;
> > };
> > 
> > /**
> >  * @status EXPERIMENTAL
> >  */
> >-[scriptable,function, uuid(3b97ca3c-5ea8-424f-b429-797477c52302)]
> >-interface mozIVisitInfoCallback : nsISupports
> >+[scriptable, uuid(eb0b406f-8f57-4f2b-b0da-8883684b138a)]
> >+interface mozIVisitInfoCallbacks : nsISupports
> > {
> >   /**
> >-   * Called for each visit added, title change, or guid change when passed to
> >-   * mozIAsyncHistory::updatePlaces.
> >+   * Called when the given mozIPlaceInfo object could not be processed.
> >+   * It is an optional method. 
> That it is optional doesn't seem relevant here and below since it is
> actually an optional param for the updatePlaces method.
> 
> sr=rs with that fixed.

Rstrong, given philikon's response in comment 33, do you still want me to remove the comment about the optional methods?
Comment 38 Allison Naaktgeboren :ally 2011-08-02 16:12:38 PDT

mak, just so we're clear, the results of the discussion above mean the changes that should be made are:
    1) The optional comment in the .idl is moved to an @note if rstrong does not want it removed. (If rstrong still wants it removed, this is moot.)

    2) 'nsresult indicating the failure reason' (add missing article)

    3) Nothing has to be changed for the 'gHistory.updatePlaces(places, {} );' in the js test file.

Things I am unclear about:
    A) "@@ +152,5 @@
>  
>  /**
>   * @status EXPERIMENTAL
>   */
> +[scriptable, uuid(e47ecb0f-0740-41fb-bd43-821a607a0287)]
clearly this change on mozIAsyncHistory won't be needed once you restore the original mozIVisitInfoCallback name"  Don't we still need to rev this since we changed the interface and removed a keyword from the [..] ?

    B) The indentation request you are making will cause violations of the style rules for that file (the 80 character limit in particular). Do you still desire this change? (and

    C) You also complain about the the same thing in the js test file. Your request to one line would also violate the 80 character limit. Do you still desire this change?
Comment 39 Philipp von Weitershausen [:philikon] 2011-08-02 16:17:00 PDT
>     A) "@@ +152,5 @@
> >  
> >  /**
> >   * @status EXPERIMENTAL
> >   */
> > +[scriptable, uuid(e47ecb0f-0740-41fb-bd43-821a607a0287)]
> clearly this change on mozIAsyncHistory won't be needed once you restore the
> original mozIVisitInfoCallback name"  Don't we still need to rev this since
> we changed the interface and removed a keyword from the [..] ?

We changed the interface and removed the [function] keyword from mozIVisitInfoCallback while Marco's comment is about mozIAsyncHistory.
Comment 40 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-02 16:31:32 PDT
(In reply to comment #38)
>     B) The indentation request you are making will cause violations of the
> style rules for that file (the 80 character limit in particular). Do you
> still desire this change? (and

I'm fine with the change, since it will greatly reduce size of the patch and number of changes.
The 80 chars limit is suggested, not mandatory. You can break it, if there is a clear advantage in doing so, either readability, or in this case size of the changes and blame preservation.
Comment 41 Allison Naaktgeboren :ally 2011-08-02 23:18:03 PDT
(In reply to comment #40)
> (In reply to comment #38)
> >     B) The indentation request you are making will cause violations of the
> > style rules for that file (the 80 character limit in particular). Do you
> > still desire this change? (and
> 
> I'm fine with the change, since it will greatly reduce size of the patch and
> number of changes.
> The 80 chars limit is suggested, not mandatory. You can break it, if there
> is a clear advantage in doing so, either readability, or in this case size
> of the changes and blame preservation.
Mostly I fear sdwilsh's censure.  When I was working on storage he was very strict about it.
Comment 42 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-03 01:22:29 PDT
no worries.
Comment 43 Allison Naaktgeboren :ally 2011-08-03 11:00:37 PDT
Created attachment 550430 [details] [diff] [review]
version 1 of the sync engine change to use api changes

philikon, the only real question I have for you is whether or not the two ending braces should stay on one line or not.  Thoughts?
Comment 44 Philipp von Weitershausen [:philikon] 2011-08-03 11:06:04 PDT
Comment on attachment 550430 [details] [diff] [review]
version 1 of the sync engine change to use api changes

>     let cb = Async.makeSyncCallback();
>-    let onPlace = function onPlace(result, placeInfo) {
>-      if (!Components.isSuccessCode(result)) {
>+    let onPlace = { handleResult: function handleResult(placeInfo) {

Um, I think you mean handleError here! I bet tests aren't passing... If they are, we have a hole in our test coverage.

>         failed.push(placeInfo.guid);
>-      }
>-    };
>+      }};

Nit: style. Please put methods on a new line. Also, `onPlace` is no longer a good variable name because we won't be called for every place. I suggest `updatePlacesCallback` or `placeInfoCallback` or whatever.

  let updatePlacesCallback = {
    handleError: function handleError(result, placeInfo) {
      failed.push(placeInfo.guid);
    }
  };
Comment 45 Robert Strong [:rstrong] (use needinfo to contact me) 2011-08-03 13:09:14 PDT
(In reply to comment #37)
>...
> Rstrong, given philikon's response in comment 33, do you still want me to
> remove the comment about the optional methods?
The comment should either be removed or it should mention that it is optional to the mozIAsyncHistory::updatePlaces method whenever it mentions it is optional since it being optional is 100% defined by mozIAsyncHistory::updatePlaces and not by the callback where the comment is made. For example, it is possible that in the future you can add new methods to mozIAsyncHistory where it is not optional at which time the comments about it being optional in the methods of mozIVisitInfoCallbacks would likely just be removed in favor of the methods in mozIAsyncHistory or whatever interface utilized mozIVisitInfoCallbacks.
Comment 46 Allison Naaktgeboren :ally 2011-08-03 16:07:32 PDT
Created attachment 550545 [details] [diff] [review]
Part 2 (v2): Make Sync use new API

philikon, you were correct, that code was incorrect. This should be better. :)
Comment 47 Allison Naaktgeboren :ally 2011-08-03 16:11:36 PDT
Created attachment 550546 [details] [diff] [review]
Part 1 (v3)

should have all agreed upon nits addressed.
Comment 48 Philipp von Weitershausen [:philikon] 2011-08-03 16:15:56 PDT
Comment on attachment 550545 [details] [diff] [review]
Part 2 (v2): Make Sync use new API

Yay!
Comment 49 Allison Naaktgeboren :ally 2011-08-03 16:19:57 PDT
Comment on attachment 550546 [details] [diff] [review]
Part 1 (v3)

appears to be wrong version of the patch. brb
Comment 50 Allison Naaktgeboren :ally 2011-08-03 20:30:50 PDT
Created attachment 550594 [details] [diff] [review]
Part 1 (v3) : API change, implementation
Comment 51 Philipp von Weitershausen [:philikon] 2011-08-03 22:52:51 PDT
Looks like the bug is nearing completion. I'll weigh in with an organizational note:

Obviously the two parts need to land together. If they don't land on services-central but on places, please check with jgriffin that TPS is still being run against places. The reason I'm bringing this up is that TPS is now part of the tree and no longer separate. So you may have to pull the latest changes in from m-c first and then ask jgriffin to set up TPS against places again.
Comment 52 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-04 06:52:44 PDT
Comment on attachment 550594 [details] [diff] [review]
Part 1 (v3) : API change, implementation

Review of attachment 550594 [details] [diff] [review]:
-----------------------------------------------------------------

r- for the useless uuid change, but the patch looks really much better! I'm sure next one will be +!

::: toolkit/components/places/History.cpp
@@ +538,1 @@
>    NotifyCompletion(mozIVisitInfoCallback* aCallback,

this newline change seems unneeded

@@ +568,5 @@
>      nsCOMPtr<mozIPlaceInfo> place =
>        new PlaceInfo(mPlace.placeId, mPlace.guid, uri.forget(), mPlace.title,
>                      -1, visits);
> +    if(mResult == NS_OK) {
> +      (void)mCallback->HandleResult(place);

add whitespace between if and (
Don't compare with NS_OK, you should instead "if (NS_SUCCEEDED(mResult)) {"

::: toolkit/components/places/mozIAsyncHistory.idl
@@ +151,5 @@
>  
>  /**
>   * @status EXPERIMENTAL
>   */
> +[scriptable, uuid(e47ecb0f-0740-41fb-bd43-821a607a0287)]

you should not rev the mozIAsyncHistory uuid, you are only changing a comment

@@ +162,5 @@
>     * @param aPlaceInfo
>     *        The mozIPlaceInfo object[s] containing the information to store or
>     *        update.  This can be a single object, or an array of objects.
>     * @param [optional] aCallback
> +   *        A mozIVisitInfoCallbacks object which consists of callbacks to be 

singular mozIVisitInfoCallback

::: toolkit/components/places/tests/unit/test_async_history_api.js
@@ +459,5 @@
>      do_check_eq(aResultCode, Cr.NS_ERROR_INVALID_ARG);
>      do_check_false(gGlobalHistory.isVisited(aPlaceInfo.uri));
>  
>      // If we have had all of our callbacks, continue running tests.
> +    if (++callbackCount == places.length) {waitForAsyncUpdates(run_next_test);}

nit: this specific line didn't need to be onelined, since it's just added code it doesn't cover previous blame information. I asked you to oneline the other changes just to avoid an indentation change, that is destructive for blame.

@@ +697,5 @@
>        stmt.finalize();
>  
>        waitForAsyncUpdates(run_next_test);
> +      }));
> +  }));

the first closing group looks like should be indented one hit less

@@ +764,5 @@
>  
> +  const EXPECTED_COUNT_SUCCESS = 2;
> +  const EXPECTED_COUNT_FAILURE = 1;
> +  let callbackCountSuccess = 0;
> +  let callbackCountFailure = 0;

nit: you may init the vars to the upper bound and decrement, then check 0. would save 2 consts. btw I don't care :)

@@ +1270,5 @@
> +    }));
> +  }));
> +}
> +
> +//test with empty mozIVisitInfoCallbacks object

singular mozIVisitInfoCallback, whitespace after //
Comment 53 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-04 06:53:35 PDT
(In reply to comment #51)
> Obviously the two parts need to land together. If they don't land on
> services-central but on places

I honestly don't care, there are not dependencies of any sort, it can land wherever you wish
Comment 54 Allison Naaktgeboren :ally 2011-08-04 09:52:26 PDT
(In reply to comment #53)
> (In reply to comment #51)
> > Obviously the two parts need to land together. If they don't land on
> > services-central but on places
> 
> I honestly don't care, there are not dependencies of any sort, it can land
> wherever you wish

then we should just land them on services-central
Comment 55 Allison Naaktgeboren :ally 2011-08-04 10:27:09 PDT
Created attachment 550738 [details] [diff] [review]
Part 1 (v4) : API change, implementation
Comment 56 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-04 15:24:13 PDT
Comment on attachment 550738 [details] [diff] [review]
Part 1 (v4) : API change, implementation

Review of attachment 550738 [details] [diff] [review]:
-----------------------------------------------------------------

you rock! just one glitch, but largely positive!

::: toolkit/components/places/History.cpp
@@ +532,5 @@
>   * Notifies a callback object about completion.
>   */
>  class NotifyCompletion : public nsRunnable
>  {
> +public: 

looks like you added a trailing space here.
Comment 57 Allison Naaktgeboren :ally 2011-08-04 16:41:59 PDT
Created attachment 550885 [details] [diff] [review]
Part 1 (v5) : API change, implementation
Comment 58 Richard Newman [:rnewman] 2011-08-04 16:52:59 PDT
Landed:

  Part 1: http://hg.mozilla.org/services/services-central/rev/8b82acd299da
  Part 2: http://hg.mozilla.org/services/services-central/rev/2167c608003e

This will hit m-c on the next scheduled train, early next week.
Comment 59 Shawn Wilsher :sdwilsh 2011-08-07 15:48:28 PDT
(In reply to :Ally Naaktgeboren from comment #7)
> sdwilsh, I've changed it, but could you please explain what convention/rules
> governs your nit so that I will know in the future? It an argument object
> meant to contain 1 or more callbacks so I don't understand why it should be
> singular and not plural.
You are only passing one callback object in, thus it is singular.  Yes, it's true that the object has multiple callbacks on it, but the argument is not an array, and therefore is singular.

(In reply to :Ally Naaktgeboren from comment #41)
> Mostly I fear sdwilsh's censure.  When I was working on storage he was very
> strict about it.
I disagree with Marco on this, but meh.  FEAR ME!
Comment 60 Shawn Wilsher :sdwilsh 2011-08-07 15:48:50 PDT
(This is also a difficult first bug to tackle.  GJ!)
Comment 61 Richard Newman [:rnewman] 2011-08-07 16:35:12 PDT
(In reply to Shawn Wilsher :sdwilsh from comment #59)

> You are only passing one callback object in, thus it is singular.  Yes, it's
> true that the object has multiple callbacks on it, but the argument is not
> an array, and therefore is singular.

ENGLISH, Y U NO DISTINGUISH AGGREGATE FROM PLURAL?!

Note You need to log in before you can comment on or make changes to this bug.