Closed Bug 573430 Opened 14 years ago Closed 14 years ago

nsIFaviconService.getFaviconData should not fail when passed the default favicon

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b1

People

(Reporter: adw, Assigned: adw)

Details

Attachments

(1 file, 1 obsolete file)

Passing nsIFaviconService.defaultFavicon to getFaviconData should return the default favicon's data just as it does for all other favicons, not throw an error.

Special-casing the default favicon (from the client's perspective anyway) makes it a pain to get the data of an arbitrary tab favicon.

Via Jetpack bug 573152.
Attached patch patch (obsolete) — Splinter Review
Reads the file synchronously, but don't see a way around that without adding an async version of GetFaviconData.  It already synchronously reads the DB anyway.
Attachment #452834 - Flags: review?(dietrich)
(In reply to comment #1)
> Created an attachment (id=452834)
>  --> (https://bugzilla.mozilla.org/attachment.cgi?id=452834)
> patch
> 
> Reads the file synchronously, but don't see a way around that without adding an
> async version of GetFaviconData.  It already synchronously reads the DB anyway.
We should fix that.  We have nsIFaviconDataCallback which we use with setAndLoadFaviconForPage, and I don't think it'd be hard to morph.

Is there any reason why jetpack just doesn't load the moz-anno: uri for the favicon, which handles this all too?
An async version of getFaviconData is planned and welcome, see bug 553489.
Another planned enh is a moz-anno:icon uri that (Differently from current one that returns icon or default if icon is not in db) works as a pass-through (if data is not in the db tries to fetch it and serve it).
(In reply to comment #3)
> see bug 553489.
I meant bug 540765.
(In reply to comment #2)
> We should fix that.  We have nsIFaviconDataCallback which we use with
> setAndLoadFaviconForPage, and I don't think it'd be hard to morph.

The Jetpack client isn't async currently.  (All it does is get the favicon of a tab (but see below).  We'd have to talk about making it async.)

> Is there any reason why jetpack just doesn't load the moz-anno: uri for the
> favicon, which handles this all too?

We need a data URI so that the URI is meaningful everywhere.  Where's the moz-anno URI impl?  Maybe we can share code.
(In reply to comment #5)
> The Jetpack client isn't async currently.  (All it does is get the favicon of a
> tab (but see below).  We'd have to talk about making it async.)
sadfaces

> We need a data URI so that the URI is meaningful everywhere.  Where's the
> moz-anno URI impl?  Maybe we can share code.
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsAnnoProtocolHandler.cpp
Thanks.  BTW, this patch caches the favicon (and we're only talking about one favicon, the default favicon) the first time it's read.
Attachment #452834 - Flags: review?(dietrich) → review?(mak77)
Comment on attachment 452834 [details] [diff] [review]
patch

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

> NS_IMETHODIMP
> nsFaviconService::GetFaviconData(nsIURI* aFaviconURI, nsACString& aMimeType,
>                                  PRUint32* aDataLen, PRUint8** aData)

>+  // The default favicon is not in the database, so it needs to be handled
>+  // separately.
>+  if (isDefaultFavicon) {
>+
>+    // Read its file (synchronously) if the data's not cached already.

I have concerns about empty lines after ifs, does not looks like any code style talks bout'em.

>+    if (mDefaultFaviconData.IsEmpty()) {
>+      nsCOMPtr<nsIInputStream> istream;
>+      rv = NS_OpenURI(getter_AddRefs(istream), defaultFaviconURI);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      rv = NS_ConsumeStream(istream, PR_UINT32_MAX, mDefaultFaviconData);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      rv = istream->Close();
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      if (mDefaultFaviconData.IsEmpty())
>+        return NS_ERROR_UNEXPECTED;
>+    }

hm, you could move this to a rv = GetDefaultFaviconData(data) helper, and specify in the header users should use this and don't directly mDefaultFaviconData
So I'd do:

if (isDefaultFavicon) {
  // The default favicon is not in the database.
  rv = GetDefaultFaviconData(defaultIconData);
  NS_ENSURE_SUCCESS(rv, rv);
  __assign_and_return__
}

>+    // Dupe the string's buffer into an array of bytes.
>+    PRUint8* bytes = reinterpret_cast<PRUint8*>
>+                     (ToNewCString(mDefaultFaviconData));
>+    NS_ENSURE_STATE(bytes);

and eventually the helper could directly return bytes (in future in case we could add an overloaded helper for nsCString if needed)

you could move the two macros
TO_INTBUFFER(_string) and TO_CHARBUFFER(_buffer) from AsyncFaviconHelpers.cpp to nsFavconService.h and use them

>diff --git a/toolkit/components/places/src/nsFaviconService.h b/toolkit/components/places/src/nsFaviconService.h
>--- a/toolkit/components/places/src/nsFaviconService.h
>+++ b/toolkit/components/places/src/nsFaviconService.h
>@@ -217,11 +217,15 @@ private:
>   nsDataHashtable<nsCStringHashKey, PRUint32> mFailedFavicons;
> 
>   nsresult SetFaviconUrlForPageInternal(nsIURI* aURI, nsIURI* aFavicon,
>                                         PRBool* aHasData);
> 
>   friend class FaviconLoadListener;
> 
>   bool mShuttingDown;
>+
>+  // A string of bytes caching the default favicon's contents.  Empty if not yet
>+  // cached.
>+  nsCString mDefaultFaviconData;

If you add the helper point out that nobody should directly use mDefaultFaviconData, but always use the helper.

>diff --git a/toolkit/components/places/tests/unit/test_favicons.js b/toolkit/components/places/tests/unit/test_favicons.js

how much does this test suck?

>+var ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
>+var istream = ios.newChannelFromURI(iconsvc.defaultFavicon).open();

just use NetUtil.newChannel(iconsvc.defaultFavicon).open();
NetUtil is already in scope for all Places xpcshell-tests.

>+var avail = bistream.available();
>+while (avail) {
>+  var chunk = bistream.readByteArray(avail);
>+  expectedData = expectedData.concat(chunk);

is chunk temp var really needed?

>+  avail = bistream.available();

why not:

let avail; 
while (avail = bistream.available())
Attached patch patch v2Splinter Review
(In reply to comment #8)
> >+    // Dupe the string's buffer into an array of bytes.
> >+    PRUint8* bytes = reinterpret_cast<PRUint8*>
> >+                     (ToNewCString(mDefaultFaviconData));
> >+    NS_ENSURE_STATE(bytes);
> 
> you could move the two macros
> TO_INTBUFFER(_string) and TO_CHARBUFFER(_buffer) from AsyncFaviconHelpers.cpp
> to nsFavconService.h and use them

TO_INTBUFFER makes a shallow copy of the string's buffer, but I want a deep copy here.

This patch implements all your other suggestions.

> >diff --git a/toolkit/components/places/tests/unit/test_favicons.js b/toolkit/components/places/tests/unit/test_favicons.js
> 
> how much does this test suck?

Hehe, its style is definitely not modern, but it looks OK.
Assignee: nobody → adw
Attachment #452834 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #454080 - Flags: review?(mak77)
Attachment #452834 - Flags: review?(mak77)
Comment on attachment 454080 [details] [diff] [review]
patch v2

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

>+  // If we're getting the default favicon, we need to handle it separately since
>+  // it's not in the database.
>+  if (isDefaultFavicon) {

nit: I'd put a simpler one-line comment ("The default favicon is not in the database.") inside the if

>diff --git a/toolkit/components/places/tests/unit/test_favicons.js b/toolkit/components/places/tests/unit/test_favicons.js

>+while (avail = bistream.available())
>+  expectedData = expectedData.concat(bistream.readByteArray(avail));

always brace loops, even if they are one-line, the nobraces rule is just for if/else afaik

r=me
Attachment #454080 - Flags: review?(mak77) → review+
http://hg.mozilla.org/mozilla-central/rev/90f4e0452bc0
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: