Closed Bug 113202 Opened 23 years ago Closed 19 years ago

site icon loads in tab even if tab doesn't

Categories

(SeaMonkey :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mozilla, Assigned: neil)

References

(Blocks 1 open bug, )

Details

(Keywords: polish)

Attachments

(1 file, 8 obsolete files)

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?
QA Contact: sairuh → claudius
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.
OS: Linux → All
Hardware: PC → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Proposed patch (obsolete) (deleted) — Splinter Review
Reassigning to current owner.
Assignee: hyatt → jaggernaut
Keywords: patch, polish, review
QA Contact: claudius → sairuh
Attached patch New patch fixes the urlbar as well (obsolete) (deleted) — Splinter Review
Attachment #86392 - Attachment is obsolete: true
+ 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
Assignee: jaggernaut → neil
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).
Attachment #89901 - Flags: needs-work+
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 :-)
Attached patch Addressed reviewers' comments (obsolete) (deleted) — Splinter Review
Attachment #89901 - Attachment is obsolete: true
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=.
Attached patch bitrot (obsolete) (deleted) — Splinter Review
I think there was also another problem with my previous patch - I was referencing an undeclared variable mBrowser, so I fixed that too.
Attachment #90119 - Attachment is obsolete: true
Attached patch bitrot² (obsolete) (deleted) — Splinter Review
/me sighs
Attachment #94333 - Attachment is obsolete: true
Attached patch Really updated for bitrot this time (obsolete) (deleted) — Splinter Review
Spotted a couple of typos.
Attachment #94475 - Attachment is obsolete: true
Attachment #94620 - Flags: review+
Attached patch New bugfix (obsolete) (deleted) — Splinter Review
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.
Attachment #94620 - Attachment is obsolete: true
Blocks: 120352
CCing jag for r/sr
QA Contact: sairuh → pmac
Comment on attachment 94708 [details] [diff] [review] New bugfix Incidentally this patch also fixes bug 109959.
Blocks: 109959
Attachment #94708 - Flags: superreview?(jaggernaut)
Attachment #94708 - Flags: review?(timeless)
Attachment #94708 - Flags: review?(timeless) → review+
> Incidentally this patch also fixes bug 109959. Not fixed yet. Why?
Comment on attachment 94708 [details] [diff] [review] New bugfix Patch has bitrotted.
Attachment #94708 - Flags: superreview?(jag)
Attachment #94708 - Flags: review-
Attachment #94708 - Flags: review+
Attached patch Updated for bitrot (obsolete) (deleted) — Splinter Review
navigator.js hadn't bitrotted but the other two files have changed alot. I haven't actually tested this yet...
Attachment #94708 - Attachment is obsolete: true
Attached patch Updated tabbrowser changes (deleted) — Splinter Review
The other bits might have been to fix bug 109959, but I'm not sure offhand.
Attachment #144993 - Attachment is obsolete: true
Attachment #187131 - Flags: superreview?(jag)
Attachment #187131 - Flags: review?(timeless)
Attachment #187131 - Flags: review?(timeless) → review+
Attachment #187131 - Flags: superreview?(jag) → superreview+
Attachment #187131 - Flags: approval1.8b3?
Attachment #187131 - Flags: approval1.8b3? → approval1.8b3+
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.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: