Closed Bug 1260460 Opened 8 years ago Closed 8 years ago

reset user's restore on demand preference during e10s migration

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
e10s m9+ ---
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed
relnote-firefox --- 47+

People

(Reporter: blassey, Assigned: Felipe)

References

Details

Attachments

(1 file)

      No description provided.
I have a followup in bug 1257554 to understand exactly why we're doing this for E10S, but assuming there's a need I don't think this should just be a pref reset. If this isn't working in E10S, we should just remove support for the pref entirely. I don't want to have a footgun pref (even if it's only in about:config).
Dolske, I brought this up on the e10s meeting yesterday with Blassey, Jeff & co. The consensus was that everyone agrees that this is a bad feature, but not strongly enough to remove it due to e10s. A pref reset is intended to avoid putting any users who might hit these edges cases in a bad situation without them being aware, while allowing people to keep using it if they desire to have it back. There's also some ongoing perf work that might alleviate the regressions and improve the situation.
Comment on attachment 8737300 [details]
MozReview Request: Bug 1260460 - reset user's restore on demand preference during e10s migration. r=mconley

https://reviewboard.mozilla.org/r/43885/#review40463

::: browser/components/nsBrowserGlue.js:2170
(Diff revision 1)
>                             "passwordCol",
>                             "hidden");
>      }
>  
> +    if (currentVersion < 37) {
> +      Services.prefs.clearUserPref("browser.sessionstore.restore_on_demand");

This is going to reset this pref for _everybody_, not just people who are migrating to using e10s. Are we sure we want to do that?
Attachment #8737300 - Flags: review?(mconley)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #4)
> Comment on attachment 8737300 [details]
> MozReview Request: Bug 1260460 - reset user's restore on demand preference
> during e10s migration. r=mconley
> 
> https://reviewboard.mozilla.org/r/43885/#review40463
> 
> ::: browser/components/nsBrowserGlue.js:2170
> (Diff revision 1)
> >                             "passwordCol",
> >                             "hidden");
> >      }
> >  
> > +    if (currentVersion < 37) {
> > +      Services.prefs.clearUserPref("browser.sessionstore.restore_on_demand");
> 
> This is going to reset this pref for _everybody_, not just people who are
> migrating to using e10s. Are we sure we want to do that?

I thought so (at least, I did that intentionally), following the principle of not having any different behavior for e10s and non-e10s users.
K, gonna needinfo canuckistani to be absolutely sure.
Flags: needinfo?(jgriffiths)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #6)
> K, gonna needinfo canuckistani to be absolutely sure.

I think we should reset for everyone, because we're removing the UI for everyone so we may be causing people to get locked into the pref being non-default if we remove the UI and do not reset the pref. Some people might find this confusing, so we should ensure that this change is well-documented in relnotes and on SUMO.
Flags: needinfo?(jgriffiths)
Comment on attachment 8737300 [details]
MozReview Request: Bug 1260460 - reset user's restore on demand preference during e10s migration. r=mconley

https://reviewboard.mozilla.org/r/43885/#review40493

Given canuckistani's comment, r=me. Thanks felipe!
Attachment #8737300 - Flags: review+
Follow-up push: https://hg.mozilla.org/integration/fx-team/rev/6f4210b98c4b
Replace "currentVersion" with "currentUIVersion" to fix XPCshell and browser-chrome bustage related to nsBrowserGlue.js.

Initial push with test failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=43f2fc45eaf759727dfed6893a8c2efc8c35ddf9
https://hg.mozilla.org/mozilla-central/rev/43f2fc45eaf7
https://hg.mozilla.org/mozilla-central/rev/6f4210b98c4b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
it will be very sad to let it go instead of real fixing,
I know it's startup & memory wise and I can restore session like before by just simply keep pressing Ctrl+Tab for a few sec, but it's only silly workaround for old feature [-(
nearly the same thing was with awesome feature like loading tabs progressively...

Many users including me use this feature & will have to consider vivaldi now
can tabs after session restore throttled and load 1–2 tabs simultaneously
(user's pref of 1 or 100 whatever the user like)

My office users use this pref a lot & a fall back would really help guys.
Hi, nothing is being removed here. The pref will just get reset, but you can go back to its previous value by just going to about:config and changing it (browser.sessionstore.restore_on_demand).
(In reply to :Felipe Gomes (needinfo me!) from comment #15)
> Hi, nothing is being removed here. The pref will just get reset, but you can
> go back to its previous value by just going to about:config and changing it
> (browser.sessionstore.restore_on_demand).

ah thanks for the info.


https://bugzilla.mozilla.org/show_bug.cgi?id=1257554#c19

confused 
browser.sessionstore.restore_on_demand
will be available right?
Yeah, maybe in the future it will be removed and replaced with something better, but for now that pref will remain available.
(In reply to :Felipe Gomes (needinfo me!) from comment #17)
> Yeah, maybe in the future it will be removed and replaced with something
> better, but for now that pref will remain available.

thank you :)
Most of our users want/like it so have to deploy it.
Glad that there is a pref.

in future version
> can tabs after session restore throttled and load 1–2 tabs simultaneously
> (user's pref of 1 or 100 whatever the user like)
> 
> My office users use this pref a lot & a fall back would really help guys.

:)
Felipe, should this fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47?
Flags: needinfo?(felipc)
Thanks for the ping, Chris. This one is conditional on bug 1257554, as both should ride together. I'll keep the needinfo here until there's an answer on bug 1257554.
Comment on attachment 8737300 [details]
MozReview Request: Bug 1260460 - reset user's restore on demand preference during e10s migration. r=mconley

Approval Request Comment
[Feature/regressing bug #]: With the removal of the "Restore on Demand" checkbox from the Preferences UI (bug 1257554), we should also reset this pref to its default value, once, on migration.
[User impact if declined]: Checkbox will be removed but pref will remain as it was before. The intention is to discourage users from using this.
[Describe test coverage new/current, TreeHerder]: landed in central
[Risks and why]: getting some users mad because their pref changed. But they can easily change it back through about:config
[String/UUID change made/needed]: none
Flags: needinfo?(felipc)
Attachment #8737300 - Flags: approval-mozilla-aurora?
Comment on attachment 8737300 [details]
MozReview Request: Bug 1260460 - reset user's restore on demand preference during e10s migration. r=mconley

Needed for e10s, Aurora47+
Attachment #8737300 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Release Note Request (optional, but appreciated)
[Why is this notable]: Some people want session restore to automatically load all their restored tabs, but this causes performance problems with e10s (bug 1037179).
[Suggested wording]:

"The browser.sessionstore.restore_on_demand preference has been reset to its default value (true)."

or maybe include an explanation, though it makes the note longer:

"The browser.sessionstore.restore_on_demand preference has been reset to its default value (true) to avoid e10s performance problems."

[Links (documentation, blog post, etc)]: This bug?
relnote-firefox: --- → ?
We try to avoid linking directly to bugzilla on release channel release notes and stick to posts or pages on mozilla.org domains.   A post might still be nice.

Would it be accurate to mark this "disabled" for 46 since e10s won't be on in 46 release? Will this affect non-e10s users on 46?
Flags: needinfo?(felipc)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #25)
> We try to avoid linking directly to bugzilla on release channel release
> notes and stick to posts or pages on mozilla.org domains.   A post might
> still be nice.
> 
> Would it be accurate to mark this "disabled" for 46 since e10s won't be on
> in 46 release? Will this affect non-e10s users on 46?

It affects all users, since the reset was meant to everyone. I agree that linking to bugzilla is not a good idea. A page on SUMO would be better.
I tried to find a page documenting this feature ("Restore on demand"), where we could add a note saying that the pref was reset, but I didn't find anything..
Flags: needinfo?(felipc)
Joni is this something you could do in SUMO? Then I can link to it.
Flags: needinfo?(jsavage)
Liz, yes, we can set up a SUMO page for this.

I might need some additional info in order to write the article, since I'm not familiar with Restore on Demand:

Can the average user get to it from the UI or is it only via about:config? Is "Restore on Demand" different from "Restore Previous session?"

So if the user has E10s enabled, it will reset their about:config pref, so they have to go back in and change it? And it's only on 47 Beta, right?

Not sure who to need info for answers, so I need info'ed Chris, but feel free to redirect to someone else.
Flags: needinfo?(jsavage) → needinfo?(cpeterson)
(In reply to Joni Savage ("need info" me) from comment #29)
> I might need some additional info in order to write the article, since I'm
> not familiar with Restore on Demand:

Joni, if you have any more questions, feel free to keep asking in this bug or just ping me on IRC or email.

> Can the average user get to it from the UI or is it only via about:config?

The setting is available in Firefox 46 and earlier. In the about:preferences page, the user can uncheck the "Don’t load tabs until selected" checkbox so background tabs will load in the background instead of waiting for the user to click focus to the tab (the default setting).

The "Don’t load tabs until selected" checkbox was removed in Firefox 47 (Beta). The feature is still available from the about:config pref "browser.sessionstore.restore_on_demand".

> Is "Restore on Demand" different from "Restore Previous session?"

"Restore Previous Session" will open placeholder tabs for all the tabs from your previous session. If "Restore on Demand" (aka the "Don’t load tabs until selected" checkbox) is enabled, then the placeholder tabs won't load their page contents until the user clicks focus to the tab.

If "Restore on Demand" is disabled, then the placeholder tabs will slowly load their page contents in the background. This severely slows down Firefox because the user is trying to interact with the current tab, but Firefox is simultaneously loading all the background tabs' pages.

> So if the user has E10s enabled, it will reset their about:config pref, so
> they have to go back in and change it? And it's only on 47 Beta, right?

The about:config pref "browser.sessionstore.restore_on_demand" will be reset for all users, whether they have e10s enabled or not, when the first run a version of Firefox >= 47.
Flags: needinfo?(cpeterson)
Thanks for the clarification, Chris. We've drafted an article here: https://support.mozilla.org/kb/allow-firefox-load-multiple-tabs-multiprocess

If you have any changes, please let us know by commenting here, emailing me, or editing the article directly. One of our reviewers will take a look and publish it.
Thanks Joni for looking into it. It's looking good but I had some suggestions to improve it, which I have posted some as an edit and in a thread on the SUMO article.
Thanks for clarifying, Felipe. I understand now. I've published your changes and changed the title too: https://support.mozilla.org/en-US/kb/allow-firefox-load-multiple-tabs-background. The old link will redirect to this new one automatically, in case anyone has already shared it.
Quick question for anyone CC'd. Do you know if pre-loading tabs (when restore_on_demand = false), will perform better once e10s-multi (bug 1207306) is available?
(In reply to Chris More [:cmore] from comment #34)
> Quick question for anyone CC'd. Do you know if pre-loading tabs (when
> restore_on_demand = false), will perform better once e10s-multi (bug
> 1207306) is available?

e10s-multi should make the Firefox UI more responsive during the stampede of restoring many tabs, but it probably won't make the tab loading any faster.
(In reply to Chris Peterson [:cpeterson] from comment #35)
> (In reply to Chris More [:cmore] from comment #34)
> > Quick question for anyone CC'd. Do you know if pre-loading tabs (when
> > restore_on_demand = false), will perform better once e10s-multi (bug
> > 1207306) is available?
> 
> e10s-multi should make the Firefox UI more responsive during the stampede of
> restoring many tabs, but it probably won't make the tab loading any faster.

Makes sense. When e10s-multi is released and loading tabs doesn't slow down the Firefox UI, would we consider bring back this feature?
(In reply to Chris More [:cmore] from comment #36)
> Makes sense. When e10s-multi is released and loading tabs doesn't slow down
> the Firefox UI, would we consider bring back this feature?

I don't know. That's a question the Product team. :)

I liked this feature and it is Chrome's behavior (when session restore is enabled), but given the startup performance problems and Dolkse's comments in bug 1257554 comment 6, I don't expect it to be prioritized.
You need to log in before you can comment on or make changes to this bug.