571812 - Remove setIcon

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Since mIconURL support has been added to browser XBL, we can remove setIcon. I don't see any need for it.

I removed it and tested loading pages without favicons, pages with favicons, closing tabs and switching tabs. Everything works.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

forgot to add the patch!

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #0)
> Since mIconURL support has been added to browser XBL, we can remove setIcon. I
> don't see any need for it.
> 
> I removed it and tested loading pages without favicons, pages with favicons,
> closing tabs and switching tabs. Everything works.

I suspect this to be needed for background loading tabs. I'll apply the patch and see what happen.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

(In reply to comment #2)
> (In reply to comment #0)
> > Since mIconURL support has been added to browser XBL, we can remove setIcon. I
> > don't see any need for it.
> > 
> > I removed it and tested loading pages without favicons, pages with favicons,
> > closing tabs and switching tabs. Everything works.
> 
> I suspect this to be needed for background loading tabs. I'll apply the patch
> and see what happen.

It work well for me!

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/2935fc0e9bb7

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

We need to backout the patch I think. :(

Steps to reproduce: 
 * start fennec on about:home
 * go to google.com
 * use the back button to return to about:home
 * use the forward button to return to google.com

Actual result:
 * the favicon is missing

Reverting to a revision before this one make it works. Sorry for have not noticed that sooner.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

I'll try to fix it first

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: followup fix

This followup patch moves the fallback code out of DOMContentLoaded and into pageshow message. DOMContentLoaded does not fire when pages are loaded from cache.

I removed the message handler for DOMContentLoaded since it is no longer used.

Comment 8

[Catalin Suciu [:csuciu]]

Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110921
Firefox/9.0a1 Fennec/9.0a1
Device: HTC Desire
OS: Android 2.2