687237 - Restore "unread" attribute after restart

Status: NEW

(Firefox :: Session Restore, enhancement)

Description

[ithinc]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:6.0.2) Gecko/20100101 Firefox/6.0.2
Build ID: 20110902133214

Steps to reproduce:

Some people prefer to persist read/unread state across sessions. This may be controlled by a pref.

Comment 1

[ithinc]

Attachment: patch

Comment 2

[Ioana (away)]

This seems to be an enhancement request rather than a bug.
ithinc, please mark it correspondingly.

Comment 3

[Len]

Comment on attachment 561176 [details] [diff] [review]
patch

/browser/base/content/tabbrowser.xml


>-                if (!this.mTab.selected)
>+                if (!this.mTab.selected && !this.mBrowser.__SS_restoreState)
>                   this.mTab.setAttribute("unread", "true");

We don't need this one. "unread" should only be set if the tab is reloaded (sessionstore.load_on_demand=false). Session restore should keep the read/unread status if the the session is restored with sessionstore.load_on_demand=true.

/browser/components/sessionstore/src/nsSessionStore.js
>-  xulAttributes: {"image": true},
>+  xulAttributes: {"image": true, "unread": true},

This is all it needs.
Is sessionstore.load_on_demand is true, pages don't automatically reload in background. So tabs keep their previous read/unread status and only get a fresh "unread" on manual reload of of unselected tabs.

Comment 4

[Len]

The pref is sessionstore.restore_on_demand, not sessionstore.load_on_demand. Sorry

Comment 5

[ithinc]

(In reply to Len from comment #3)
> We don't need this one. "unread" should only be set if the tab is reloaded
> (sessionstore.load_on_demand=false). Session restore should keep the
> read/unread status if the the session is restored with
> sessionstore.load_on_demand=true.

I mean to keep the read/unread status regardless of the restore_on_demand pref. The restore is usually not a reload.

Comment 6

[Len]

(In reply to ithinc from comment #5)
> I mean to keep the read/unread status regardless of the restore_on_demand
> pref. The restore is usually not a reload.

The default is to restore _and_ reload. With restore_on_demand you can change it to restore and wait until a user demands a reload (select, manual reload).
I ever think of it as 'reload on demand only', that's why I first named it wrong in comment 3.


The latest change for "unread" (see bug 687754) makes sure that attribute is only set after a reload happened (if an existing "busy" is removed). Add your patch of nsSesionStore.js to it and we are done.
What happens now is the following:
- restore_on_demand = false (default)
All tabs are restored with the attributes that were saved, than the tabs are reloaded part by part and are getting "unread" after they finish reloading.
This makes sense because they are reloaded since you looked at them last time.
- restore_on_demand = true (set by user)
All tabs are restored with the attributes that were saved and because no automatic reload happens, they keep their read/unread status until you do a manual reload.

That's how I expect it to work.


To keep the read/unread status regardless of the restore_on_demand pref is wrong in my eyes. Let me give you an example:
You looked at tab 2, changed to tab 5 and now you save your session or it crashes. You are restarting that session and your selected tab will be no 5. Tab 2 (and the others too) gets reloaded with possibly changed content (i.e. a news site); shouldn't it become "unread" now? If you ignore the restore_on_demand setting you will still see the read status even after a _reload_ happened, which I think is wrong.

Comment 7

[ithinc]

(In reply to Len from comment #6)
> Tab 2 (and the others too) gets reloaded with possibly changed content (i.e.
> a news site); shouldn't it become "unread" now? If you ignore the
> restore_on_demand setting you will still see the read status even after a
> _reload_ happened, which I think is wrong.

Tab 2 (and the others) won't get reloaded after Firefox restarts unless the cache is expired.
With restore_on_demand set to true, you'll need to Reload Tab twice to get the page updated.

Comment 8

[Len]

There must be a reason why the reload behavior looks so different for you and me.
And I never had to reload twice to get a page updated. Sounds like a major bug to me if you have to.

In which FF version did you test it and what is your setting of browser.sessionstore.restore_on_demand in about:config?
I was testing in FF9.0a1 build 20110918030911 with an actual tabbrowser.xml, with browser.sessionstore.restore_on_demand false and true. Just retested in FF9.0a1 build 20110924030858 with same results as before.

Comment 9

[ithinc]

(In reply to Len from comment #8)
> In which FF version did you test it and what is your setting of
> browser.sessionstore.restore_on_demand in about:config?

Session restore works so in all Firefox versions, just like what bug 635453 reported.

Comment 10

[Len]

Bug 635453 - Apps tabs (pin tabs) aren't refresh on startup 
Not really related here.
And there are many changes in sessionstore going on related to restoring, prefs, pinned tabs etc.

Let me try to make my point clearer:
On adding unread to the xulAttributes sessionstore saves/restores the tabs with existing unread attributes.
Case 1: If a page is restored from cache, there is no change of the unread attribute.
Case 2: If a page is reloaded from the web, it is marked unread because it is no longer the page you saw before. It might be identical but that would need a detailed compare of the page content.

If restoring includes a reload, we have case 2 and unread is to be set.

FF9 is the version that has the patch to handle "unread". So it makes sense to test related patches in that version.

Comment 11

[ithinc]

(In reply to Len from comment #10)
> Bug 635453 - Apps tabs (pin tabs) aren't refresh on startup 
> Not really related here.

I was not saying bug 635453 is related with this one, but that bug just provides some information that you seem to have no in mind.

> Case 1: If a page is restored from cache, there is no change of the unread
> attribute.
> Case 2: If a page is reloaded from the web, it is marked unread because it
> is no longer the page you saw before. It might be identical but that would
> need a detailed compare of the page content.
> 
> If restoring includes a reload, we have case 2 and unread is to be set.

Restoring from cache is what session restore (including on-demand session restore) does, so we won't have case 2 usually.

> FF9 is the version that has the patch to handle "unread". So it makes sense
> to test related patches in that version.

Yes, tested.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 561176 [details] [diff] [review]
patch

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -563,17 +581,17 @@
>                 if (this.mBlank)
>                   this.mBlank = false;
> 
>                 if (this.mTab.hasAttribute("busy")) {
>                   this.mTab.removeAttribute("busy");
>                   this.mTabBrowser._tabAttrModified(this.mTab);
>                 }
>                 this.mTab.removeAttribute("progress");
>-                if (!this.mTab.selected)
>+                if (!this.mTab.selected && !this.mBrowser.__SS_restoreState)
>                   this.mTab.setAttribute("unread", "true");

tabbrowser shouldn't mess with sessionstore internals like this

Comment 13

[ithinc]

Attachment: patch

Comment 14

[Len]

To save and restore the read/unread status between browser sessions, we only need the attribute added to the xulAttributes list. Adding it one time by hand in sessionstore.js does the trick too (as long as you keep at least 1 'unread' tab). That's what I see since we have the unread attribute. The browser starts with the previous read/unread and if sessionstore is reloading a page from the web, the tab becomes 'unread' again; exactly how it should be.

> -                  if (!this.mTab.selected)
> +                  if (!this.mTab.selected && !this.mTab.hasAttribute("restoring"))
>                      this.mTab.setAttribute("unread", "true");

If a page is set to 'read' and sessionstore is reloading it from the web with fresh content, you want to keep it as read instead setting it to 'unread' as it should be?
Or am I misunderstanding the 'restoring' attribute?

Comment 15

[ithinc]

Session restore reloads a page from the cache instead of the web. Read bug 706970 and bug 792585.

(bug 792585 comment #27 by Dietrich Ayala (:dietrich) )
> 
> 1. The session restore feature is designed to have Firefox behave as if you
> had not restarted the browser. There are definitely places where we can't or
> don't do that exactly for various reasons, but it's the general approach.
> Firefox would not force-reload tabs for a user without their or the
> website's prompting. So this feature should also not do that.
> ...

Comment 16

[Len]

> Session restore reloads a page from the cache instead of the web.
Can you give us your restore settings and an example url of a page that is reloaded from cache but gets unread set?

(bug 792585 comment #27 by Dietrich Ayala (:dietrich) )
> 
> 1. The session restore feature is designed to have Firefox behave as if you
> had not restarted the browser. There are definitely places where we can't or
> don't do that exactly for various reasons, but it's the general approach.

The second sentence hints to situations where sessionstore isn't using the cache 'for various reasons'.

If sessionstore reloads from web instead from cache when it should take from cache, than it's a problem of sessionstore and not of tabbrowser.

Example 1:
- load a news page, select it (read) and than switch to a different tab
- restart the browser
- sessionstore decides to reload the news page from cache (with the old content)
- the tab should show as read

Example 2:
- load a news page, select it (read) and than switch to a different tab
- restart the browser
- sessionstore decides to reload the news page from server (possibly with new content)
- the tab should show as unread

It's the job of sessionstore to decide from where to reload and if it's reloaded from the server, unread should show that. Doesn't matter who forced a reload from server (me or sessionstore), it should result in setting the tab to unread. Anything else is a regression.
If you want it differently marked, you might be able to use your restoring attribute in CSS (read + restoring = reloading from cache, unread + restoring = reloading from server).

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 692654 [details] [diff] [review]
patch

Review of attachment 692654 [details] [diff] [review]:
-----------------------------------------------------------------

I think all we need to do is to add "unread" to the xulAttributes property.

Comment 18

[ithinc]

(In reply to Tim Taubert [:ttaubert] from comment #17)
> I think all we need to do is to add "unread" to the xulAttributes property.

It's not enough. The "unread" attribute will always be set to "true" after restoration, thus not "restored".