I'm about to land bug 977177 that is removing the moz_favicons table, and will likely break some of the queries joining with it (very likely nsPlacesAutoComplete.js) The solution is to change the code to return a "page-icon:page_url" icon url. It is no more possible to directly join with icons in a query, since every page can have multiple icons of different sizes. Sorry for the late notification, it just came to my mind now :(

Build still works but autocomplete shows the generic icon. Importance setting "normal" is ok for this bug I think.

Part one of the fix. This removes references to the new removed moz_favicons table. A second patch will be coming to get the icons form the new database.

I am having trouble trying to figure out what to do with this part of the code in suite/common/places/nsPlacesAutoComplete.js: // Obtain the favicon for this URI. let favicon; if (aFaviconSpec) { let uri = NetUtil.newURI(aFaviconSpec); favicon = PlacesUtils.favicons.getFaviconLinkForIcon(uri).spec; } favicon = favicon || PlacesUtils.favicons.defaultFavicon.spec; this._result.appendMatch(aURISpec, aTitle, favicon, aStyle); }, particularly on the line: favicon = PlacesUtils.favicons.getFaviconLinkForIcon(uri).spec; I have tried all kinds of things based on the changes to toolkit/components/places/UnifiedComplete.js with no positive results. I am probably being really stupid here, but need some help.

Sorry but it seems Marco is not accepting needinfo requests.

Canselled the needinfo. I don't think that line actually needs to be changed at all. THe issue with the code I tested was elsewhere.

Well this code is now dead simple.

Same basic patch I just restored a blank line that I had accidentally removed in the first attempt.

I don't think bug 1391944 is related: 1. I first encountered this bug quite some time ago - at least a couple of years I think, but was reluctant to report until I could figure out what sequence of events was leading to it. 2. It is not related to a version upgrade but occurs suddenly in the middle of browsing. 3. My favicons.sqlite database is dated May 29th, and the screenshots I've attached are made on August 19th 4. I don't use favicons - browser.chrome.site_icons=false and moz_favicons database has 0 records in places.sqlite as well as moz_icons in favicons.sqlite. Yes, at the moment both are present.

Comment on attachment 8897857 [details] [diff] [review] 1355780-part-1-remove-references-to-moz_favicons.patch Bill is the number and count of url columns relevant? If the order is the same for all I think you will need to just replace the favicon url with NULL in them. > - `SELECT t.url, t.url, NULL, NULL, NULL, NULL, NULL, NULL, NULL, > + `SELECT t.url, t.url, NULL, NULL, NULL, NULL, NULL, NULL, > :query_type, t.open_count, NULL > FROM moz_openpages_temp t > - SELECT h.url, h.title, f.url, ${kBookTagSQLFragment}, > + SELECT h.url, h.title, ${kBookTagSQLFragment},

(In reply to Frank-Rainer Grahl (:frg) from comment #10) > Comment on attachment 8897857 [details] [diff] [review] > 1355780-part-1-remove-references-to-moz_favicons.patch > > Bill is the number and count of url columns relevant? If the order is the > same for all I think you will need to just replace the favicon url with NULL > in them. OK my original patch worked that way, but then I thought it better ti bite the bullet and make a more major change to go along with what was done for Firefox. This part of the patch: @@ -50,25 +50,24 @@ const MATCH_BOUNDARY_ANYWHERE = Ci.mozIP const MATCH_BOUNDARY = Ci.mozIPlacesAutoComplete.MATCH_BOUNDARY; const MATCH_BEGINNING = Ci.mozIPlacesAutoComplete.MATCH_BEGINNING; const MATCH_BEGINNING_CASE_SENSITIVE = Ci.mozIPlacesAutoComplete.MATCH_BEGINNING_CASE_SENSITIVE; // AutoComplete index constants. All AutoComplete queries will provide these // columns in this order. const kQueryIndexURL = 0; const kQueryIndexTitle = 1; -const kQueryIndexFaviconURL = 2; -const kQueryIndexBookmarked = 3; -const kQueryIndexBookmarkTitle = 4; -const kQueryIndexTags = 5; -const kQueryIndexVisitCount = 6; -const kQueryIndexTyped = 7; -const kQueryIndexPlaceId = 8; -const kQueryIndexQueryType = 9; -const kQueryIndexOpenPageCount = 10; +const kQueryIndexBookmarked = 2; +const kQueryIndexBookmarkTitle = 3; +const kQueryIndexTags = 4; +const kQueryIndexVisitCount = 5; +const kQueryIndexTyped = 6; +const kQueryIndexPlaceId = 7; +const kQueryIndexQueryType = 8; +const kQueryIndexOpenPageCount = 9; removes that completely such that I either need to not have this section land and keep a null placeholder or just remove the parameter altogether. Let me know which approach you prefer. I thought keeping it closer to the Firefox code was better for the next time this are gets changed in Firefox to keep it as close as possible.

I just recounted all columns in the queries and they seem to be ok. Not sure what the additional null column at the end of the _openPagesQuery query does but it was there before. So all is fine.

Comment on attachment 8898596 [details] [diff] [review] 1355780-part-2-use-page-icon-protocol.patch As IanN would say: LGTM Thanks.

OH so is there an extra null at the end? I can look into that and remove it but if it has always been there I would prefer to do it in a different bug.

Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/8e63fdcf0e3c Part 1 - Remove references to the moz_favicons table. r=frg https://hg.mozilla.org/comm-central/rev/a6d47656d6e3 Bug - 1355780 - Part 2 - Use the "page-icon:" protocol. r=frg

No this is fine. Was always the case and we can always look at it later. Just ask for beta approval.

Comment on attachment 8897857 [details] [diff] [review] 1355780-part-1-remove-references-to-moz_favicons.patch [Approval Request Comment] Regression caused by (bug #): 977177 User impact if declined: No Favicons in urlbar autocomplete suggestions Testing completed (on m-c, etc.): I have tested this locally for several weeks Risk to taking this patch (and alternatives if risky): none String changes made by this patch: none

Comment on attachment 8898596 [details] [diff] [review] 1355780-part-2-use-page-icon-protocol.patch [Approval Request Comment] Regression caused by (bug #): 977177 User impact if declined: No Favicons in urlbar autocomplete suggestions Testing completed (on m-c, etc.): I have tested this locally for several weeks Risk to taking this patch (and alternatives if risky): none String changes made by this patch: none

https://hg.mozilla.org/releases/comm-beta/rev/960dbd4b271a409e757ad8057308d6c6010c6671 https://hg.mozilla.org/releases/comm-beta/rev/eb4b5c610b215fb4f989be10d6c5bd85fb7fd264