Use GTK stock icon for netError favicon instead of Windows icon

RESOLVED FIXED in Firefox 3.6a1

Status

()

Firefox
Theme
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: reed, Assigned: sdwilsh)

Tracking

({fixed1.9.1, polish})

Trunk
Firefox 3.6a1
x86
Linux
fixed1.9.1, polish
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
Created attachment 320105 [details] [diff] [review]
patch - v1 (backed out)

Until bug 429721 came along, the favicon for the netError pages was the same for all platforms. Now, it can be customized fairly easily. Do that for Tango.
Attachment #320105 - Flags: superreview?(jst)
Attachment #320105 - Flags: review?(jst)
Attachment #320105 - Flags: review?(gavin.sharp)
(Reporter)

Updated

10 years ago
Summary: Use Tango icon for netError favicon → Use GTK stock icon for netError favicon instead of Windows icon
(Reporter)

Updated

10 years ago
Blocks: 425582, 428923
(Reporter)

Updated

10 years ago
Attachment #320105 - Flags: review?(dietrich)

Updated

10 years ago
Depends on: 429721

Comment 1

10 years ago
The docs need to be updated if this lands.  See bug 430792 comment 22, 24 and 26.  I wonder if it would be better to use a redirect rule to use moz-icon://stock/gtk-dialog-warning?size=menu instead of chrome://global/skin/icons/warning-16.png in the manifest file for gnomestripe?

Updated

10 years ago
Attachment #320105 - Flags: superreview?(jst)
Attachment #320105 - Flags: superreview+
Attachment #320105 - Flags: review?(jst)
Attachment #320105 - Flags: review+
Comment on attachment 320105 [details] [diff] [review]
patch - v1 (backed out)

r=me for the places bit
Attachment #320105 - Flags: review?(dietrich) → review+
(Reporter)

Comment 3

10 years ago
Comment on attachment 320105 [details] [diff] [review]
patch - v1 (backed out)

Replaces another use of a Windows icon with a GTK stock icon.
Attachment #320105 - Flags: review?(gavin.sharp) → approval1.9?

Updated

10 years ago
Keywords: dev-doc-needed

Comment 4

10 years ago
Comment on attachment 320105 [details] [diff] [review]
patch - v1 (backed out)

a+ for GTK fix

Updated

10 years ago
Attachment #320105 - Flags: approval1.9? → approval1.9+
(Reporter)

Comment 5

10 years ago
Checking in docshell/resources/content/jar.mn;
/cvsroot/mozilla/docshell/resources/content/jar.mn,v  <--  jar.mn
new revision: 1.3; previous revision: 1.2
done
Checking in docshell/resources/content/netError.xhtml;
/cvsroot/mozilla/docshell/resources/content/netError.xhtml,v  <--  netError.xhtml
new revision: 1.33; previous revision: 1.32
done

Seems I accidentally landed the toolkit part of this last night at 2008-05-09 00:25. I'll file a bug to get the commit message fixed. :(
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Depends on: 433241
Backed out per schrep/beltzner, to fix 433241.

mozilla/docshell/resources/content/jar.mn 	1.4
mozilla/docshell/resources/content/netError.xhtml 	1.34
mozilla/toolkit/components/places/src/nsFaviconService.h 	1.13 
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: dev-doc-needed
Flags: in-testsuite?

Updated

10 years ago
Attachment #320105 - Attachment description: patch - v1 → patch - v1 (backed out)
Whiteboard: [not needed for 1.9]
(Reporter)

Comment 7

9 years ago
Created attachment 343834 [details] [diff] [review]
patch - v2

Same as v1 but adds Kai Liu's fix in bug 433241 to deal with the regression it caused. Carrying over review for v1, and just asking for review on Kai's part.
Attachment #320105 - Attachment is obsolete: true
Attachment #343834 - Flags: superreview+
Attachment #343834 - Flags: review?(gavin.sharp)
(Reporter)

Updated

9 years ago
Attachment #343834 - Flags: review+
(Reporter)

Updated

9 years ago
Status: REOPENED → ASSIGNED
Whiteboard: [not needed for 1.9]
Target Milestone: Firefox 3 → Firefox 3.1b2
Keywords: polish
Whiteboard: [polish-easy] [polish-visual]
(Assignee)

Comment 8

9 years ago
Out of curiosity, could we not do a chrome override for that image in the linux theme?  If it works, this is a really simple patch that doesn't touch code everywhere else...

  override chrome://global/skin/icons/warning-16.png moz-icon://stock/gtk-dialog-warning?size=menu

Comment 9

9 years ago
I seem to remember that chrome overrides wouldn't work with moz-icon, insisting it was not a local file.  I have no idea in which bug I discovered that though
(Assignee)

Comment 10

9 years ago
Is moz-icon ever not a local file?  It seems like that's the cleaner/better solution here.
I don't see anything in the chrome registry code which would cause that to fail, after a quick look. Worth a try!
(Assignee)

Comment 12

9 years ago
With the override suggestion, it gets no favicon.  Investigating why...
(Assignee)

Comment 13

9 years ago
It does, in fact, fail a security check:
WARNING: Remote chrome not allowed! Only file:, resource:, jar:, and cached chrome channels are valid.
: file /home/sdwilsh/central/chrome/src/nsChromeProtocolHandler.cpp, line 582

Looking into if we can make an exception for moz-icon stuff...
(Assignee)

Updated

9 years ago
Depends on: 466582
(Assignee)

Comment 14

9 years ago
Created attachment 349894 [details]
screenshot

Got this working with the patch in bug 466582 the patch I'm about to upload.
Assignee: reed → sdwilsh
Attachment #343834 - Attachment is obsolete: true
Attachment #343834 - Flags: review?(gavin.sharp)
(Assignee)

Comment 15

9 years ago
Created attachment 349896 [details] [diff] [review]
v3.0
Attachment #349896 - Flags: review?(gavin.sharp)
(Assignee)

Updated

9 years ago
Whiteboard: [polish-easy] [polish-visual] → [has patch][needs review gavin][polish-easy][polish-visual]
Target Milestone: Firefox 3.1b2 → Firefox 3.1
Attachment #349896 - Flags: review?(gavin.sharp) → review+
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review gavin][polish-easy][polish-visual] → [has patch][has review][polish-easy][polish-visual]
(Assignee)

Updated

9 years ago
Attachment #349896 - Flags: approval1.9.1?
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][has review][polish-easy][polish-visual] → [has patch][has review][needs approval][needs dependent bug][polish-easy][polish-visual]
Comment on attachment 349896 [details] [diff] [review]
v3.0

a191=beltzner
Attachment #349896 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][has review][needs approval][needs dependent bug][polish-easy][polish-visual] → [has patch][has review][has approval][needs dependent bug][polish-easy][polish-visual]
(Assignee)

Comment 17

9 years ago
http://hg.mozilla.org/mozilla-central/rev/3744204c1576

This cannot land on branch until the dependent bug lands there.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has approval][needs dependent bug][polish-easy][polish-visual]
Target Milestone: Firefox 3.1 → Firefox 3.2a1
(Assignee)

Comment 18

9 years ago
I had to back this out because the dependent bug got backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 19

9 years ago
http://hg.mozilla.org/mozilla-central/rev/42795e157035
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 20

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/50b90e593abd
Keywords: fixed1.9.1
(In reply to comment #11)
> I don't see anything in the chrome registry code which would cause that to
> fail, after a quick look. Worth a try!

Rather conveniently, *stripe doesn't count as a theme, and is therefore allowed to do chrome overrides, unlike 3rd party themes...
You need to log in before you can comment on or make changes to this bug.