Closed Bug 429832 Opened 17 years ago Closed 16 years ago

Add API to nsIFaviconService to handle data URIs

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Firefox 3.1a1

People

(Reporter: mozbugs, Assigned: mozbugs)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attachment #316592 - Flags: review?(dietrich)
Blocks: 423126
Comment on attachment 316592 [details] [diff] [review]
Add API to nsIFaviconService to handle data URIs

requesting first-r from Marco.
Attachment #316592 - Flags: review?(dietrich) → review?(mak77)
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.
Attachment #316592 - Flags: review?(mak77) → review+
> 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?
(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?
> (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
>    /**
> +   * 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|.
Attached patch New patch, addresses comments (obsolete) — Splinter Review
Attachment #316592 - Attachment is obsolete: true
Attachment #318047 - Flags: review?(dietrich)
Comment on attachment 318047 [details] [diff] [review]
New patch, addresses comments

Carrying forward Marco's r+
Attachment #318047 - Flags: review+
Attached patch Sync up patch with current code (obsolete) — Splinter Review
Attachment #318047 - Attachment is obsolete: true
Attachment #319966 - Flags: review?(dietrich)
Attachment #318047 - Flags: review?(dietrich)
Comment on attachment 319966 [details] [diff] [review]
Sync up patch with current code

Carrying forward Marco's r+
Attachment #319966 - Flags: review+
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)
So what terminology do you suggest I use instead?
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
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.
aDataURL is fine imo, is self explaining and clearly not an URI (intended as an nsIURI)
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?
i would use an AString, hwv for this you should ask Dietrich
Attachment #319966 - Attachment is obsolete: true
Attachment #319996 - Flags: review?(dietrich)
Attachment #319966 - Flags: review?(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
Attachment #319996 - Flags: review?(dietrich) → review+
Carrying forward r=mak77,dietrich. Requesting a1.9?
Attachment #319996 - Attachment is obsolete: true
Attachment #320358 - Flags: review+
Attachment #320358 - Flags: approval1.9?
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.
Attachment #320358 - Flags: approval1.9? → approval1.9-
Will you actually accept adding additional functions to nsIFaviconService in a 3.0.x release?
Whiteboard: [3.1]
Priority: -- → P3
Whiteboard: [3.1]
Target Milestone: --- → Firefox 3.1
Blocks: 437245
Ok, this patch has r+, and still applies cleanly to mozilla-central, and tests pass. Marking checkin-needed.
Keywords: checkin-needed
fixed (c76f2eb67122)
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → Firefox 3.1a1
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
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...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: