373353 - add favicons to location bar autocomplete results

Status: VERIFIED FIXED

(Firefox :: Address Bar, enhancement)

Description

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

add favicons to the results in the location bar.

a while back, mconnor asked:

"do we have favions in urlbar autocomplete?" (answer: no)

and

"if not, shouldn't it be easier now with places?" (answer: yes, due to the favicon service)

this bug is to track adding them to the location bar autocompete results.

Comment 1

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

taking

Comment 2

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

Attachment: screen shot from draft patch

Comment 3

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

Attachment: wip patch, v1

Comment 4

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

todo items:

0)  check with gavin / mano (and see what they think of this approach)

1)  fix these occurances in the rest of the tree:

http://lxr.mozilla.org/seamonkey/search?string=AppendMatch

/extensions/cck/prefAutoCompleteService.js, line 76 -- simpleresult.appendMatch(prefArray[i], "");
/extensions/cck/prefAutoCompleteService.js, line 202 -- appendMatch : function appendMatch(aValue, aComment) {


http://lxr.mozilla.org/seamonkey/search?string=getCommentAt


/xpfe/components/filepicker/src/nsFileView.cpp, line 168 -- NS_IMETHODIMP nsFileResult::GetCommentAt(PRInt32 index, nsAString & aComment)
/xpfe/components/history/src/nsGlobalHistory.cpp, line 4174 -- nsGlobalHistory::GetCommentAt(PRInt32 aIndex, nsAString& aValue)
/xpfe/components/autocomplete/resources/content/autocomplete.xml, line 362 -- comment: this.lastResult.getCommentAt(aIndex),
/extensions/cck/prefAutoCompleteService.js, line 174 -- getCommentAt : function getCommentAt(index) {

2)  limit favicons to 16px x 16px

3)  default blank space if no favicon

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

>3)  default blank space if no favicon

Showing the default favicon (page with corner folded) might look better than using white space.

Comment 6

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

Attachment: screen shot from patch, v2

1) http://www.yelp.com/favicon.ico is bigger than 16 px x 16 px
2) yahoo page doesn't have a favicon

Comment 7

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

Attachment: patch, v2

Comment 8

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

> Showing the default favicon (page with corner folded) might look better than
> using white space.

Yes, I agree.  

I've followed this part of your mockup in http://wiki.mozilla.org/Places:User_Interface/Search/i2 (http://people.mozilla.com/~faaborg/files/20070510-PlacesUI/i2Search.png)

I've added this as explicit notes to 
http://wiki.mozilla.org/Places:User_Interface/Search/i2#Notes

Comment 9

[John P Baker]

(In reply to comment #5)
> >3)  default blank space if no favicon
> 
> Showing the default favicon (page with corner folded) might look better than
> using white space.

Sounds right to me; Although I am also hoping that my HashColouredTabs+ extension already works with any new code here!

Presumably the default favicon will stay as it is irrespective of any (proposed) changes made to the icon in the location bar itself, and that the location bar icon won't be a page with corner folded for any protocol; Otherwise there could be some confusion as they will line up with each other but with different meaning.

Comment 10

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

John, keep in mind I'm just adding favicons to the autocomplete results.

hopefully this will get reviewed and can land in M7 and you can test with your extension.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 272885 [details] [diff] [review]
patch, v2

>Index: extensions/cck/prefAutoCompleteService.js
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/cck/prefAutoCompleteService.js,v
>retrieving revision 1.3
>diff -u -8 -p -r1.3 prefAutoCompleteService.js
>--- extensions/cck/prefAutoCompleteService.js	25 Apr 2006 16:18:39 -0000	1.3
>+++ extensions/cck/prefAutoCompleteService.js	18 Jul 2007 23:34:43 -0000
>@@ -68,17 +68,17 @@ PrefAutoCompleteSearchHandler.prototype 
>     simpleresult.setSearchString(searchString);
>     simpleresult.setDefaultIndex(0);
>     var prefCount = { value: 0 };
>     var prefArray = gPrefBranch.getChildList("", prefCount);
>     prefArray = prefArray.sort();
>     for (var i = 0; i < prefCount.value; ++i) 
>     {
>       if (prefArray[i].indexOf(searchString) == 0) { 
>-       simpleresult.appendMatch(prefArray[i], "");
>+       simpleresult.appendMatch(prefArray[i], "", "", "");
>       }
>     }
>     if (simpleresult.matchCount > 0) {
>       simpleresult.setSearchResult(Components.interfaces.nsIAutoCompleteResult.RESULT_SUCCESS);
>     } else {
>       simpleresult.setSearchResult(Components.interfaces.nsIAutoCompleteResult.RESULT_NOMATCH);
>     }
>     listener.onSearchResult(this, simpleresult);  
>@@ -171,16 +171,21 @@ AutoCompleteSimpleResult.prototype = {
>     return this.values[index];
>   },
> 
>   getCommentAt : function getCommentAt(index) {
>     return this.comments[index];
>   },
> 
>   getStyleAt : function getStyleAt(index) {
>+    return "";
>+  },
>+
>+  getImageAt: function getImageAt(index) {
>+    return "";
>   },
> 
>   removeValueAt : function removeValueAt(rowIndex, removeFromDb) {
>     this.values.splice(rowIndex, 1);
>   },
> 
>   /* nsIAutoCompleteSimpleResult */
>   setSearchString : function setSearchString(aSearchString) {
>@@ -194,15 +199,15 @@ AutoCompleteSimpleResult.prototype = {
>   setDefaultIndex : function setDefaultIndex(aDefaultIndex) {
>     this.defaultIndex = aDefaultIndex;
>   },
> 
>   setSearchResult : function setSearchResult(aSearchResult) {
>     this.searchResult = aSearchResult;
>   },
> 
>-  appendMatch : function appendMatch(aValue, aComment) {
>+  appendMatch : function appendMatch(aValue, aComment, aImage, aStyle) {
>     this.values.push(aValue);
>     this.comments.push(aComment);
>     this.matchCount++;
>   }
> }
> 
>Index: xpfe/components/filepicker/src/nsFileView.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/components/filepicker/src/nsFileView.cpp,v
>retrieving revision 1.31
>diff -u -8 -p -r1.31 nsFileView.cpp
>--- xpfe/components/filepicker/src/nsFileView.cpp	8 Jul 2007 07:08:55 -0000	1.31
>+++ xpfe/components/filepicker/src/nsFileView.cpp	18 Jul 2007 23:34:45 -0000
>@@ -172,16 +172,22 @@ NS_IMETHODIMP nsFileResult::GetCommentAt
> }
> 
> NS_IMETHODIMP nsFileResult::GetStyleAt(PRInt32 index, nsAString & aStyle)
> {
>   aStyle.Truncate();
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP nsFileResult::GetImageAt(PRInt32 index, nsAString & aImage)
>+{
>+  aImage.Truncate();
>+  return NS_OK;
>+}
>+
> NS_IMETHODIMP nsFileResult::RemoveValueAt(PRInt32 rowIndex, PRBool removeFromDb)
> {
>   return NS_OK;
> }
> 
> class nsFileComplete : public nsIAutoCompleteSearch
> {
> public:
>Index: xpfe/components/history/src/nsGlobalHistory.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/components/history/src/nsGlobalHistory.cpp,v
>retrieving revision 1.226
>diff -u -8 -p -r1.226 nsGlobalHistory.cpp
>--- xpfe/components/history/src/nsGlobalHistory.cpp	8 Jul 2007 07:08:55 -0000	1.226
>+++ xpfe/components/history/src/nsGlobalHistory.cpp	18 Jul 2007 23:34:45 -0000
>@@ -4181,16 +4181,24 @@ NS_IMETHODIMP
> nsGlobalHistory::GetStyleAt(PRInt32 aIndex, nsAString& aValue)
> {
>   NS_ENSURE_ARG(aIndex >= 0 && aIndex < mResults.Count());
>   aValue.Truncate();
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
>+nsGlobalHistory::GetImageAt(PRInt32 aIndex, nsAString& aValue)
>+{
>+  NS_ENSURE_ARG(aIndex >= 0 && aIndex < mResults.Count());
>+  aValue.Truncate();
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
> nsGlobalHistory::RemoveValueAt(PRInt32 aIndex, PRBool aRemoveFromDb)
> {
>   NS_ENSURE_ARG(aIndex >= 0 && aIndex < mResults.Count());
>   if (aRemoveFromDb) {
>     nsCAutoString url;
>     nsCOMPtr<nsIMdbRow> row = mResults[aIndex];
>     nsresult rv = GetRowValue(row, kToken_URLColumn, url);
>     if (NS_FAILED(rv))
>Index: toolkit/components/autocomplete/public/nsIAutoCompleteController.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/autocomplete/public/nsIAutoCompleteController.idl,v
>retrieving revision 1.14
>diff -u -8 -p -r1.14 nsIAutoCompleteController.idl
>--- toolkit/components/autocomplete/public/nsIAutoCompleteController.idl	4 Jul 2007 15:49:43 -0000	1.14
>+++ toolkit/components/autocomplete/public/nsIAutoCompleteController.idl	18 Jul 2007 23:34:45 -0000
>@@ -36,17 +36,17 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsISupports.idl"
> 
> interface nsIAutoCompleteInput;
> 
>-[scriptable, uuid(476E1472-4357-4CD0-AFE3-FEA3112617B2)]
>+[scriptable, uuid(b865d5cf-2ce1-4a5b-bb99-3d8a71df5ee9)]
> interface nsIAutoCompleteController : nsISupports
> {
>   /*
>    * Possible values for the searchStatus attribute
>    */
>   const unsigned short STATUS_NONE = 1;
>   const unsigned short STATUS_SEARCHING = 2;
>   const unsigned short STATUS_COMPLETE_NO_MATCH = 3;
>@@ -133,12 +133,17 @@ interface nsIAutoCompleteController : ns
>   AString getCommentAt(in long index);
> 
>   /*
>    * Get a the style hint for the result at a given index in the last completed search
>    */
>   AString getStyleAt(in long index);
> 
>   /*
>+   * Get the url of the image of the result at a given index in the last completed search
>+   */
>+  AString getImageAt(in long index);
>+
>+  /*
>    * Set the current search string, but don't start searching
>    */
>   void setSearchString(in AString aSearchString);
> };
>Index: toolkit/components/autocomplete/public/nsIAutoCompleteResult.idl
>===================================================================

rev the uuid.

>Index: toolkit/components/autocomplete/public/nsIAutoCompleteSimpleResult.idl
>===================================================================

>@@ -70,20 +70,20 @@ interface nsIAutoCompleteSimpleResult : 
>   /**
>    * A writer for the readonly attribute 'searchResult' which should contain
>    * one of the constants nsIAutoCompleteResult.RESULT_* indicating the success
>    * of the search.
>    */
>   void setSearchResult(in unsigned short aSearchResult);
> 
>   /**
>-   * Appends a result item consisting of the given value and comment. This is
>-   * how you add results.
>+   * Appends a result item consisting of the given value, comment, image and style. 
>+   * This is how you add results.
>    */
>-  void appendMatch(in AString aValue, in AString aComment);
>+  void appendMatch(in AString aValue, in AString aComment, in AString aImage, in AString aStyle);


Maybe make them optional params?

> 
>   /**
>    * Sets a listener for changes in the result.
>    */
>   void setListener(in nsIAutoCompleteSimpleResultListener aListener);
> };
> 
> [scriptable, uuid(004efdc5-1989-4874-8a7a-345bf2fa33af)]
>Index: toolkit/components/autocomplete/src/nsAutoCompleteController.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp,v
>retrieving revision 1.60
>diff -u -8 -p -r1.60 nsAutoCompleteController.cpp
>--- toolkit/components/autocomplete/src/nsAutoCompleteController.cpp	8 Jul 2007 07:08:43 -0000	1.60
>+++ toolkit/components/autocomplete/src/nsAutoCompleteController.cpp	18 Jul 2007 23:34:45 -0000
>@@ -599,16 +599,33 @@ nsAutoCompleteController::GetStyleAt(PRI
>   NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
> 
>   result->GetStyleAt(rowIndex, _retval);
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
>+nsAutoCompleteController::GetImageAt(PRInt32 aIndex, nsAString & _retval)
>+{
>+  PRInt32 searchIndex;
>+  PRInt32 rowIndex;
>+  RowIndexToSearch(aIndex, &searchIndex, &rowIndex);
>+  NS_ENSURE_TRUE(searchIndex >= 0 && rowIndex >= 0, NS_ERROR_FAILURE);
>+  
>+  nsCOMPtr<nsIAutoCompleteResult> result;
>+  mResults->GetElementAt(searchIndex, getter_AddRefs(result));
>+  NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
>+
>+  result->GetImageAt(rowIndex, _retval);
>+
>+  return NS_OK;


return result->GetImageAt(rowIndex, _retval);


> NS_IMETHODIMP
> nsAutoCompleteController::GetImageSrc(PRInt32 row, nsITreeColumn* col, nsAString& _retval)
> {
>+  const PRUnichar* colID;
>+  col->GetIdConst(&colID);
>+
>+  if (NS_LITERAL_STRING("treecolAutoCompleteValue").Equals(colID))
>+    GetImageAt(row, _retval);

ditto.

>Index: toolkit/components/autocomplete/src/nsAutoCompleteSimpleResult.cpp
>===================================================================


>+  mStyles.StringAt(aIndex, _retval);
>+  return NS_OK;

and here.

>Index: toolkit/components/autocomplete/src/nsAutoCompleteSimpleResult.h
>===================================================================

>-  // What we really want is an array of structs with value/comment contents.
>+  // What we really want is an array of structs with value/comment/image/style contents.
>   // But then we'd either have to use COM or manage object lifetimes ourselves.
>-  // Having two arrays of string simplifies this, but is stupid.
>+  // Having four arrays of string simplifies this, but is stupid.
>   nsStringArray mValues;
>   nsStringArray mComments;
>+  nsStringArray mImages;
>+  nsStringArray mStyles;
>

We should start using a struct at this point IMO.

Comment 12

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

Attachment: patch, v3.

Comment 13

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

> rev the uuid.

Fixed, thanks.

>-  void appendMatch(in AString aValue, in AString aComment);
>+  void appendMatch(in AString aValue, in AString aComment, in AString aImage, in AString aStyle);

> Maybe make them optional params?

Fixed, thanks.

>+  result->GetImageAt(rowIndex, _retval);
>+
>+  return NS_OK;

> return result->GetImageAt(rowIndex, _retval);

fixed, thanks.

>+  if (NS_LITERAL_STRING("treecolAutoCompleteValue").Equals(colID))
>+    GetImageAt(row, _retval);

>ditto.

fixed, thanks.

>+  mStyles.StringAt(aIndex, _retval);
>+  return NS_OK;

> and here.

I didn't change this code, because nsStringArray's StringAt does not return a nsresult.

http://lxr.mozilla.org/mozilla/source/xpcom/glue/nsVoidArray.h#223

Note, I also added the "return NS_OK;" I forgot in nsAutoCompleteSimpleResult::GetImageAt() 

> We should start using a struct at this point IMO.

I have not addressed this yet.

Comment 14

[Asaf Romano (gone)]

For the sake of making some progress, I'm fine with leaving the re-factoring to a follow up.

Comment 15

[Asaf Romano (gone)]

Comment on attachment 273135 [details] [diff] [review]
patch, v3.

r=mano

Comment 16

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

I'll spin off the refactoring to a new bug.

fixed.

Checking in extensions/cck/prefAutoCompleteService.js;
/cvsroot/mozilla/extensions/cck/prefAutoCompleteService.js,v  <--  prefAutoCompl
eteService.js
new revision: 1.4; previous revision: 1.3
done
Checking in xpfe/components/filepicker/src/nsFileView.cpp;
/cvsroot/mozilla/xpfe/components/filepicker/src/nsFileView.cpp,v  <--  nsFileVie
w.cpp
new revision: 1.32; previous revision: 1.31
done
Checking in xpfe/components/history/src/nsGlobalHistory.cpp;
/cvsroot/mozilla/xpfe/components/history/src/nsGlobalHistory.cpp,v  <--  nsGloba
lHistory.cpp
new revision: 1.227; previous revision: 1.226
done
Checking in toolkit/components/autocomplete/public/nsIAutoCompleteController.idl
;
/cvsroot/mozilla/toolkit/components/autocomplete/public/nsIAutoCompleteControlle
r.idl,v  <--  nsIAutoCompleteController.idl
new revision: 1.15; previous revision: 1.14
done
Checking in toolkit/components/autocomplete/public/nsIAutoCompleteResult.idl;
/cvsroot/mozilla/toolkit/components/autocomplete/public/nsIAutoCompleteResult.id
l,v  <--  nsIAutoCompleteResult.idl
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/components/autocomplete/public/nsIAutoCompleteSimpleResult.i
dl;
/cvsroot/mozilla/toolkit/components/autocomplete/public/nsIAutoCompleteSimpleRes
ult.idl,v  <--  nsIAutoCompleteSimpleResult.idl
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/autocomplete/src/nsAutoCompleteController.cpp;
/cvsroot/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteController.cp
p,v  <--  nsAutoCompleteController.cpp
new revision: 1.61; previous revision: 1.60
done
Checking in toolkit/components/autocomplete/src/nsAutoCompleteMdbResult.cpp;
/cvsroot/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteMdbResult.cpp
,v  <--  nsAutoCompleteMdbResult.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in toolkit/components/autocomplete/src/nsAutoCompleteSimpleResult.cpp;
/cvsroot/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteSimpleResult.
cpp,v  <--  nsAutoCompleteSimpleResult.cpp
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/components/autocomplete/src/nsAutoCompleteSimpleResult.h;
/cvsroot/mozilla/toolkit/components/autocomplete/src/nsAutoCompleteSimpleResult.
h,v  <--  nsAutoCompleteSimpleResult.h
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/components/autocomplete/tests/unit/test_378079.js;
/cvsroot/mozilla/toolkit/components/autocomplete/tests/unit/test_378079.js,v  <-
-  test_378079.js
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/components/passwordmgr/src/nsLoginManager.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginManager.js,v  <--  ns
LoginManager.js
new revision: 1.11; previous revision: 1.10
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <
--  nsNavHistoryAutoComplete.cpp
new revision: 1.14; previous revision: 1.13
done
Checking in toolkit/components/satchel/src/nsStorageFormHistory.cpp;
/cvsroot/mozilla/toolkit/components/satchel/src/nsStorageFormHistory.cpp,v  <--
 nsStorageFormHistory.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in browser/components/search/nsSearchSuggestions.js;
/cvsroot/mozilla/browser/components/search/nsSearchSuggestions.js,v  <--  nsSear
chSuggestions.js
new revision: 1.16; previous revision: 1.15
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.cs
s
new revision: 1.57; previous revision: 1.56
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.cs
s
new revision: 1.67; previous revision: 1.66
done

Comment 17

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

code refactoring bug spun off to bug #389005

While testing with a different profile, I found a problem.  I need to convert the favicon results. see bug #389003 for more details and a patch.

Comment 18

[Bill Gianopoulos [:WG9s]]

For consistency with the urlbar, this should only be done if browser.chrome.site_icons is true.

Comment 19

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

Thanks for pointing out that pref, Bill.

Remarking as fixed, spinning off the pref issue to bug #389169

Comment 20

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

http://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=4463

in-litmus+

Comment 21

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

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102904 Minefield/3.0a9pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007102904 Minefield/3.0a9pre

and

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007102904 Minefield/3.0a9pre

Verified FIXED

Comment 22

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

(That last build ID should be Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007102904 Minefield/3.0a9pre; apparently my Linux VM hates me, and won't allow cut and paste from it to my Mac host...)