Last Comment Bug 557374 - history.pushState forgets favicon and feeds
: history.pushState forgets favicon and feeds
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
http://planet.mozilla.org/
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-05 16:16 PDT by neil@parkwaycc.co.uk
Modified: 2010-04-28 16:47 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (1.49 KB, patch)
2010-04-05 16:31 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review
Proof of concept (4.48 KB, patch)
2010-04-26 16:01 PDT, neil@parkwaycc.co.uk
iann_bugzilla: feedback+
Details | Diff | Splinter Review
With actual code removal (4.74 KB, patch)
2010-04-27 16:23 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2010-04-05 16:16:04 PDT
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 neil@parkwaycc.co.uk 2010-04-05 16:27:46 PDT
In fact it looks like anchor navigation also causes favicons and feeds to be forgotten. Oops.
Comment 2 neil@parkwaycc.co.uk 2010-04-05 16:31:03 PDT
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.
Comment 3 Ian Neal 2010-04-20 04:59:50 PDT
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 2010-04-26 13:43:35 PDT
(In reply to comment #2)
> Created an attachment (id=437160) [details]
> Proposed patch
Seems like the patch is bit rotted now.
Comment 5 neil@parkwaycc.co.uk 2010-04-26 16:01:12 PDT
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.)
Comment 6 Ian Neal 2010-04-27 13:45:20 PDT
Comment on attachment 441644 [details] [diff] [review]
Proof of concept

yeah, looks good to me - codewise
Comment 7 neil@parkwaycc.co.uk 2010-04-27 16:23:35 PDT
Created attachment 441975 [details] [diff] [review]
With actual code removal

This is attachment 441644 [details] [diff] [review] but with the /* */ code blocks completely removed.
Comment 8 Ian Neal 2010-04-27 16:39:34 PDT
(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 neil@parkwaycc.co.uk 2010-04-28 01:53:14 PDT
(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 neil@parkwaycc.co.uk 2010-04-28 16:32:49 PDT
Pushed changeset 8d6f4763c863 to comm-central.

Note You need to log in before you can comment on or make changes to this bug.