This is a subset of 423126 that covers import-only. Fennec uses JSON for its default bookmarks, and we need the ability to specify favicons.

This seems to be all that's needed for Fennec. 1) Is my use of the fake URI OK? Not sure if that has any implications for the faviconservice. 2) Is 0 for expiration time guaranteed to avoid expiration? I just copied nsPlacesImportExportService. 3) Is "favicon" a good property name? Could just use "icon" to match bookmarks.html (and potentially an icon_uri in the future?)...

(In reply to comment #1) > 1) Is my use of the fake URI OK? Not sure if that has any implications for the > faviconservice. hm, this is an interesting question, we should ideally allow to set a null faviconuri, the table definition allows that and does not make sense to save fake data (should always be considered a bad habit on a DB, per size and performance reasons). I think we should fix FaviconService methods to allow a null favicon uri. But, if we would really end up being forced to use a fake uri, i'd say to use the shortest uri we can since DB size matters (i'd say "moz-anno:favicon:imported:" then). Btw i'd do the latter only if this would end up being a blocker with not enough time to fix the service. > 2) Is 0 for expiration time guaranteed to avoid expiration? I just copied > nsPlacesImportExportService. Yes and no. Since: - the icon is associated with a page, if the page goes away from the db the icon will be removed because orphan (i think this is ok also from your point of view) - the icon is associated with a faviconuri and a pageuri, as soon as the user will visit the bookmark, a new favicon entry with the correct faviconuri will be created for that pageuri, then the page will have 2 favicons associated with it, which one will be selected? We can't know, but for sure one of them is an icon that won't never be used again (so it's useless data). I think we will usually return the older icon, since has a lower ROWID, but that depends on sqlite internals. The latter is a real bug, since as you said import service is already doing that. But should be fixable, when we associate a faviconuri with a page we should check if the page as already a null-faviconuri-not-expirable and take a decision if we want to remove the null-faviconuri entry or discard the new icon. This opens new questions, if i set an icon for a page as never expirable, should that icon persist till i set a new one through the API? should it be replaced by the real page icon on visit? should ExpireAllFavicons get rid of it? Btw, since this bug already exists, we can handle it in a followup. - looking at the code at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsFaviconService.cpp#628 the check looks wrong to consider 0 == "never expire": 628 if (hasData && now < expiration && ! aForceReload) { 629 // data still valid, no need to reload if expiration == 0 the icon will be considered expired, it should probably check (!expiration || now < expiration). But in your case you don't have a faviconuri, so this code will never be able to touch your favicon since it's unlikely a page can query that faviconuri. - ExpireAllFavicons API will expire it even if it has 0 expiration time. Is this a problem? > 3) Is "favicon" a good property name? Could just use "icon" to match > bookmarks.html (and potentially an icon_uri in the future?)... i prefer the more generic "icon"

I guess I don't really care about whether it expires or not... as long as the bookmark always has an icon, we're fine. re: fake faviconURI, don't they need to be unique? I thought it if wasn't then all bookmarks would effectively be associated to the same icon (which is why I included the page URI - similar to how nsPlacesImportExportService uses a "serial number"). Or do you just want me to change the prefix? I'm not sure I want to undertake fixing the favicon service to accept null URIs changes right now, so I'd like to defer fixing the fake-URI issues to a post-1.9.2 followup - does that sound reasonable?

Thinking about this further, we might actually want to use chrome images for our favicons (this would also allow us to have the Firefox favicon differ in branded vs. unbranded builds). Is there a way to associate chrome:// images as favicons for the page? I don't really understand the comment at: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/public/nsIFaviconService.idl#122

(In reply to comment #4) > Thinking about this further, we might actually want to use chrome images for > our favicons (this would also allow us to have the Firefox favicon differ in > branded vs. unbranded builds). Is there a way to associate chrome:// images as > favicons for the page? I don't really understand the comment at: > > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/public/nsIFaviconService.idl#122 seeing at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsFaviconService.cpp#1042 we explicitly skip chrome uris. Notice that's "original" code, from first Places appearance :) So, usually getFaviconLinkForPage/Icon/IconString returns an nsIURI that has a spec like moz-anno:favicon:faviconuri, the ui uses that as src for an image and loads it (through an async channel). I guess the above comment has been added for perf purposes, i mean, if you know you have a certain chrome: address, the frontend is most of the times able to set that as image without passing through the favicon service. Apart that i think chrome uris will be saved like any other uri, and the only difference is that instead of getting moz-anno:favicon:chrome:XXX you'll just get chrome:XXX as src for the image.

PS: all of the above issues would still be valid also using chrome uris, so on first visit you *could* lose the bookmark's icon since the page would be associated with a new faviconuri (The one from page's code).

i also guess there could be other problems with chrome, the favicon would have no data associated with it, that means getFaviconData calls would always throw. Also we need to check expiration code, to be sure it does not expire favicons without data associated (and i think it could do) nothing unfixable, but it will add meat on the barbecue.

(In reply to comment #6) > PS: all of the above issues would still be valid also using chrome uris, so on > first visit you *could* lose the bookmark's icon since the page would be > associated with a new faviconuri (The one from page's code). I'm fine with the default favicon being replaced by the one on the site - that's even desirable. (In reply to comment #7) > i also guess there could be other problems with chrome, the favicon would have > no data associated with it, that means getFaviconData calls would always throw. > Also we need to check expiration code, to be sure it does not expire favicons > without data associated (and i think it could do) > nothing unfixable, but it will add meat on the barbecue. If I call setAndLoadFaviconForPage with a chrome:// URI as aFaviconURI, it looks to me like it will fetch the data and save it in the favicon DB (and indeed it seems to in testing).

like so...

Comment on attachment 407577 [details] [diff] [review] updated patch >diff --git a/toolkit/components/places/src/utils.js b/toolkit/components/places/src/utils.js >+ // Create a fake faviconURI to use - its value doesn't matter, since >+ // we should never expire this favicon. I suppose I should remove the second part of this comment (about expiration) and just file a followup to get rid of both of these "fake" URIs.

the example JSON file is the patch... > (In reply to comment #6) > I'm fine with the default favicon being replaced by the one on the site - > that's even desirable. then you end up with useless files in chrome content, as soon as the user uses the bookmark, why is data url bad then? just because a chrome icon is easier to replace? > (In reply to comment #7) > > i also guess there could be other problems with chrome, the favicon would have > > no data associated with it, that means getFaviconData calls would always throw. > > Also we need to check expiration code, to be sure it does not expire favicons > > without data associated (and i think it could do) > > nothing unfixable, but it will add meat on the barbecue. > > If I call setAndLoadFaviconForPage with a chrome:// URI as aFaviconURI, it > looks to me like it will fetch the data and save it in the favicon DB (and > indeed it seems to in testing). yes if you call setAndLoadFaviconForPage it will fetch the icon, through an async channel and save it to the db, that is useless from a pure performance point of view, since the icon data is already available in chrome content. Do you have a guess of how much time do w e have to fix this? since i feel like we are workarounding a bunch of favicons service issues that instead should be properly fixed. Looking at the patch i still don't like having to set a fake uri (at least not the one you used, see my suggestion above, but i really think we should set a null faviconuri), i don't see why we can't detect if we have a data url in "icon" property looking at the schema of the url, without having 2 separate properties. So if this is not in a hurry, i could probably fix the favicon service to make this better in the next days.

(In reply to comment #12) > > (In reply to comment #6) > > I'm fine with the default favicon being replaced by the one on the site - > > that's even desirable. > > then you end up with useless files in chrome content, as soon as the user uses > the bookmark, why is data url bad then? just because a chrome icon is easier to > replace? We'd only use the chrome icons for the "firefox" bookmarks, which point to about: pages that specify the same chrome:// favicon. > So if this is not in a hurry, i could probably fix the favicon service to make > this better in the next days. Sure, feel free to do that if you'd like. I just want something that works, landed on 1.9.2 in the near future.

(In reply to comment #13) > Sure, feel free to do that if you'd like. I just want something that works, > landed on 1.9.2 in the near future. Concretely: Fennec b5 freeze is Oct 28th.

Comment on attachment 407577 [details] [diff] [review] updated patch ok, since i don't see the light with my hardware problems (most likely won't be able to work on this before monday) and you want this in 6 days, i guess we can take this for now, but you should file a followup to properly fix being able to set a null faviconuri and all derivatives issues. That bug should also add tests, i don't see the need to add tests here if this is only a temporary workaround to something that has still to be fixed properly. But please do a manual test. Even if i guess an automatic test will help 1.9.2 approval. >diff --git a/toolkit/components/places/src/utils.js b/toolkit/components/places/src/utils.js >--- a/toolkit/components/places/src/utils.js >+++ b/toolkit/components/places/src/utils.js >@@ -1426,16 +1426,32 @@ var PlacesUtils = { > var tags = aData.tags.split(", "); > if (tags.length) > this.tagging.tagURI(this._uri(aData.uri), tags); > } > if (aData.charset) > this.history.setCharsetForURI(this._uri(aData.uri), aData.charset); > if (aData.uri.substr(0, 6) == "place:") > searchIds.push(id); >+ if (aData.icon) { >+ try { why are you hiding all exceptions? what are possible failures? we should handle them better, we don't want to hide valid errors. >+ // Create a fake faviconURI to use - its value doesn't matter, since >+ // we should never expire this favicon. drop second part of the comment as you said >+ let faviconURI = this._uri("fake-favicon-uri:" + aData.uri); only use "moz-anno:favicon:imported:" as the faviconURI please i want all favicons having the same (short) value so we can eventually recognize and fix them when moving to NULLs >+ this.favicons.setFaviconUrlForPage(this._uri(aData.uri), faviconURI); >+ this.favicons.setFaviconDataFromDataURL(faviconURI, aData.icon, 0); >+ } catch (ex) {} >+ } >+ if (aData.icon_uri) { ok, now i think you're right and we want both .icon and ,icon_uri, in some case we could set both indeed. Since we have .dateAdded and .lastModified this should probably be .iconUri, btw i don't have strong feelings about that. >+ try { ditto, don't hide errors, handle them if needed. >+ this.favicons.setAndLoadFaviconForPage(this._uri(aData.uri), >+ this._uri(aData.icon_uri), >+ false); >+ } catch (ex) {} >+ } > break; > case this.TYPE_X_MOZ_PLACE_SEPARATOR: > id = this.bookmarks.insertSeparator(aContainer, aIndex); > break; > default: > // Unknown node type > } > r=me with the above fixed.

(In reply to comment #15) > only use "moz-anno:favicon:imported:" as the faviconURI please > i want all favicons having the same (short) value so we can eventually > recognize and fix them when moving to NULLs hum wait for this, i want to be sure how we handle favicons with same url.

(In reply to comment #15) > (From update of attachment 407577 [details] [diff] [review]) > ok, since i don't see the light with my hardware problems (most likely won't be > able to work on this before monday) and you want this in 6 days, i guess we can > take this for now, but you should file a followup to properly fix being able to > set a null faviconuri and all derivatives issues. OK, will do. > >diff --git a/toolkit/components/places/src/utils.js b/toolkit/components/places/src/utils.js > >+ try { > > why are you hiding all exceptions? what are possible failures? we should handle > them better, we don't want to hide valid errors. Well, it seems like we shouldn't fail to import bookmarks if the icon entries are corrupt for some reason (invalid data: URI, for example). What's the benefit to propagating failure here? I can reportError() the exception, if you'd prefer. > >+ let faviconURI = this._uri("fake-favicon-uri:" + aData.uri); > > only use "moz-anno:favicon:imported:" as the faviconURI please > i want all favicons having the same (short) value so we can eventually > recognize and fix them when moving to NULLs Do you mean ("moz-anno:favicon:imported:" + aData.uri)? If not, won't using the same URI for all icons cause problems (per comment 3 para #2)?

(In reply to comment #17) > (In reply to comment #15) > > (From update of attachment 407577 [details] [diff] [review] [details]) > Well, it seems like we shouldn't fail to import bookmarks if the icon entries > are corrupt for some reason (invalid data: URI, for example). What's the > benefit to propagating failure here? I can reportError() the exception, if > you'd prefer. hm, yes we should reportError, sounds fine to not lose precious real errors. > > >+ let faviconURI = this._uri("fake-favicon-uri:" + aData.uri); > > > > only use "moz-anno:favicon:imported:" as the faviconURI please > > i want all favicons having the same (short) value so we can eventually > > recognize and fix them when moving to NULLs > > Do you mean ("moz-anno:favicon:imported:" + aData.uri)? If not, won't using the > same URI for all icons cause problems (per comment 3 para #2)? We don't directly associate pages to favicons, instead we do the opposite, we associate favicons to pages, in such vision could make sense to allow having favicons with the same faviconuri when it is NULL. Unfortunatly who created the db set the url column to UNIQUE (well makes sense from a certain point of view, even if it is downgrading INSERT performances). Luckily Sqlite allows to have multiple NULLs in a unique column (yay!) but unfortunatly (again) that would require us to fix this properly (see above, bla bla). So if we want to rush this we need a unique url, the only problem with aData.uri is that it could be really large, thus bloating the table, that data uri won't even be optimized. But in your case i guess it won't hurt since we are talking about less than 10 icons so we are bloting by 20, 30 KB? Go for it.

(In reply to comment #18) > So if we want to rush this we need a unique url, the only problem with > aData.uri is that it could be really large aData.uri is the page URI, not the icon URI (i.e. .uri, not .icon or .iconURI), so that shouldn't be a problem.

(In reply to comment #18) > We don't directly associate pages to favicons, instead we do the opposite, we > associate favicons to pages The nsIFaviconService interface methods seems to require using a unique faviconURI for setting data or associating icons to pages, though, so I'm not sure I understand how we'd go about allowing null URIs without some interface changes. I filed bug 523932, best to discuss that there, I guess.

(In reply to comment #19) > (In reply to comment #18) > aData.uri is the page URI, not the icon URI (i.e. .uri, not .icon or .iconURI), > so that shouldn't be a problem. right, i misread it, dunno how. (In reply to comment #20) > The nsIFaviconService interface methods seems to require using a unique > faviconURI for setting data or associating icons to pages for setting Data i think we can just pass NULL, for associating favicon to a page, well you can't, but this would be done for import case since navigating you would never be able to download a fake uri, and when you import an icon without a faviconuri you directly call SetData, that is probably going to be the only method accepting a null icon uri.

I have a WIP test for this, I'll post it tomorrow.

Blocks a fennec blocker.