add favicons to location bar autocomplete results

VERIFIED FIXED in Firefox 3 alpha7

Status

()

Firefox
Location Bar
--
enhancement
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: (not reading, please use seth@sspitzer.org instead))

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

Trunk
Firefox 3 alpha7
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
wanted-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
taking
Assignee: nobody → sspitzer
Flags: blocking-firefox3?
Created attachment 272843 [details]
screen shot from draft patch
Created attachment 272844 [details] [diff] [review]
wip patch, v1
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
Status: NEW → ASSIGNED
>3)  default blank space if no favicon

Showing the default favicon (page with corner folded) might look better than using white space.
Created attachment 272884 [details]
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
Attachment #272843 - Attachment is obsolete: true
Attachment #272844 - Attachment is obsolete: true
Created attachment 272885 [details] [diff] [review]
patch, v2
> 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
Attachment #272885 - Flags: review?(mano)

Comment 9

10 years ago
(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.
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.
Target Milestone: --- → Firefox 3 M7
Whiteboard: [SWAG: patch in hand awaiting review]
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.
Attachment #272885 - Flags: review?(mano) → review-
Created attachment 273135 [details] [diff] [review]
patch, v3.
Attachment #272885 - Attachment is obsolete: true
> 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.
For the sake of making some progress, I'm fine with leaving the re-factoring to a follow up.
Comment on attachment 273135 [details] [diff] [review]
patch, v3.

r=mano
Attachment #273135 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Blocks: 389003
Blocks: 389005
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.
Whiteboard: [SWAG: patch in hand awaiting review]

Updated

10 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Whiteboard: [wanted-firefox3]
For consistency with the urlbar, this should only be done if browser.chrome.site_icons is true.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Thanks for pointing out that pref, Bill.

Remarking as fixed, spinning off the pref issue to bug #389169
Blocks: 389169
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Summary: add favicons to the results in the location bar → add favicons to location bar autocomplete results

Updated

10 years ago
Depends on: 389075
Flags: in-litmus?

Updated

10 years ago
Depends on: 389642
http://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=4463

in-litmus+
Flags: in-litmus? → in-litmus+
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
Status: RESOLVED → VERIFIED
(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...)
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
You need to log in before you can comment on or make changes to this bug.