557374 - history.pushState forgets favicon and feeds

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[[e-mail]]

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.

Comment 1

[[e-mail]]

In fact it looks like anchor navigation also causes favicons and feeds to be forgotten. Oops.

Comment 2

[[e-mail]]

Attachment: 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.

Comment 3

[Ian Neal]

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

Comment 4

[Ian Neal]

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

Comment 5

[[e-mail]]

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

Comment 6

[Ian Neal]

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

yeah, looks good to me - codewise

Comment 7

[[e-mail]]

Attachment: With actual code removal

This is attachment 441644 [details] [diff] [review] but with the /* */ code blocks completely removed.

Comment 8

[Ian Neal]

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

Comment 9

[[e-mail]]

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

Comment 10

[[e-mail]]

Pushed changeset 8d6f4763c863 to comm-central.