834539 - Implement getPlacesInfo, async successor for getPageTitle

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

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

getPageTitle is synchronous, should be replaced with something else.
Maybe we could make an API that given an uri or a guid returns all of its infos.

Comment 1

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

Andres, I suspect this bug is a big can of worms, all of the code pieces using this API are totally synchronous.
Though if you want to look into it and start filing bugs to make those pieces async I'm fine with that.
I think probably the "set" part of bug 834543 could be easier (in the sense not requiring deep changes of the codebase).

Comment 2

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

that basically means before making a new async API here, we must change/rewrite consumers to accept async behavior.
One of the blocking pieces here is most likely bug 822200, since backups should first be async to be able to read a title in an async fashion.
If you are looking for more short-term wins, there is some interesting dependency on bug 818399 too.

Comment 3

[Asaf Romano (gone)]

Attachment: getPlaceInfo

Comment 4

[Asaf Romano (gone)]

Attachment: PlacesUtils helper

Comment 5

[Asaf Romano (gone)]

Attachment: error-page bookmarking case

Comment 6

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

Comment on attachment 726631 [details] [diff] [review]
PlacesUtils helper

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

::: toolkit/components/places/PlacesUtils.jsm
@@ +2107,5 @@
> +   * @param aURI
> +   *        the page's URI (nsIURI).
> +   * @return {Promise}
> +   */
> +  getPageTitle: function PU_getPageTitle(aURI) {

so, would be cool if this would be able to get the title given uri or guid, like the API. I'm not sure whether this should just get a placeinfo or check if input is a nsIURI or try to use it as a guid otherwise.
What do you think?
I'm also not sure why you need _getPlaceInfoPromised instead of directly invoking getPlaceInfo here and resolving/rejecting the promise here

Comment 7

[Asaf Romano (gone)]

(In reply to Marco Bonardo [:mak] from comment #6)

> 
> so, would be cool if this would be able to get the title given uri or guid,
> like the API. I'm not sure whether this should just get a placeinfo or check
> if input is a nsIURI or try to use it as a guid otherwise.
> What do you think?

I would certainly like the helpers to hide the places-info stuff. I will accept nsIURI or guids (strings, that is) as you suggested.

> I'm also not sure why you need _getPlaceInfoPromised instead of directly
> invoking getPlaceInfo here and resolving/rejecting the promise here

We'll likely have more helpers (getFrecency? getGUIDForURI? getAnnoations?).

Comment 8

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

Comment on attachment 726650 [details] [diff] [review]
error-page bookmarking case

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

we should convert consumers in separate bugs, imo, let's just implement API and tests here.

::: browser/base/content/browser-places.js
@@ +271,5 @@
> +      if (aTitle === undefined) {
> +        try {
> +          let isErrorPage = /^about:(neterror|certerror|blocked)/
> +                            .test(webNav.document.documentURI);
> +          if (isErrorPage) {

I think

if (aTitle === undefined && /^about:(neterror|certerror|blocked)/.test(webNav.document.documentURI))

would make the logic a bit more readable, the comment may cover the lack of self documentation of isErrorPage

@@ +286,5 @@
> +          }
> +        }
> +        catch(ex) {
> +          Cu.reportError(ex)
> +          PlacesCommandHook.bookmarkPage(aBrowser, aParent, aShowEditUI, webNav.document.title);

why catch uses webNav.document.title while onFailure uses etBestTitleForDocument() ?

@@ +313,3 @@
>                                                      PlacesUtils.bookmarks.DEFAULT_INDEX,
> +                                                    aTitle || getBestTitleForDocument(), null,
> +                                                    [descAnno]);

title is assigned but unused...

Comment 9

[Asaf Romano (gone)]

Attachment: getPlacesInfo

I still need to update some idl, rewrite my test and rewrite the PlacesUtils helper, but otherwise this is ready for review.

Comment 10

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

Comment on attachment 735094 [details] [diff] [review]
getPlacesInfo

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

::: toolkit/components/places/History.cpp
@@ +824,5 @@
> +        nsresult rv = mHistory->FetchPageInfo(place, &known, false, false);
> +        if (NS_FAILED(rv)) {
> +          nsCOMPtr<nsIRunnable> event =
> +            new NotifyVisitInfoCallback(mCallback, place, rv);
> +          return NS_DispatchToMainThread(event);

we are in a loop, we usually notify handleError for each bad entry and proceed to the next one, but this is bailing out on the first error.
If this is not tested then we need a test for it...

@@ +1238,5 @@
> +    nsCOMPtr<nsIRunnable> event =
> +      new NotifyVisitInfoCallback(mCallback, mPlace, rv);
> +
> +    nsresult rv2 = NS_DispatchToMainThread(event);
> +    NS_ENSURE_SUCCESS(rv2, rv2);

why the need for rv2, rv is passed by value to the notify func

@@ +1319,5 @@
>                      "This should not be called on the main thread");
>  
>      // First, see if the page exists in the database (we'll need its id later).
> +    bool exists;
> +    nsresult rv = mHistory->FetchPageInfo(mPlace, &exists);

what are you doing with this rv?

@@ +2097,5 @@
> +  nsresult rv;
> +
> +  // GUID takes precedence.
> +  nsCOMPtr<mozIStorageStatement> stmt;
> +  bool selectByGUID = !aForceSelectByURI && !_place.guid.IsEmpty();

As I suspected this is a nightmare to follow with all these guid conditions...
What about if we go the easy path that is disallow passing both a guid and a uri to GetPlacesInfo? would that avoid this craziness?

::: toolkit/components/places/History.h
@@ +73,5 @@
>     *
>     * @param _place
>     *        The VisitData for the place we need to know information about.
> +   * @param [out] _exists
> +   *        Whether or the page was recorded in moz_places, false otherwise.

"Whether the page exists in moz_places."

@@ +75,5 @@
>     *        The VisitData for the place we need to know information about.
> +   * @param [out] _exists
> +   *        Whether or the page was recorded in moz_places, false otherwise.
> +   * @param [optional] aForceSelectByURI
> +   *        Whether or not to dismiss _place.guid, if set.

better documentation of what happens internally is welcome

::: toolkit/components/places/mozIAsyncHistory.idl
@@ +130,5 @@
> +   *
> +   * @param aPlaceInfo
> +   *        The mozIPlaceInfo object[s] for which to retrieve information,
> +   *        each containing either the place GUID or the place URI.
> +   *        If both are provided, the URI is ingored.

latest phrase should be updated depending on this morning discussion

@@ +136,5 @@
> +   *        A mozIVisitInfoCallback object which consists of callbacks to be 
> +   *        notified for successful & failed retrievals.
> +   *        If there's no information available for a given place, HandleResult
> +   *        is called with a stub place info object, containing just the provided
> +   *        data (GUID or URI).

hm, I wonder if wouldn't be better to invoke handleError, since the consumer is trying to request information for a page that doesn't exist. I'm pretty sure the old API was throwing in such a case.
Rememeber handleError is per place, not global.
Otherwise what should I do to figure if the page exists, check each properties of the place?

Comment 11

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

Comment on attachment 735094 [details] [diff] [review]
getPlacesInfo

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

reflagging for second look over FetchPageInfo

Comment 12

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

Comment on attachment 735094 [details] [diff] [review]
getPlacesInfo

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

I'd say to try at least removing one of the 2 conditions in FetchPageInfo.
The only problematic case is when both uri and guid are provided. The last time I said the guid should win, but actually you pointed out we allow to change the guid and that creates a code complexity problem.
Thus, I'll just accept the opposite, if both uri and guid are provided we select by uri, and then we check the guid, if it's consistent everything is fine, otherwise we check the aOverrideGUID, this should remove the aForceSelectByURI argument

Comment 13

[Asaf Romano (gone)]

Attachment: getPlacesInfo

The test is incomplete and I still need to polish some comments, but this is good for a "first" pass.

Comment 14

[Asaf Romano (gone)]

Attachment: test more stuff, add utils

Comment 15

[Asaf Romano (gone)]

Attachment: fix some comments

Comment 16

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

Comment on attachment 740672 [details] [diff] [review]
fix some comments

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

first-pass, some comments!

::: toolkit/components/places/History.cpp
@@ +647,5 @@
>    const nsCString mGUID;
>  };
>  
>  /**
> + * Notifies a callback object when a place has been handled.

well, this is not totally true, in updatePlaces there is a double loop, the external one goes through placeInfo, the internal one through visits, and then visits are added one by one, and NotifyVisitInfoCallback is invoked then for each visit.

In your case you have no visits, so it's invoked for each place...

@@ +659,4 @@
>                            nsresult aResult)
>    : mCallback(aCallback)
>    , mPlace(aPlace)
> +  , mIncludeVisits(aIncludeVisits)

couldn't this ask the mPlace is visits are available instead of getting the info through a new param?

@@ +687,5 @@
> +      (void)visits.AppendElement(visit);
> +
> +      place =
> +        new PlaceInfo(mPlace.placeId, mPlace.guid, uri.forget(), mPlace.title,
> +                      mPlace.frecency, visits);

since we are changing the fact frecency is notified (before it was not), please ensure that when we reach this point we always have read the frecency value.
Now that I think about that, iirc in some cases we just do frecency = CALCULATE_FRECENCY(:page_id) that means we will not know the frecency value when we notify, or we'd notify the wrong value

@@ +1255,5 @@
> +  static nsresult Start(mozIStorageConnection* aConnection,
> +                        VisitData& aPlace,
> +                        mozIVisitInfoCallback* aCallback) {
> +    NS_PRECONDITION(NS_IsMainThread(),
> +                    "This should be called on the main thread");

please, use MOZ_ASSERT wherever possible

@@ +1258,5 @@
> +    NS_PRECONDITION(NS_IsMainThread(),
> +                    "This should be called on the main thread");
> +
> +    nsRefPtr<GetPlaceInfo> event =
> +      new GetPlaceInfo(aPlace, aCallback);

doesn't need to newline, I guess

@@ +1282,5 @@
> +
> +    nsCOMPtr<nsIRunnable> event =
> +      new NotifyVisitInfoCallback(mCallback, mPlace, false, rv);
> +
> +    nsresult rv2 = NS_DispatchToMainThread(event);

I think rv is passed by value, so there should be no problem reusing it?

@@ +1301,5 @@
> +    nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
> +    NS_ABORT_IF_FALSE(navHistory, "Could not get nsNavHistory?!");
> +
> +    // We AddRef on the main thread, and release it when we are destroyed.
> +    NS_IF_ADDREF(mCallback);

I suppose we could even have a simple nsCOMPtr member and just pass it to NS_ProxyRelease, it should swap the ownership and keep it alive until it's released on the proper thread, without manual AddRefing. Though please verify that :)

@@ +1363,5 @@
>                      "This should not be called on the main thread");
>  
>      // First, see if the page exists in the database (we'll need its id later).
> +    bool exists;
> +    nsresult rv = mHistory->FetchPageInfo(mPlace, &exists);

this error is not handled?

@@ +2702,5 @@
> +    nsString fatGUID;
> +    GetJSValueAsString(aCtx, placeIdentifier, fatGUID);
> +    if (!fatGUID.IsVoid()) {
> +      VisitData& placeInfo = *placesInfo.AppendElement(VisitData());
> +      placeInfo.guid = NS_ConvertUTF16toUTF8(fatGUID);

I think we should at least check the guid is in the expected format, let's say at least the length :)

::: toolkit/components/places/PlacesUtils.jsm
@@ +1736,5 @@
> +  /**
> +   * Promised wrapper for mozIAsyncHistory::updatePlaces for a single place.
> +   *
> +   * @param aPlaces
> +   *        a single place info object

mozIPlaceInfo object

::: toolkit/components/places/mozIAsyncHistory.idl
@@ +86,5 @@
>     * @param aResultCode
>     *        nsresult indicating the failure reason.
>     * @param aPlaceInfo
> +   *        The information that was being entered into the database
> +   *        in the form of mozIPlacesInfo object.

In the getPlacesInfo case this information is not entered, later you used "The current info stored for the place." that while doesn't sound perfect (and I don't have better suggestion) is more generic

@@ +134,5 @@
> +   *
> +   * If a given place does not exist in the database, aCallback.handleError is
> +   * called for it with NS_ERROR_NOT_AVAILABLE result code.
> +   *
> +   * @param aPlaceIdentifires

typo: Identifires

later you use aPlaceInfo for the param name...

@@ +136,5 @@
> +   * called for it with NS_ERROR_NOT_AVAILABLE result code.
> +   *
> +   * @param aPlaceIdentifires
> +   *        The place[s] for which to retrieve information, identified by either
> +   *        the place GUID or the place URI.

should better specify it may be a single place or an array of places, identified by...

@@ +138,5 @@
> +   * @param aPlaceIdentifires
> +   *        The place[s] for which to retrieve information, identified by either
> +   *        the place GUID or the place URI.
> +   * @param aCallback
> +   *        A mozIVisitInfoCallback object which consists of callbacks to be 

trailing space

@@ +139,5 @@
> +   *        The place[s] for which to retrieve information, identified by either
> +   *        the place GUID or the place URI.
> +   * @param aCallback
> +   *        A mozIVisitInfoCallback object which consists of callbacks to be 
> +   *        notified for successful & failed retrievals.

please avoid "&" in comments, it makes me mad :) Btw, in this case I'd rather use "or"

@@ +140,5 @@
> +   *        the place GUID or the place URI.
> +   * @param aCallback
> +   *        A mozIVisitInfoCallback object which consists of callbacks to be 
> +   *        notified for successful & failed retrievals.
> +   *        If there's no information available for a given place, aCallbac

typo aCallbac

@@ +147,5 @@
> +   *
> +   * @throws NS_ERROR_INVALID_ARG
> +   *         - Passing in NULL for aPlaceInfo or aCallback.
> +   *         - Not providing at least one valid guid, or uri for all
> +   *           mozIPlaceInfo object[s].

s/for all mozIPlaceInfo object[s]//

@@ +158,5 @@
>     * Adds a set of visits for one or more mozIPlaceInfo objects, and updates
>     * each mozIPlaceInfo's title or guid.
>     *
> +   * aCallback.handleResult is called for each visit added, title change or
> +   * guid change.

I think this is partially false. What it's trying to express is that if you pass in a place with multiple visits, handleResult is invoked for each visit.
Not sure why it's also referring to title or guid changes...

::: toolkit/components/places/tests/unit/test_getPlacesInfo.js
@@ +9,5 @@
> +      { transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
> +        visitDate: Date.now() * 1000 }
> +    ]
> +  });
> +}

so.... why don't you use promiseAddVisits?

@@ +90,5 @@
> +}
> +
> +[
> +test_getPlacesInfoExistentPlace,
> +test_getPlacesInfoNonExistentPlace,

some additional tests like checking after updatePlaces that changes something (like title) we get the right updated info
a test that passes-in an invalid guid
a test for selection by guid
a test for mixed selection (a guid and an uri)
...

@@ +93,5 @@
> +test_getPlacesInfoExistentPlace,
> +test_getPlacesInfoNonExistentPlace,
> +test_promisedHelper,
> +test_infoByGUID
> +].forEach(add_task);

I honestly prefer add_task inlined to function tests, so that adding.removing tests doesn't have to update this list

Comment 17

[Asaf Romano (gone)]

I'll post the revised patch once we figure out the frecency issue (see below).

(In reply to Marco Bonardo [:mak] from comment #16)
> Comment on attachment 740672 [details] [diff] [review]
> fix some comments
> 
> Review of attachment 740672 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> first-pass, some comments!
> 
> ::: toolkit/components/places/History.cpp
> @@ +647,5 @@
> >    const nsCString mGUID;
> >  };
> >  
> >  /**
> > + * Notifies a callback object when a place has been handled.
> 

Fixed. Here's the new comment:

/**
 * Helper class for methods methods which notify their callers through the
 * mozIVisitInfoCallback interface.
 */

> @@ +659,4 @@
> >                            nsresult aResult)
> >    : mCallback(aCallback)
> >    , mPlace(aPlace)
> > +  , mIncludeVisits(aIncludeVisits)
> 
> couldn't this ask the mPlace is visits are available instead of getting the
> info through a new param?


Given that mPlace is of type "VisitData" and is used for other stuff, I prefer the current setup. If you still disagree, I can change that.

> @@ +687,5 @@
> > +      (void)visits.AppendElement(visit);
> > +
> > +      place =
> > +        new PlaceInfo(mPlace.placeId, mPlace.guid, uri.forget(), mPlace.title,
> > +                      mPlace.frecency, visits);
> 
> since we are changing the fact frecency is notified (before it was not),
> please ensure that when we reach this point we always have read the frecency
> value.
> Now that I think about that, iirc in some cases we just do frecency =
> CALCULATE_FRECENCY(:page_id) that means we will not know the frecency value
> when we notify, or we'd notify the wrong value

Haven't fixed that yet. Indeed UpdateFrecency doesn't not update the place object. What's the right way to fix that? I would prefer to avoid another query.

> @@ +1255,5 @@
> > +  static nsresult Start(mozIStorageConnection* aConnection,
> > +                        VisitData& aPlace,
> > +                        mozIVisitInfoCallback* aCallback) {
> > +    NS_PRECONDITION(NS_IsMainThread(),
> > +                    "This should be called on the main thread");
> 
> please, use MOZ_ASSERT wherever possible
> 

Done.

> @@ +1258,5 @@
> > +    NS_PRECONDITION(NS_IsMainThread(),
> > +                    "This should be called on the main thread");
> > +
> > +    nsRefPtr<GetPlaceInfo> event =
> > +      new GetPlaceInfo(aPlace, aCallback);
> 
> doesn't need to newline, I guess
>

Fixed.
 
> @@ +1282,5 @@
> > +
> > +    nsCOMPtr<nsIRunnable> event =
> > +      new NotifyVisitInfoCallback(mCallback, mPlace, false, rv);
> > +
> > +    nsresult rv2 = NS_DispatchToMainThread(event);
> 
> I think rv is passed by value, so there should be no problem reusing it?

Yup. My copasting fault.
 
> @@ +1301,5 @@
> > +    nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
> > +    NS_ABORT_IF_FALSE(navHistory, "Could not get nsNavHistory?!");
> > +
> > +    // We AddRef on the main thread, and release it when we are destroyed.
> > +    NS_IF_ADDREF(mCallback);
> 
> I suppose we could even have a simple nsCOMPtr member and just pass it to
> NS_ProxyRelease, it should swap the ownership and keep it alive until it's
> released on the proper thread, without manual AddRefing. Though please
> verify that :)

It seems you're right, of course.  Should I also fix InsertVisitedURIs?

> @@ +1363,5 @@
> >                      "This should not be called on the main thread");
> >  
> >      // First, see if the page exists in the database (we'll need its id later).
> > +    bool exists;
> > +    nsresult rv = mHistory->FetchPageInfo(mPlace, &exists);
> 
> this error is not handled?
> 

Fixed. Note that this API doesn't report errors, so it's just NS_ENSURE_SUCCESS in a nsIRunnable::Run method.

> @@ +2702,5 @@
> > +    nsString fatGUID;
> > +    GetJSValueAsString(aCtx, placeIdentifier, fatGUID);
> > +    if (!fatGUID.IsVoid()) {
> > +      VisitData& placeInfo = *placesInfo.AppendElement(VisitData());
> > +      placeInfo.guid = NS_ConvertUTF16toUTF8(fatGUID);
> 
> I think we should at least check the guid is in the expected format, let's
> say at least the length :)


Now calling IsValidGUID, and I also fixed this helper to take the string as ns*A*CString.

> ::: toolkit/components/places/PlacesUtils.jsm
> @@ +1736,5 @@
> > +  /**
> > +   * Promised wrapper for mozIAsyncHistory::updatePlaces for a single place.
> > +   *
> > +   * @param aPlaces
> > +   *        a single place info object
> 
> mozIPlaceInfo object
>

Fixed.
 
> ::: toolkit/components/places/mozIAsyncHistory.idl
> @@ +86,5 @@
> >     * @param aResultCode
> >     *        nsresult indicating the failure reason.
> >     * @param aPlaceInfo
> > +   *        The information that was being entered into the database
> > +   *        in the form of mozIPlacesInfo object.
> 
> In the getPlacesInfo case this information is not entered, later you used
> "The current info stored for the place." that while doesn't sound perfect
> (and I don't have better suggestion) is more generic

Changed to "The information that was given to the caller for the place".

> @@ +134,5 @@
> > +   *
> > +   * If a given place does not exist in the database, aCallback.handleError is
> > +   * called for it with NS_ERROR_NOT_AVAILABLE result code.
> > +   *
> > +   * @param aPlaceIdentifires
> 
> typo: Identifires
> 
> later you use aPlaceInfo for the param name...
> 

Fixed.

> @@ +136,5 @@
> > +   * called for it with NS_ERROR_NOT_AVAILABLE result code.
> > +   *
> > +   * @param aPlaceIdentifires
> > +   *        The place[s] for which to retrieve information, identified by either
> > +   *        the place GUID or the place URI.
> 
> should better specify it may be a single place or an array of places,
> identified by...
> 

"
   *        The place[s] for which to retrieve information, identified by either
   *        a single place GUID, a single URI, or a JS array of URIs and/or GUIDs.
"

Sounds good?

> @@ +138,5 @@
> > +   * @param aPlaceIdentifires
> > +   *        The place[s] for which to retrieve information, identified by either
> > +   *        the place GUID or the place URI.
> > +   * @param aCallback
> > +   *        A mozIVisitInfoCallback object which consists of callbacks to be 
> 
> trailing space
> 
> @@ +139,5 @@
> > +   *        The place[s] for which to retrieve information, identified by either
> > +   *        the place GUID or the place URI.
> > +   * @param aCallback
> > +   *        A mozIVisitInfoCallback object which consists of callbacks to be 
> > +   *        notified for successful & failed retrievals.
> 
> please avoid "&" in comments, it makes me mad :) Btw, in this case I'd
> rather use "or"
> 
> @@ +140,5 @@
> > +   *        the place GUID or the place URI.
> > +   * @param aCallback
> > +   *        A mozIVisitInfoCallback object which consists of callbacks to be 
> > +   *        notified for successful & failed retrievals.
> > +   *        If there's no information available for a given place, aCallbac
> 
> typo aCallbac
>
> @@ +147,5 @@
> > +   *
> > +   * @throws NS_ERROR_INVALID_ARG
> > +   *         - Passing in NULL for aPlaceInfo or aCallback.
> > +   *         - Not providing at least one valid guid, or uri for all
> > +   *           mozIPlaceInfo object[s].
> 
> s/for all mozIPlaceInfo object[s]//
>


Fixed. 
 
> @@ +158,5 @@
> >     * Adds a set of visits for one or more mozIPlaceInfo objects, and updates
> >     * each mozIPlaceInfo's title or guid.
> >     *
> > +   * aCallback.handleResult is called for each visit added, title change or
> > +   * guid change.
> 
> I think this is partially false. What it's trying to express is that if you
> pass in a place with multiple visits, handleResult is invoked for each visit.
> Not sure why it's also referring to title or guid changes...
>

Remove the reference, but note that while at least one visit must be added, you can also use this method to update titles and guides (as long as at least one visit is added).

So, I guess that in practice, you're notified for the title/guid change once for each visit added...


> ::: toolkit/components/places/tests/unit/test_getPlacesInfo.js
> @@ +9,5 @@
> > +      { transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
> > +        visitDate: Date.now() * 1000 }
> > +    ]
> > +  });
> > +}
> 
> so.... why don't you use promiseAddVisits?
>

Fixed.


> @@ +90,5 @@
> > +}
> > +
> > +[
> > +test_getPlacesInfoExistentPlace,
> > +test_getPlacesInfoNonExistentPlace,
> 
> some additional tests like checking after updatePlaces that changes
> something (like title) we get the right updated info
> a test that passes-in an invalid guid
> a test for mixed selection (a guid and an uri)

On it.

> a test for selection by guid

It's already there.




> 
> @@ +93,5 @@
> > +test_getPlacesInfoExistentPlace,
> > +test_getPlacesInfoNonExistentPlace,
> > +test_promisedHelper,
> > +test_infoByGUID
> > +].forEach(add_task);
> 
> I honestly prefer add_task inlined to function tests, so that
> adding.removing tests doesn't have to update this list

No hard feelings :) I just copied that from some other test.

Comment 18

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

(In reply to Mano from comment #17)

> > @@ +659,4 @@
> > >                            nsresult aResult)
> > >    : mCallback(aCallback)
> > >    , mPlace(aPlace)
> > > +  , mIncludeVisits(aIncludeVisits)
> > 
> > couldn't this ask the mPlace is visits are available instead of getting the
> > info through a new param?
> 
> 
> Given that mPlace is of type "VisitData" and is used for other stuff, I
> prefer the current setup. If you still disagree, I can change that.

sigh, that's why I was confused.
VisitData is supposed to represent a single visit, as well as NotifyVisitInfoCallback is supposed to notify about a single visit.
So calling it without a visit is a nonsense and mIncludeVisits param is conceptually wrong.

We could rename this NotifyVisitInfoCallback to NotifyPlaceInfoCallback, since it's what it does in the end.  And rename the parameter to aIsVisit...
It will still suck but should be a little bit more coherent.

> > @@ +687,5 @@
> > > +      (void)visits.AppendElement(visit);
> > > +
> > > +      place =
> > > +        new PlaceInfo(mPlace.placeId, mPlace.guid, uri.forget(), mPlace.title,
> > > +                      mPlace.frecency, visits);
> > 
> > since we are changing the fact frecency is notified (before it was not),
> > please ensure that when we reach this point we always have read the frecency
> > value.
> > Now that I think about that, iirc in some cases we just do frecency =
> > CALCULATE_FRECENCY(:page_id) that means we will not know the frecency value
> > when we notify, or we'd notify the wrong value
> 
> Haven't fixed that yet. Indeed UpdateFrecency doesn't not update the place
> object. What's the right way to fix that? I would prefer to avoid another
> query.

Well, indeed it was not fixed, that's why we were always passing out -1, to avoid passing out a wrong value. I don't have any idea to fix that, I also would like to avoid querying. Is this really really needed or just a nice-to-have?

> > @@ +1301,5 @@
> > > +    nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
> > > +    NS_ABORT_IF_FALSE(navHistory, "Could not get nsNavHistory?!");
> > > +
> > > +    // We AddRef on the main thread, and release it when we are destroyed.
> > > +    NS_IF_ADDREF(mCallback);
> > 
> > I suppose we could even have a simple nsCOMPtr member and just pass it to
> > NS_ProxyRelease, it should swap the ownership and keep it alive until it's
> > released on the proper thread, without manual AddRefing. Though please
> > verify that :)
> 
> It seems you're right, of course.  Should I also fix InsertVisitedURIs?

Yes I think you should for coherency 


> > @@ +158,5 @@
> > >     * Adds a set of visits for one or more mozIPlaceInfo objects, and updates
> > >     * each mozIPlaceInfo's title or guid.
> > >     *
> > > +   * aCallback.handleResult is called for each visit added, title change or
> > > +   * guid change.
> > 
> > I think this is partially false. What it's trying to express is that if you
> > pass in a place with multiple visits, handleResult is invoked for each visit.
> > Not sure why it's also referring to title or guid changes...
> >
> 
> Remove the reference, but note that while at least one visit must be added,
> you can also use this method to update titles and guides (as long as at
> least one visit is added).
> 
> So, I guess that in practice, you're notified for the title/guid change once
> for each visit added...

I suppose you want to write a little test and check what happens, I honestly forgot.

The other comments look fine.

Comment 19

[Asaf Romano (gone)]

Attachment: patch

Comment 20

[Asaf Romano (gone)]

Comment on attachment 764067 [details] [diff] [review]
patch

Asking Gavin for sr on the API additions (mozIAsyncHistory and PlacesUtils)

Comment 21

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

Comment on attachment 764067 [details] [diff] [review]
patch

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

Please add @deprecated to getPageTitle in the idl, pointing to the new API.
Also please add PLACES_WARN_DEPRECATED() to the ::GetPageTitle() implementation.

Finally, please file a bug to replace usage of getPageTitle (won't be funny I think) and make it block bug 699850.

::: toolkit/components/places/History.cpp
@@ +224,5 @@
> + * @param [out] _arrayLength
> + *        _array's length.
> + */
> +nsresult
> +GetJSArrayFromJSValue(const JS::Value& aValue, JSContext* aCtx, JSObject** _array, uint32_t* _arrayLength) {

I suspect this is far over 80 chars and should be reindented

@@ +226,5 @@
> + */
> +nsresult
> +GetJSArrayFromJSValue(const JS::Value& aValue, JSContext* aCtx, JSObject** _array, uint32_t* _arrayLength) {
> +  if (JS_IsArrayObject(aCtx, aValue.toObjectOrNull())) {
> +    *_array = aValue.toObjectOrNull();

seems like you may invoke toObjectOrNull just once

@@ +234,5 @@
> +  else {
> +    // Build a temporary array to store this one item so the code below can
> +    // just loop.
> +    *_arrayLength = 1;
> +    *_array = JS_NewArrayObject(aCtx, 0, NULL);

nullptr

@@ +237,5 @@
> +    *_arrayLength = 1;
> +    *_array = JS_NewArrayObject(aCtx, 0, NULL);
> +    NS_ENSURE_TRUE(*_array, NS_ERROR_OUT_OF_MEMORY);
> +
> +    JSBool rc = JS_DefineElement(aCtx, *_array, 0, aValue, NULL, NULL, 0);

nullptr

@@ +287,5 @@
>  {
>    JS::Rooted<JS::Value> uriVal(aCtx);
>    JSBool rc = JS_GetProperty(aCtx, aObject, aProperty, uriVal.address());
>    NS_ENSURE_TRUE(rc, nullptr);
> +	return GetJSValueAsURI(aCtx, uriVal);

indentation

@@ +326,2 @@
>  }
> + 

trailing space

@@ +349,1 @@
>      return;

In previous code we were _string.SetIsVoid(true), I don't think it really matters in practice, but just to avoid unwanted changes I'd restore that here.

@@ +645,5 @@
>    const nsCString mGUID;
>  };
>  
>  /**
> + * Helper class for methods methods which notify their callers through the

typo: methods methods

@@ +656,2 @@
>                            const VisitData& aPlace,
> +                          bool aIsVisit,

maybe even better aIsSingleVisit (and the property as well)

@@ +914,5 @@
>  
>      (void)mPlaces.SwapElements(aPlaces);
>      (void)mReferrers.SetLength(mPlaces.Length());
>  
> +    NS_ABORT_IF_FALSE(nsNavHistory::GetHistoryService(), "Could not get nsNavHistory?!");

This is not equivalent to the previous code, we must ensure nsNavHistory is alive at this point, but since you moved it into NS_ABORT_IF_FALSE it won't happen in release... 
please restore it and add a comment explaining why it's done that way.

@@ +1236,5 @@
>     */
>    nsRefPtr<History> mHistory;
>  };
>  
> +class GetPlaceInfo : public nsRunnable {

annotate with MOZ_FINAL please

@@ +1284,5 @@
> +  , mCallback(aCallback)
> +  , mHistory(History::GetService())
> +  {
> +    MOZ_ASSERT(NS_IsMainThread(), "This should be called on the main thread");
> +    NS_ABORT_IF_FALSE(nsNavHistory::GetHistoryService(), "Could not get nsNavHistory?!");

I suspect this is wrong for the same reason I pointed out before, and I'm not sure why this is done here instead of in GetPlacesInfo API call, wouldn't be better to just do it once instead for each entry?

@@ +1341,5 @@
>  
>    NS_IMETHOD Run()
>    {
>      NS_PRECONDITION(!NS_IsMainThread(),
>                      "This should not be called on the main thread");

MOZ_ASSERT please

@@ +1345,5 @@
>                      "This should not be called on the main thread");
>  
>      // First, see if the page exists in the database (we'll need its id later).
> +    bool exists;
> +    nsresult rv = mHistory->FetchPageInfo(mPlace, &exists);

are we doing anything with this rv value? it looks unused, you should probably just NS_ENSURE_SUCCESS

@@ +2672,5 @@
> +                       mozIVisitInfoCallback* aCallback,
> +                       JSContext* aCtx) {
> +  uint32_t placesIndentifiersLength;
> +  JS::Rooted<JSObject*> placesIndentifiers(aCtx);
> +  nsresult rv = GetJSArrayFromJSValue(aPlaceIdentifiers, aCtx, placesIndentifiers.address(), &placesIndentifiersLength);

reindent please

@@ +2723,5 @@
> +
> +    nsCOMPtr<nsIEventTarget> backgroundThread = do_GetInterface(dbConn);
> +    NS_ENSURE_TRUE(backgroundThread, NS_ERROR_UNEXPECTED);
> +    nsCOMPtr<nsIRunnable> event = new NotifyCompletion(aCallback);
> +    (void)backgroundThread->Dispatch(event, NS_DISPATCH_NORMAL);

at this point just return this instead of swallowing the error

::: toolkit/components/places/PlacesUtils.jsm
@@ +1767,5 @@
> +  promiseUpdatePlace: function PU_promiseUpdatePlaces(aPlace) {
> +    let deferred = Promise.defer();
> +    PlacesUtils.asyncHistory.updatePlaces(aPlace, {
> +      handleResult: function handleResult(aPlaceInfo) {
> +        deferred.resolve();

I'd prefer to call resolve() in handleCompletion for better serialization.
Also, would not be nice if resolve would pass out aPlaceInfo?
you may just store aPlaceInfo in a local var, and on completion if it's set resolve to it.
fwiw I think you could even invoke resolve regardless of failure, once a promise is rejected, invoking resolve is a no-op and viceversa.

@@ +1789,5 @@
> +  promisePlaceInfo: function PU_promisePlaceInfo(aPlaceIdentifier) {
> +    let deferred = Promise.defer();
> +    PlacesUtils.asyncHistory.getPlacesInfo(aPlaceIdentifier, {
> +      handleResult: function handleResult(aPlaceInfo) {
> +        deferred.resolve(aPlaceInfo);

ditto

::: toolkit/components/places/mozIAsyncHistory.idl
@@ +135,5 @@
> +   * called for it with NS_ERROR_NOT_AVAILABLE result code.
> +   *
> +   * @param aPlaceIdentifiers
> +   *        The place[s] for which to retrieve information, identified by either
> +   *        a single place GUID, a single URI, or a JS array of URIs and/or GUIDs.

It may be worth to @note what happens if both uri and guid are passed into for the same place

::: toolkit/components/places/tests/unit/test_getPlacesInfo.js
@@ +9,5 @@
> +      { transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
> +        visitDate: Date.now() * 1000 }
> +    ]
> +  });
> +}

I'm not sure I understand what this is adding over promiseAddVisits, you may just promiseAddVisits({ uri: aURI }), transition_link is implicit iirc, as well as visitDate and a custom title is made up automatically.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 764067 [details] [diff] [review]
patch

>diff -r bfc3d2853ee3 toolkit/components/places/PlacesUtils.jsm

>+  promiseUpdatePlace: function PU_promiseUpdatePlaces(aPlace) {

>+  promisePlaceInfo: function PU_promisePlaceInfo(aPlaceIdentifier) {

I don't have much of an opinion here, you and Marco should sort out whether these are useful additions since you have much better knowledge of potential users. If there are no existing users we should generally be conservative with API additions, IMO.

>diff -r bfc3d2853ee3 toolkit/components/places/mozIAsyncHistory.idl

> interface mozIAsyncHistory : nsISupports

>+  void getPlacesInfo(in jsval aPlaceInfo,

aPlaceIdentifiers?

>+                     [optional] in mozIVisitInfoCallback aCallback);

Why optional? I don't see how it could make sense to call this without a callback.

Comment 23

[Asaf Romano (gone)]

Attachment: as checked in

https://hg.mozilla.org/integration/mozilla-inbound/rev/8416c29fecda

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd9245e21812

https://tbpl.mozilla.org/php/getParsedLog.php?id=24296907&tree=Mozilla-Inbound

Comment 25

[Asaf Romano (gone)]

Attachment: with the crash fixed

https://hg.mozilla.org/integration/mozilla-inbound/rev/734147446def

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/734147446def

Comment 27

[Asaf Romano (gone)]

dev-doc-needed for both the IDL method and the PlacesUtils helpers.

Comment 28

[Sebastian Zartner [:sebo]]

I updated the documentation of mozIAsyncHistory[1] to include getPlacesInfo().

Sebastian

[1] https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/mozIAsyncHistory