429832 - Add API to nsIFaviconService to handle data URIs

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement, P3)

Description

[Manish Singh]

Attachment: Add API to nsIFaviconService to handle data URIs

This API would make it easier for JS consumers to use, as dealing with byte arrays can be annoying and could perform poorly in JS. I basically moved the code from nsPlacesImportExportService to nsFaviconService, and exposed the two functions.

Comment 1

[Dietrich Ayala (:dietrich)]

Comment on attachment 316592 [details] [diff] [review]
Add API to nsIFaviconService to handle data URIs

requesting first-r from Marco.

Comment 2

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

Comment on attachment 316592 [details] [diff] [review]
Add API to nsIFaviconService to handle data URIs

as said this is mainly a code move so should be safe enough, the code has been ported 1:1.

   /**
+   * This function is a convenience wrapper around setFaviconData that
+   * consumes a string containing a data: URI instead of a byte array.
+   *
+   * @see setFaviconData
+   *
+   * @param aFavicon
+   *        URI of the favicon whose data is being set.
+   * @param aData
+   *        Binary contents of the favicon to save
+   * @param aExpiration
+   *        Time in microseconds since the epoch when this favicon expires.
+   *        Until this time, we won't try to load it again.
+   */
+  void setFaviconDataFromDataURI(in nsIURI aFavicon, in ACString aDataURI,
+                                 in PRTime aExpiration);

use aFaviconURI.
i also would comment this without talking about "convenience wrapper", simply explain what this function does, and forward user to SetFaviconData usage for more details.
 
   /**
+   * This function is a convenience wrapper around getFaviconData that
+   * returns a string containing a data: URI instead of a byte array.
+   *
+   * @see getFaviconData
+   *
+   * @param aFavicon
+   *        URL of the favicon whose data is being read
+   * @returns A data URI containing the data of the favicon. This will be
+   *          null if we have this URL but have no data associated with it.
+   * @throws NS_ERROR_NOT_AVAILABLE
+   *         Thrown when we have never heard of this favicon URL.
+   */
+  ACString getFaviconDataAsDataURI(in nsIURI aFavicon);

As before for the method comment.
use aFaviconURI.
s/URL of the favicon/URI of the favicon/.
s/A data URI containing the data of the favicon. This will be null if we have this URL but have no data associated with it./A data URI containing data of the favicon, null if we have no data associated with aFaviconURI./


+// nsFaviconService::SetFaviconDataFromDataURI
+//
+//    See the IDL for this function for lots of info. Note from there: we don't
+//    send out notifications.

It's already clear enough that the user should check the idl, so remove first comment, put only:

// This does not send out notifications.


+
+NS_IMETHODIMP
+nsFaviconService::SetFaviconDataFromDataURI(nsIURI* aFavicon,
+                                            const nsACString& aDataURI,
+                                            PRTime aExpiration)
+{

Use aFaviconURI


+  nsresult rv;
+
+  nsCOMPtr<nsIURI> dataURI;
+  rv = NS_NewURI(getter_AddRefs(dataURI), aDataURI);
+  NS_ENSURE_SUCCESS(rv, rv);

remove the rv def and do directly 

nsresult rv = NS_NewURI(getter_AddRefs(dataURI), aDataURI);


+// nsFaviconService::GetFaviconDataAsDataURI
+
+NS_IMETHODIMP
+nsFaviconService::GetFaviconDataAsDataURI(nsIURI* aFavicon,
+                                          nsACString& aDataURI)
+{

use aFaviconURI

+  nsresult rv;
+
+  PRUint8* data;
+  PRUint32 dataLen;
+  nsCAutoString mimeType;
+
+  rv = GetFaviconData(aFavicon, mimeType, &dataLen, &data);
+  NS_ENSURE_SUCCESS(rv, rv);

as before define rv at first use, nsresult rv = GetFaviconData(aFavicon, mimeType, &dataLen, &data);


Index: toolkit/components/places/tests/unit/test_favicons.js
===================================================================
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_favicons.js,v
retrieving revision 1.3
diff -u -p -r1.3 test_favicons.js
--- toolkit/components/places/tests/unit/test_favicons.js	20 Jan 2008 06:48:07 -0000	1.3
+++ toolkit/components/places/tests/unit/test_favicons.js	19 Apr 2008 14:50:20 -0000
@@ -72,6 +72,9 @@ function setAndGetFaviconData(aFilename,
                            aData, aData.length, aMimeType,
                            Number.MAX_VALUE);
 
+    var dataURI = iconsvc.getFaviconDataAsDataURI(iconURI);
+    iconsvc.setFaviconDataFromDataURI(iconURI, dataURI, Number.MAX_VALUE);
+

The test should probably check if created data and saved data are consistent with what we are expecting, simply getting and setting does not appear a useful one (apart from catching a throw). So you should probably compare the result with a known one pre-built in a known-working way.

Apart the test that needs to be revised, the rest should not create any issue since it's mostly a move of an already used code.

please, provide an updated patch and ask final review to Dietrich.

Comment 3

[Manish Singh]

> Index: toolkit/components/places/tests/unit/test_favicons.js
> ===================================================================
> RCS file:
> /cvsroot/mozilla/toolkit/components/places/tests/unit/test_favicons.js,v
> retrieving revision 1.3
> diff -u -p -r1.3 test_favicons.js
> --- toolkit/components/places/tests/unit/test_favicons.js       20 Jan 2008
> 06:48:07 -0000      1.3
> +++ toolkit/components/places/tests/unit/test_favicons.js       19 Apr 2008
> 14:50:20 -0000
> @@ -72,6 +72,9 @@ function setAndGetFaviconData(aFilename,
>                             aData, aData.length, aMimeType,
>                             Number.MAX_VALUE);
> 
> +    var dataURI = iconsvc.getFaviconDataAsDataURI(iconURI);
> +    iconsvc.setFaviconDataFromDataURI(iconURI, dataURI, Number.MAX_VALUE);
> +
> 
> The test should probably check if created data and saved data are consistent
> with what we are expecting, simply getting and setting does not appear a useful
> one (apart from catching a throw). So you should probably compare the result
> with a known one pre-built in a known-working way.

It is useful because of where I put it: setAndGetFaviconData now does setFaviconData -> getFaviconDataAsDataURI -> setFaviconDataFromDataURI -> getFaviconData, so if any one of those mess up, it'd be caught by the calling code.

A more granular alternative would be to define a setAndGetFaviconDataURI that only touches the new functions, and then duplicating the calling code, but that makes the test harder to maintain. Do you think it's worthwhile?

Comment 4

[Manish Singh]

(In reply to comment #2)
>    /**
> +   * This function is a convenience wrapper around setFaviconData that
> +   * consumes a string containing a data: URI instead of a byte array.
> +   *
> +   * @see setFaviconData
> +   *
> +   * @param aFavicon
> +   *        URI of the favicon whose data is being set.
> +   * @param aData
> +   *        Binary contents of the favicon to save
> +   * @param aExpiration
> +   *        Time in microseconds since the epoch when this favicon expires.
> +   *        Until this time, we won't try to load it again.
> +   */
> +  void setFaviconDataFromDataURI(in nsIURI aFavicon, in ACString aDataURI,
> +                                 in PRTime aExpiration);
> 
> use aFaviconURI.

setFaviconData/getFaviconData use aFavicon, should I change those for consistency as well?

Comment 5

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

> (In reply to comment #4)
> setFaviconData/getFaviconData use aFavicon, should I change those for
> consistency as well?
> 

i'll change them in another bug, thanks :)

So, we surely need better tests for the favicon service in general, but probably in this case setting and getting the data is enough if they are checked by other code for consistency

Comment 6

[John P Baker]

>    /**
> +   * This function is a convenience wrapper around setFaviconData that
> +   * consumes a string containing a data: URI instead of a byte array.
> +   *
> +   * @see setFaviconData
> +   *
> +   * @param aFavicon
> +   *        URI of the favicon whose data is being set.
> +   * @param aData
> +   *        Binary contents of the favicon to save
> +   * @param aExpiration
> +   *        Time in microseconds since the epoch when this favicon expires.
> +   *        Until this time, we won't try to load it again.
> +   */
> +  void setFaviconDataFromDataURI(in nsIURI aFavicon, in ACString aDataURI,
> +                                 in PRTime aExpiration);

|@param aData Binary ... to save| needs edit after copy & paste to match actuality |@param aDataURI string containing a data URI|.

Comment 7

[Manish Singh]

Attachment: New patch, addresses comments

Comment 8

[Manish Singh]

Comment on attachment 318047 [details] [diff] [review]
New patch, addresses comments

Carrying forward Marco's r+

Comment 9

[Manish Singh]

Attachment: Sync up patch with current code

Comment 10

[Manish Singh]

Comment on attachment 319966 [details] [diff] [review]
Sync up patch with current code

Carrying forward Marco's r+

Comment 11

[John P Baker]

Comment on attachment 319966 [details] [diff] [review]
Sync up patch with current code

> +   * @param aDataURI
> +   *        string containing a data URI that represents the contents of
> +   *        the favicon to save

Now that URI suffix is being used consistently to indicate an nsURI argument (bug 428481), I am a little worried about the confusion that could be caused by aDataURI as name.

> +   * @param aFaviconURI
> +   *        URI of the favicon whose data is being read
> +   * @returns A data URI containing the data of the favicon. This will be

like this ---------^^^^^^^^ for example.

> +   *          null if we have this URL but have no data associated with it.
> +   * @throws NS_ERROR_NOT_AVAILABLE
> +   *         Thrown when we have never heard of this favicon URL.
> +   */
> +  ACString getFaviconDataAsDataURI(in nsIURI aFaviconURI);
     ^^^^^^^^

ditto for aDataURI in nsFaviconService.cpp (cf dataURI which is URI)

Comment 12

[Manish Singh]

So what terminology do you suggest I use instead?

Comment 13

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

maybe UrlData? the URI suffix should indeed be used only if the param is an nsIURI, i hope we don't have the same problem in other definitions

Comment 14

[Manish Singh]

How about DataURL? That's used in two places in public API (nsIDOMHTMLCanvasElement and nsIDOMFile)

There's a still a handful of places in the tree that uses "dataURI" to hold a string, including the code that this patch moved out of nsPlacesImportExportService.cpp.

Comment 15

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

aDataURL is fine imo, is self explaining and clearly not an URI (intended as an nsIURI)

Comment 16

[Manish Singh]

Should I also change the interface to represent data URLs as UTF-16 strings instead of UTF-8 to be consistent with to APIs in the nsIDOM* world? Or do you think it doesn't matter?

Comment 17

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

i would use an AString, hwv for this you should ask Dietrich

Comment 18

[Manish Singh]

Attachment: New patch, uses AString and makes naming consistent

Comment 19

[Dietrich Ayala (:dietrich)]

Comment on attachment 319996 [details] [diff] [review]
New patch, uses AString and makes naming consistent


> 
>+// nsFaviconService::SetFaviconDataFromDataURL
>+//
>+//    See the IDL for this function for lots of info.

please remove this unnecessary comment.

one more request, please update the test below, so that your changes to the importer are under automated testing:

http://mxr.mozilla.org/seamonkey/source/browser/components/places/tests/unit/test_bookmarks_html.js

it imports this bookmarks.html file, which has icons in it:

http://mxr.mozilla.org/seamonkey/source/browser/components/places/tests/unit/bookmarks.preplaces.html?force=1

r=me otherwise

Comment 20

[Manish Singh]

Attachment: Address review nit, and add test

Carrying forward r=mak77,dietrich. Requesting a1.9?

Comment 21

[Mike Schroepfer]

Comment on attachment 320358 [details] [diff] [review]
Address review nit, and add test

Sorry we are closed for Firefox 3.0.  This may be a possibility for 3.0.1 or definitely 3.1.

Comment 22

[Manish Singh]

Will you actually accept adding additional functions to nsIFaviconService in a 3.0.x release?

Comment 23

[Manish Singh]

Ok, this patch has r+, and still applies cleanly to mozilla-central, and tests pass. Marking checkin-needed.

Comment 24

[Dietrich Ayala (:dietrich)]

fixed (c76f2eb67122)

Comment 25

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 26

[[e-mail]]

Comment on attachment 320358 [details] [diff] [review]
Address review nit, and add test

>+  char* encoded = PL_Base64Encode(reinterpret_cast<const char*>(data),
>+                                  dataLen, nsnull);

>+  nsMemory::Free(encoded);
Should be PR_Free, of course...