1355780 - Update favicons handling

Status: RESOLVED FIXED

(SeaMonkey :: Autocomplete, defect)

Description

[Marco Bonardo [:mak]]

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 :(

Comment 1

[Frank-Rainer Grahl (:frg)]

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

Comment 2

[Bill Gianopoulos [:WG9s]]

Attachment: 1355780-part-1-remove-references-to-moz_favicons.patch

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.

Comment 3

[Bill Gianopoulos [:WG9s]]

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.

Comment 4

[Bill Gianopoulos [:WG9s]]

Sorry but it seems Marco is not accepting needinfo requests.

Comment 5

[Bill Gianopoulos [:WG9s]]

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.

Comment 6

[Bill Gianopoulos [:WG9s]]

Attachment: 1355780-part-2-use-page-icon-protocol.patch

Well this code is now dead simple.

Comment 7

[Bill Gianopoulos [:WG9s]]

Attachment: 1355780-part-2-use-page-icon-protocol.patch

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

Comment 9

[lvm]

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 10

[Frank-Rainer Grahl (:frg)]

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},

Comment 11

[Bill Gianopoulos [:WG9s]]

(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.

Comment 12

[Frank-Rainer Grahl (:frg)]

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 13

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8898596 [details] [diff] [review]
1355780-part-2-use-page-icon-protocol.patch

As IanN would say: LGTM 

Thanks.

Comment 14

[Bill Gianopoulos [:WG9s]]

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.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Frank-Rainer Grahl (:frg)]

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

Comment 17

[Bill Gianopoulos [:WG9s]]

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 18

[Bill Gianopoulos [:WG9s]]

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

Comment 19

[Frank-Rainer Grahl (:frg)]

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