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

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

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

Comment 1

[Justin Dolske [:Dolske]]

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.

Comment 2

[(no longer active)]

Attachment: Patch (v1) [Checkin: Comment 11]

Am I missing something here?  This patch seems to work beautifully (on Vista at least).

Comment 3

[Justin Dolske [:Dolske]]

Does it work even when the file is packaged in chrome?

Comment 4

[(no longer active)]

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

[Stefan [:stefanh]]

Not sure if Mano is the right person, maybe you'll need a docshell peer

Comment 6

[(no longer active)]

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

Not sure if this needs sr...

Comment 7

[Johnny Stenback (:jst)]

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

Does this need UI review? Probably not, r+sr=jst

Comment 8

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 9

[(no longer active)]

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

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a1.9=beltzner

Comment 11

[:Gavin Sharp [email: [e-mail]]]

mozilla/docshell/resources/content/netError.xhtml 	1.27

Ehsan, can you file a followup to investigate comment 8?

Comment 12

[(no longer active)]

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

Done. -> bug 430433.

Comment 13

[[e-mail]]

It would bave been nice to at least let us know that you busted us...

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[John P Baker]

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.

Comment 16

[(no longer active)]

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.