Update favicons handling

RESOLVED FIXED in seamonkey2.54

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mak, Assigned: wgianopoulos)

Tracking

(Blocks 1 bug)

unspecified
seamonkey2.54
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.53 fixed, seamonkey2.54 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Assignee: nobody → wgianopoulos
Status: NEW → ASSIGNED
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.
Attachment #8897857 - Flags: review?(frgrahl)
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.
Flags: needinfo?(adw)
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.
Flags: needinfo?(adw)
Well this code is now dead simple.
Attachment #8898460 - Flags: review?(frgrahl)
Same basic patch I just restored a blank line that I had accidentally removed in the first attempt.
Attachment #8898460 - Attachment is obsolete: true
Attachment #8898460 - Flags: review?(frgrahl)
Attachment #8898596 - Flags: review?(frgrahl)
Duplicate of this bug: 1391944
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},
Flags: needinfo?(wgianopoulos)
Attachment #8897857 - Flags: feedback+
(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.
Flags: needinfo?(wgianopoulos)
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.
Attachment #8897857 - Flags: review?(frgrahl) → review+
Comment on attachment 8898596 [details] [diff] [review]
1355780-part-2-use-page-icon-protocol.patch

As IanN would say: LGTM 

Thanks.
Attachment #8898596 - Flags: review?(frgrahl) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
No this is fine. Was always the case and we can always look at it later. Just ask for beta approval.
Target Milestone: --- → seamonkey2.54
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
Attachment #8897857 - Flags: approval-comm-beta?
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
Attachment #8898596 - Flags: approval-comm-beta?
Attachment #8897857 - Flags: approval-comm-beta? → approval-comm-beta+
Attachment #8898596 - Flags: approval-comm-beta? → approval-comm-beta+
Duplicate of this bug: 1420614
You need to log in before you can comment on or make changes to this bug.