Last Comment Bug 524369 - Port Bug 491168 [Allow SessionStore to save/restore referrer field] to SeaMonkey
: Port Bug 491168 [Allow SessionStore to save/restore referrer field] to SeaMo...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Misak Khachatryan
:
Mentors:
Depends on: 491168
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-25 11:33 PDT by Misak Khachatryan
Modified: 2010-02-21 02:25 PST (History)
1 user (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch plus test (4.92 KB, patch)
2009-11-03 03:44 PST, Misak Khachatryan
no flags Details | Diff | Review
unbitrotted patch plus test (5.35 KB, patch)
2010-02-07 02:56 PST, Misak Khachatryan
no flags Details | Diff | Review
patch with fixed test [Checkin: Comment 15] (5.78 KB, patch)
2010-02-08 03:07 PST, Misak Khachatryan
neil: review+
neil: superreview+
Details | Diff | Review

Description Misak Khachatryan 2009-10-25 11:33:26 PDT
From parent bug:

The referrer field for documents in tabs is currently not saved nor restored. 
This includes the case where a tab is closed and then recovered via the
"Recently Closed Tabs" menu .  It would be nice for this field to be saved in
the case where a page is not cached and the page checks the referrer field
before allowing the page to load.

As the document.referrer field is read only, this is not something that can be
fixed in nsSessionStore.js alone.

Test case:

Go to URL and click on link on the page, close tab, reopen tab and refresh
page.  Referrer is lost.
Comment 1 Misak Khachatryan 2009-11-03 03:44:14 PST
Created attachment 409904 [details] [diff] [review]
patch plus test
Comment 2 neil@parkwaycc.co.uk 2009-11-03 04:03:25 PST
Comment on attachment 409904 [details] [diff] [review]
patch plus test

Our Undo Close Tab saves the referrer. Undo Close Window might not, but that needs a different test...
Comment 3 Misak Khachatryan 2010-02-04 23:27:40 PST
As bug 530735 fixed, this patch becomes actual. Neil, can You review it ?
Comment 4 neil@parkwaycc.co.uk 2010-02-05 09:59:59 PST
Comment on attachment 409904 [details] [diff] [review]
patch plus test

>+//      getBrowser().removeTab(tab);
>+//      let newTab = ss.undoCloseTab(window, 0);
>+//      newTab.addEventListener("SSTabRestored", function() {
>+//        newTab.removeEventListener("SSTabRestored", arguments.callee, false);
>+
>+//        is(window.content.document.referrer, REFERRER2, "document.referrer is still correct after closing and reopening the tab.");
>+//        getBrowser().removeTab(newTab);
>+
>+        finish();
>+//      }, true);
OK, so if you set browser.tabs.max_tabs_undo to 0, does this part now work?
Comment 5 neil@parkwaycc.co.uk 2010-02-05 13:55:43 PST
It would also be good if we could test the referrer field with both forms of undo close tab, but I assume that to get that to work we need to tweak tabbrowser's restoreTab to generate the appropriate SSTab* events?
Comment 6 Misak Khachatryan 2010-02-07 02:56:11 PST
Created attachment 425691 [details] [diff] [review]
unbitrotted patch plus test

(In reply to comment #4)
> OK, so if you set browser.tabs.max_tabs_undo to 0, does this part now work?

Yes, but i noticed that my test is a bit different, so I'm resubmitting patch.

(In reply to comment #5)
> It would also be good if we could test the referrer field with both forms of
> undo close tab, but I assume that to get that to work we need to tweak
> tabbrowser's restoreTab to generate the appropriate SSTab* events?

Agreed, but it seems that restoreTab is asynchronous, so i don't know where to fire these events.
Comment 7 neil@parkwaycc.co.uk 2010-02-07 08:40:55 PST
(In reply to comment #6)
> Created an attachment (id=425691) [details]
> unbitrotted patch plus test
> 
> (In reply to comment #4)
> > OK, so if you set browser.tabs.max_tabs_undo to 0, does this part now work?
> Yes, but i noticed that my test is a bit different, so I'm resubmitting patch.
So this patch only works with browser.tabs.max_tabs_undo set to 0? In which case you should set it to 0 during test set up and clear it when you finish.
Comment 8 Misak Khachatryan 2010-02-07 22:49:40 PST
(In reply to comment #7)
> So this patch only works with browser.tabs.max_tabs_undo set to 0? In which
> case you should set it to 0 during test set up and clear it when you finish.

Why ? It should work regardless of browser.tabs.max_tabs_undo, test may fail only if sessionstore is unfunctional/removed.
Comment 9 neil@parkwaycc.co.uk 2010-02-08 01:42:27 PST
(In reply to comment #8)
> (In reply to comment #7)
> > So this patch only works with browser.tabs.max_tabs_undo set to 0? In which
> > case you should set it to 0 during test set up and clear it when you finish.
> Why ? It should work regardless of browser.tabs.max_tabs_undo, test may fail
> only if sessionstore is unfunctional/removed.
Well, there appear to be two issues:
1. If the tabbrowser restores the tab, you already get the referrer, even without the patch. So you're not actually testing your patch.
2. If the tabbrowser restores the tab, you don't get an SSTabRestored event yet.
Comment 10 Misak Khachatryan 2010-02-08 03:07:06 PST
Created attachment 425775 [details] [diff] [review]
patch with fixed test
[Checkin: Comment 15]

OK, got it. Test changed accordingly.
Comment 11 Robert Kaiser (not working on stability any more) 2010-02-16 09:14:41 PST
Serge, could you look into checking this in?
Comment 12 Serge Gautherie (:sgautherie) 2010-02-16 10:20:37 PST
Misak, have you considered asking for a commit access?
Comment 13 Misak Khachatryan 2010-02-16 21:55:28 PST
No. Do You think I'm ready ;) ?
Comment 14 Serge Gautherie (:sgautherie) 2010-02-17 09:01:11 PST
(In reply to comment #13)
> No. Do You think I'm ready ;) ?

I think you should be...

http://www.mozilla.org/hacking/committer/
https://bugzilla.mozilla.org/show_bug.cgi?id=451304
Comment 15 Serge Gautherie (:sgautherie) 2010-02-19 16:13:20 PST
Comment on attachment 425775 [details] [diff] [review]
patch with fixed test
[Checkin: Comment 15]


http://hg.mozilla.org/comm-central/rev/c9a7a0a12175
Comment 16 Serge Gautherie (:sgautherie) 2010-02-19 16:14:50 PST
(In reply to comment #14)
> I think you should be...

Bug 546586 :-)
Comment 17 neil@parkwaycc.co.uk 2010-02-21 02:25:13 PST
Comment on attachment 425775 [details] [diff] [review]
patch with fixed test
[Checkin: Comment 15]

>+    if (aEntry.referrer) 
After all that I forgot to mention the trailing space. Maybe next time...

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