699843 - (asyncFavicons) Make all Favicons APIs asynchronous

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

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

Some has already been moved to mozIAsyncFavicons, all remaining APIs should be converted, as well.

Comment 1

[Felix Fung (:felix)]

Attachment: Asynchronous setFaviconData

Added the interface
mozIAsyncFavicons::SetFaviconDataAsync

Created the helper
AsyncSetFaviconData

Comment 2

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

Comment on attachment 574852 [details] [diff] [review]
Asynchronous setFaviconData

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

I think we may want to slightly improve the API since is error prone (just read the part where it says you can use it but only if you ensure the message loop doesn't spin), and the data added this way may disappear at any time before you can actually be able to use it.
Let's see at the use-cases. In our code we have just one so far, that is setting an icon to a page where we already know its data and that requires calling setFaviconData and then setFaviconURLForPage. For this use-case we may rather provide an API that does both.
Something like
SetFaviconForPage(aURI, aFaviconURI, aData, aDataLen, aMimeType, aExpiration,
		 [optional]aCallback)
What would be really awesome, would be to be able to have this method accept different kind of data, that is plain octet like now, a datauri or even an imagecontainer (?). That may be tricky since idl doesn't allow overloading, we have have to come with SetFaviconDataURLForPage (sigh).

The only use case I could still think about is that you need to associate an icon to a page, but you know those at different moments (thinking about your patch in imageDocument). The existing API seem to support this, but that's untrue, the fact it may work is a lottery, regarding the fact you are faster than expiration.
I think for that case we should do differently from what we do today, instead of storing a not associated icon in the database, we should store it in a memory hash for a certain amount of time, and when we associate an icon an any api, we first go looking if the hash has it.

What do you think of this proposal?

Comment 3

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

there is one last use-case I see from add-ons, that is to replace an existing icon data with custom content.
the setFaviconDataAsync (bad name) api I suggested above may first check if the icon exists in the database and replace its contents, otherwise store it in the memory hash. Along with getFaviconURLForPage should cover this use-case.

Comment 4

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

This may be even simplified further, if setAndFetchFaviconForPage (that we already have) would check the memory hash before hitting the network, then we may just add
replaceFaviconData(aFaviconURI, data)
replaceFaviconDataFromDataURL(aFaviconURI, dataURL)
replace...FromImageContainer
...

these would:
- if the iconuri is in the database
a) replace the eventual existing data for that favicon uri
b) notify any associated page of the changed icon
- if the iconuri is not in the database
a) store it in a memory hash (data hashed by iconuri)

Calling these before setAndFetchFaviconForPage you'd actually override the icon and we could avoid SetFaviconForPage completely.

Comment 5

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

Comment on attachment 574852 [details] [diff] [review]
Asynchronous setFaviconData

clearing request while the new APIs are being worked on

Comment 6

[Felix Fung (:felix)]

Attachment: SetFaviconData No Longer Persists Data

Summary of Changes:
- nsFaviconService.h defines new struct UnassociatedIconEntry. When SetFaviconData is called, we create an UnassociatedIconEntry and insert it into our memory hash.

- nsFaviconService initializes a single timer that fires each minute to expire entries from the UnassociatedIconEntry hashtable. We expire any entries that are over a minute old. Because we are only using the single timer, entries may in the worst case live up to two minutes, (twice the firing interval.) I think that should still be OK.

- nsFaviconService::SetAndFetchFaviconForPage (aka SetAndLoad) checks the memory hash for the favicon. If found, calls AsyncSetFaviconDataForPage. Otherwise, calls AsyncFetchAndSetIconForPage. AsyncSetFaviconDataForPage is basically a wrapper start method around AsyncAssociateIconToPage taking in a PageURI and IconData we can get from our memory hash.

Comment 7

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

Comment on attachment 575566 [details] [diff] [review]
SetFaviconData No Longer Persists Data

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

Sorry if it took a while, but I wanted to be able to have a better global vision of this.
The most important issue is that you can't just change SetFaviconData and hope it will work, please make a new API and leave the old alone, since otherwise you have to fix ALL of the old APIs that work in connection with SetFaviconData. We'll just deprecate everything in nsIFaviconService in future and keep mozIAsyncFavicons as the only interface.
Then, I think you may simplify the code by doing a small change to the AsyncSetAndFetchFaviconForPage object, rather than spawning a new kickstarter for AsyncAssociateIconToPage.
Finally, setting new data should update the database data if that's present, otherwise put in the memory hash, or we'll have a de-sync between the database and the memory icon.

Here's a more detailed breakdown.

::: toolkit/components/places/AsyncFaviconHelpers.cpp
@@ +372,5 @@
> + * @param _page
> + *        The PageData object to be returned.
> + */
> +nsresult
> +GetPageDataForURI(nsIURI* aPageURI, PageData &_page)

decorators towards type please, btw if you edit the main setAndFetch starter, these changes are not needed

::: toolkit/components/places/AsyncFaviconHelpers.h
@@ +50,5 @@
>  #define ICON_STATUS_UNKNOWN 0
>  #define ICON_STATUS_CHANGED 1 << 0
>  #define ICON_STATUS_SAVED 1 << 1
>  #define ICON_STATUS_ASSOCIATED 1 << 2
> +#define ICON_STATUS_CACHED 1 << 3

this is unused, btw you may use it according to my suggestions

::: toolkit/components/places/nsFaviconService.cpp
@@ +74,5 @@
>  
>  #define MAX_FAVICON_CACHE_SIZE 256
>  #define FAVICON_CACHE_REDUCE_COUNT 64
>  
> +#define UNLINKED_ICON_EXPIRY_INTERVAL 60000

ditto on naming coherence (UNASSOCIATED or UNLINKED everywhere)

@@ +146,5 @@
>    // Init failed favicon cache.
>    if (!mFailedFavicons.Init(MAX_FAVICON_CACHE_SIZE))
>      return NS_ERROR_OUT_OF_MEMORY;
>  
> +  if (!mUnassociatedFavicons.Init(MAX_FAVICON_CACHE_SIZE))

256 entries are a lot for this cache, you probably want to use a separate and smaller define for this, 64 entries maybe.

@@ +155,5 @@
>    );
>  
> +  nsresult rv;
> +  mExpireUnassociatedTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);

don't check rv, instead NS_ENSURE_STATE(mExpireUnassociatedTimer) and don't pass &rv

@@ +158,5 @@
> +  mExpireUnassociatedTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  mExpireUnassociatedTimer->InitWithCallback(
> +    this, UNLINKED_ICON_EXPIRY_INTERVAL, nsITimer::TYPE_REPEATING_SLACK);

A repeating timer would hurt mobile and more generally battery devices.
We should either stop the timer on idle (but the service is not listening to it) or use TYPE_ONE_SHOT timers fired everytime a new entry is added.
Or an alternative may be to cap the size of the cache and have a single TYPE_ONE_SHOT timer. When an entry is added first check if we are at the cap, if so discard the older entry and add the new one, then Cancel the current timer and start a new one. An helper method may do this. (The cap is to avoids that calling the api more often than the interval may cause a OOM, since cancelling the timer may cause it to never run in such a case).
I'm probably fine with whatever solution that avoids a repeating timer.

@@ +199,5 @@
> +////////////////////////////////////////////////////////////////////////////////
> +//// nsITimerCallback
> +
> +static PLDHashOperator
> +ExpireUnassociatedFaviconsCallback(nsCStringHashKey::KeyType aKey,

while it's a bit more verbose, I'd prefer ExpireNonrecentUnassociatedIconsCallback

@@ +393,5 @@
>        return NS_OK;
>    }
>  
> +  nsCAutoString faviconSpec;
> +  rv = aFaviconURI->GetSpec(faviconSpec);

if you use the nsIURI hash, you can avoid needing the spec here

@@ +400,5 @@
> +  UnassociatedIconEntry* iconEntry = mUnassociatedFavicons.Get(faviconSpec);
> +  if (iconEntry) {
> +    rv = AsyncSetFaviconDataForPage::start(iconEntry->iconData, aPageURI, aCallback);
> +    mUnassociatedFavicons.Remove(faviconSpec);
> +    NS_ENSURE_SUCCESS(rv, rv);

If we fail storing the icon we should probably not remove it from the cache

@@ +409,5 @@
> +      aFaviconURI, aPageURI, aForceReload ? FETCH_ALWAYS : FETCH_IF_MISSING,
> +      aCallback
> +    );
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }

There is an alternative (that I think is safer and does not require a special kickstarter to init the event) to doing an if/else here, that is to change AsyncFetchAndSetIconForPage::start to try to fetch the icon from the cache, set fetchMode to FETCH_NEVER if one is found. Then FetchIconInfo may just bail out early if ICON_STATUS_CACHED is set on the icon. Note you can't make FetchIconInfo directly read the cache, since it's on another thread and the hash is not thread safe.
AsyncFetchAndSetIconForPage would then skip the network fetch and directly start the AsyncAssociateIconToPage event.
See http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/AsyncFaviconHelpers.cpp#506

@@ +445,5 @@
> +
> +  IconData* cachedIcon = &(iconEntry->iconData);
> +  cachedIcon->expiration = aExpiration;
> +  nsresult rv = aFaviconURI->GetSpec(cachedIcon->spec);
> +  NS_ENSURE_SUCCESS(rv, rv);

The problem with this, is that you are changing the behavior of setFaviconData API (and all the other calling into it), but you are not changing the behavior of setFaviconURLForPage or getFaviconData.
There are lots of connections here, thus I suggest you just add a plain new API to mozIAsyncFavicons (I suggested replaceFaviconData or overrideFaviconData or even just SetFavicon, whatever does not clash with SetFaviconData name) and add @deprecated annotation to setFaviconData and any related method.
Otherwis,e you'll have to fix each single method in nsIFaviconService, that is something we may want to do, but most likely not in this bug, since it's lot of changes.
The new method should change the in-database existing data (asynchronously), if that icon is already registered, otherwise register it in the memory hash.

::: toolkit/components/places/nsFaviconService.h
@@ +67,5 @@
>  
> +struct UnassociatedIconEntry {
> +  mozilla::places::IconData iconData;
> +  PRTime entryCreation;
> +};

I think it would be simpler if you use a nsTHashTable and the icon nsIURI as the key

class UnassociatedIconHashKey : public nsURIHashKey
{
public:
  UnassociatedIconHashKey(const nsIURI* aURI)
  : nsURIHashKey(aURI)
  {
  }
  UnassociatedIconHashKey(const UnassociatedIconHashKey& aOther)
  : nsURIHashKey(aOther)
  {
  NS_NOTREACHED("Do not call me!");
  }
  mozilla::places::IconData data;
  PRTime created;
};
 
nsTHashtable<UnassociatedIconHashKey> mUnassociatedIcons;

UnassociatedIconHashKey* icon = mUnassociatedIcons.PutEntry(iconuri);
if (icon) {
  icon.created =
  icon.data.something =
}

it's more self-contained and similar to what we do in other Places code

@@ +150,5 @@
>    ~nsFaviconService();
>  
>    nsRefPtr<mozilla::places::Database> mDB;
>  
> +  nsCOMPtr<nsITimer> mExpireUnassociatedTimer;

keep using the same term, so if you stick with UnassociatedIcon, here use mExpireUnassociatedIconTimer. UnlinkedIcon may be fine too, if you want to use a shorter term, just use the same term in a coherent way

@@ +175,5 @@
>  
>    PRUint32 mFailedFaviconSerial;
>    nsDataHashtable<nsCStringHashKey, PRUint32> mFailedFavicons;
>  
> +  nsClassHashtable<nsCStringHashKey, UnassociatedIconEntry> mUnassociatedFavicons;

ditto, UnassociatedIcons, not UnassociatedFavicons (coherent naming)

Comment 8

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

we may want to slightly improve the suggested API to allow Sync setting the guid, so cc-ing rnewman to check how we can improve that.

Comment 9

[Felix Fung (:felix)]

(In reply to Marco Bonardo [:mak] from comment #7)
> Comment on attachment 575566 [details] [diff] [review] [diff] [details] [review]
> SetFaviconData No Longer Persists Data
> 
> Review of attachment 575566 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> Finally, setting new data should update the database data if that's present,
> otherwise put in the memory hash, or we'll have a de-sync between the
> database and the memory icon.

If we have to check the database for data before putting it into the memory hash, how can we guarantee that the database check will complete before another call checks against the hash considering also that it isn't thread-safe.

Comment 10

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

(In reply to Felix Fung (:felix) from comment #9)
> If we have to check the database for data before putting it into the memory
> hash, how can we guarantee that the database check will complete before
> another call checks against the hash considering also that it isn't
> thread-safe.

what about immediately adding to the hash, and then update the database?

Comment 11

[Felix Fung (:felix)]

(In reply to Marco Bonardo [:mak] from comment #10)
> (In reply to Felix Fung (:felix) from comment #9)
> > If we have to check the database for data before putting it into the memory
> > hash, how can we guarantee that the database check will complete before
> > another call checks against the hash considering also that it isn't
> > thread-safe.
> 
> what about immediately adding to the hash, and then update the database?

Say we've added to the hash and, indeed we do update the database. The issue then is that we may call AsyncAssociateIconToPage with the cached IconData because we haven't invalidated the hash yet so we might INSERT OR REPLACE into the database an extra time...

Comment 12

[Felix Fung (:felix)]

Alternatively, we could not bail on FetchIconInfo and then compare the data from the fetch and the cache and update if necessary.

Comment 13

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

(In reply to Felix Fung (:felix) from comment #11)
> we might INSERT OR REPLACE
> into the database an extra time...

Well, it's probably a minor problem, would be problematic only on really strict loops related to the same icon.

(In reply to Felix Fung (:felix) from comment #12)
> Alternatively, we could not bail on FetchIconInfo and then compare the data
> from the fetch and the cache and update if necessary.

Well, the cache should always win, if we consider it an override. Btw, may even work.

Comment 14

[Felix Fung (:felix)]

Attachment: ReplaceFaviconData

Additional Changes:
- Modified AsyncFetchAndSetIconForPage to lookup against hash before performing any network IO.

- On ReplaceFaviconData:
-- Adds to hash -> Overwrites data in database if it exists -> Removes from hash

- Expiration:
-- We maintain the invariant that a timer is running if and only if there are entries in the hash.
-- Adding first entry to cache will kickoff the timer. Removing the last entry will cancel the timer. After expiration is run, if there are still entries, we kickoff the timer again.

Comment 15

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

Comment on attachment 578260 [details] [diff] [review]
ReplaceFaviconData

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

At this point I think we should handle guids separately in a new bug, or this may become a neverending story. These APIs are relatively new and marked experimental, so we should not be too worried of changing them for now. I also wonder if we should add a callback to this new API, that would be invoked only when we actually update an existing icon. But I'm not sure how much that'd be useful now, and btw it is easily addable later.

Globally this looks pretty good, thank you for investigation solutions to the issues we discusses!

What would be absolutely welcome (possibly in a separate patch) is an xpcshell-test for this. There are some favicons tests already you may take a look at in toolkit/components/places/tests/unit/, feel free to ping me on IRC for assistance.

f+, I'll take a quick final look at the next iteration.

::: toolkit/components/places/AsyncFaviconHelpers.cpp
@@ +191,5 @@
> + *        Icon that should be stored.
> + */
> +nsresult
> +SetIconInfo(nsRefPtr<Database>& aDB,
> +            IconData& _icon)

why is icon prefixed with _, it's not an output param. input params get "a" prefix, output params get "_" prefix

@@ +502,5 @@
> +    PRUint32 beforeIconCount = favicons->mUnassociatedIcons.Count();
> +    favicons->mUnassociatedIcons.RemoveEntry(aFaviconURI);
> +    if (beforeIconCount > 0 &&
> +        favicons->mUnassociatedIcons.Count() == 0) {
> +      favicons->mExpireUnassociatedTimer->Cancel();

I think we may even let the timer continue, it would just do an empty sweep, wouldn't it? If so it's not worth the added complexity.

@@ +1011,5 @@
> +  NS_ENSURE_ARG(aIcon);
> +  NS_PRECONDITION(NS_IsMainThread(),
> +                  "This should be called on the main thread.");
> +
> +  nsCOMPtr<nsIFaviconDataCallback> callback(NULL);

there should be no need to init a nsCOMPtr to NULL (should be nsnull btw)

@@ +1040,5 @@
> +{
> +  NS_PRECONDITION(!NS_IsMainThread(),
> +                  "This should not be called on the main thread");
> +
> +  // for the purposes of the replace, we don't consider the icon cached

Make this a bit more verbose, explaining FetchIconInfo would bail out if it'd consider the icon cached.

@@ +1053,5 @@
> +
> +  // we need to invalidate the cache...
> +  nsCOMPtr<nsIRunnable> event = new RemoveIconDataCacheEntry(mIcon, mCallback);
> +  rv = NS_DispatchToMainThread(event);
> +  NS_ENSURE_SUCCESS(rv, rv);

But we should do this only if we actually updated an icon, so I think you should rather

if (!mIcon.id)
  return NS_OK;

and then all the rest?

@@ +1092,5 @@
> +  PRUint32 beforeIconCount = favicons->mUnassociatedIcons.Count();
> +  favicons->mUnassociatedIcons.RemoveEntry(iconURI);
> +  if (beforeIconCount > 0 &&
> +      favicons->mUnassociatedIcons.Count() == 0) {
> +    favicons->mExpireUnassociatedTimer->Cancel();

As well as here if the issue is just an empty sweep, we may ignore it and simplify the code.

::: toolkit/components/places/mozIAsyncFavicons.idl
@@ +91,5 @@
>                                   in nsIURI aFaviconURI,
>                                   in boolean aForceReload,
>                                   [optional] in nsIFaviconDataCallback aCallback);
> +  /**
> +   * Stores the data for a given favicon URI.

add a newline before /**

@@ +96,5 @@
> +   *
> +   * You can set the data even if you haven't called SetFaviconUrlForPage
> +   * yet but it will only be stored in memory. These are guaranteed a lifespan
> +   * of 1 minute but will be routinely expired thereafter. You shouild
> +   * associate the icon within that time.

The comment needs some updating, for example it's talking about SetFaviconUrlForPage. We should probably rewrite it in a less verbose manner.

"You can set the data even if the icon has not yet been associated to a page.  In such a case the data will be retained in a memory cache for a limited time and will be used in the next call to setAndFetchFaviconForPage."

@@ +132,5 @@
> +  void replaceFaviconData(in nsIURI aFaviconURI,
> +                          [const,array,size_is(aDataLen)] in octet aData,
> +                          in unsigned long aDataLen,
> +                          in AUTF8String aMimeType,
> +                          in PRTime aExpiration);

We should probably make aExpiration [optional], if unused it will use default that is the cache expiration time.

::: toolkit/components/places/nsFaviconService.cpp
@@ +76,5 @@
>  #define FAVICON_CACHE_REDUCE_COUNT 64
>  
> +#define MAX_UNASSOCIATED_FAVICONS 64
> +
> +#define UNLINKED_ICON_EXPIRY_INTERVAL 60000

Add a comment above this briefly explaining what we use it for

@@ +214,5 @@
> +{
> +  PRTime now = PR_Now();
> +  mUnassociatedIcons.EnumerateEntries(
> +    ExpireNonrecentUnassociatedIconsEnumerator, &now);
> +  // reinit the expiry timer since there still exist entries

comments-nit: ucfirst and end with a period.

I'm not english but the phrase sounds strange to me, the "there still exist", maybe I'd say 
// Reinit the expiry time if the cache is not empty."

@@ +427,5 @@
> +                                     PRUint32 aDataLen,
> +                                     const nsACString& aMimeType,
> +                                     PRTime aExpiration)
> +{
> +  NS_ENSURE_ARG(aFaviconURI);

I think it's work to check also aData, aDataLen, aMimeType for validity
and add a MOZ_ASSERT(NS_IsMainThread());

@@ +440,5 @@
> +
> +  iconKey->created = PR_Now();
> +
> +  // There should be an expiry timer if and only if there are
> +  // unassociated icons in our cache

This comment may be improved, something like "// If the cache had unassociated icons there should be an expiry timer already, otherwise fire a new one."

@@ +469,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +  } else {
> +    iconData->mimeType.Assign(aMimeType);
> +    iconData->data.Assign(reinterpret_cast<const char*>(aData), aDataLen);

fwiw, you may move TO_CHARBUFFER and TO_INTBUFFER macros from AsyncFaviconHelpers.cpp to the .h and use them

@@ +472,5 @@
> +    iconData->mimeType.Assign(aMimeType);
> +    iconData->data.Assign(reinterpret_cast<const char*>(aData), aDataLen);
> +  }
> +
> +  rv = AsyncReplaceFaviconData::start(iconData);

add a comment above explaining what happens

@@ +635,5 @@
>    nsCAutoString mimeType;
>    rv = channel->GetContentType(mimeType);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // SetFaviconData can now do the dirty work

nit: While you touch this, please add a closing period

::: toolkit/components/places/nsFaviconService.h
@@ +163,5 @@
>    ~nsFaviconService();
>  
>    nsRefPtr<mozilla::places::Database> mDB;
>  
> +  nsCOMPtr<nsITimer> mExpireUnassociatedTimer;

mExpireUnassociatedIconsTimer

Comment 16

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

important note: you should rev the uuid

Comment 17

[Felix Fung (:felix)]

Attachment: ReplaceFaviconData

Additional Changes:
- Addressed changes above.

Tests are coming.

Comment 18

[Felix Fung (:felix)]

Note: aExpiration is unrelated to the cache expiration time. As in setFaviconData, it shouldn't be optional then.

Comment 19

[Felix Fung (:felix)]

Attachment: Tests for ReplaceFaviconData

These tests cover some of the more basic functionality. I don't know what else is testable. I suppose a separate style of tests is appropriate for testing expiration functionality?

Comment 20

[Felix Fung (:felix)]

Attachment: ReplaceFaviconDataFromDataURL.diff

Summary of Changes:
- Copied over setFaviconDataFromDataURL and then just replaced setFaviconData with replaceFaviconData.

Comment 21

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

(In reply to Felix Fung (:felix) from comment #18)
> Note: aExpiration is unrelated to the cache expiration time. As in
> setFaviconData, it shouldn't be optional then.

yes, but I should be able to say "I'm fine with the default expiration time" and don't have to provide one, that's what I meant by optional. The cache expiration time is just used as the default expiration time by the service.

Comment 22

[Felix Fung (:felix)]

(In reply to Marco Bonardo [:mak] from comment #21)
> (In reply to Felix Fung (:felix) from comment #18)
> > Note: aExpiration is unrelated to the cache expiration time. As in
> > setFaviconData, it shouldn't be optional then.
> 
> yes, but I should be able to say "I'm fine with the default expiration time"
> and don't have to provide one, that's what I meant by optional. The cache
> expiration time is just used as the default expiration time by the service.

I think there is some confusion... aExpiration defines the amount of time a favicon will stay in the DB (even after it has been associated) before the service decides it is no longer valid. If indeed you think that there should be such a default expiration time, what would be a good amount? We have nothing now that defines such a thing.

Comment 23

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

(In reply to Felix Fung (:felix) from comment #22)
> I think there is some confusion... aExpiration defines the amount of time a
> favicon will stay in the DB (even after it has been associated) before the
> service decides it is no longer valid. If indeed you think that there should
> be such a default expiration time, what would be a good amount? We have
> nothing now that defines such a thing.

Sorry, I think I may have been unclear, we have a default "expiration" time, that is exactly what you said, and that default value IS the cache expiration time. We don't use the cache, we just use its time as default because we thought was an acceptable value to respect user's will.
So right now, if I pass 0 to your new API, I'm pretty sure the icon will be added to the database with an expire_time of one week from now, that is the default and is the default cache expiration time. That's why I think making it optional would be clearer than passing a 0.

Comment 24

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

Btw, to be even clearer I'm not blocking review on that, I'll review latest iteration tomorrow morning, I couldn't before due to all the network bustage around.

Comment 25

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

Comment on attachment 578985 [details] [diff] [review]
ReplaceFaviconData

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

You have not fixed some of my comments on nsFaviconService.cpp from previous review:

@@ +427,5 @@
> +                                     PRUint32 aDataLen,
> +                                     const nsACString& aMimeType,
> +                                     PRTime aExpiration)
> +{
> +  NS_ENSURE_ARG(aFaviconURI);

Check also aData, aDataLen, aMimeType for validity and add a MOZ_ASSERT(NS_IsMainThread());

@@ +469,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +  } else {
> +    iconData->mimeType.Assign(aMimeType);
> +    iconData->data.Assign(reinterpret_cast<const char*>(aData), aDataLen);

you moved TO_CHARBUFFER and TO_INTBUFFER but you didn't use them

@@ +472,5 @@
> +    iconData->mimeType.Assign(aMimeType);
> +    iconData->data.Assign(reinterpret_cast<const char*>(aData), aDataLen);
> +  }
> +
> +  rv = AsyncReplaceFaviconData::start(iconData);

add a comment above briefly explaining why we do this, it's clear for me and you, likely not for others.

And now the new comments, mostly nits, I can't find anything blocking.

::: toolkit/components/places/AsyncFaviconHelpers.cpp
@@ +182,5 @@
>    return NS_OK;
>  }
>  
>  /**
> + * Stores information on a icon in the Places database.

nit: s/Places// it's pretty clear which module we are in.

::: toolkit/components/places/mozIAsyncFavicons.idl
@@ +95,5 @@
>    /**
> +   * Stores the data for a given favicon URI.
> +   *
> +   * You can set the data even if you haven't called SetFaviconUrlForPage
> +   * yet but it will only be stored in memory. These are guaranteed a lifespan

add comma after yet

@@ +96,5 @@
> +   * Stores the data for a given favicon URI.
> +   *
> +   * You can set the data even if you haven't called SetFaviconUrlForPage
> +   * yet but it will only be stored in memory. These are guaranteed a lifespan
> +   * of 1 minute but will be routinely expired thereafter. You shouild

we may tweak the lifespan in near future if we found it being sucky, so for now I'd prefer being a bit generic here saying "a limited lifespan"

typo: shouild

@@ +97,5 @@
> +   *
> +   * You can set the data even if you haven't called SetFaviconUrlForPage
> +   * yet but it will only be stored in memory. These are guaranteed a lifespan
> +   * of 1 minute but will be routinely expired thereafter. You shouild
> +   * associate the icon within that time.

this doesn't explain "how" to associate, I'd add "using setAndFetchFaviconForPage"

::: toolkit/components/places/nsFaviconService.cpp
@@ +76,5 @@
>  #define FAVICON_CACHE_REDUCE_COUNT 64
>  
> +#define MAX_UNASSOCIATED_FAVICONS 64
> +
> +// When replaceFaviconData is called, we store the icons in a in-memory cache

nit: I'm not english but I think should be "an in-memory cache"

@@ +78,5 @@
> +#define MAX_UNASSOCIATED_FAVICONS 64
> +
> +// When replaceFaviconData is called, we store the icons in a in-memory cache
> +// instead of in storage. While icons are in the cache, we run a timer with
> +// this interval to expire old entries.

nit: I think this phrase may work better "Icons in the cache are expired according to this interval."

Comment 26

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

Comment on attachment 579005 [details] [diff] [review]
Tests for ReplaceFaviconData

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

Please use "let" everywhere, we stick to that in this module.
It's possible you copied most of the test from some old one, so you may find my comments surprising, don't worry, we have a lot of tests and not all of them have a decent quality yet.

Clearing since this needs sort of rewrite and some more tests

Some things that may be worth testing:
- replace an icon, add a page with that icon, ensure we associate it (you did this)
- replace an icon, add a page with another icon, ensure we don't wrongly associate the cached icon to any icon request
- check the API throws correctly for certain invalid inputs
- add a page with a certain icon, replace the icon, check the page icon is now the new one (you can use waitForAsyncUpdates helper to wait asynchronous db writes)
- check invoking replace twice on the same icon uri keeps the data from the second call, not the first one
- check expiration works (like replace icon with one with expiration of 1 microsecond, setAndFetch and see it gets replaced) (harder to do, so fine if you don't feel like doing it)

::: toolkit/components/places/tests/unit/test_doReplaceFaviconData.js
@@ +33,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

nit: If you wish, in tests you can use the much shorter pd license boilerplate http://www.mozilla.org/MPL/boilerplate-1.1/pd-c

@@ +72,5 @@
> +};
> +
> +var tests = [
> +  {
> +    desc: "test replaceFaviconData for valid history uri",

this should be

add_test(function test_replaceFaviconData_validHistoryURI() {
...

the check property could just be an helper function

@@ +80,5 @@
> +
> +      iconsvc.replaceFaviconData(
> +        this.favicon.uri, this.favicon.data, this.favicon.data.length,
> +        this.favicon.mimetype, Number.MAX_VALUE);
> +      iconsvc.setAndLoadFaviconForPage(this.pageURI, this.favicon.uri, true);

please use the setAndFetch API, that is the newest, and use its callback. then just invoke run_next_test() in the callback itself.

btw, this is not a valid history uri, in the sense it's an orphan entry (no visits and no bookmarks, expiration may kill it at any time), you should add a visit to it to make it really valid.

@@ +99,5 @@
> +        this.originalFavicon.uri, this.replacingFavicon.data,
> +        this.replacingFavicon.data.length, this.replacingFavicon.mimetype,
> +        Number.MAX_VALUE);
> +      iconsvc.setAndLoadFaviconForPage(
> +        this.pageURI, this.originalFavicon.uri, true);

ditto for everything...

Btw I don't understand the difference from this test to the previous one... are not they going through the same code path

@@ +138,5 @@
> +      }
> +    }
> +    else
> +      do_throw("Received PageChanged for a non-current test!");
> +  },

if you use the setAndFetch[...] callback, you may completely kill this observer.

@@ +140,5 @@
> +    else
> +      do_throw("Received PageChanged for a non-current test!");
> +  },
> +
> +  QueryInterface: function(iid) {

btw, this should use XPCOMUtils.generateQI... but as I said it's possible this is useless

@@ +151,5 @@
> +};
> +
> +var currentTestIndex = 0;
> +function run_test() {
> +  do_test_pending();

instead of this, we now use the
run_next_test() and add_test() habit.
so in the global scope you:

add_test(function test_name() {
  // do whatever you need
  run_next_test();
});

run_test should then just setup whatever global thing is needed, and invoke run_next_test() (no need for do_test_pending() or do_test_finished())

@@ +157,5 @@
> +  // check that the favicon loaded correctly
> +  do_check_eq(originalFavicon.data.length, 286);
> +  do_check_eq(replacingFavicon.data.length, 344);
> +
> +  PlacesUtils.history.addObserver(historyObserver, false);

you should remove this observer somewhere

@@ +161,5 @@
> +  PlacesUtils.history.addObserver(historyObserver, false);
> +
> +  // Start the tests
> +  do_log_info(tests[currentTestIndex].desc);
> +  tests[currentTestIndex].go();

here you should just run_next_test();

::: toolkit/components/places/tests/unit/xpcshell.ini
@@ +63,5 @@
>  [test_crash_476292.js]
>  [test_database_replaceOnStartup.js]
>  [test_doSetAndLoadFaviconForPage.js]
>  [test_doSetAndLoadFaviconForPage_failures.js]
> +[test_doReplaceFaviconData.js]

I know we are not really good at doing that, but please keep alphabetical order.

Comment 27

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

Comment on attachment 579008 [details] [diff] [review]
ReplaceFaviconDataFromDataURL.diff

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

This will love a test, clearly!

::: toolkit/components/places/mozIAsyncFavicons.idl
@@ +139,5 @@
>    /**
> +   * Same as replaceFaviconData but the data is provided by a string
> +   * containing a data URL.
> +   *
> +   * @see replaceFaviconData

ok if it lands with the other patch, otherwise will need to rev uuid.
For the sake of easy reviews, you may coalesce both idl changes to a separate patch and ask SR on that simple patch.

::: toolkit/components/places/nsFaviconService.cpp
@@ +591,5 @@
> +nsFaviconService::ReplaceFaviconDataFromDataURL(nsIURI* aFaviconURI,
> +                                                const nsAString& aDataURL,
> +                                                PRTime aExpiration)
> +{
> +  NS_ENSURE_ARG(aFaviconURI);

also check aDataURL.Length() > 0 and MOZ_ASSERT(NS_IsMainThread())

@@ +619,5 @@
> +  PRUint32 available;
> +  rv = stream->Available(&available);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (available == 0)
> +    return NS_ERROR_FAILURE;

I'd probably throw an NS_ERROR_INVALID_ARG

@@ +630,5 @@
> +  PRUint32 numRead;
> +  rv = stream->Read(reinterpret_cast<char*>(buffer), available, &numRead);
> +  if (NS_FAILED(rv) || numRead != available) {
> +    nsMemory::Free(buffer);
> +    return rv;

is it guaranteed that if numRead != available rv is an error condition? (I think so, but doesn't seem obvious, maybe would be safer to always throw an NS_ERROR_UNEXPECTED)

Comment 28

[Felix Fung (:felix)]

Attachment: Tests for ReplaceFaviconData

Summary of Changes:
- Addressed above comments
- Added all suggested tests except for timeout

Comment 29

[Felix Fung (:felix)]

Attachment: Interface Changes to mozIAsyncFavicons

Comment 30

[Felix Fung (:felix)]

Attachment: ReplaceFaviconDataFromDataURL

Additional Changes:
- Separated interface changes

Comment 31

[Felix Fung (:felix)]

Attachment: ReplaceFaviconData

Additional Changes:
- Added comments
- Asserted some args
- Separated interface changes

Comment 32

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

Comment on attachment 581106 [details] [diff] [review]
Interface Changes to mozIAsyncFavicons

I'm not a superreviewer, forwarding to gavin.

I'd probably not add [deprecated] annotations since I think it would still warn (we still implement those methods) on compile.
on the other side the @deprecated javadoc is fine.

Comment 33

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

Comment on attachment 581106 [details] [diff] [review]
Interface Changes to mozIAsyncFavicons

>diff --git a/toolkit/components/places/mozIAsyncFavicons.idl b/toolkit/components/places/mozIAsyncFavicons.idl

>+   * You can set the data even if you haven't called SetFaviconUrlForPage
>+   * yet, but it will only be stored in memory. These are given a limited
>+   * lifespan but will be routinely expired thereafter. You shouild
>+   * associate the icon using setAndFetchFaviconForPage as soon as you can.

Is there any reason someone wouldn't always just call setFaviconUrlForPage first? setFaviconData on nsIFaviconService's comment implies it needs to be done this way to get proper notifications? That's kind of gross. Can we fix that? (in another bug, of course)

I'd reword this slightly:

"Favicon data for favicon URIs that are not associated with a page URI via setFaviconUrlForPage will be stored in memory, but may be expired at any time, so you should make an effort to associate favicon URIs with page URIs as soon as possible."

Some nits:
- s/SetFaviconUrlForPage/setFaviconUrlForPage/
- typo: shouild

I agree with Marco re: [deprecated].

Having to make up some other name for the method to avoid conflicts with nsIFaviconService is also sucky. "replaceFaviconData" really isn't quite as good of a name as "setFaviconData". Can we use "assignFaviconData"? (Do we have a plan to remove the sync methods at some point?)

sr=me with those addressed.

Comment 34

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

(In reply to Gavin Sharp (use [e-mail] for email) from comment #33)
> >diff --git a/toolkit/components/places/mozIAsyncFavicons.idl b/toolkit/components/places/mozIAsyncFavicons.idl
> 
> >+   * You can set the data even if you haven't called SetFaviconUrlForPage
> >+   * yet, but it will only be stored in memory. These are given a limited
> >+   * lifespan but will be routinely expired thereafter. You shouild
> >+   * associate the icon using setAndFetchFaviconForPage as soon as you can.
> 
> Is there any reason someone wouldn't always just call setFaviconUrlForPage
> first? setFaviconData on nsIFaviconService's comment implies it needs to be
> done this way to get proper notifications? That's kind of gross. Can we fix
> that? (in another bug, of course)

Hm, this comment is just wrong, nothing in the new API requires calls to the old one... I asked this to be fixed in comment 15, but looks like my review comment was not applied. The new API clarifies the functionality by using the word Replace and by using the in memory cache, so it's like you are overriding data for the next call to setAndFetchFaviconForPage.

> - typo: shouild

I also notified this typo in comment 25...

I'm starting thinking we have to improve readability of bugzilla queries, since comments should not fall in the cracks.

> I agree with Marco re: [deprecated].
> 
> Having to make up some other name for the method to avoid conflicts with
> nsIFaviconService is also sucky. "replaceFaviconData" really isn't quite as
> good of a name as "setFaviconData". Can we use "assignFaviconData"? (Do we
> have a plan to remove the sync methods at some point?)

As I explained, replace is clearer imo, since:
- if the icon does not exist it will override the next query, replacing its data
- if the icon exists it will actually just replace it in the database

Unfortunately the idl comments are not good enough to clarify that

Comment 35

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

(In reply to Gavin Sharp (use [e-mail] for email) from comment #33)
> (Do we
> have a plan to remove the sync methods at some point?)

Yes the plan is deprecate and remove all of nsIFaviconService interface.

Comment 36

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

Comment on attachment 581109 [details] [diff] [review]
ReplaceFaviconDataFromDataURL

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

::: toolkit/components/places/nsFaviconService.cpp
@@ +603,5 @@
> +                                                const nsAString& aDataURL,
> +                                                PRTime aExpiration = MAX_FAVICON_EXPIRATION)
> +{
> +  NS_ENSURE_ARG(aFaviconURI);
> +  if (mFaviconsExpirationRunning)

This is still missing some input checks (like non empty url)

the optional argument is handled differently from ReplaceFaviconData implementation. could you stick to one of the 2?

@@ +610,5 @@
> +  nsCOMPtr<nsIURI> dataURI;
> +  nsresult rv = NS_NewURI(getter_AddRefs(dataURI), aDataURL);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // use the data: protocol handler to convert the data

global nit: UCfirst and use punctuation (like final period) in comments

@@ +646,5 @@
> +  }
> +
> +  nsCAutoString mimeType;
> +  rv = channel->GetContentType(mimeType);
> +  NS_ENSURE_SUCCESS(rv, rv);

you are not freeing buffer if this fails.

Comment 37

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

Comment on attachment 581100 [details] [diff] [review]
Tests for ReplaceFaviconData

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

::: toolkit/components/places/tests/unit/test_doReplaceFaviconData.js
@@ +17,5 @@
> +
> +let uniqueFaviconId = 0;
> +function createFavicon(fileName) {
> +  let dirsvc = Cc["@mozilla.org/file/directory_service;1"]
> +    .getService(Ci.nsIProperties);

Services.dirsvc.

@@ +26,5 @@
> +  let outfile = tempdir.clone();
> +  outfile.append(fileName);
> +  try { outfile.remove(false); } catch (e) {}
> +
> +  originalFavicon.file.copyTo(tempdir, fileName);

Just in case, use copyToFollowingLinks

@@ +63,5 @@
> +
> +function checkAddSucceeded(pageURI, mimetype, data) {
> +  let savedFaviconURI = iconsvc.getFaviconForPage(pageURI);
> +  let outMimeType = {};
> +  let outData = iconsvc.getFaviconData(savedFaviconURI, outMimeType, {});

If possible, we should use the new APIs in mozIAsyncFavicons ( getFaviconURLForPage, getFaviconDataForPage), not the old ones. Though that may add some async complication to your test, if so we'll fix this at a later stage of deprecation.

@@ +88,5 @@
> +
> +  iconsvc.replaceFaviconData(favicon.uri, favicon.data, favicon.data.length,
> +    favicon.mimetype);
> +  iconsvc.setAndFetchFaviconForPage(pageURI, favicon.uri, true,
> +    function test_replaceFaviconData_validHistoryURI_check() {

The callback gives you the favicon data, so you would not need a complicate checkAddSucceeded, or you may check both.

This is valid for all setAndFetchFaviconForPage calls

@@ +179,5 @@
> +  } catch (e) {
> +    ex = e;
> +  } finally {
> +    do_check_true(!!ex);
> +  }

in the catch you can do_throw("Error bla bla " + e), no need to do complicate tracking

Comment 38

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

PS: are tests for the DataURL version still missing or i lost them in the middle of the review?

Comment 39

[Felix Fung (:felix)]

Attachment: Tests for AssignFaviconDataFromDataURL

Summary of Changes:
- Copied the ReplaceFaviconData tests, using FromDataURL instead
- Added some tests combining regular assign and assignfromdataurl

Comment 40

[Felix Fung (:felix)]

Attachment: Interface Changes to mozIAsyncFavicons

Additional Changes:
- s/Assign/Replace/
- Added lengthier comment to replaceFaviconData which should help clarify why it's called replace.

Comment 41

[Felix Fung (:felix)]

Attachment: Tests for ReplaceFaviconData

Additional Changes:
- Addressed comments above
- Used waitForClearHistory as necessary

Comment 42

[Felix Fung (:felix)]

Attachment: Tests for ReplaceFaviconDataFromDataURL

s/Assign/Replace/

Comment 43

[Felix Fung (:felix)]

Attachment: ReplaceFaviconDataFromDataURL

Additional Changes:
- Addressed above comments

Comment 44

[Felix Fung (:felix)]

Attachment: ReplaceFaviconData

Additional Changes:
- Guarantee that only one running timer at a time. (i.e. InitWithCallback can be called multiple times without cancel ever being called between them)

Comment 45

[Felix Fung (:felix)]

Pending review of the ReplaceFaviconDataFromDataURL Tests, I plan on pushing all of the above. Objections?

Comment 46

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

Comment on attachment 581536 [details] [diff] [review]
Interface Changes to mozIAsyncFavicons

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

::: toolkit/components/places/mozIAsyncFavicons.idl
@@ +99,5 @@
> +   *
> +   * Favicon data for favicon URIs that are not associated with a page URI via
> +   * setFaviconUrlForPage will be stored in memory, but may be expired at any
> +   * time, so you should make an effort to associate favicon URIs with page URIs
> +   * as soon as possible.

This is still talking about setFaviconUrlForPage, that API is in the old interface, should not be here.
s/setFaviconUrlForPage/setAndFetchFaviconForPage/

Comment 47

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

Comment on attachment 581539 [details] [diff] [review]
Tests for ReplaceFaviconDataFromDataURL

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

I assume it's mostly the same as the other test.

Comment 48

[Felix Fung (:felix)]

Attachment: Interface Changes to mozIAsyncFavicons

Additional Changes:
- Replaced setFaviconUrlForPage with setAndFetchFaviconForPage in comment.

Comment 49

[Felix Fung (:felix)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46bc102ad373
https://hg.mozilla.org/integration/mozilla-inbound/rev/e28b8215c930
https://hg.mozilla.org/integration/mozilla-inbound/rev/1617e3a6711b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1fd708628b
https://hg.mozilla.org/integration/mozilla-inbound/rev/13390e878a3c

Comment 50

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/46bc102ad373
https://hg.mozilla.org/mozilla-central/rev/e28b8215c930
https://hg.mozilla.org/mozilla-central/rev/1617e3a6711b
https://hg.mozilla.org/mozilla-central/rev/6c1fd708628b
https://hg.mozilla.org/mozilla-central/rev/13390e878a3c