history.pushState forgets favicon and feeds

RESOLVED FIXED in seamonkey2.1a1

Status

SeaMonkey
Tabbed Browser
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
seamonkey2.1a1

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Steps to reproduce problem:
1. Browse to a site with a custom favicon and an RSS feed
2. Load the URL javascript:history.pushState(null, document.title, "?#");
3. The URL changes, and the favicon and RSS feed are forgotten.

Note that the link toolbar, if used, does not forget subscriptions. This because it uses a different notification model and assumes that the extra location change caused by the pushState is in fact caused by a tab switch.
(Assignee)

Comment 1

8 years ago
In fact it looks like anchor navigation also causes favicons and feeds to be forgotten. Oops.
(Assignee)

Comment 2

8 years ago
Created attachment 437160 [details] [diff] [review]
Proposed patch

We only want to clear favicon and feed data when a toplevel load has a network request associated with it (which will generate a document with new favicon and feed data). But navigator.js will assume that the forwarded location change is due to a tab switch, so if we are in fact forwarding a location change for a toplevel load without a network request then we also need to resend the favicon and feed data, just as if we'd switched tabs.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #437160 - Flags: review?(iann_bugzilla)

Comment 3

7 years ago
Comment on attachment 437160 [details] [diff] [review]
Proposed patch

>+++ b/suite/browser/tabbrowser.xml	Tue Apr 06 00:16:24 2010 +0100
> 
>+                    if (!aRequest && aWebProgress.DOMWindow == this.mBrowser.contentWindow) {
>+                      if (this.mBrowser.mIconURL && "onLinkIconAvailable" in element)
>+                        element.onLinkIconAvailable(this.mBrowser.mIconURL);
>+                      if ("onFeedAvailable" in element) {
>+                        this.mFeeds.forEach(
>+                          function notifyFeedAvailable(feed) {
>+                            element.onFeedAvailable(feed);
>+                          }
>+                        );
>+                      }
>+                    }
Is it worth looking at having a helper and sharing code with the updateCurrentBrowser method?
r=me either way
Attachment #437160 - Flags: review?(iann_bugzilla) → review+

Comment 4

7 years ago
(In reply to comment #2)
> Created an attachment (id=437160) [details]
> Proposed patch
Seems like the patch is bit rotted now.
(Assignee)

Comment 5

7 years ago
Created attachment 441644 [details] [diff] [review]
Proof of concept

This was the best shared function that I could come up with. I guess it could be simplified if I could make mFeeds live on mBrowser, but I would have to be careful to delete it.

The patch has otherwise also been updated for bitrot (but most of that is commented out assuming the shared function is acceptable.)
Attachment #441644 - Flags: feedback?(iann_bugzilla)

Comment 6

7 years ago
Comment on attachment 441644 [details] [diff] [review]
Proof of concept

yeah, looks good to me - codewise
Attachment #441644 - Flags: feedback?(iann_bugzilla) → feedback+
(Assignee)

Comment 7

7 years ago
Created attachment 441975 [details] [diff] [review]
With actual code removal

This is attachment 441644 [details] [diff] [review] but with the /* */ code blocks completely removed.
Attachment #441644 - Attachment is obsolete: true
Attachment #441975 - Flags: review?(iann_bugzilla)

Comment 8

7 years ago
(In reply to comment #7)
> Created an attachment (id=441975) [details]
> With actual code removal
> 
> This is attachment 441644 [details] [diff] [review] but with the /* */ code blocks completely removed.

Does this.mTabListeners[this.mTabContainer.selectedIndex].mBrowser equal this.mPanelContainer.selectedPanel.firstChild (i.e. newBrowser) for the second change?
(Assignee)

Comment 9

7 years ago
(In reply to comment #8)
> Does this.mTabListeners[this.mTabContainer.selectedIndex].mBrowser equal
> this.mPanelContainer.selectedPanel.firstChild (i.e. newBrowser) for the second
> change?
Yes; by that time the new browser is now the selected browser (and its tab is the selected tab and it is of course its own tab listener's browser).

Updated

7 years ago
Attachment #441975 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 10

7 years ago
Pushed changeset 8d6f4763c863 to comm-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a1
You need to log in before you can comment on or make changes to this bug.