Closed
Bug 593683
Opened 14 years ago
Closed 14 years ago
Small fixes in tabbed browser: also treat about:sessionrestore as initial page.
Categories
(SeaMonkey :: Tabbed Browser, defect)
SeaMonkey
Tabbed Browser
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: misak.bugzilla, Assigned: misak.bugzilla)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
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.
Attachment #472263 -
Flags: superreview?(neil)
Attachment #472263 -
Flags: review?(neil)
Comment 1•14 years ago
|
||
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.
Attachment #472263 -
Flags: superreview?(neil)
Attachment #472263 -
Flags: review?(neil)
Attachment #472263 -
Flags: review-
Comment 2•14 years ago
|
||
(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
Assignee | ||
Comment 3•14 years ago
|
||
Attachment #472263 -
Attachment is obsolete: true
Attachment #477125 -
Flags: superreview?(neil)
Attachment #477125 -
Flags: review?(neil)
Assignee | ||
Comment 4•14 years ago
|
||
Renaming bug as second part is not valid.
Summary: Small fixes in tabbed browser: also treat about:sessionrestore as initial page, and while calling "onLocationChange" also notify global observers. → Small fixes in tabbed browser: also treat about:sessionrestore as initial page.
Comment 5•14 years ago
|
||
(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•14 years ago
|
||
Comment on attachment 477125 [details] [diff] [review] patch v2 [Personally I don't feel the need to add the ()s]
Attachment #477125 -
Flags: superreview?(neil)
Attachment #477125 -
Flags: superreview+
Attachment #477125 -
Flags: review?(neil)
Attachment #477125 -
Flags: review+
Assignee | ||
Comment 7•14 years ago
|
||
Pushed: http://hg.mozilla.org/comm-central/rev/8d035a86d9e5
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 8•14 years ago
|
||
I accidentally clobbered this patch in a bad merge. Reapplied as: http://hg.mozilla.org/comm-central/rev/4d36594ea0e8
You need to log in
before you can comment on or make changes to this bug.
Description
•