Closed Bug 1297996 Opened 3 years ago Closed 3 years ago

Implement pref to toggle different page 2 page transitions

Categories

(Core :: Layout, defect)

All
Unspecified
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: Dominik, Assigned: mconley)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

During assessment of performance issues, we heard from Firefox UX that the way we do page to page transitions right now is very different from competitor products and feels slow.

To be able to give users choice and to be able to test different options with users, we propose to add a pref to control page to page transitions.
mconley has recently looked into this issue already to create a build for initially understanding the issue after it was raised by UX and produced a first patch to implement layout.show_previous_page. Asking him to comment here.
Flags: needinfo?(mconley)
Blocks: 1283300
I looked it up and mconley's patch is here:
https://hg.mozilla.org/try/rev/687e801178ffcdc40d4ae2778830d648711fc4f2

In a previous email he wrote:
"Along with the layout.show_previous_page pref, I've also modified this build so that it has e10s _disabled_ by default. This is because the page-to-page transitions work quite a bit differently in content processes, and instrumenting them with the layout.show_previous_page pref is going to take quite a bit more research and understanding."

I am fine with having the pref on non-e10s-enabled builds only. In addition, it would be great if the default pref could be false so that we would need a layout.blank_previous_page as a pref that is by default FALSE to not change the current behavior.
So my patch up here is a quick and dirty hack to make us not show the previous view once a new one has been constructed. This only works for non-e10s - making a pref for this kind of behaviour for e10s is harder.

What are we wanting to do with this?
Flags: needinfo?(mconley) → needinfo?(dstrohmeier)
The goal is to get this pref at least into Beta so that we'll be able to reach out and recruit a large enough sample of users to test the impact of page to page transitions with users. Easiest and maybe most valid testing would be through SHIELD, but that will require us to have access to the pref to toggle different options for users trough an addon.

I am fine with having this pref only available on non-e10s first and also think that this could be marked as an experimental pref and not be exposed in about:config.

Does this help?
Flags: needinfo?(dstrohmeier) → needinfo?(mconley)
It does indeed, thanks.
Flags: needinfo?(mconley)
Comment on attachment 8786459 [details]
Bug 1297996 - Add preference to show background colour between pageloads for non-e10s.

https://reviewboard.mozilla.org/r/75412/#review75218

::: layout/generic/nsSubDocumentFrame.cpp:237
(Diff revision 4)
>      nsView* nextView = subdocView->GetNextSibling();
>      nsIFrame* frame = nullptr;
>      if (nextView) {
>        frame = nextView->GetFrame();
>      }
> -    if (frame) {
> +    if (frame && sShowPreviousPage) {

I think you want the |&& sShowPreviousPage| to be added to the |(ps && !ps->IsPaintingSuppressed())| check two lines down. For example, if |presShell| is null we want to try to find a presshell to draw whether or not |sShowPreviousPage| is true.
Comment on attachment 8786459 [details]
Bug 1297996 - Add preference to show background colour between pageloads for non-e10s.

https://reviewboard.mozilla.org/r/75412/#review75654
Attachment #8786459 - Flags: review?(tnikkel)
Comment on attachment 8786459 [details]
Bug 1297996 - Add preference to show background colour between pageloads for non-e10s.

https://reviewboard.mozilla.org/r/75412/#review77118
Attachment #8786459 - Flags: review?(tnikkel) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9590f183073b
Add preference to show background colour between pageloads for non-e10s. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/9590f183073b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Now that uplifts are done, I'll see if I can get this uplifted to Beta.
Flags: needinfo?(mconley)
mconley, that would be great if we could get this uplifted to Beta.

The problem is that with the e10s rollout to a larger population, the non-e10s population for which this pref will work will get smaller and smaller. So, it would be great if we could get into testing as early as possible through uplifting.

@mgrimes: Do you think that the e10s rollout and this pref only working on non-e10s will be an issue? Will we be able to recruit users for SHIELD study based on the non-e10s build requirement?
Flags: needinfo?(mgrimes)
Gregg can confirm, but that should be easy enough to handle through the experiment add-on itself. That may mean it takes longer to fill enrollment since we'll be disqualifying people that have already consented. If we waited until the system add-on was in place we could set that criteria at the offer stage.
Flags: needinfo?(mgrimes) → needinfo?(glind)
Comment on attachment 8786459 [details]
Bug 1297996 - Add preference to show background colour between pageloads for non-e10s.

Approval Request Comment
[Feature/regressing bug #]:

Doesn't fix anything, but adds a pref that we can do some SHIELD experiments on for perceived performance. The pref defaults is hidden by default, and nothing changes if the pref isn't set, so this doesn't change any behaviour for our users.

[User impact if declined]:

None.

[Describe test coverage new/current, TreeHerder]:

This has been on Nightly for a while now, and was recently uplifted to Aurora.

[Risks and why]: 

Very low risk. The patch is inert by default, since the pref is hidden.

[String/UUID change made/needed]:

None.
Flags: needinfo?(mconley)
Attachment #8786459 - Flags: approval-mozilla-beta?
Comment on attachment 8786459 [details]
Bug 1297996 - Add preference to show background colour between pageloads for non-e10s.

If this helps do perf measurements and is a hidden pref, seems low risk to take it in Beta50.
Attachment #8786459 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee: nobody → mconley
Flags: needinfo?(glind)
You need to log in before you can comment on or make changes to this bug.