perf($animate): listen for document visibility changes #14071
Conversation
| this.$get = ['$q', '$sniffer', '$$animateAsyncRun', '$document', '$timeout', | ||
| function($q, $sniffer, $$animateAsyncRun, $document, $timeout) { | ||
| this.$get = ['$q', '$sniffer', '$$animateAsyncRun', '$document', '$$isDocumentHidden', '$timeout', | ||
| function($q, $sniffer, $$animateAsyncRun, $document, $$isDocumentHidden, $timeout) { |
There was a problem hiding this comment.
$document is not needed anymore. (You can also align the arguments for consistency.)
|
@Narretz These changes should also be propagated to ngAnimate as the issue continues to exist if it is enabled. |
Narretz
force-pushed
the
fix-animate-hidden
branch
from
b3b1f2b to
8ddf2b7
Compare
|
@ArekSredzki the runner is the same with or without ngAnimate included, and the only other playe where hidden is checked is the queue, so this should be covered |
Narretz
force-pushed
the
fix-animate-hidden
branch
from
8ddf2b7 to
3b4cdda
Compare
|
It looks like the test errors are because adding the event listener populates the jqlite cache. Maybe this causes another memory leak. |
|
@Narretz I had checked it before you amended your commit and the problem was still very apparent with ngAnimate included... please check now, or I will on monday :) |
|
I've only changed a few cosmetic things in the ammended commit. |
|
@Narretz Sorry for the sluggish response. You are correct, and I believe that this solves the issue! Thanks! |
Accessing the document for the hidden state is costly for platforms like Electron. Closes angular#14066
Narretz
force-pushed
the
fix-animate-hidden
branch
from
3b4cdda to
25bddfa
Compare
| @@ -7245,22 +7245,22 @@ describe('$compile', function() { | |||
| }); | |||
|
|
|||
| inject(function($compile, $rootScope) { | |||
| expect(jqLiteCacheSize()).toEqual(0); | |||
| var cacheSize = jqLiteCacheSize(); | |||
There was a problem hiding this comment.
What's the reason for this change?
There was a problem hiding this comment.
This and a few other tests assume that the cacheSize is 0 after bootstrap. The listener for visibilitychange on the document adds a cache entry. So I thought it makes more sense to not assume a specific cacheSize on bootstrap.
|
LGTM. |
|
@mgol I had to change the expects for the add/removeEventListener calls to account for the fact that jquery 2.1 always calls them with the third argument set to false. Is this a functional change in jquery 2.1? |
|
jQuery has always passed the 3rd argument ( The way jQuery calls addEventListener is not its public API so we were free to change it even in a patch release. I adjusted jqLite code to do the same but only on |
|
Thanks for the explanation! I've adjusted the tests to simply check the first two arguments and ignore the third. |
perf(ngAnimate): listen for document visibility changes Accessing the document for the hidden state is costly for platforms like Electron. Instead, listen for visibilitychange and store the state. (angular#14071) Closes angular#14066
perf(ngAnimate): listen for document visibility changes Accessing the document for the hidden state is costly for platforms like Electron. Instead, listen for visibilitychange and store the state. (angular#14071) Closes angular#14066
Accessing the document for the hidden state is costly for
platforms like Electron.
Closes #14066