Last Comment Bug 649088 - Use default favicon consistent with rest of browser
: Use default favicon consistent with rest of browser
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: x86 Linux
: -- trivial
: Firefox 7
Assigned To: Tim Taubert [:ttaubert]
:
:
Mentors:
Depends on: 660206
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-11 12:14 PDT by Adam [:hobophobe]
Modified: 2016-04-12 14:00 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Adds CSS changes, removes script setting of image[src] to the defaultFavicon.png (4.19 KB, patch)
2011-04-11 12:22 PDT, Adam [:hobophobe]
no flags Details | Diff | Splinter Review
Adds styles to use default favicons, removes script setting the default favicon. (7.22 KB, patch)
2011-04-11 13:16 PDT, Adam [:hobophobe]
no flags Details | Diff | Splinter Review
patch v1 (1.40 KB, patch)
2011-05-27 04:31 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v2 (1.40 KB, patch)
2011-05-27 04:35 PDT, Tim Taubert [:ttaubert]
raymond: feedback-
Details | Diff | Splinter Review
patch v3 (3.27 KB, patch)
2011-05-27 05:09 PDT, Tim Taubert [:ttaubert]
sdwilsh: review+
raymond: feedback+
Details | Diff | Splinter Review

Description Adam [:hobophobe] 2011-04-11 12:14:25 PDT
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 [:hobophobe] 2011-04-11 12:22:38 PDT
Created attachment 525130 [details] [diff] [review]
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 [:kbrosnan] 2011-04-11 12:52:21 PDT
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 [:hobophobe] 2011-04-11 13:16:35 PDT
Created attachment 525143 [details] [diff] [review]
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 2011-04-13 11:45:12 PDT
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] 2011-05-27 04:31:25 PDT
Created attachment 535605 [details] [diff] [review]
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] 2011-05-27 04:35:27 PDT
Created attachment 535606 [details] [diff] [review]
patch v2

Oops, now with the correct favicon service variable.
Comment 7 Raymond Lee [:raymondlee] 2011-05-27 04:49:51 PDT
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] 2011-05-27 04:51:32 PDT
(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] 2011-05-27 05:09:52 PDT
Created attachment 535609 [details] [diff] [review]
patch v3
Comment 10 Raymond Lee [:raymondlee] 2011-05-27 06:19:16 PDT
Comment on attachment 535609 [details] [diff] [review]
patch v3

Looks good!
Comment 11 Tim Taubert [:ttaubert] 2011-05-27 06:21:22 PDT
Comment on attachment 535609 [details] [diff] [review]
patch v3

Trying to distribute reviews, hope Shawn has some time left :)
Comment 12 Shawn Wilsher :sdwilsh 2011-05-31 11:11:13 PDT
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] 2011-05-31 13:09:13 PDT
http://hg.mozilla.org/mozilla-central/rev/e8b74cf2e32b
Comment 14 Virgil Dicu [:virgil] [QA] 2011-08-23 06:44:04 PDT
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] 2011-08-24 04:15:30 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.