The URL listed regularly fails to load the first time and pops up the standard dns failure message, but if you are trying to load it in a tab it will still replace the tab's site icon - but not the title, nor the content, and not the url bar contents, ONLY the tab icon. A second load attempt works just fine. I belive the failure of the first request is due to the site having a short dns ttl value, and it taking longer than the standard timeout to lookup the address. Once it's cached in our local server it will load successfully until the ttl field expires again and requires a new authoratative lookup be performed.

Since this is filed under tabbed browser: Are you requesting site icons to be removed from tabs when DNS error/s occur during page-load?

Yes, I suppose that would be one way to word it. If the page itself isn't going to load, then we do not change the tab title, nor the url bar content, nor the url bar icon; but the tab bar icon does change. I view that as a bug, and is what I intended this bug for. The existing icon (which matches the tab title, url bar icon and url bar content) should remain in place. Aslo changed to all/all as I very highly doubt this is unique to linux.

Reassigning to current owner.

+ if (!aURI) + return null; + if (!("schemeIs" in aURI)) + return null; + if (!(aURI.schemeIs("http") || aURI.schemeIs("https"))) + return null; + if (!this.mPrefs.getBoolPref("browser.chrome.site_icons")) + return null; + if (!this.mPrefs.getBoolPref("browser.chrome.favicons")) + return null; imo this can be handled as one if, or something. how about: if (!(aURI && "schemeIs" in aURI && (aURI.schemeIs("http") || aURI.schemeIs("https")) && this.mPrefs.getBoolPref("browser.chrome.site_icons")) && this.mPrefs.getBoolPref("browser.chrome.favicons") ) ) return null; nit, you're adding an extra blank line (w/ whitespace on it) at the end of @@ -592,8 +589,11 @@ please address those for r=timeless

Comment on attachment 89901 [details] [diff] [review] New patch fixes the urlbar as well It looks like this patch contains a few other fixes as well. shouldLoadFavIcon could be given a more appropriate name, like "getFavIconURL". Then again, you're now duplicating the logic of |if (iconURL) someElt.setAttribute("image", iconURL)|. How about "updateFavIcon(uri, element)"? In addTab you still want to enterTabbedMode (which does the fixing up of the first browser/listener pair) instead of just showing the tab bar. And like timeless said, keep the now-merged shouldLoadFavIcon logic as one condition, not a bunch of |return null|s. I'll take another look at this patch or an update later today (hopefully).

Comment on attachment 89901 [details] [diff] [review] New patch fixes the urlbar as well >- if (gBrowser.shouldLoadFavIcon(aURI)) { >- var favStr = gBrowser.buildFavIconString(aURI); >- if (favStr != gProxyFavIcon.src) { >- gBrowser.loadFavIcon(aURI, "src", gProxyFavIcon); >- gProxyDeck.selectedIndex = 0; >- } >- else gProxyDeck.selectedIndex = 1; >+ var favStr = gBrowser.shouldLoadFavIcon(aURI); >+ if (favStr) { >+ if (favStr != gProxyFavIcon.src) >+ gProxyFavIcon.setAttribute("src", favStr); >+ gProxyDeck.selectedIndex = 1; I didn't want to lose anything while converting this bit of code. I'll rename the function to buildFavIconString which might be clearer. >+ if (!aURI) >+ return null; >+ if (!("schemeIs" in aURI)) >+ return null; >+ if (!(aURI.schemeIs("http") || aURI.schemeIs("https"))) >+ return null; >+ if (!this.mPrefs.getBoolPref("browser.chrome.site_icons")) >+ return null; >+ if (!this.mPrefs.getBoolPref("browser.chrome.favicons")) >+ return null; I honestly thought that it would be clearer this way. >- if (!this.mTabbedMode) >- this.enterTabbedMode(); >+ if (this.mStrip.collapsed) >+ this.setStripVisibilityTo(true); >+ >+ this.mPrefs.setBoolPref("browser.tabs.forceHide", false); >+ Just for the record, this.setStripVisibilityTo(true) contains if (!this.mTabbedMode) this.enterTabbedMode(); anyway. If only PatchMaker could manage my multiple patches to the same file more effectively :-)

I thought enterTabbedMode would also show the strip. After reading the code once again I'm actually fine with you changing that to setStripVisibility(true) and removing the explicit enterTabbedMode. Is there a bug yet on making stripVisibility a property instead of explitic getters and setters?

Explicit even. I like your patch so far, let me see it in action and look at it a bit more before I give sr=.

I think there was also another problem with my previous patch - I was referencing an undeclared variable mBrowser, so I fixed that too.

/me sighs

Spotted a couple of typos.

As part of ongoing testing for this feature I discovered that I had erroneously uncorrected the correct code path for clearing the proxy state; that old code path has now been reinstated and the incorrect code path has now been corrected.

CCing jag for r/sr

Comment on attachment 94708 [details] [diff] [review] New bugfix Incidentally this patch also fixes bug 109959.

> Incidentally this patch also fixes bug 109959. Not fixed yet. Why?

Comment on attachment 94708 [details] [diff] [review] New bugfix Patch has bitrotted.

navigator.js hadn't bitrotted but the other two files have changed alot. I haven't actually tested this yet...

The other bits might have been to fix bug 109959, but I'm not sure offhand.

The URL in the URL field no longer replaces the tab icon in a tab when first loaded. VERIFIED FIXED? Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050819 Firefox/1.0+ ID:2005081905

For me at least that url is currently unable to be resolved at all (and has been for a long time). So I'm not willing to accept any test based on it. Try with this new URL. An easier recreation method that doesn't rely on a wonky server: 1. bookmark a url that has a favicon, preferably one that's slow to load (such as TWOP) 2. load the url from the bookmark somewhere (allow it to finish, just want to prime the icon into the cache) 3. load somethign else in a tab (allow it to finish as well) 4. click the bookmark above and as soon as the favicon is changed to the reload arrows hit esc to cancel the load. 5. observe that the urlbar, tab title, tab content have all returned to the previous state, (or been left in the previous state as the case with the contents) However, note that the favicon when it stops the reload circles is the new locations icon. (I've not yet gotten a 1.8 build going, so I can't currently confirm this.)

I can't reproduce this - I suspect that the speed of the connection is responsible for covering up whatever residual race condition you may be experiencing.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050827 Firefox/1.0+ ID:2005082705 Still can't reproduce with comment #23's steps. I really think this has been fixed.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050915 Firefox/1.4 ID:2005091504 WFM can this bug be resolved/fixed - fixed1.8 ?

(In reply to comment #21) >Created an attachment (id=187131) [edit] >Updated tabbrowser changes > >The other bits might have been to fix bug 109959, but I'm not sure offhand. Actually it was bug 114548, so I can close this bug now.