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

RESOLVED WONTFIX

Status

()

Firefox
Session Restore
RESOLVED WONTFIX
4 years ago
4 years ago

People

(Reporter: tabmix.onemen, Assigned: lpy)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:P-])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

4 years ago
Why is this a problem?
Flags: needinfo?(onemen.one)
(Reporter)

Comment 2

4 years ago
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?
Flags: needinfo?(ttaubert)
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.
Flags: needinfo?(ttaubert)

Comment 4

4 years ago
(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?
Flags: needinfo?(onemen.one) → needinfo?(ttaubert)
(Reporter)

Comment 5

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

Updated

4 years ago
Whiteboard: [Australis:P-]
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
Flags: needinfo?(ttaubert)
Whiteboard: [Australis:P-] → [Australis:P-][good first bug][mentor=ttaubert][lang=js]
Summary: "Recently closed tabs" list should exclude about:customizing → "Recently closed tabs" list should exclude tabs with about: URIs as the only history entry
(Assignee)

Comment 7

4 years ago
Created attachment 8339328 [details] [diff] [review]
bug940617.patch
Assignee: nobody → pylaurent1314
Attachment #8339328 - Flags: review?(ttaubert)
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:").
Attachment #8339328 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 9

4 years ago
Created attachment 8339369 [details] [diff] [review]
bug940617.patch
Attachment #8339328 - Attachment is obsolete: true
Attachment #8339369 - Flags: review?(ttaubert)
(Assignee)

Comment 10

4 years ago
I run the tests, and it didn't break any of them.
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:") &&
Attachment #8339369 - Flags: review?(ttaubert)
(Assignee)

Comment 12

4 years ago
Created attachment 8339945 [details] [diff] [review]
bug940617.patch

Thank you. My mistake.
Attachment #8339369 - Attachment is obsolete: true
Attachment #8339945 - Flags: review?(ttaubert)
Comment on attachment 8339945 [details] [diff] [review]
bug940617.patch

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

Thanks!
Attachment #8339945 - Flags: review?(ttaubert) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/6efabfb144b1
Keywords: checkin-needed
Whiteboard: [Australis:P-][good first bug][mentor=ttaubert][lang=js] → [Australis:P-][good first bug][mentor=ttaubert][lang=js][fixed-in-fx-team]
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
Whiteboard: [Australis:P-][good first bug][mentor=ttaubert][lang=js][fixed-in-fx-team] → [Australis:P-][good first bug][mentor=ttaubert][lang=js]
Looks like you didn't run tests, eh? :(
Status: NEW → ASSIGNED
We should make sure to push it to try before marking it as checkin-needed again.
(Assignee)

Comment 18

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

Updated

4 years ago
Flags: needinfo?(ttaubert)
Attachment #8339945 - Flags: review+
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! :)
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(ttaubert)
Resolution: --- → WONTFIX
Whiteboard: [Australis:P-][good first bug][mentor=ttaubert][lang=js] → [Australis:P-]

Updated

4 years ago
Duplicate of this bug: 964327

Updated

4 years ago
Duplicate of this bug: 974328
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

4 years ago
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.
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Comment 25

4 years ago
Tim, is this something we could implement reasonably quickly?

Philipp, is this something which you think deserves priority?
Flags: needinfo?(ttaubert)
Flags: needinfo?(philipp)
(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.
Flags: needinfo?(philipp)
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?)
Flags: needinfo?(ttaubert)
Added bug 979476.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.