649088 - Use default favicon consistent with rest of browser

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Adam]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20110327 Firefox/4.0 Iceweasel/4.0
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20110327 Firefox/4.0 Iceweasel/4.0

Firefox 4 moved away from using defaultFavicon.png for Linux, instead opting for moz-icon://stock/gtk-file via CSS.  Panorama's use of defaultFavicon.png on app tabs breaks that consistency.

Reproducible: Always

Steps to Reproduce:
1.Open a tab that uses default favicon.
2.Pin as app tab
3.Open panorama view
Actual Results:  
See the old defaultFavicon.png

Expected Results:  
See the default favicon as used elsewhere in the browser.

In order to fix this, the app tabs need to be moved to using CSS for fallback when a custom favicon isn't there.  CSS allows different values to be applied by platform, which is how the problem is handled elsewhere.

Comment 1

[Adam]

Attachment: Adds CSS changes, removes script setting of image[src] to the defaultFavicon.png

I've left the definition that points to the defaultFavicon.png in tabview/modules/utils.jsm, as it is still used in tabview/tabitems.js:_update, which likely needs fixing as well.

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

Please ask for review from one of the browser peers. 

https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch#Getting_Reviews

Comment 3

[Adam]

Attachment: Adds styles to use default favicons, removes script setting the default favicon.

Includes previous changes, but finishes the job of removing the script-based default favicon code from panorama.

Comment 4

[Kevin Hanes]

Comment on attachment 525143 [details] [diff] [review]
Adds styles to use default favicons, removes script setting the default favicon.

Let's have ttaubert do feedback before getting a review.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Thanks for your patch, Adam! When reviewing it I thought about what would be the best solution for this problem and I think we shouldn't fix the symptoms but rather the source of the error. So this should depend on bug 660206 and then we're done with a tiny patch.

Sorry for stealing this but the patch is so small I just though I'd attach it :)

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Oops, now with the correct favicon service variable.

Comment 7

[Raymond Lee [:raymondlee]]

Comment on attachment 535606 [details] [diff] [review]
patch v2

Please check browser_tabview_bug600645.js.  It's using contentWindow.Utils.defaultFaviconURL which is removed in this patch.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #7)
> Please check browser_tabview_bug600645.js.  It's using
> contentWindow.Utils.defaultFaviconURL which is removed in this patch.

You're right I should've run the tests before asking for feedback, my bad.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

Comment 10

[Raymond Lee [:raymondlee]]

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

Looks good!

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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

Trying to distribute reviews, hope Shawn has some time left :)

Comment 12

[Shawn Wilsher :sdwilsh]

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

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

r=sdwilsh

::: browser/base/content/tabview/groupitems.js
@@ +2031,5 @@
>  
>      if (UI.shouldLoadFavIcon(xulTab.linkedBrowser))
>        iconUrl = UI.getFavIconUrlForTab(xulTab);
>      else
> +      iconUrl = gFavIconService.defaultFavicon.spec;

Linux should use moz-icon://stock/gtk-file, but I see you are trying to fix that in general in bug 660206, so I'm fine with landing this like this as now (it's no worse than the current situation).

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/e8b74cf2e32b

Comment 14

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0

Panorama's use of the default favicon is still inconsistent on Linux. The problem can be reproduced on Firefox7 beta1 with the same steps as in the description. 

Is this issue known and intended for the Firefox 7 release? 

For Firefox 8, the default favicon is updated, as specified in Bug 648668.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Virgil Dicu from comment #14)
> Panorama's use of the default favicon is still inconsistent on Linux. The
> problem can be reproduced on Firefox7 beta1 with the same steps as in the
> description. 

Indeed.

> Is this issue known and intended for the Firefox 7 release?

It is known but will not be fixed in Fx 7.

> For Firefox 8, the default favicon is updated, as specified in Bug 648668.

Yep, will be fixed in Fx 8.