1308727 - Refactor favicon handling for session persistence and guaranteed resolution of error icons.

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[alta88]

This patch attempts to fix the 2 main problems with the original implementation, as described in Bug 748950#c4.

1. Favicons are loaded into an html:img node, which returns errors well, thus guaranteeing resolution and getting rid of the current gross hack.
2. The folder URI keyed favicon urls are moved to a cache object on gFolderTreeView from the current rowMap item location, ensuring session persistence once resolved.
3. Some tweaks to ensure getImageSrc does as little unnecessary work as possible.

Comment 1

[alta88]

Attachment: favicons.patch

Note: this patch is based off the patches in Bug 497488. It is also intended to lay the groundwork for making it a simple fix for bug 748950 mail server favicons.

Comment 2

[alta88]

Attachment: favicons.patch

tweaks.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 8799231 [details] [diff] [review]
favicons.patch

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

I'm not sure why you want to distinguish between null and undefined, but that would be error prone.

::: mail/base/content/folderPane.js
@@ +919,5 @@
>      if (folder.server.type != "rss" || folder.isServer)
>        return "";
>  
> +    let favicon = this.getFolderCacheProperty(folder, "favicon");
> +    if (favicon != undefined)

if (favicon)

@@ +1491,5 @@
> +   * @return value or null.
> +   */
> +  getFolderCacheProperty: function(aFolder, aProperty) {
> +    if (!aFolder || !aProperty)
> +      return null;

wouldn't that be programmer error? seems better to just let things blow up if you pass in nulls

::: mail/base/content/mailTabs.js
@@ +282,4 @@
>            return;
>          }
>  
> +        if (favicon == undefined) {

if (!favicon)

::: mailnews/extensions/newsblog/content/FeedUtils.jsm
@@ +635,2 @@
>  
> +    if (aIconUrl != undefined)

if (aIconUrl)

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +315,5 @@
>        {
>          for (let child of item.children)
>          {
> +          if (!child.container) {
> +            if (child.favicon != undefined)

here too just falsy/truthy comparisons please

Comment 4

[alta88]

(In reply to Magnus Melin from comment #3)
> Comment on attachment 8799231 [details] [diff] [review]
> favicons.patch
> 
> Review of attachment 8799231 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure why you want to distinguish between null and undefined, but
> that would be error prone.
> 

it's an inherent part of the design.  the possible values are undefined, "", or the valid url string. it's very important to only do the favicon processing once, and the initial state is represented most appropriately by undefined.  using null could be possible, but it would preclude null as a "never previously processed" value. (yes, i know it would need stricter === equality, but currently null isn't used as a permanent resolved value).

so "" means resolved, but with no valid url, go use the default favicon from css.  the change that should be made here is somevar === or !== undefined to be futureproof.

Comment 5

[Magnus Melin [:mkmelin]]

Thx for the explanation. I still think it would be better to have null mean unprocessed, and "" for processed-but-n/a

Comment 6

[alta88]

but that hardly makes any difference, it would just mean changing to somevar == null or != null, instead of undefined. is that what you want? a falsy evaluationn like (!somevar) simply won't work. and it means somevar can't ever be resolved to null, other things may want to use this cache.

Comment 7

[alta88]

the other thing about null as a resolved value - it's certainly preferable to "", but it's only due to some issue (i recall it from the original favicon review) with getImageSrc having a problem with null.  but probably getCellProperties and others are fine with null, and this cache would be a good use there.  that's why i did it this way.

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to alta88 from comment #6)
> but that hardly makes any difference, it would just mean changing to somevar
> == null or != null, instead of undefined. is that what you want? a falsy

I think that would be preferable yes.

Comment 9

[alta88]

Attachment: favicons.patch

updated.  this also makes some tweaks, mainly removing old skool callback arg as a fat arrow style function binds automatically to |this|.

Comment 10

[alta88]

ping?

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 8800286 [details] [diff] [review]
favicons.patch

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

Looks good to me, sorry for the delay! r=mkmelin

Comment 12

[alta88]

thanks.

Comment 13

[Richard Marti (:Paenglab)]

https://hg.mozilla.org/comm-central/rev/68d48640a8e782e3308328927fed0043c8c3d8de