Closed Bug 127349 Opened 23 years ago Closed 22 years ago

Remove Partial Favicon support

Categories

(SeaMonkey :: UI Design, defect)

Product:
SeaMonkey
Component:
UI Design
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: tpringle, Assigned: bugzilla)

References

(Blocks 1 open bug)

Details

(Whiteboard: [adt3])

Attachments

(1 file, 2 obsolete files)

favicon's last stand.
3.72 KB, patch
bugs
: review+
alecf
: superreview+
asa
: approval+
Details | Diff | Splinter Review 
Summary:  Remove partial support of favicons in the personal toolbar.  Specifics:

Current state of Favicon support is as follows:

1)  Favicons are picked up in the URL bar
2)  Favicons are picked up in tabbed browser tabs
3)  Favicons are picked up on a per session basis in the personal toolbar
4)  Favicons are not picked up in bookmarks elsewhere

For the purposes of our next release we should remove the support for 3) above
because I believe this will appear broken to users and ultimately be annoying as
well.  We should look at implementing items 3) and 4) at some later date.
Nominating nsbeta1 and plussing as per our earlier conversations.  Let me know
if this is a problem.
Keywords: nsbeta1+
QA Contact: sairuh → claudius
->ben
Assignee: trudelle → ben
Well, if we're sure we want to do this...

-> me
Assignee: ben → blaker
Attached patch patch (obsolete) — — Splinter Review 

    
    

    
  
Comment on attachment 72265 [details] [diff] [review]
patch

oh crap. I'll miss my partial favicons
r=hewitt

        Attachment #72265 -
        Flags: review+

    
    

    
  
fixed.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
Why are these changes necessary? There already exist two prefs for favicons and
siteicons and currently all.js contains 

pref("browser.chrome.site_icons", true);
pref("browser.chrome.favicons", false);

Isn't it possibly to disable site_icons in all.js, but still leave it alone for
those who care enough to manually re-enable them in prefs.js???

Blocks: 120352
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
No...we're not removing favicon support from the places where it works (urlbar,
tabbrowser).  We're removing it from the places that it does not work
consistently (the personal toolbar, the Bookmarks menu).  That functionality is
too broken to bring up to release-level quality, so it should be disabled for
Mozilla 1.0.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED

    
    

    
  
You've removed the coolest thing added to Mozilla since POP3 over SSL :-(

    
    

    
  
I don't want to start a flamewar here, but just wanted to say thank you. Finally
NSCPs presidency over Mozilla comes in handy.

    
    

    
  
If you're gonna comment about how bad removing the support is, don't.
I'm sure there's already a bug filed for *fully functioning favicons in these
areas*. Find it and monitor it. 

Don't spam here.

    
    

    
  
> I'm sure there's already a bug filed for *fully functioning favicons in these
> areas*.

I don't think so. There is only:
Bug 118070: "favicon of current site on personal toolbar changes when clicking
on another site in the toolbar" and
Bug 121030: "pref->apperance->show web site icons doesn't reset favicons in
personal toolbar until restart"

Both seem impossible to fix if there are no favicons in the personal toolbar to
test any patch :-(

I believe this feature can be only disabled by a default pref, something like:
pref("browser.chrome.favicons_in_personal_toolbar", false); not just removed.

Reopening, and let someone explain why I'm wrong.
 


Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
  
Blocks: 118070

    
  
Blocks: 121030

    
    

    
  
Blake already explained this, in Comment #10.  We can add them back in once we
can do it properly, which won't be until after after the upcoming releases. 
Please don't reopen this bug again, it won't do anyone any good. ->FIXED
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED

    
    

    
  
OK, bug 113574 now asks for favicons to be added back to 
- bookmarks sidebar
- manage bookmarks menu
- personal toolbar
- toolbar bookmarks menu

I set bug 113574 to block all the bugs I could find that talk about issues with
favicons in those places. I also attached the patch reversing this one (e.g. to
add favicons back to toolbars) - hopefully that could go in early in 1.1 cycle
so that there is a chance things get fixed properly.
No longer blocks: 118070, 121030

    
    

    
  
why couldn't this at least be a pref, that is off by default? Many of us love
our partial favicon support!

    
    

    
  
please please tell me I'm wrong but as I read and understand this bug, there
should be no favicons in the bookmark menus(toplevel and from the ptoolbar). In
my  2002030803 Win98 build they are most certainly there, even though they are
indeed absent from the Personal toolbar itself.

REOPENING
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
Gee, after I asked everyone nicely not to... ;-)

I also see this, using yesterday's build on Win2K.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        rest of the fix (1 line) (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #72265 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 73483 [details] [diff] [review]
rest of the fix (1 line)

sr=hewitt, but you should also remove validate="never"

        Attachment #73483 -
        Flags: superreview+

    
    

    
  
Comment on attachment 73483 [details] [diff] [review]
rest of the fix (1 line)

*sigh* for consistency's sake :)
r=alecf

I'm going to make an xpinstallable dynamic overlay one of these days that adds
all these attributes back. :)

        Attachment #73483 -
        Flags: review+

    
    

    
  
Comment on attachment 73483 [details] [diff] [review]
rest of the fix (1 line)

*sigh* for consistency's sake :)
r=alecf

I'm going to make an xpinstallable dynamic overlay one of these days that adds
all these attributes back. :)

    
    

    
  
minus per adt triage
Keywords: nsbeta1+ → nsbeta1-
Whiteboard: [

    
    

    
  
re-nominating - there's a patch in hand with reviews already, and this is a
HIGHLY visible screw-up. The bookmarks menu for goodness sake!

Keywords: nsbeta1- → nsbeta1

    
    

    
  
Comment on attachment 73483 [details] [diff] [review]
rest of the fix (1 line)

a=brendan@mozilla.org for 1.0.

/be

        Attachment #73483 -
        Flags: approval+

    
    

    
  
Let's file this under 'Do the Right Thing', ->nsbeta1+/ADT3
Keywords: nsbeta1 → nsbeta1+

    
    

    
  
fixed (again).
Status: REOPENED → RESOLVED
Closed: 23 years ago → 22 years ago
Resolution: --- → FIXED

    
    

    
  
Reopening.  I see favicons in my personal toolbar in the 2002032803 build.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
I don't see them in a nightly from today. The last commercial nightly I have (I
can't get another) is 3/26 and I don't see them there, either. Can others
comment on their findings?

    
    

    
  
No favicons in personal toolbar in build 2002032603 on Win2k.

    
    

    
  
ah! I see them - they're in bookmarks that are in subfolders in the personal toolbar
1) create a folder in your personal toolbar
2) visit a bugzilla query, and bookmark it in that folder
3) use the personal toolbar to visit that bugzilla query again


    
    

    
  
Alec, yep, I was just about to post a screenshot but crashed.  It seems that it
is fixed except for this case.

    
    

    
  


  

        Attachment #73483 -
        Attachment is obsolete: true

    
    

    
  
the bug that never dies.  can I get r/sr on this?  (removes lots of attribute
cruft, the important changes are the two to <menuitem/>)

    
    

    
  
Comment on attachment 77413 [details] [diff] [review]
favicon's last stand.

yikes. sr=alecf

        Attachment #77413 -
        Flags: superreview+

    
    

    
  
Comment on attachment 77413 [details] [diff] [review]
favicon's last stand.

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

        Attachment #77413 -
        Flags: approval+

    
    

    
  
adt3
Whiteboard: [ → [adt3]

    
    

    
  
Am I right assuming that the attachment 77413 [details] [diff] [review] will only go into the branch? When
will it be OK to put favicons back on the trunk (see bug 113574 for the patch)
so that we can try to figure out how to do them correctly?

    
    

    
  
fixed on branch and trunk.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED

    
    

    
  
adding fixed1.0.0 keyword (branch resolution). This bug has comments saying it
was fixed on the 1.0 branch and a bonsai checkin comment that agrees. To verify
the bug has been fixed on the 1.0 branch please replace the fixed1.0.0 keyword
with verified1.0.0.
Keywords: fixed1.0.0

    
    

    
  
The "put them back in" is now bug 143687.

    
    

    
  
favicons appear in tabs and the urlbar. they don't appear in the PT or bookmarks
menu (as expected with current implementation). tested with 2003.02.19 comm
trunk builds.
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: