Port Bug 491168 [Allow SessionStore to save/restore referrer field] to SeaMonkey

RESOLVED FIXED in seamonkey2.1a1

Status

SeaMonkey
Session Restore
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Misak Khachatryan, Assigned: Misak Khachatryan)

Tracking

Trunk
seamonkey2.1a1
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
Created attachment 409904 [details] [diff] [review]
patch plus test
Attachment #409904 - Flags: superreview?(neil)
Attachment #409904 - Flags: review?(neil)

Comment 2

8 years ago
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...
(Assignee)

Comment 3

7 years ago
As bug 530735 fixed, this patch becomes actual. Neil, can You review it ?

Comment 4

7 years ago
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

7 years ago
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?
(Assignee)

Comment 6

7 years ago
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.
Attachment #409904 - Attachment is obsolete: true
Attachment #425691 - Flags: superreview?(neil)
Attachment #425691 - Flags: review?(neil)
Attachment #409904 - Flags: superreview?(neil)
Attachment #409904 - Flags: review?(neil)

Comment 7

7 years ago
(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.
(Assignee)

Comment 8

7 years ago
(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

7 years ago
(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.
(Assignee)

Comment 10

7 years ago
Created attachment 425775 [details] [diff] [review]
patch with fixed test
[Checkin: Comment 15]

OK, got it. Test changed accordingly.
Attachment #425691 - Attachment is obsolete: true
Attachment #425775 - Flags: superreview?(neil)
Attachment #425775 - Flags: review?(neil)
Attachment #425691 - Flags: superreview?(neil)
Attachment #425691 - Flags: review?(neil)

Updated

7 years ago
Attachment #425775 - Flags: superreview?(neil)
Attachment #425775 - Flags: superreview+
Attachment #425775 - Flags: review?(neil)
Attachment #425775 - Flags: review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed

Comment 11

7 years ago
Serge, could you look into checking this in?
Misak, have you considered asking for a commit access?
(Assignee)

Comment 13

7 years ago
No. Do You think I'm ready ;) ?
(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 on attachment 425775 [details] [diff] [review]
patch with fixed test
[Checkin: Comment 15]


http://hg.mozilla.org/comm-central/rev/c9a7a0a12175
Attachment #425775 - Attachment description: patch with fixed test → patch with fixed test [Checkin: Comment 15]
(In reply to comment #14)
> I think you should be...

Bug 546586 :-)
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a1
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...
You need to log in before you can comment on or make changes to this bug.