593683 - Small fixes in tabbed browser: also treat about:sessionrestore as initial page.

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Misak Khachatryan]

Attachment: patch

While investigating one of failing tests I've found some little bugs, that i
miss when ported relevant portions of FF code to SM.

This incorporates:

about:sessionrestore should be considered as initial page,

and while calling "onLocationChange" also notify global observers.

Comment 1

[[e-mail]]

Comment on attachment 472263 [details] [diff] [review]
patch

>+var gInitialPages = [
>+  "about:blank",
>+  "about:sessionrestore"
>+];
Not sure what the point of this is. Is there a bug for it?

>   // Replace "about:blank" with an empty string
>   // only if there's no opener (bug 370555).
>-  if (uri.spec == "about:blank")
>-    value = content.opener || getWebNavigation().canGoBack ? "about:blank" : "";
>+  if (gInitialPages.indexOf(uri.spec) != -1)
>+    value = content.opener ? uri.spec : "";
What happened to the getWebNavigation().canGoBack?

>-              if (this.mTabBrowser.mCurrentTab == this.mTab)
>-                this.mTabBrowser.updateUrlBar(aWebProgress, aRequest, aLocation,
>-                                              null, this.mBrowser, this.mFeeds);
>-
>               this.mTabBrowser._callProgressListeners(this.mBrowser, "onLocationChange",
>-                                                      [aWebProgress, aRequest, aLocation],
>-                                                      false);
>+                                                      [aWebProgress, aRequest, aLocation]);
We really need to update the URL bar on a location change! That already notifies the global listeners for location, security, favicon and feeds.

Comment 2

[Justin Wood (:Callek)]

(In reply to comment #1)
> Comment on attachment 472263 [details] [diff] [review]
> patch
> 
> >+var gInitialPages = [
> >+  "about:blank",
> >+  "about:sessionrestore"
> >+];
> Not sure what the point of this is. Is there a bug for it?

Yes. http://hg.mozilla.org/mozilla-central/rev/315b94a53825 (Bug 471512)

Though the cset and bug was about private browsing mode, which we don't have [yet] it does benefit about:sessionrestore as well.

> >   // Replace "about:blank" with an empty string
> >   // only if there's no opener (bug 370555).
> >-  if (uri.spec == "about:blank")
> >-    value = content.opener || getWebNavigation().canGoBack ? "about:blank" : "";
> >+  if (gInitialPages.indexOf(uri.spec) != -1)
> >+    value = content.opener ? uri.spec : "";
> What happened to the getWebNavigation().canGoBack?

I was thinking the same, but I also think we should wrap in ()'s

Comment 3

[Misak Khachatryan]

Attachment: patch v2

Comment 4

[Misak Khachatryan]

Renaming bug as second part is not valid.

Comment 5

[[e-mail]]

(In reply to comment #2)
> (In reply to comment #1)
> > (From update of attachment 472263 [details] [diff] [review])
> > >   // Replace "about:blank" with an empty string
> > >   // only if there's no opener (bug 370555).
> > >-  if (uri.spec == "about:blank")
> > >-    value = content.opener || getWebNavigation().canGoBack ? "about:blank" : "";
> > >+  if (gInitialPages.indexOf(uri.spec) != -1)
> > >+    value = content.opener ? uri.spec : "";
> > What happened to the getWebNavigation().canGoBack?
> I was thinking the same, but I also think we should wrap in ()'s
I don't care for spare ()s myself although I know other people need them to keep themselves sane while reading complex expressions.

Comment 6

[[e-mail]]

Comment on attachment 477125 [details] [diff] [review]
patch v2

[Personally I don't feel the need to add the ()s]

Comment 7

[Misak Khachatryan]

Pushed:
http://hg.mozilla.org/comm-central/rev/8d035a86d9e5

Comment 8

[Philip Chee]

I accidentally clobbered this patch in a bad merge. Reapplied as:
http://hg.mozilla.org/comm-central/rev/4d36594ea0e8