STR: Open Firefox to mozilla.org Wait for mozilla.org to finish loading Open a second tab to deelay.me While deelay.me is loading, switch tabs to the mozilla.org tab ER: No animation when switching stop/reload states AR: The stop->reload animation plays

http://deelay.me/3000/http://deelay.me/img/10000ms.gif might be a better URL to test since it completes loading in only 3 seconds vs the long delay for deelay.me.

Comment on attachment 8897547 [details] Bug 1390579 - Check if aRequest is an instanceof nsIRequest instead of just a simple null check since when switching tabs aRequest is an Object that maintains some of the request details. https://reviewboard.mozilla.org/r/168812/#review174208 There might be an issue with initial requests (see below), but more generally, this code feels like it should really be using per-tab data instead of having a single `timeWhenSwitchedToStop` value, which then affects all tabs. So it feels like, if I load tab A and immediately switch to tab B that also has a load pending, whether tab B shows an animation when the stop button switches to a reload one depends on how close the start of the load in A and the end of the load in B are together (time-wise), rather than only on tab B. That doesn't seem right. Or am I missing something? (in which case, what? :-) ) ::: browser/base/content/browser.js:5073 (Diff revision 1) > - return !this.timeWhenSwitchedToStop || > + return this.timeWhenSwitchedToStop && > window.performance.now() - this.timeWhenSwitchedToStop > 150; Naively, I would expect this would have prevented the animation from showing for initial loads in the window, becuase presumably timeWhenSwitchedToStop would be 0/undefined. After your change, that's no longer the case. Is that intentional, or is there something else that's preventing the animation for initial loads?

Comment on attachment 8897547 [details] Bug 1390579 - Check if aRequest is an instanceof nsIRequest instead of just a simple null check since when switching tabs aRequest is an Object that maintains some of the request details. (In reply to :Gijs from comment #3) > There might be an issue with initial requests (see below), but more > generally, this code feels like it should really be using per-tab data > instead of having a single `timeWhenSwitchedToStop` value, which then > affects all tabs. So it feels like, if I load tab A and immediately switch > to tab B that also has a load pending, whether tab B shows an animation when > the stop button switches to a reload one depends on how close the start of > the load in A and the end of the load in B are together (time-wise), rather > than only on tab B. That doesn't seem right. Or am I missing something? (in > which case, what? :-) ) This is actually a case where per-tab data doesn't make as much sense. Switching to a tab that immediately plays the animation would be distracting in the same way that the initial bug was trying to fix (bug 1384180). In that case, we would be switching from reload->stop->reload all within 150ms which is something that UX has asked us not to animate. > ::: browser/base/content/browser.js:5073 > (Diff revision 1) > > - return !this.timeWhenSwitchedToStop || > > + return this.timeWhenSwitchedToStop && > > window.performance.now() - this.timeWhenSwitchedToStop > 150; > > Naively, I would expect this would have prevented the animation from showing > for initial loads in the window, becuase presumably timeWhenSwitchedToStop > would be 0/undefined. After your change, that's no longer the case. Is that > intentional, or is there something else that's preventing the animation for > initial loads? The old code here would have returned true if timeWhenSwitchedStop was 0/undefined, which would have played the animation when we didn't want it to. So this in itself was wrong.

Comment on attachment 8897547 [details] Bug 1390579 - Check if aRequest is an instanceof nsIRequest instead of just a simple null check since when switching tabs aRequest is an Object that maintains some of the request details. https://reviewboard.mozilla.org/r/168812/#review174388 Alright, sorry for the confusion!

Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/aa46b8eed093 Check if aRequest is an instanceof nsIRequest instead of just a simple null check since when switching tabs aRequest is an Object that maintains some of the request details. r=Gijs

User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID 20170830100230 This bug fix is Verified with latest Nightly 57.0a1.