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)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla2.0b1
People
(Reporter: adw, Assigned: adw)
Details
Attachments
(1 file, 1 obsolete file)
7.00 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
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)
Comment 2•14 years ago
|
||
(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?
Comment 3•14 years ago
|
||
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).
Comment 4•14 years ago
|
||
(In reply to comment #3) > see bug 553489. I meant bug 540765.
Assignee | ||
Comment 5•14 years ago
|
||
(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.
Comment 6•14 years ago
|
||
(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
Assignee | ||
Comment 7•14 years ago
|
||
Thanks. BTW, this patch caches the favicon (and we're only talking about one favicon, the default favicon) the first time it's read.
Updated•14 years ago
|
Attachment #452834 -
Flags: review?(dietrich) → review?(mak77)
Comment 8•14 years ago
|
||
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())
Assignee | ||
Comment 9•14 years ago
|
||
(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 10•14 years ago
|
||
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+
Assignee | ||
Comment 11•14 years ago
|
||
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.
Description
•