745301 - Show favicon next to RSS/Atom feeds

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, enhancement)

Description

[st.do]

Right now there is a generic feed icon next to each feed.

I would like to see the favicon of the feed's website there instead.

The icon would help to make the individual feeds more distinct. It would also look prettier.
Some other feed readers do this, like RSSOwl or Akregator.

Comment 1

[Hashem Masoud]

(In reply to st.do from comment #0)
There are some RSS add-ons and you can check if they support that feature: https://addons.mozilla.org/en-US/thunderbird/search/?q=rss&appver=&platform=

Comment 2

[st.do]

Thanks for your reply.

I've searched for a while and none of the add-ons support favicons.

Comment 3

[Hashem Masoud]

(In reply to st.do from comment #2)
> Thanks for your reply.
> 
> I've searched for a while and none of the add-ons support favicons.

In that case, you can send the add-on developer a request to enhance it with this feature.

Comment 4

[st.do]

All right. I apologize if this was the wrong place for my request.

Comment 5

[alta88]

Attachment: favicon.patch

Comment 6

[alta88]

This is a significant UX improvement for folderpane feeds.  The patch uses half the method used by content tabs to get the tab favicon, ie getting the favicon.ico from the feed's <link> url (homepage).  This method works in a large majority of cases.

To get to the sophistication level of Places and its favicon service:
1) history would need to be enabled (disabled in Tb by default).
2) a feed could then store the <link> as a history item and relate a favicon blob to it in the places db.
3) a feed could also get the homepage and check for its <link> rel attribute for a string containing "icon" and store its href (which points to the favicon) - if there's no favicon.ico found.
4) if history is enabled, there needs to be a corresponding privacy UI, as in Firefox.

The above is a followup bug, and doesn't have any effect on email urls.

Comment 7

[Richard Marti (:Paenglab)]

I get with updates tree:
patching file mailnews/extensions/newsblog/content/feed-subscriptions.js
Hunk #4 FAILED at 545
Hunk #5 FAILED at 618
2 out of 5 hunks FAILED -- saving rejects to file mailnews/extensions/newsblog/content/feed-subscriptions.js.rej

Comment 8

[alta88]

ah.  this patch was built on top of the work in bug 946279, could you try to apply that patch first?  maybe you would also like to ui-r it too ;)

Comment 9

[Richard Marti (:Paenglab)]

Comment on attachment 8397239 [details] [diff] [review]
favicon.patch

Review of attachment 8397239 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. What about adding this favicons also to the tab and folder menulists? Without them there it could confuse the user because of the different icons.

::: mail/themes/osx/mail/folderPane.css
@@ +132,5 @@
>    treechildren::-moz-tree-image(folderNameCol, isFeedFolder-true) {
>      list-style-image: url("chrome://messenger/skin/icons/[e-mail]");
>      -moz-image-region: rect(0 448px 32px 416px);
> +    max-width: 32px;
> +    max-height: 32px;

Are this two lines needed? The width/height of 16px in LoDPI rule sets it to the needed dimensions. The same for the rule in feed-subscriptions.css.

Comment 10

[alta88]

(In reply to Richard Marti (:Paenglab) from comment #9)
> Comment on attachment 8397239 [details] [diff] [review]
> favicon.patch
> 
> Review of attachment 8397239 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. What about adding this favicons also to the tab and folder
> menulists? Without them there it could confuse the user because of the
> different icons.
>

Thanks.  Yes, a feed folder in a tab should also get the favicon. It may be clunky for tabs and menulists, as the icons are gotten async, and treeview works since any mouseover refreshes the previous attempt (which after a bit resolves) and so far the setAndFetchFaviconForPage() callback isn't working.

> ::: mail/themes/osx/mail/folderPane.css
> @@ +132,5 @@
> >    treechildren::-moz-tree-image(folderNameCol, isFeedFolder-true) {
> >      list-style-image: url("chrome://messenger/skin/icons/[e-mail]");
> >      -moz-image-region: rect(0 448px 32px 416px);
> > +    max-width: 32px;
> > +    max-height: 32px;
> 
> Are this two lines needed? The width/height of 16px in LoDPI rule sets it to
> the needed dimensions. The same for the rule in feed-subscriptions.css.

I'm not sure.  Since we're not using the full Places favicon service (which will resize favicons before storing them in the db), it turns out some favicons in the default location can be huge, which is why the 16px rules are needed.  By returning the icon in treeview, the css list style image (and region I assume) is overridden, and we'd want to go up to 32 of hidpi?  I have no way of testing this, so your advice is needed here.

Comment 11

[Richard Marti (:Paenglab)]

(In reply to alta88 from comment #10)
> (In reply to Richard Marti (:Paenglab) from comment #9)
> > Comment on attachment 8397239 [details] [diff] [review]
> > favicon.patch
> 
> > ::: mail/themes/osx/mail/folderPane.css
> > @@ +132,5 @@
> > >    treechildren::-moz-tree-image(folderNameCol, isFeedFolder-true) {
> > >      list-style-image: url("chrome://messenger/skin/icons/[e-mail]");
> > >      -moz-image-region: rect(0 448px 32px 416px);
> > > +    max-width: 32px;
> > > +    max-height: 32px;
> > 
> > Are this two lines needed? The width/height of 16px in LoDPI rule sets it to
> > the needed dimensions. The same for the rule in feed-subscriptions.css.
> 
> I'm not sure.  Since we're not using the full Places favicon service (which
> will resize favicons before storing them in the db), it turns out some
> favicons in the default location can be huge, which is why the 16px rules
> are needed.  By returning the icon in treeview, the css list style image
> (and region I assume) is overridden, and we'd want to go up to 32 of hidpi? 
> I have no way of testing this, so your advice is needed here.

Josiah, you have a Retina Mac. Please can you look at this if the max-width/max-height is needed?

Comment 12

[alta88]

Attachment: favicon.patch

this patch adds favicon switching to existing feed folder tabs.  getting the favicon right for newly opened feed folder/message tabs will be part2.  folder menuitems would be part3.

this patch also fixes a tab icon problem with feed folders; special folders were being unset (this was partially worked around in win but broken in osx and linux).

Comment 13

[alta88]

Attachment: favicon.patch

tweak for folderpane multiselect and tabs.

Comment 14

[Josiah Bruner [:jsbruner]]

Comment on attachment 8398251 [details] [diff] [review]
favicon.patch

Review of attachment 8398251 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/themes/osx/mail/newsblog/feed-subscriptions.css
@@ +33,5 @@
>    treechildren::-moz-tree-image(folderNameCol, isFeed-true) {
>      list-style-image: url("chrome://messenger-newsblog/skin/[e-mail]");
>      -moz-image-region: rect(0 32px 32px 0);
> +    max-width: 32px;
> +    max-height: 32px;

The patch has doesn't apply cleanly, so I haven't tested, but these lines should not be needed. 

The CSS implementation of sizes is weird with retina displays, in that, although a 32px *icon* is needed, the *width* and *height* should still be 16 x 16 pixels. This is because the 32px icon is treated as a 32px "virtual" icon, the "actual" size we want it is half of that. So doubling the width and height like you've done should instead double the size the box takes up and in addition, negate it's retina-like quality.

But of course I will definitely test this when the patch is updated.

Comment 15

[alta88]

Attachment: favicon.patch

Thanks.  The patch has that removed then, and has been rebased to not depend on comment 8.

One thing: osx folderPane.css seems to be missing a .tabmail-tab rule for ServerType=rss and isServer-true, unlike win/linux.  Is this intentional?

Comment 16

[Richard Marti (:Paenglab)]

Comment on attachment 8398505 [details] [diff] [review]
favicon.patch

Review of attachment 8398505 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

(In reply to alta88 from comment #15)
> Created attachment 8398505 [details] [diff] [review]
> favicon.patch
> 
> One thing: osx folderPane.css seems to be missing a .tabmail-tab rule for
> ServerType=rss and isServer-true, unlike win/linux.  Is this intentional?

I think it's not intentional. This can be done in a new bug.

Comment 17

[alta88]

Attachment: favicon2.patch

Part 2:

1) new tabs/first tab and startup tabs get the favicon.
2) feed message tabs get an attribute and favicon more relevant than the email envelope.
3) getFavicon() tweaked to be more generic.
4) osx css tweak for comment 16.

the feed message tab should also get the favicon (part 3).

Comment 18

[Richard Marti (:Paenglab)]

Comment on attachment 8398951 [details] [diff] [review]
favicon2.patch

Review of attachment 8398951 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to alta88 from comment #17)
> Created attachment 8398951 [details] [diff] [review]
> favicon2.patch
> 
> Part 2:
> 
> 1) new tabs/first tab and startup tabs get the favicon.

Not sure what you mean but I see the icon already with part 1 in first tab.

> 2) feed message tabs get an attribute and favicon more relevant than the
> email envelope.

When I open a feed message through "Open Message in New Tab" there is still the message icon and "IsFeedMessage" isn't set in tab attributes.

::: mail/themes/osx/mail/folderPane.css
@@ +285,5 @@
>      -moz-image-region: rect(0 160px 32px 128px);
>    }
>  
>    /* ..... Feed Folders ..... */
> +.tabmail-tab[type="folder"][IsServer="true"][ServerType="rss"],

Please indent by two spaces.

::: mail/themes/osx/mail/mailWindow1.css
@@ +561,5 @@
> +  height: 16px;
> +}
> +
> +@media (min-resolution: 2dppx) {
> +.tabmail-tab[type="message"][IsFeedMessage] {

Please indent by two spaces.

Comment 19

[alta88]

(In reply to Richard Marti (:Paenglab) from comment #18)
> Comment on attachment 8398951 [details] [diff] [review]
> favicon2.patch
> 
> Review of attachment 8398951 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to alta88 from comment #17)
> > Created attachment 8398951 [details] [diff] [review]
> > favicon2.patch
> > 
> > Part 2:
> > 
> > 1) new tabs/first tab and startup tabs get the favicon.
> 
> Not sure what you mean but I see the icon already with part 1 in first tab.

It just means first-tab is special cased, as an efficiency difference is made between icon urls that have to be gotten (new background tabs) and ones we already have (from folderpane).

> 
> > 2) feed message tabs get an attribute and favicon more relevant than the
> > email envelope.
> 
> When I open a feed message through "Open Message in New Tab" there is still
> the message icon and "IsFeedMessage" isn't set in tab attributes.

I assume you applied part 2 on top of part 1.  If so, the only way this happens is if the feed message was downloaded in a pre Tb15 version, before the FeedMsg flag was implemented.  This flag allows a feed message to be so known even if you move it to a non feed account folder.

Otherwise, wfm; any errors?

> 
> Please indent by two spaces.

ok.

Comment 20

[Richard Marti (:Paenglab)]

Comment on attachment 8398951 [details] [diff] [review]
favicon2.patch

Review of attachment 8398951 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to alta88 from comment #19)
> (In reply to Richard Marti (:Paenglab) from comment #18)
> > > 2) feed message tabs get an attribute and favicon more relevant than the
> > > email envelope.
> > 
> > When I open a feed message through "Open Message in New Tab" there is still
> > the message icon and "IsFeedMessage" isn't set in tab attributes.
> 
> I assume you applied part 2 on top of part 1.  If so, the only way this
> happens is if the feed message was downloaded in a pre Tb15 version, before
> the FeedMsg flag was implemented.  This flag allows a feed message to be so
> known even if you move it to a non feed account folder.

Good catch, it was a old feed. With a new feed it works.

Comment 21

[alta88]

Attachment: favicon2.patch

updated.

Comment 22

[alta88]

Attachment: favicon3.patch

Part 3 - favicon for feed message tabs.

Comment 23

[alta88]

Attachment: favicon4.patch

part 4 - there are enough favicons in a page's <link rel> rather than the default location that this is implemented as well.

Comment 24

[alta88]

Attachment: faviconCumulative.patch

this is a cumulative rollup of the prior 4 parts; it's probably easier to review this one.

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 8402025 [details] [diff] [review]
faviconCumulative.patch

Review of attachment 8402025 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to be working. 
Some of the things mentioned are in more places than i've pointed out.

::: mail/base/content/folderPane.js
@@ +720,5 @@
>        return this._rowMap[aRow].text;
>      return "";
>    },
>  
> +  getImageSrc: function ftv_getImageSrc(aRow, aCol) {

Please add documentation. Should it be named getFavicon?
FTR, the ftv_getImageSrc is no longer of much use (a godo name is generated automatically)

@@ +731,5 @@
> +      return rowItem._favicon;
> +
> +    let tree = this._tree;
> +    function callback(iconUrl, domain, arg) {
> +FeedUtils.log.debug("FeedUtils.getImageSrc: aRow:iconUrl:rowItem - "+aRow+ ":"+iconUrl+ ":"+rowItem._favicon);

spaces missing, and please align FeedUtils properly

::: mail/base/content/mailTabs.js
@@ +278,5 @@
> +            return;
> +
> +          let iconUrl = FeedUtils.getFavicon(folder, feedUrls[0], null, null, null);
> +          if (iconUrl) {
> +FeedUtils.log.debug("folder.onTitleChanged: iconUrl - "+ iconUrl);

align, and add space before +

@@ +290,5 @@
> +            }
> +            let onerror = "FeedUtils.getFaviconFromPage('" + url + "', "
> +                                                           + callback + ", " +
> +                                                            "this" + ");"
> +            aTabNode.setAttribute("onerror", onerror);

ugh, this should probably use an eventlistener instead

@@ +318,5 @@
> +          let iconUrl = FeedUtils.getFavicon(folder, feedUrls[0], null, null, null);
> +          if (iconUrl) {
> +FeedUtils.log.debug("folder.onTitleChanged: iconUrl - "+ iconUrl);
> +            let url = Services.io.newURI(iconUrl, null, null).prePath;
> +            function callback(iconUrl, domain, tabNode) {

also, i think this produces a js warning in strict mode
you can use let callback = function() instead...

@@ +328,5 @@
> +            }
> +            let onerror = "FeedUtils.getFaviconFromPage('" + url + "', "
> +                                                           + callback + ", " +
> +                                                            "this" + ");"
> +            aTabNode.setAttribute("onerror", onerror);

please make this too use an event listener

::: mailnews/extensions/newsblog/content/FeedUtils.jsm
@@ +288,5 @@
> +    if (this._mFaviconService)
> +      return this._mFaviconService;
> +
> +    return this._mFaviconService = Cc["@mozilla.org/browser/favicon-service;1"]
> +                                     .getService(Ci.nsIFaviconService);

I believe the way this is usually done is
http://mxr.mozilla.org/comm-central/source/mail/base/content/specialTabs.js#453

@@ +301,5 @@
> + * @param  string aUrl          - a url (feed, message, other) or null if aFolder
> + * @param  string aIconUrl      - the icon url if already determined, else null
> + * @param  nsIDOMWindow aWindow - null or caller's window if aCallback needed
> + * @param  function aCallback   - null or callback
> + * @return string               - the favicon url or empty string

maybe null?

@@ +394,5 @@
> +    function onload(aEvent) {
> +      let request = aEvent.target;
> +      responseDomain = request.channel.URI.prePath;
> +FeedUtils.log.debug("FeedUtils.getFaviconFromPage: onload - " +
> +                    request.status+ ":"+responseDomain);

align, spaces

@@ +426,5 @@
> +    function onerror() {
> +      if (aCallback)
> +        aCallback(null, responseDomain, aArg);
> +FeedUtils.log.debug("FeedUtils.getFaviconFromPage: onerror - ''");
> +    }

this function is in a very odd place, does it actually work?
In general I'd prefer the functions not to be so generically named (even if local), as it helps understand where it would be called withough looking it up.

This is also a case where you should avoid strict promblems, maybe using let foo = function()

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 8402024 [details] [diff] [review]
favicon4.patch

Reviewing the cumulative patch.

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 8399441 [details] [diff] [review]
favicon3.patch

Reviewing the cumulative patch.

Comment 28

[alta88]

(In reply to Magnus Melin from comment #25)
> Comment on attachment 8402025 [details] [diff] [review]
> faviconCumulative.patch
> 
> Review of attachment 8402025 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems to be working. 
> Some of the things mentioned are in more places than i've pointed out.
> 
> ::: mail/base/content/folderPane.js
> @@ +720,5 @@
> >        return this._rowMap[aRow].text;
> >      return "";
> >    },
> >  
> > +  getImageSrc: function ftv_getImageSrc(aRow, aCol) {
> 
> Please add documentation. Should it be named getFavicon?
> FTR, the ftv_getImageSrc is no longer of much use (a godo name is generated
> automatically)
> 

getImageSrc is a required function in an nsITreeView implementation.  if it returns, it appears it needs to return a "" empty string; this is found all over the codebase, in Places, etc.


> @@ +731,5 @@
> > +      return rowItem._favicon;
> > +
> > +    let tree = this._tree;
> > +    function callback(iconUrl, domain, arg) {
> > +FeedUtils.log.debug("FeedUtils.getImageSrc: aRow:iconUrl:rowItem - "+aRow+ ":"+iconUrl+ ":"+rowItem._favicon);
> 
> spaces missing, and please align FeedUtils properly

forgot to mention the debug statements were all going to be removed on the checkin patch..
> 
> ::: mail/base/content/mailTabs.js
> @@ +278,5 @@
> > +            return;
> > +
> > +          let iconUrl = FeedUtils.getFavicon(folder, feedUrls[0], null, null, null);
> > +          if (iconUrl) {
> > +FeedUtils.log.debug("folder.onTitleChanged: iconUrl - "+ iconUrl);
> 
> align, and add space before +
> 
> @@ +290,5 @@
> > +            }
> > +            let onerror = "FeedUtils.getFaviconFromPage('" + url + "', "
> > +                                                           + callback + ", " +
> > +                                                            "this" + ");"
> > +            aTabNode.setAttribute("onerror", onerror);
> 
> ugh, this should probably use an eventlistener instead

ok.

> @@ +318,5 @@
> > +          let iconUrl = FeedUtils.getFavicon(folder, feedUrls[0], null, null, null);
> > +          if (iconUrl) {
> > +FeedUtils.log.debug("folder.onTitleChanged: iconUrl - "+ iconUrl);
> > +            let url = Services.io.newURI(iconUrl, null, null).prePath;
> > +            function callback(iconUrl, domain, tabNode) {
> 
> also, i think this produces a js warning in strict mode
> you can use let callback = function() instead...
> 

ok.

> @@ +328,5 @@
> > +            }
> > +            let onerror = "FeedUtils.getFaviconFromPage('" + url + "', "
> > +                                                           + callback + ", " +
> > +                                                            "this" + ");"
> > +            aTabNode.setAttribute("onerror", onerror);
> 
> please make this too use an event listener
> 

ok.

> ::: mailnews/extensions/newsblog/content/FeedUtils.jsm
> @@ +288,5 @@
> > +    if (this._mFaviconService)
> > +      return this._mFaviconService;
> > +
> > +    return this._mFaviconService = Cc["@mozilla.org/browser/favicon-service;1"]
> > +                                     .getService(Ci.nsIFaviconService);
> 
> I believe the way this is usually done is
> http://mxr.mozilla.org/comm-central/source/mail/base/content/specialTabs.
> js#453
> 

ok. 

> @@ +301,5 @@
> > + * @param  string aUrl          - a url (feed, message, other) or null if aFolder
> > + * @param  string aIconUrl      - the icon url if already determined, else null
> > + * @param  nsIDOMWindow aWindow - null or caller's window if aCallback needed
> > + * @param  function aCallback   - null or callback
> > + * @return string               - the favicon url or empty string
> 
> maybe null?
> 

it's for getImageSrc, above.

> @@ +394,5 @@
> > +    function onload(aEvent) {
> > +      let request = aEvent.target;
> > +      responseDomain = request.channel.URI.prePath;
> > +FeedUtils.log.debug("FeedUtils.getFaviconFromPage: onload - " +
> > +                    request.status+ ":"+responseDomain);
> 
> align, spaces
> 
> @@ +426,5 @@
> > +    function onerror() {
> > +      if (aCallback)
> > +        aCallback(null, responseDomain, aArg);
> > +FeedUtils.log.debug("FeedUtils.getFaviconFromPage: onerror - ''");
> > +    }
> 
> this function is in a very odd place, does it actually work?
> In general I'd prefer the functions not to be so generically named (even if
> local), as it helps understand where it would be called withough looking it
> up.
> 
> This is also a case where you should avoid strict promblems, maybe using let
> foo = function()

it worked, names were just xhr callback names.  but it's been reworked.

Comment 29

[alta88]

Attachment: faviconCumulative.patch

Comment 30

[Magnus Melin [:mkmelin]]

Comment on attachment 8402200 [details] [diff] [review]
faviconCumulative.patch

Review of attachment 8402200 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, with the below addressed. r=mkmelin

::: mailnews/extensions/newsblog/content/FeedUtils.jsm
@@ +405,5 @@
> +
> +    let onDownloadError = function() {
> +      if (aCallback)
> +        aCallback(null, responseDomain, aArg);
> +    }

This is still in an odd place, and at least it this form I can't imagine if works to call onDownloadError before its declaration.

Comment 31

[alta88]

Attachment: faviconCumulative.patch

updated for comment.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/b58fa42c3102

Comment 33

[Stanzilla]

I still have some feeds that don't display a favicon:

http://diablofans.com/blizz-tracker.rss
http://git.chromium.org/gitweb/?p=git/chromium.git;a=rss
http://feeds.feedburner.com/WinRumors

They show in the subcribe options but not in the feed category itself.

Comment 34

[Stanzilla]

*pane

Comment 35

[alta88]

see Bug 998963.

Comment 36

[jwhelan]

Attachment: Newegg.com RSS feed favicon is rendered incorrectly (too large)

There is a slight bug that I found in the favicon rendering. See attached.

The RSS feed in question is located at: 

http://www.newegg.com/Product/RSS.aspx?Submit=RSSDailyDeals&Depa=0

Comment 37

[Magnus Melin [:mkmelin]]

Please file a new bug, this one is closed.