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

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [reserve-photon-animation])

Attachments

(1 attachment)

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.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 56.4 - Aug 1
Flags: qe-verify?
Priority: -- → P1
Flags: qe-verify? → qe-verify+
QA Contact: jwilliams
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?
Flags: needinfo?(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.
Flags: needinfo?(mcmanus) → needinfo?(valentin.gosu)
oh, and a timedChannel corresponds to a single URL.. its not a page concept.
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?
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?
(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.
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.
That sounds overly complicated. We could just drop the timestamp when switching tabs.
Comment hidden (mozreview-request)
Flags: needinfo?(valentin.gosu)
Comment hidden (mozreview-request)

Comment 11

a year ago
mozreview-review
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 hidden (mozreview-request)

Comment 13

a year ago
mozreview-review
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?
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
Comment hidden (mozreview-request)

Comment 15

a year ago
mozreview-review
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
Attachment #8890928 - Flags: review?(dao+bmo) → review+
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 hidden (mozreview-request)

Comment 18

a year ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/714713b0c49e
Don't animate the stop->reload if the page loads within 150ms (too distracting). r=dao
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 hidden (mozreview-request)

Comment 23

a year ago
Pushed by jwein@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/63f67675955d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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
QA Contact: jwilliams → stefan.georgiev
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Blocks: 1379480
You need to log in before you can comment on or make changes to this bug.