1384180 - Don't animate the stop->reload if the page loads within 150ms (too distracting)

Status: VERIFIED FIXED

(Firefox :: General, enhancement, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

From bug 1379480 comment 7, we should not run the animation for stop->reload if the page has finished loading within 150ms. This way we won't show the reload->stop animation and then immediately the stop->reload animation.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Hey Patrick, for fixing this bug I'm looking at taking the aRequest object that we get in onStateChange, and running QueryInterface on it to convert it to a nsITimedChannel.

With that, I get access to the following properties that describe how long the request took to load: http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/netwerk/base/nsITimedChannel.idl#77-96

However, the values that I get from `responseEndTime - responseStartTime` doesn't seem to match up with what I expect. It's always a lot lower, around 100ms, for something that visually appears to take around 250ms. Doing `responseEndTime - channelCreationTime` doesn't lead to better results. Should I be doing this calculation differently? Perhaps I need to sum up the differences for each timing pair? I see that http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/devtools/shared/webconsole/network-monitor.js#1484-1491 calculates the totalTime by getting the sum of the pairs, though that calculation uses values from nsITimedChannel plus values from some httpActivity.timings object.

Is this achievable using only the nsITimedChannel?

Comment 2

[Patrick McManus [:mcmanus]]

I'm going to shift your ni to valentin, as he's the timedChannel expert.

But I suspect a huge part of what you're seeing is that timedChannel mostly measures network time (as its supposed to), but there are a whole lot of other things that have to happen before your code gets a hold of the channel. If reproducible it might be useful quantum flow fodder.

Comment 3

[Patrick McManus [:mcmanus]]

oh, and a timedChannel corresponds to a single URL.. its not a page concept.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Ideally what I would like is the ability to get `window.performance.timing.responseEnd - window.performance.timing.navigationStart`

Is this not achievable with either a web progress listener or a request channel?

Comment 5

[Dão Gottwald [:dao]]

We know exactly when we switch from reload to stop. Seem like we should keep that timestamp and compare it when switching from stop to reload? Why would you want to use nsITimedChannel for that?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [::dao] from comment #5)
> We know exactly when we switch from reload to stop. Seem like we should keep
> that timestamp and compare it when switching from stop to reload? Why would
> you want to use nsITimedChannel for that?

Because the same stop and reload functions are used for all tabs, so if someone switched tabs then the numbers wouldn't coordinate.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

I suppose I could store those values on the tab itself, though we would need to make sure it doesn't get broken by tearing off tabs.

Comment 8

[Dão Gottwald [:dao]]

That sounds overly complicated. We could just drop the timestamp when switching tabs.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1384180 - Don't animate the stop->reload if the page loads within 150ms (too distracting).

Review commit: https://reviewboard.mozilla.org/r/162132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162132/

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8890928 [details]
Bug 1384180 - Don't animate the stop->reload if the page loads within 150ms (too distracting).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162132/diff/1-2/

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8890928 [details]
Bug 1384180 - Don't animate the stop->reload if the page loads within 150ms (too distracting).

https://reviewboard.mozilla.org/r/162132/#review167408

::: browser/base/content/browser.js:5053
(Diff revision 2)
>        return;
>  
> +    // Store the time that we switched to the stop button only if a request
> +    // is active. Requests are null if the switch is related to a tabswitch.
> +    // This is used to determine if we should show the stop->reload animation.
> +    this.timeWhenSwitchedToStop = aRequest instanceof Ci.nsIRequest ?

Why aRequest instanceof Ci.nsIRequest rather than just a null check?

::: browser/base/content/browser.js:5053
(Diff revision 2)
>        return;
>  
> +    // Store the time that we switched to the stop button only if a request
> +    // is active. Requests are null if the switch is related to a tabswitch.
> +    // This is used to determine if we should show the stop->reload animation.
> +    this.timeWhenSwitchedToStop = aRequest instanceof Ci.nsIRequest ?

I think it would be safer if you called something like CombinedStopReload.onTabSelect() from XULBrowserWindow.onUpdateCurrentBrowser, because switching tabs won't necessarily call switchToStop.

::: browser/base/content/browser.js:5085
(Diff revision 2)
> +    // If we don't know when we switched to stop (a tabswitch occured while
> +    // the page was loading), then we will not prevent the animation from
> +    // occuring.
> +    let loadTimeExceedsMinimumForAnimation = this.timeWhenSwitchedToStop ?
> +      window.performance.now() - this.timeWhenSwitchedToStop > 150 :
> +      true;

!this.timeWhenSwitchedToStop ||
window.performance.now() - this.timeWhenSwitchedToStop > 150;

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8890928 [details]
Bug 1384180 - Don't animate the stop->reload if the page loads within 150ms (too distracting).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162132/diff/2-3/

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8890928 [details]
Bug 1384180 - Don't animate the stop->reload if the page loads within 150ms (too distracting).

https://reviewboard.mozilla.org/r/162132/#review167422

::: browser/base/content/browser.js:4874
(Diff revision 3)
>  
>    // simulate all change notifications after switching tabs
>    onUpdateCurrentBrowser: function XWB_onUpdateCurrentBrowser(aStateFlags, aStatus, aMessage, aTotalProgress) {
>      if (FullZoom.updateBackgroundTabs)
>        FullZoom.onLocationChange(gBrowser.currentURI, true);
> +    CombinedStopReload.onTabSwitch();

nit: add a blank line before and after this

::: browser/base/content/browser.js:5092
(Diff revision 3)
> +    // If we don't know when we switched to stop (a tabswitch occured while
> +    // the page was loading), then we will not prevent the animation from
> +    // occuring.
> +    let loadTimeExceedsMinimumForAnimation =
> +      !this.timeWhenSwitchedToStop ||
> +      window.performance.now() - this.timeWhenSwitchedToStop > 150;

Running this (especially calling performance.now()) when we don't need it because of the other conditions is wasteful. Can you either inline this in the expression or make loadTimeExceedsMinimumForAnimation a function rather than a boolean value?

::: browser/base/content/browser.js:5095
(Diff revision 3)
> +    let loadTimeExceedsMinimumForAnimation =
> +      !this.timeWhenSwitchedToStop ||
> +      window.performance.now() - this.timeWhenSwitchedToStop > 150;
> +
>      let shouldAnimate = AppConstants.MOZ_PHOTON_ANIMATIONS &&
>                          aRequest instanceof Ci.nsIRequest &&

nit: replace this with a null-check as well for consistency?

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8890928 [details]
Bug 1384180 - Don't animate the stop->reload if the page loads within 150ms (too distracting).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162132/diff/3-4/

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8890928 [details]
Bug 1384180 - Don't animate the stop->reload if the page loads within 150ms (too distracting).

https://reviewboard.mozilla.org/r/162132/#review169282

Comment 16

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s e6a3ee892b6a -d 7c6f894e8a3e: rebasing 411030:e6a3ee892b6a "Bug 1384180 - Don't animate the stop->reload if the page loads within 150ms (too distracting). r=dao" (tip)
merging browser/base/content/browser.js
warning: conflicts while merging browser/base/content/browser.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8890928 [details]
Bug 1384180 - Don't animate the stop->reload if the page loads within 150ms (too distracting).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162132/diff/4-5/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/714713b0c49e
Don't animate the stop->reload if the page loads within 150ms (too distracting). r=dao

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out as requested by jaws because bug 1379620 got backed out:

https://hg.mozilla.org/integration/autoland/rev/cdf6256ae8c70325bee62b19c88f49a225f87c02

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79706a6ff965a06ce3e09df04f51e537e897da31

Comment 21

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7f3d3f23d67e -d 742dfaf7631d: rebasing 411366:7f3d3f23d67e "Bug 1384180 - Don't animate the stop->reload if the page loads within 150ms (too distracting). r=dao" (tip)
merging browser/base/content/browser.js
warning: conflicts while merging browser/base/content/browser.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8890928 [details]
Bug 1384180 - Don't animate the stop->reload if the page loads within 150ms (too distracting).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162132/diff/5-6/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63f67675955d
Don't animate the stop->reload if the page loads within 150ms (too distracting). r=dao

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/63f67675955d

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

I see some improvements:
== Change summary for alert #8817 (as of August 03 2017 14:26 UTC) ==

Improvements:

  3%  tp5o % Processor Time windows7-32 pgo e10s     56.70 -> 55.20

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8817

Comment 26

[Stefan [:StefanG_QA]]

Build   ID    20170908220146
User Agent    Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

This bug is Verified with latest Nightly 57.0a1 !

There is no animation if the page loads within 150ms.