Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Use default favicon consistent with rest of browser

RESOLVED FIXED in Firefox 7

Status

Firefox Graveyard
Panorama
--
trivial
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: hobophobe, Assigned: ttaubert)

Tracking

Trunk
Firefox 7
x86
Linux

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
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.
Please ask for review from one of the browser peers. 

https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch#Getting_Reviews
Status: UNCONFIRMED → NEW
Component: Panorama → General
Ever confirmed: true
QA Contact: panorama → general
(Reporter)

Comment 3

6 years ago
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.
Attachment #525130 - Attachment is obsolete: true
Attachment #525143 - Flags: review?(ian)

Comment 4

6 years ago
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.
Attachment #525143 - Flags: review?(ian) → feedback?(tim.taubert)

Updated

6 years ago
Component: General → Panorama
QA Contact: general → panorama
(Assignee)

Updated

6 years ago
Depends on: 660206
Summary: Use default favicon consistent with rest of browser (via CSS). → Use default favicon consistent with rest of browser
Version: unspecified → Trunk
(Assignee)

Comment 5

6 years ago
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 :)
Attachment #525143 - Attachment is obsolete: true
Attachment #525143 - Flags: feedback?(tim.taubert)
Attachment #535605 - Flags: feedback?(raymond)
(Assignee)

Comment 6

6 years ago
Created attachment 535606 [details] [diff] [review]
patch v2

Oops, now with the correct favicon service variable.
Attachment #535605 - Attachment is obsolete: true
Attachment #535605 - Flags: feedback?(raymond)
Attachment #535606 - Flags: feedback?(raymond)
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.
Attachment #535606 - Flags: feedback?(raymond) → feedback-
(Assignee)

Comment 8

6 years ago
(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.
(Assignee)

Comment 9

6 years ago
Created attachment 535609 [details] [diff] [review]
patch v3
Attachment #535606 - Attachment is obsolete: true
Attachment #535609 - Flags: feedback?(raymond)
Comment on attachment 535609 [details] [diff] [review]
patch v3

Looks good!
Attachment #535609 - Flags: feedback?(raymond) → feedback+
(Assignee)

Comment 11

6 years ago
Comment on attachment 535609 [details] [diff] [review]
patch v3

Trying to distribute reviews, hope Shawn has some time left :)
Attachment #535609 - Flags: review?(sdwilsh)
(Assignee)

Updated

6 years ago
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
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).
Attachment #535609 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 13

6 years ago
http://hg.mozilla.org/mozilla-central/rev/e8b74cf2e32b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
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.
(Assignee)

Comment 15

6 years ago
(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.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.