940617 - "Recently closed tabs" list should exclude tabs with about: URIs as the only history entry

Status: RESOLVED WONTFIX

(Firefox :: Session Restore, defect)

Description

[tabmix.onemen]

In Nightly with Australis SessionStore save about:customizing in closed tabs list

Comment 1

[:Gijs (he/him)]

Why is this a problem?

Comment 2

[tabmix.onemen]

we don't need to save about: tabs in "Recently closed tabs" list
the propose of the list is to easily reopen closed tab.
we don't need this for customize tab

we probably don't need to save almost all "about" tabs like: about:preferences, about:addons ans so on...

Tim, what do you think about it?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

I don't agree that we should never save about: pages but excluding about:customizing sounds fine to me. It's rather a different mode than a page with content, no?

OTOH we have a lot of about: pages and don't exclude most of them, so why now? This didn't seem to be a big problem in the past.

Also, excluding would mean that we only exclude tabs with a single history entry. Navigating away from about:customizing (which is weird but works) would keep the history entry and tab around.

Comment 4

[:Gijs (he/him)]

(In reply to Tim Taubert [:ttaubert] from comment #3)
> I don't agree that we should never save about: pages but excluding
> about:customizing sounds fine to me. It's rather a different mode than a
> page with content, no?
> 
> OTOH we have a lot of about: pages and don't exclude most of them, so why
> now? This didn't seem to be a big problem in the past.
> 
> Also, excluding would mean that we only exclude tabs with a single history
> entry. Navigating away from about:customizing (which is weird but works)
> would keep the history entry and tab around.

Not only that, it works fine to restore or navigate to/from the about:customizing page. Where it doesn't that's a bug (and there are various bugs on file).

I think this should be WONTFIX. It's only extra code to make stuff not work that works today, and I haven't heard a good reason why it's bad to be able to restore into customize mode. Tim, if you agree can you mark the bug as such?

Comment 5

[tabmix.onemen]

Each tab you save in the list can trigger removal of and older tab from the list
i don't think that saving unnecessary data in the list help users.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

I'm starting to think that maybe we should just not save tabs with a single entry having an about: URI. That way we wouldn't need an ever growing white list and as about: pages are mostly and only internal I don't think it makes sense to save those - whenever they're the only history entry.

We can easily modify _shouldSaveTabState() [1] to achieve that.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#3420

Comment 7

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug940617.patch

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8339328 [details] [diff] [review]
bug940617.patch

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

Thanks! Did you check if that breaks any tests?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +3422,5 @@
>      // session history, and no userTypedValue, then we don't actually want to
>      // store this tab's data.
>      return aTabState.entries.length &&
>             !(aTabState.entries.length == 1 &&
> +             aTabState.entries[0].url.indexOf("about:") == 0 &&

Please use url.startsWith("about:").

Comment 9

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug940617.patch

Comment 10

[Peiyong Lin[:lpy](UTC+8)]

I run the tests, and it didn't break any of them.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8339369 [details] [diff] [review]
bug940617.patch

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +3422,5 @@
>      // session history, and no userTypedValue, then we don't actually want to
>      // store this tab's data.
>      return aTabState.entries.length &&
>             !(aTabState.entries.length == 1 &&
> +             aTabState.entries[0].url.startsWith("about:") == 0 &&

startsWith() returns a boolean, you don't need to compare it to a number. Should be:

> aTabState.entries[0].url.startsWith("about:") &&

Comment 12

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug940617.patch

Thank you. My mistake.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8339945 [details] [diff] [review]
bug940617.patch

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

Thanks!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/6efabfb144b1

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc failures.
https://hg.mozilla.org/integration/fx-team/rev/594d63fbf864

https://tbpl.mozilla.org/php/getParsedLog.php?id=31331130&tree=Fx-Team

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Looks like you didn't run tests, eh? :(

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

We should make sure to push it to try before marking it as checkin-needed again.

Comment 18

[Peiyong Lin[:lpy](UTC+8)]

Hi Tim.
Sorry about that.
I looked into some failed browser test, for example browser_615394-SSWindowState_events.js, I found that it uses about: URIs.
When I reset my patch, and added some lines of code like |aTabState.entries[0].url == "about:rights"| in SessionStore.jsm, and that test failed.
So when tabs with about: URIs are excluded "Recently closed tabs" list as the only history entry, I think some tests used about: URIs would fail.
Is that right?

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

I took some more time to think about this and agree with Gijs. Working on about:preferences lately I changed my mind and think that there definitely is value in being able to restore those tabs. I don't think we should change the current behavior.

lpy, sorry for marking this as a good first bug. The key to landing your patch would have been to move all our tests from using about: pages to something else, just like you said. But luckily we don't have to do that anymore.

I hope I can make up for that in another mentored bug! :)

Comment 22

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

It would make sense to avoid duplicate entries of about: pages in the recent tab history.
Otherwise, changing some prefs or customizing Firefox can quickly clutter the menu. Instead, opening the same tab again should just move it to the top position of the recently closed tabs.

Comment 23

[Tim Nguyen :ntim]

For about:preferences, about:config,... It's OK to log in history, but I don't get the benefit to do this for about:customizing , previous versions of firefox didn't do this for customization mode.

Comment 24

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

I'm reopening this bug with the following proposal:

We shouldn't show duplicate entries for tabs that only have shown about: pages (and have no other history items).
For these tabs, we should instead move the last entry to the top of the list.

That should reduce the clutter to a minimum while also preserving the possibility to re-open those tabs.

Comment 25

[:Gijs (he/him)]

Tim, is this something we could implement reasonably quickly?

Philipp, is this something which you think deserves priority?

Comment 26

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #25)
> Tim, is this something we could implement reasonably quickly?
> 
> Philipp, is this something which you think deserves priority?

It's kind of an edge case, so I wouldn't block anything over it.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Can we please file another bug for a new proposal? This is becoming confusing.

(I like the idea, shouldn't we do that for every URL, not just the about: ones?)

Comment 28

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Added bug 979476.