Closed Bug 429721 Opened 16 years ago Closed 16 years ago

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


(Core :: General, defect)

Not set





(Reporter: faaborg, Assigned: ehsan.akhgari)




(1 file)

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 → ---
Am I missing something here?  This patch seems to work beautifully (on Vista at least).
Attachment #316487 - Flags: review?(mano)
Target Milestone: --- → Firefox 3
Does it work even when the file is packaged in chrome?
Yes.  I tested this by entering a non-existing domain name in the location bar (like in order to trigger the address not found error.
Not sure if Mano is the right person, maybe you'll need a docshell peer
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?
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]

Attachment #316487 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
mozilla/docshell/resources/content/netError.xhtml 	1.27

Ehsan, can you file a followup to investigate comment 8?
Closed: 16 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
(In reply to comment #11)
> Ehsan, can you file a followup to investigate comment 8?

Done. -> bug 430433.
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?
Blocks: 430792
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.
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
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.