Update 16x16 warning favicon in netError.xhtml to be platform specific

RESOLVED FIXED in mozilla1.9

Status

()

Core
General
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: faaborg, Assigned: Away for a while)

Tracking

Trunk
mozilla1.9
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
netError.xhtml should (if technically possible) display platform specific favicons:

XP:
http://mxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/icons/warning-16.png

Vista:
http://mxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/icons/warning-16-aero.png

OS X:
http://mxr.mozilla.org/seamonkey/source/toolkit/themes/pinstripe/global/icons/warning-16.png

Linux:
http://mxr.mozilla.org/seamonkey/source/toolkit/themes/gnomestripe/global/icons/notfound.png
(Assignee)

Updated

10 years ago
Assignee: nobody → ehsan.akhgari
Target Milestone: --- → Firefox 3
I'm not sure this is easy to do... I ran into this in with the favicon for the about:robots page -- which is basically a copy of netError.xhtml. It seems the way our security policy works prevents just linking to packaged icon url (eg, chrome://whatever). This was also noted as a problem in bug 229737, which added favicons to the error page. Dunno if a clever workaround of some sort might be possible now.

See also, bug 314416.
Target Milestone: Firefox 3 → ---
(Assignee)

Comment 2

10 years ago
Created attachment 316487 [details] [diff] [review]
Patch (v1)
[Checkin: Comment 11]

Am I missing something here?  This patch seems to work beautifully (on Vista at least).
Attachment #316487 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Target Milestone: --- → Firefox 3
Does it work even when the file is packaged in chrome?
(Assignee)

Comment 4

10 years ago
Yes.  I tested this by entering a non-existing domain name in the location bar (like sgyadjfyskduykduvcgvyf.com) in order to trigger the address not found error.

Comment 5

10 years ago
Not sure if Mano is the right person, maybe you'll need a docshell peer
(Assignee)

Comment 6

10 years ago
Comment on attachment 316487 [details] [diff] [review]
Patch (v1)
[Checkin: Comment 11]

Not sure if this needs sr...
Attachment #316487 - Flags: superreview?(jst)
Attachment #316487 - Flags: review?(mano)
Attachment #316487 - Flags: review?(jst)
Comment on attachment 316487 [details] [diff] [review]
Patch (v1)
[Checkin: Comment 11]

Does this need UI review? Probably not, r+sr=jst
Attachment #316487 - Flags: superreview?(jst)
Attachment #316487 - Flags: superreview+
Attachment #316487 - Flags: review?(jst)
Attachment #316487 - Flags: review+
If that patch really works we should be able to undo the patch from bug 301119 right? Would be nice to understand what's changed... Did the fix for bug 342485 make the fix for bug 301119 unnecessary?
(Assignee)

Comment 9

10 years ago
Comment on attachment 316487 [details] [diff] [review]
Patch (v1)
[Checkin: Comment 11]

Super-low risk patch to change a favicon.  Shouldn't have any negative side effects
Attachment #316487 - Flags: approval1.9?
Comment on attachment 316487 [details] [diff] [review]
Patch (v1)
[Checkin: Comment 11]

a1.9=beltzner
Attachment #316487 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
mozilla/docshell/resources/content/netError.xhtml 	1.27

Ehsan, can you file a followup to investigate comment 8?
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Component: Theme → General
Keywords: checkin-needed
Product: Firefox → Core
QA Contact: theme → general
Resolution: --- → FIXED
Target Milestone: Firefox 3 → ---
Target Milestone: --- → mozilla1.9
(Assignee)

Comment 12

10 years ago
(In reply to comment #11)
> Ehsan, can you file a followup to investigate comment 8?

Done. -> bug 430433.

Comment 13

10 years ago
It would bave been nice to at least let us know that you busted us...
This could use some tests.  Then they could also be used to check whether bug 430433 makes sense.  The tests should test both error pages and normal pages linking to chrome:// favicons.
Flags: in-testsuite?
Flags: in-litmus?

Updated

10 years ago
Blocks: 430792

Comment 15

10 years ago
I think that, unless the regression (bug 430792) is definitely going to be fixed by RC1, this should be backed out [1]. If the choice is releasing with a platform specific icon that could appear unexpectedly in history and bookmarks - sometimes permanently [2] - or a generic icon that knows its place, we should go with the latter.

[1] It can always be reapplied when regression is fixed - ideally made part of same patch.

[2] If a location provides a site icon it should be reset on next visit but if not, and for some non-default values for site_icons and favicons preferences, the error icon will stick. I suspect, but haven't checked, that redirects and RSS feeds could also be affected.
(Assignee)

Comment 16

10 years ago
I have a patch ready for review on that bug, but it's been sitting there for a while.  I'm going to see if I can ask someone else for a more quick review.  In the mean time, it would be good if you can include this comment in that bug as well.
No longer blocks: 430792
Depends on: 430792
Depends on: 431955
(Assignee)

Updated

10 years ago
Blocks: 432938
Attachment #316487 - Attachment description: Patch (v1) → Patch (v1) [Checkin: Comment 11]
You need to log in before you can comment on or make changes to this bug.