455877 - port bug 254714 to SeaMonkey (blank location bar while loading page in a new tab/window)

Status: RESOLVED WORKSFORME

(SeaMonkey :: Location Bar, defect)

Description

[Misak Khachatryan]

Bug 254714 exist long time ago and now fixed. This bug for porting it to Seamonkey. Thanks to Philip Chee for suggestion.

Comment 1

[Misak Khachatryan]

Attachment: WIP, works ok, some things to clear

This patch does his,job. It needs patch from bug 425480 to be applied first.
One thing to make clear - i don't know what to do with gURIFixup removal and make it local variable part from original patch. I need suggestions here.

Comment 2

[Misak Khachatryan]

Attachment: fixes bad behavior

This one fixed case when user needs hit ESC twice to reset URL in location bar. Also i noticed some good (for me) change in Location bar. When user starts typing - favicon not going to default, but resets to blank. When user hits ESC favicon updated to loaded page's, in contrast of staying default.

Comment 3

[Misak Khachatryan]

Attachment: updated patch against current tree

I've updated patch against current tree with some minor changes.

Comment 4

[[e-mail]]

Comment on attachment 360694 [details] [diff] [review]
updated patch against current tree

>+  if (!gURLBar)
>+    return;
Don't need this, we always have a URL bar

>-    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;
>-    }
>-    else {
>-      gProxyDeck.selectedIndex = 0;
>-      gProxyFavIcon.removeAttribute("src");
>-    }
>+
>+    PageProxySetIcon(gBrowser.mCurrentBrowser.mIconURL);
>   } else if (aState == "invalid") {
>     gURLBar.removeEventListener("input", UpdatePageProxyState, false);
>-    gProxyDeck.selectedIndex = 0;
>+    PageProxyClearIcon();
Not sure what the point of these changes is.

>+    if (gURLBar &&
>+        gURLBar.value == "" &&
>+        getWebNavigation().currentURI.spec == "about:blank")
>+      URLBarSetURI(uri);
Won't this set the page proxy state to valid? Not sure that's a good thing, since there are some cases where we never get a location change (which is what this code is trying to work around, I guess.)

Comment 5

[Philip Chee]

>>-    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;
>>-    }
>>-    else {
>>-      gProxyDeck.selectedIndex = 0;
>>-      gProxyFavIcon.removeAttribute("src");
>>-    }
>>+
>>+    PageProxySetIcon(gBrowser.mCurrentBrowser.mIconURL);
>>   } else if (aState == "invalid") {
>>     gURLBar.removeEventListener("input", UpdatePageProxyState, false);
>>-    gProxyDeck.selectedIndex = 0;
>>+    PageProxyClearIcon();
> Not sure what the point of these changes is.

The tabbrowser code already does*almost* all that the deleted code does when it updates .mIconURL. So here we just catch a free ride by using whatever is in .mIconURL. I say *almost* because see below:

>+function PageProxySetIcon (aURL)
>+{
>+  if (!gProxyFavIcon)
>+    return;
>+
>+  if (!aURL)
>+    PageProxyClearIcon();
>+  else if (gProxyFavIcon.getAttribute("src") != aURL)
>+    gProxyFavIcon.setAttribute("src", aURL);
>+}
>+
>+function PageProxyClearIcon ()
>+{
>+  gProxyFavIcon.removeAttribute("src");
>+}

I think you need to update the |gProxyDeck.selectedIndex| in the following:

+function PageProxySetIcon(aURL)
+{
+  if (!gProxyFavIcon)
+    return;
+  if (!aURL) {
+    PageProxyClearIcon();
+    return;
+  }
+  if (gProxyFavIcon.getAttribute("src") != aURL)
+    gProxyFavIcon.setAttribute("src", aURL);
+  if (gProxyDeck.selectedIndex != 1)
+    gProxyDeck.selectedIndex = 1;
+}
+
+function PageProxyClearIcon()
+{
+  if (gProxyDeck.selectedIndex != 0)
     gProxyDeck.selectedIndex = 0;
-  }
+  if (gProxyFavIcon.hasAttribute("src"))
+    gProxyFavIcon.removeAttribute("src");
 }

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #5)
> I think you need to update the |gProxyDeck.selectedIndex| in the following:

> +  if (gProxyFavIcon.getAttribute("src") != aURL)
> +    gProxyFavIcon.setAttribute("src", aURL);
> +  if (gProxyDeck.selectedIndex != 1)
> +    gProxyDeck.selectedIndex = 1;

> +  if (gProxyDeck.selectedIndex != 0)
>      gProxyDeck.selectedIndex = 0;

> +  if (gProxyFavIcon.hasAttribute("src"))
> +    gProxyFavIcon.removeAttribute("src");

These checks don't seem to make much sense. At least, I would expect the underlying code to deal with such things smartly...

Comment 7

[Philip Chee]

> These checks don't seem to make much sense. At least, I would expect the
> underlying code to deal with such things smartly...

Yes, you are right. This is from a very bit rotted patch I wrote a long time ago when I knew less.

Comment 8

[Misak Khachatryan]

patch from this bug moved to bug 425480, Neil suggested

Comment 9

[Misak Khachatryan]

Attachment: remaining bits

Remaining bits after landing of bug 425480.

Comment 10

[Misak Khachatryan]

Attachment: proper patch

Oops, forgone one line.

Comment 11

[Philip Chee]

Comment on attachment 362871 [details] [diff] [review]
proper patch

I don't think application/octet-stream is the proper mime-type for a patch.

Comment 12

[[e-mail]]

Comment on attachment 362871 [details] [diff] [review]
proper patch

Our page proxy favicon works in a completely different way to Firefox's, so none of these changes makes sense.

Comment 13

[Philip Chee]

> (From update of attachment 362871 [details] [diff] [review])
> Our page proxy favicon works in a completely different way to Firefox's, so
> none of these changes makes sense.

These changes look reasonable to me. Why don't they make sense to you?

Comment 14

[[e-mail]]

Comment on attachment 362871 [details] [diff] [review]
proper patch

>-  gProxyButton.setAttribute("pageproxystate", aState);
>+  gURLBar.setAttribute("pageproxystate", aState);
>+  gProxyFavIcon.setAttribute("pageproxystate", aState);
This change is definitely wrong, but maybe I misread the rest of the patch.

Comment 15

[[e-mail]]

Since bug 254714 was ported in bug 425480 can you please resummarise this bug to whatever it is you're trying to do here?

Comment 16

[[e-mail]]

Comment on attachment 362871 [details] [diff] [review]
proper patch

>+    PageProxySetIcon(gBrowser.mCurrentBrowser.mIconURL);
Unfortunately mIconURL won't be set in the case of a /favicon.ico icon so this would break those completely if they hadn't already been broken already.

Comment 17

[Philip Chee]

Aargh, so we have two paths.

onLinkAdded calls setIcon which updates mIconURL.

Then you have mTabProgressListener::onStateChange() which calls loadFavIcon() which updates the corresponding tab if mIconURL is empty. And it's loadFavIcon() which checks for /favicon.ico.

What should we do here?

Comment 18

[Robert Kaiser]

From comments here, it's unclear if this is still needed or not. I don't see any reason to give this special priority, but it's also not unwanted, so cancelling the wanted flag.

Comment 19

[Phoenix]

> From comments here, it's unclear if this is still needed or not.
Bump?

Comment 20

[Philip Chee]

>> From comments here, it's unclear if this is still needed or not.
> Bump?
Did you try out the STR in Bug 254714?

Comment 21

[Phoenix]

Yes, I can't reproduce the issue

Comment 22

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0 SeaMonkey/2.14a1 ID:20120717003004 CSet: fc60215fa2fa

When following the STR in bug 254714 comment #58, I cannot reproduce the issue. Nor can I reproduce it by loading (after opening a new window ;-) ) a homepage of several hundreds of tabs, which used to suffer from the problem in the past.