395423 - Favicon in location bar can get out of sync with actual page favicon when closing or switching tabs while editing location bar

Status: RESOLVED FIXED

(Camino Graveyard :: Location Bar & Autocomplete, defect)

Description

[Chris Lawson (gone)]

STR with latest trunk builds (haven't tried with 1.5 yet):

1) Open, for example, a Bugzilla page and a page on another host with a different favicon.
2) Edit text in location bar, for example by selecting everything and hitting the space bar.
3) Close tab.
4) Do something to remove focus from location bar or hit Escape to cancel edit in location bar. Notice that favicon from site 2 persists despite URL of site 1 now being (properly) present in location bar. This favicon will also be part of the drag image if you drag from the location bar (although the proper favicon will be given to the drag destination where applicable).

Not a big deal, but potentially a source of user confusion, especially when the favicon in the tab doesn't match the one in the location bar.

Comment 1

[Chris Lawson (gone)]

Yeah, this happens in 1.5 as well, so both branch and trunk.

Comment 2

[Chris Lawson (gone)]

Heh. Step 3 can also be "change tabs"; you don't even have to close a tab to get it to happen as long as there's an active edit in the location bar when you change tabs.

Note to self: we should definitely update the favicon when undoing an edit

http://mxr.mozilla.org/mozilla/source/camino/src/browser/AutoCompleteTextField.mm#836

and we should probably update it on didEndEditing as well (which should catch the "something to remove focus" case, such as clicking in the content area).

I would also go so far as to argue we should update it whenever the tab changes, since we do that with the RSS feed indicator, the lock icon, and the secure background colour, and the favicon is at least as much a part of a site's "identity" as the two https indications are.

Comment 3

[Chris Lawson (gone)]

Attachment: fix

Nick, do you mind taking a look at this? (It also incorporates the change from the feed bug and its associated whitespace fixes, so the BrowserWrapper changes look a lot bigger than normal.)

Comment 4

[Chris Lawson (gone)]

smfr, is there a historical reason we had the ignoreTyping: parameter on the updateSiteIcons: method? It seemed to me that the only thing that parameter was doing was allowing this bug to happen ;) so I just removed it.

Comment 5

[Simon Fraser [no longer active]]

Alas my checkin comment was not useful. I don't recall what ignoreTyping: was for.

Comment 6

[Chris Lawson (gone)]

Attachment: fix v1 with less epic bitrot

Bitrotted by other landings. Here's an updated patch against today's CVS.

Comment 7

[Chris Lawson (gone)]

Attachment: fix v1.1

Whoops. Forgot one of the important changes. That'll teach me to ensure patches build :-p

Comment 8

[Chris Lawson (gone)]

Comment on attachment 324477 [details] [diff] [review]
fix v1.1

Asking for sr from pink. Dunno if Nick is going to get to this anytime soon, but at least it has one review.

Comment 9

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 324477 [details] [diff] [review]
fix v1.1

sr=pink

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk.