Closed Bug 113202 Opened 19 years ago Closed 15 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) — Splinter Review
Reassigning to current owner.
Assignee: hyatt → jaggernaut
Keywords: patch, polish, review
QA Contact: claudius → sairuh
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) — 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) — 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) — Splinter Review
/me sighs
Attachment #94333 - Attachment is obsolete: true
Spotted a couple of typos.
Attachment #94475 - Attachment is obsolete: true
Attachment #94620 - Flags: review+
Attached patch New bugfix (obsolete) — 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) — 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
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: 15 years ago
Resolution: --- → FIXED
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.