113202 - site icon loads in tab even if tab doesn't

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Chris Abbey]

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.

Comment 1

[R.K.Aa.]

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?

Comment 2

[Chris Abbey]

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.

Comment 3

[[e-mail]]

Attachment: Proposed patch

Comment 4

[[e-mail]]

Reassigning to current owner.

Comment 5

[[e-mail]]

Attachment: New patch fixes the urlbar as well

Comment 6

[timeless]

+            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 7

[jag (Peter Annema)]

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 8

[[e-mail]]

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 :-)

Comment 9

[[e-mail]]

Attachment: Addressed reviewers' comments

Comment 10

[jag (Peter Annema)]

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?

Comment 11

[jag (Peter Annema)]

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=.

Comment 12

[[e-mail]]

Attachment: bitrot

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

Comment 13

[[e-mail]]

Attachment: bitrot²

/me sighs

Comment 14

[[e-mail]]

Attachment: Really updated for bitrot this time

Spotted a couple of typos.

Comment 15

[[e-mail]]

Attachment: New bugfix

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.

Comment 16

[[e-mail]]

CCing jag for r/sr

Comment 17

[[e-mail]]

Comment on attachment 94708 [details] [diff] [review]
New bugfix

Incidentally this patch also fixes bug 109959.

Comment 18

[aynilove [So, Jae-yoon]]

> Incidentally this patch also fixes bug 109959.

Not fixed yet. Why?

Comment 19

[Ali Ebrahim]

Comment on attachment 94708 [details] [diff] [review]
New bugfix

Patch has bitrotted.

Comment 20

[[e-mail]]

Attachment: Updated for bitrot

navigator.js hadn't bitrotted but the other two files have changed alot.

I haven't actually tested this yet...

Comment 21

[[e-mail]]

Attachment: Updated tabbrowser changes

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

Comment 22

[Bradley Chapman (not reading bugmail, still gone but not forever)]

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

Comment 23

[Chris Abbey]

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.)

Comment 24

[Bradley Chapman (not reading bugmail, still gone but not forever)]

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.

Comment 25

[Bradley Chapman (not reading bugmail, still gone but not forever)]

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.

Comment 26

[Peter van der Woude [:Peter6]]

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 ?

Comment 27

[[e-mail]]

(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.