Closed Bug 869500 Opened 11 years ago Closed 10 years ago

Customization mode should have a favicon

Categories

(Firefox :: Theme, enhancement)

Product:
Firefox
Component:
Theme
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 29

People

(Reporter: u428464, Assigned: dhenein)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P5] )

Attachments

(3 files, 4 obsolete files)

customizeFavicon.ico
1.06 KB, image/x-icon
Details
bug-869500-customizeFavicon.patch v3
6.46 KB, patch
MattN
: review+
Details | Diff | Splinter Review
also add Australis customization favicon in aero,
1.89 KB, patch
MattN
: review+
Details | Diff | Splinter Review 
As customization mode is one of Firefox's in-content pages it should have a favicon (like a wrench and a hammer).
Blocks: 869104
No longer blocks: 869104
Whiteboard: [Australis:M7]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [Australis:M7] → [Australis:M8]
Blocks: 750106
Whiteboard: [Australis:M8] → [Australis:M?]
Related: bug 750106
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P5]
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M?][Australis:P3]
Mike, why did you make this a P3 rather than P5? I would happily ship without a favicon, if it wasn't for all the other bugs, tbh... :-)
Flags: needinfo?(mdeboer)
I may be wrong but I think that most visual design changes are now P3.
Nope, we're still following the priorities from the top of https://people.mozilla.org/~mnoorenberghe/australis/?resolved=0
IIRC it was a decision from UX.
Assignee: nobody → mmaslaney
Would take a patch, but something this minor is a P5.
Flags: needinfo?(mdeboer)
Whiteboard: [Australis:M?][Australis:P3] → [Australis:P5]
Blocks: fxdesktopbacklog
Whiteboard: [Australis:P5] → [Australis:P5] [feature] p=0
(In reply to mmaslaney from comment #7)
> Created attachment 8344098 [details]
> Customize-Favicon.zip

Can we get these icons as transparent pngs?
Flags: needinfo?(mmaslaney)
Attached file Customize-Favicon.zip (obsolete) — 
PNG format.
Flags: needinfo?(mmaslaney)
Assignee: mmaslaney → dhenein
Added customization favicon.
Attachment #8363196 - Flags: review?(jaws)
Comment on attachment 8363196 [details] [diff] [review]
bug-869500-customizeFavicon.patch v1

Review of attachment 8363196 [details] [diff] [review]:
-----------------------------------------------------------------

We need to use an ICO file that has the two icon sizes contained so that the browser can pick the size that is better suited for display (scaling the 32x32 icon down to 16x16 won't look as nice). I've created the ICO file and will attach it to this bug next.

Please place the icon file in the browser/themes/*/customizableui/ directory. You can then modify browser/themes/*/jar.mn to reference these files. The path to the file will then be chrome://browser/skin/customizableui/customizeFavicon.ico.
Attachment #8363196 - Flags: review?(jaws) → review-
Attached image customizeFavicon.ico — 

        Attachment #8344098 -
        Attachment is obsolete: true

        Attachment #8363011 -
        Attachment is obsolete: true
Status: NEW → ASSIGNED

    
    

    
  
Erm, actually can you place the ICO file in the /browser/themes/shared/customizableui/ directory? Since it is the same for all platforms we don't need duplicate copies in the source control.

    
    

    
  


  
Now using customizeFavicon.ico. Updated corresponding jar.mn files.

        Attachment #8363196 -
        Attachment is obsolete: true

        Attachment #8363699 -
        Flags: review?(jaws)

    
    

    
  
Comment on attachment 8363699 [details] [diff] [review]
bug-869500-customizeFavicon.patch v2

>+    <link rel="icon" type="image/png" id="favicon"
>+          href="chrome://browser/skin/customizableui/customizeFavicon.ico"/>

This element needs no id.

    
    

    
  
Comment on attachment 8363699 [details] [diff] [review]
bug-869500-customizeFavicon.patch v2

Review of attachment 8363699 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/content/aboutCustomizing.xhtml
@@ +17,5 @@
>  <html xmlns="http://www.w3.org/1999/xhtml"
>        disablefastfind="true">
>  	<head>
>  		<title>&customizeMode.tabTitle;</title>
> +    <link rel="icon" type="image/png" id="favicon"

This type attribute is now incorrect (maybe image/x-icon instead) Is it even necessary? Drop the id like Dão said.

Also fix the indentation so this lines up with <title>

::: browser/components/customizableui/content/jar.mn
@@ -7,5 @@
>    content/browser/customizableui/panelUI.css
>    content/browser/customizableui/panelUI.js
>    content/browser/customizableui/panelUI.xml
>    content/browser/customizableui/toolbar.xml
> -

Nit: revert this change:
hg revert -r tip^ browser/components/customizableui/content/jar.mn

        Attachment #8363699 -
        Flags: review?(jaws) → review-

    
    

    
  


  
Thanks for the review! Changes applied.

        Attachment #8363699 -
        Attachment is obsolete: true

        Attachment #8363788 -
        Flags: review?(MattN+bmo)

    
    

    
  
Comment on attachment 8363788 [details] [diff] [review]
bug-869500-customizeFavicon.patch v3

Review of attachment 8363788 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your help! Looks good to me.

Pushed: https://hg.mozilla.org/integration/fx-team/rev/0c921868e9c5

        Attachment #8363788 -
        Flags: review?(MattN+bmo) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/0c921868e9c5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29

    
    

    
  
Is the favicon supposed to take time before appearing ? Because I see a blank space where it should be ?

    
    

    
  
(In reply to Guillaume C. [:ge3k0s] from comment #21)
> Is the favicon supposed to take time before appearing ? Because I see a
> blank space where it should be ?

I think this patch should have had a line for aero, and didn't.

    
  

        Attachment #8365142 -
        Flags: review?(MattN+bmo) → review+
No longer blocks: fxdesktopbacklog
Whiteboard: [Australis:P5] [feature] p=0 → [Australis:P5]

    
  
QA Contact: cornel.ionce

    
    

    
  
Verified that the proposed fiveicon is present for customization in latest Aurora on Windows 8 32bit, Windows 7 64bit, Ubuntu 13.10 64bit, Mac OS X 10.8.5.
Status: RESOLVED → VERIFIED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: