389003 - sometimes favicons in url results are blank, need to use the favicon service

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[(not reading, please use [e-mail] instead)]

sometimes favicons in url results are blank, need to use the favicon service

spin off bug #373353

fix in hand.  caught this while testing against migrated profile from firefox 2 where the favicon was:

http://www.mozilla.org/2005/made-up-favicon/308-1893059051

We need to convert that to:

moz-anno:favicon:http://www.mozilla.org/2005/made-up-favicon/308-1893059051

We can use nsFaviconService::GetFaviconSpecForIconString(), which is designed to do this without nsIURI creation (and parsing).

Patch in hand.

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: screen shot, before the fix

Comment 3

[(not reading, please use [e-mail] instead)]

Attachment: screen shot, after the fix

Comment 4

[(not reading, please use [e-mail] instead)]

to avoid the string conversions, I can change nsFaviconService::GetFaviconSpecForIconString(), as I'm the only consumer right now.

also, with this fix, I can remove the following rules from css that I added to pinstripe/winstripe for bug #373353

+.autocomplete-treebody::-moz-tree-image(favicon, treecolAutoCompleteValue) {
+  list-style-image: url("chrome://mozapps/skin/places/defaultFavicon.png");

as GetFaviconSpecForIconString() will return the url for the default icon if the string is empty.

Comment 5

[(not reading, please use [e-mail] instead)]

Attachment: patch, v2 (remove css rules, remove string conversion)

switching review to mano, who reviewed the patch for bug #373353

Comment 6

[Asaf Romano (gone)]

Comment on attachment 273170 [details] [diff] [review]
patch, v2 (remove css rules, remove string conversion)

This should be in sync with getImageSrc in http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/treeView.js#1001

I'm kinda ambivalent on which approach is better code-sanity-wise, treeView's one seems to be a little bit more efficient though.

Comment 7

[(not reading, please use [e-mail] instead)]

over irc, mano is ok with the patch in general, but still review-, but for a different reason:

<Mano> i don't like the nsString->nsCString change though
<Mano> CString is actually correct
<Mano> and it is what most consumers would have
<Mano> (uri specs are nsCStrings)
<Mano> we should do the conversion on our side for this case.

I'll fix that and attach a new patch.

See bug #389090 about fixing getImageSrc() to be more efficient (not require nsIURI creation).

Comment 8

[(not reading, please use [e-mail] instead)]

Attachment: patch v3

Comment 9

[Asaf Romano (gone)]

Comment on attachment 273259 [details] [diff] [review]
patch v3

>Index: toolkit/components/places/src/nsNavHistoryAutoComplete.cpp
>===================================================================

>+  nsFaviconService* faviconService = nsFaviconService::GetFaviconService();
>+  NS_ENSURE_TRUE(faviconService, NS_ERROR_NO_INTERFACE);
>+

Out of memory, more likely. Pointless comment though ;)


>+    nsCAutoString faviconSpec;
>+    faviconService->GetFaviconSpecForIconString(NS_ConvertUTF16toUTF8(matches[0].image), faviconSpec);
>+    rv = aResult->AppendMatch(matches[0].url, matches[0].title, NS_ConvertUTF8toUTF16(faviconSpec), NS_LITERAL_STRING("favicon"));

long line.

r=mano otherwise.

Comment 10

[(not reading, please use [e-mail] instead)]

Attachment: patch v4, as checked in

Comment 11

[(not reading, please use [e-mail] instead)]

fixed.

Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.cs
s
new revision: 1.58; previous revision: 1.57
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.cs
s
new revision: 1.68; previous revision: 1.67
done
Checking in toolkit/components/places/src/nsFaviconService.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsFaviconService.cpp,v  <--  nsFa
viconService.cpp
new revision: 1.15; previous revision: 1.14
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <
--  nsNavHistoryAutoComplete.cpp
new revision: 1.15; previous revision: 1.14
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  ns
NavHistoryResult.cpp
new revision: 1.107; previous revision: 1.106
done

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

I tested this by dropping in a private places.sqlite file into my top-level profile directory and typing "go" to get autocomplete results in the URL bar, successfully.

Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007081905 Minefield/3.0a8pre