737133 - getFaviconURLForPage and getFaviconDataForPage should invoke nsIFaviconDataCallback even if the favicon is not available.

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[:Paolo Amadini]

Currently, the two functions getFaviconURLForPage and getFaviconDataForPage in
mozIAsyncFavicons don't invoke the specified nsIFaviconDataCallback when there
is no favicon associated with the page.

The caller might need to execute a default action if the favicon data is not
available, like displaying a default favicon.

To allow this, the function should invoke the onFaviconDataAvailable function
in all cases, passing a null value as its aURI argument when favicon data is
not available.

Comment 1

[:Paolo Amadini]

Attachment: The patch

Marco, the only question I have upfront on this patch is when it's possible to
use anonymous functions, as I've seen you used them in Places tests recently.

I've cleaned up the terminology on the comments for the functions I touched, so
that we use the same terms "page URI", "favicon URI" and "favicon link URI" when
naming the same type of URI.

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 610519 [details] [diff] [review]
The patch

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

::: toolkit/components/places/AsyncFaviconHelpers.cpp
@@ +890,5 @@
>    nsCAutoString iconSpec;
>    nsresult rv = FetchIconURL(mDB, mPageSpec, iconSpec);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // Now notify our callback of the icon spec we retrieved, even if empty.

you want to change the above NS_ENSURE_SUCCESS to a MOZ_ASSERT or you won't notify.

@@ +953,5 @@
>                    "This should not be called on the main thread.");
>  
>    nsCAutoString iconSpec;
>    nsresult rv = FetchIconURL(mDB, mPageSpec, iconSpec);
>    NS_ENSURE_SUCCESS(rv, rv);

ditto

@@ +963,5 @@
>    pageData.spec.Assign(mPageSpec);
>  
> +  if (!iconSpec.IsEmpty()) {
> +    rv = FetchIconInfo(mDB, iconData);
> +    NS_ENSURE_SUCCESS(rv, rv);

ditto

@@ +1092,5 @@
>  {
>    NS_PRECONDITION(NS_IsMainThread(),
>                    "This should be called on the main thread");
>  
> +  nsCOMPtr<nsIURI> iconURI = nsnull;

no reason to init to nsnull, that's the default afaik

::: toolkit/components/places/mozIAsyncFavicons.idl
@@ +167,3 @@
>     * @param aCallback
> +   *        This callback is always invoked to notify the result of the lookup,
> +   *        unless an exception occurs during the method's execution.  The aURI

Yeah, I think we should make this consistent, always callback on failure, regardless the failure reason.

@@ +168,5 @@
> +   *        This callback is always invoked to notify the result of the lookup,
> +   *        unless an exception occurs during the method's execution.  The aURI
> +   *        parameter will be the favicon URI, or null if no favicon is
> +   *        associated with the page.  Note that aDataLen will be 0, aData will
> +   *        be an empty array, and aMimeType will be an empty string.

move the note to a @note

@@ +184,3 @@
>     * @param aCallback
> +   *        This callback is always invoked to notify the result of the lookup,
> +   *        unless an exception occurs during the method's execution.  The aURI

as above

::: toolkit/components/places/nsIFaviconService.idl
@@ +335,5 @@
>     * Called when the required favicon's information is available.
>     *
> +   * Unless otherwise specified in the method's documentation, this callback
> +   * will be called only if the operation is successful, otherwise you won't get
> +   * notified.

I think, to avoid confusing the user, here we should just say:

It's up to the invoking method to state if the callback is always invoked or only on
success.  Check the method documentation to ensure that.

We should file a bug to make SetAndFetch notify failures through the callback and then remove this differentiation completely, since it's just complicating the idl. Doing that may not be that easy fwiw...

@@ +344,5 @@
>     * get it, or the method does not require we actually have any data).
>     * It's up to the caller to check aDataLen > 0 before using any data-related
>     * information like mime-type or data itself.
>     *
>     * @param aURI

while here, may we rename this to aFaviconURI (also in the method obviously)? should be clearer.

@@ +349,5 @@
>     *        Depending on caller method it could be:
>     *          - a dataURI: has "data:" scheme, with base64 encoded icon data.
>     *          - a faviconURI: the URI of the icon in the favicon service.
>     *          - a faviconLinkURI: has "moz-anno" scheme, used to get data async.
> +   *        This can also be null if specified in the method's documentation.

and here:

Is null when the callback is notifying a failure.

@@ +361,2 @@
>     */
>    void onFaviconDataAvailable(in nsIURI aURI,

At this point, since we are doing a quite important change (that requires null check), we should also rename this, from onFaviconDataAvailable (that makes no sense when we don't find an icon) to onCompletion.

Comment 3

[:Paolo Amadini]

Attachment: Addressed review comments

There's likely another iteration here, as assertions aren't that easy to do
right the first time! Also the comment on the main callback function was
outdated, please check if it now reflects reality :-)

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 610582 [details] [diff] [review]
Addressed review comments

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

almost there, but no r+ cause an assertion is bogus

::: browser/base/content/browser.js
@@ +3596,5 @@
>      item.setAttribute("label", entry.title || uri);
>      item.setAttribute("index", j);
>  
>      if (j != index) {
>        function FHM_getFaviconURLCallback(aURI) {

actually, if I read it correctly this is not allowed anymore by ecmascript strict mode
(http://whereswalden.com/2011/01/24/new-es5-strict-mode-requirement-function-statements-not-at-top-level-of-a-program-or-function-are-prohibited/)
while here we may fix it.

::: toolkit/components/places/AsyncFaviconHelpers.cpp
@@ +1121,5 @@
> +void
> +NotifyIconObservers::SendGlobalNotifications(nsIURI* aIconURI)
> +{
> +  nsCOMPtr<nsIURI> pageURI;
> +  MOZ_ASSERT(NS_SUCCEEDED(NS_NewURI(getter_AddRefs(pageURI), mPage.spec)));

you can't do this, MOZ_ASSERT exists only in debug, in release you would always have a null pageURI then.

@@ +1123,5 @@
> +{
> +  nsCOMPtr<nsIURI> pageURI;
> +  MOZ_ASSERT(NS_SUCCEEDED(NS_NewURI(getter_AddRefs(pageURI), mPage.spec)));
> +  if (pageURI)
> +  {

brace lined with the if

::: toolkit/components/places/mozIAsyncFavicons.idl
@@ +167,4 @@
>     * @param aCallback
> +   *        This callback is always invoked to notify the result of the lookup.
> +   *        The aURI parameter will be the favicon URI, or null if no favicon is
> +   *        associated with the page.

or an error occured while fetching it.

@@ +185,4 @@
>     * @param aCallback
> +   *        This callback is always invoked to notify the result of the lookup.  The aURI
> +   *        parameter will be the favicon URI, or null if no favicon is
> +   *        associated with the page.  If aURI is not null, the other parameters

ditto

::: toolkit/components/places/nsIFaviconService.idl
@@ +349,5 @@
> +   *        be null if there is no associated favicon URI, or the callback is
> +   *        notifying a failure.  If you want to open a network channel to
> +   *        access the favicon, it's recommended that you call the
> +   *        getFaviconLinkForIcon method to convert this favicon URI into a
> +   *        "favicon link URI".

the latter part should be moved to a @note

Comment 5

[:Paolo Amadini]

Attachment: Updated patch

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 611163 [details] [diff] [review]
Updated patch

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

::: toolkit/components/places/AsyncFaviconHelpers.cpp
@@ +1099,5 @@
>    nsCOMPtr<nsIURI> iconURI;
> +  if (!mIcon.spec.IsEmpty()) {
> +    // We continue even if the favicon URI spec we retrieved is invalid.
> +    (void)NS_NewURI(getter_AddRefs(iconURI), mIcon.spec);
> +    if (iconURI)

not sure what the comment is trying to say, but if it's invalid iconURI continues being null and we don't enter the if regardless.
You may do the same you are doing below with MOZ_ALWAYS_TRUE(NS_SUCCEEDED...

::: toolkit/components/places/mozIAsyncFavicons.idl
@@ +169,5 @@
> +   *        The aURI parameter will be the favicon URI, or null when no favicon
> +   *        is associated with the page or an error occurred while fetching it.
> +   *
> +   * @note When the callback is invoked, aDataLen will be always 0, aData will
> +   *       be an empty array, and aMimeType will be an empty string.

I'd add something like "regardless existence of the icon", just to further clarify.

::: toolkit/components/places/nsIFaviconService.idl
@@ +327,5 @@
>     */
>    readonly attribute nsIURI defaultFavicon;
>  };
>  
> +[scriptable, function, uuid(c85e5c82-b70f-4621-9528-beb2aa47fb44)]

please request SR, maybe gavin, not sure who else may be willing to look at this.

Comment 7

[:Paolo Amadini]

Attachment: Minor changes

Requesting sr to Gavin.

Comment 8

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

Comment on attachment 611738 [details] [diff] [review]
Minor changes

>diff --git a/toolkit/components/places/AsyncFaviconHelpers.cpp b/toolkit/components/places/AsyncFaviconHelpers.cpp

>+    if (!NS_SUCCEEDED(rv)) {

NS_FAILED

>+NotifyIconObservers::SendGlobalNotifications(nsIURI* aIconURI)

>+  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_NewURI(getter_AddRefs(pageURI), mPage.spec)));
>+  if (pageURI) {

Isn't this a non sequitur? If it's always true why are you checking it? Seems like you should remove the MOZ_ALWAYS_TRUE, there are plenty of valid reasons why URI creation could fail.

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

> interface nsIFaviconDataCallback : nsISupports

>-   * @param aURI
>-   *        Depending on caller method it could be:
>-   *          - a dataURI: has "data:" scheme, with base64 encoded icon data.
>-   *          - a faviconLinkURI: has "moz-anno" scheme, used to get data async.

Were these results ever possible? I.e. was this comment just wrong, or did we just forget to update it when we removed some users of nsIFaviconDataCallback?

>    * @param aData
>-   *        Icon data, null if aDataLen is 0.
>+   *        Icon data, null or empty array if aDataLen is 0.
>    * @param aMimeType
>-   *        Mime type of the icon, null if aDataLen is 0.
>+   *        Mime type of the icon, null or empty string if aDataLen is 0.

This seems odd - why are there now three options for these? Why would these ever be empty arrays/empty string instead of null?

Comment 9

[Marco Bonardo [:mak]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #8)
> >+  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_NewURI(getter_AddRefs(pageURI), mPage.spec)));
> >+  if (pageURI) {
> 
> Isn't this a non sequitur? If it's always true why are you checking it?
> Seems like you should remove the MOZ_ALWAYS_TRUE, there are plenty of valid
> reasons why URI creation could fail.

That is there exactly to notify us if it should ever happen in a debug build. We don't expect it to happen, but we want to be notified if a test would ever cause it.
The name is misleading, but basically it's the same as MOZ_ASSERT with the difference in opt builds its contents are executed (but not checked).

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

> >-   * @param aURI
> >-   *        Depending on caller method it could be:
> >-   *          - a dataURI: has "data:" scheme, with base64 encoded icon data.
> >-   *          - a faviconLinkURI: has "moz-anno" scheme, used to get data async.
> 
> Were these results ever possible? I.e. was this comment just wrong, or did
> we just forget to update it when we removed some users of
> nsIFaviconDataCallback?

Ehr, I missed this, those were there to support future additions to the API, I think we should keep them, until we declare the API "complete".

Comment 10

[:Paolo Amadini]

Attachment: Updated comments

(In reply to Gavin Sharp (use [e-mail] for email) from comment #8)
> >    * @param aData
> >-   *        Icon data, null if aDataLen is 0.
> >+   *        Icon data, null or empty array if aDataLen is 0.
> >    * @param aMimeType
> >-   *        Mime type of the icon, null if aDataLen is 0.
> >+   *        Mime type of the icon, null or empty string if aDataLen is 0.
> 
> This seems odd - why are there now three options for these? Why would these
> ever be empty arrays/empty string instead of null?

Looking at the current code, it looks they are never null. I suppose either they
were never null, or this comment refers to a very old implementation.

I think the documentation in the source code should reflect the current state,
so I updated the comment accordingly. We could investigate if old code used to
pass null, to document it in MDN, because the behavior in Firefox 10 can be
interesting to developers wanting to support the ESR version also. I couldn't
find the MXR tree for the Firefox ESR version, though.

(In reply to Marco Bonardo [:mak] from comment #9)
> > >-   * @param aURI
> > >-   *        Depending on caller method it could be:
> > >-   *          - a dataURI: has "data:" scheme, with base64 encoded icon data.
> > >-   *          - a faviconLinkURI: has "moz-anno" scheme, used to get data async.
> > 
> > Were these results ever possible? I.e. was this comment just wrong, or did
> > we just forget to update it when we removed some users of
> > nsIFaviconDataCallback?
> 
> Ehr, I missed this, those were there to support future additions to the API,
> I think we should keep them, until we declare the API "complete".

Here, I'd suggest to document the current version of the API, because in the
future, if we add a new method that returns a different type of URI (for example,
a "favicon link URI" instead of a "favicon URI"), we might also decide to use a
different interface instead, to be unambiguous (callers wouldn't be able to
reuse the same callback implementation in any case, as we'll pass totally
different information).

But if we really want an overloaded API, we should call the argument generically
aURI, instead of aFaviconURI.

Comment 11

[:Paolo Amadini]

(In reply to Paolo Amadini [:paolo] from comment #10)
> Created attachment 613251 [details] [diff] [review]
> Updated comments

Note that the Bugzilla interdiff with the previous patch is broken.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 613251 [details] [diff] [review]
Updated comments

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

::: browser/components/preferences/aboutPermissions.js
@@ +105,5 @@
> +    // Try to find favicon for both URIs, but always prefer the https favicon.
> +    gFaviconService.getFaviconURLForPage(this.httpsURI, function (aURI) {
> +      if (aURI) {
> +        invokeCallback(aURI);
> +      } else {

I have no idea if this file used cuddled bracing on if/else, please check the surrounding code.

::: toolkit/components/places/AsyncFaviconHelpers.cpp
@@ +1091,5 @@
> +  if (!mIcon.spec.IsEmpty()) {
> +    // We don't assert that the favicon URI spec is always valid, because it was
> +    // retrieved from the database, and it's possible that on this system or
> +    // product version the URI spec is not valid anymore.
> +    (void)NS_NewURI(getter_AddRefs(iconURI), mIcon.spec);

Any API uses nsIURI, if something enters the database it went through nsIURI. the possibilities this fails are not worth all of this verbosity, just assert and we'll know.

Comment 13

[:Paolo Amadini]

Pushed with the requested assertion change:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a445c628b53

Comment 14

[:Paolo Amadini]

To use these functions from JavaScript in a way that is compatible with all the
recent platform versions, this technique can be used:


function callbackFn(aURI, aDataLen, aData, aMimeType) {
  if (aURI) {
    // This indicates that the icon is present.
    // In this case, the callback is invoked in all recent Gecko versions.
  } else {
    // This indicates a failure or that the icon is missing.
    // This is only notified in Gecko 14 and later.
  }
}

var pageURI = ...
var asyncFavicons = Cc["@mozilla.org/browser/favicon-service;1"]
                    .getService(Ci.mozIAsyncFavicons);
asyncFavicons.getFaviconDataForPage(pageURI, callbackFn);



This code does not work anymore and should be changed to the above:

asyncFavicons.getFaviconDataForPage(pageURI,
                                    { onFaviconDataAvailable: callbackFn });

Comment 15

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1a445c628b53