Closed Bug 432938 Opened 16 years ago Closed 16 years ago

Use GTK stock icon for netError favicon instead of Windows icon

Categories

(Firefox :: Theme, defect)

Product:
Firefox
Component:
Theme
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 3.6a1

People

(Reporter: reed, Assigned: sdwilsh)

References

Details

(Keywords: fixed1.9.1, polish)

Attachments

(2 files, 2 obsolete files)

screenshot
195.49 KB, image/png
Details
v3.0
894 bytes, patch
Gavin
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
Attached patch patch - v1 (backed out) (obsolete) — — Splinter Review 
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)
Summary: Use Tango icon for netError favicon → Use GTK stock icon for netError favicon instead of Windows icon
Blocks: 425582, 428923
Attachment #320105 - Flags: review?(dietrich)
Depends on: 429721
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?
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+
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?
Keywords: dev-doc-needed
Comment on attachment 320105 [details] [diff] [review]
patch - v1 (backed out)

a+ for GTK fix
Attachment #320105 - Flags: approval1.9? → approval1.9+
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
Closed: 16 years ago
Resolution: --- → FIXED
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 → ---
Attachment #320105 - Attachment description: patch - v1 → patch - v1 (backed out)
Whiteboard: [not needed for 1.9]
Attached patch patch - v2 (obsolete) — — Splinter Review 
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)
Attachment #343834 - Flags: review+
Status: REOPENED → ASSIGNED
Whiteboard: [not needed for 1.9]
Target Milestone: Firefox 3 → Firefox 3.1b2
Keywords: polish
Whiteboard: [polish-easy] [polish-visual]
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
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
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!
With the override suggestion, it gets no favicon.  Investigating why...
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...
Depends on: 466582
Attached image 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)
Attached patch v3.0 — — Splinter Review 

        Attachment #349896 -
        Flags: review?(gavin.sharp)

    
  
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+

    
  
Whiteboard: [has patch][needs review gavin][polish-easy][polish-visual] → [has patch][has review][polish-easy][polish-visual]

    
  

        Attachment #349896 -
        Flags: approval1.9.1?

    
  
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+

    
  
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]

    
    

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

This cannot land on branch until the dependent bug lands there.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago → 16 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

    
    

    
  
I had to back this out because the dependent bug got backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/42795e157035
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED

    
    

    
  
(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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: