Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

mdMedia improvements #978

Closed
wants to merge 3 commits into from
Closed

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Dec 18, 2014

This PR contains some improvements to mdMedia, split-up in 3 commits as follows:

  1. fix(mdUtil): remove/delete cacheFactory keys when clearing/destroying cache:
    mdUtil's custom cacheFactory factory, adds/removes keys when putting/removing a single key, but leaves keys untouched when calling removeAll() or destroy(). As a result, the following problems arise:

    • Keys of destroyed caches continue to take up memory.
    • The keys() method return incorrect results after calling removeAll() (i.e. it continues to return the old keys, although they are not present in the cache any more).

    This commit fixes these issues, by overriding the removeAll and
    destroy methods as well.

  2. fix(mdMedia): avoid unnecessary digest and make changes apply quicker:
    Previously, the updateAll() method relied on $timeout for scheduling a digest. This made the possible changes apply after the next rendering and could possibly lead to extra digests.
    This commit, replaces the call to $timeout with a call to $evalAsync(), which avoids an extra digest if one is already in progress and also applies the changes before the next rendering.

  3. test(mdMedia): add more tests and move .spec file to correct location:
    In commit 6397040 mdMedia was moved out of the sidenav component and into core. The .spec file remained in the original folder though.*
    This commit moves the 'media.spec.jsfile to the correct location and adds more tests to more thoroughly covermdMedia`'s functionality.

    * Since this commit, the file has been moved ot the right location in cfd48f8.

@googlebot
Copy link

CLAs look good, thanks!

@gkalpak gkalpak force-pushed the mdMedia-improvements branch 6 times, most recently from 14c1144 to 0ec65a1 Compare December 24, 2014 10:05
… cache

`mdUtil`'s custom `cacheFactory` factory, adds/removes keys when
putting/removing a single key, but leaves `keys` untouched when calling
`removeAll()` or `destroy()`. As a result, the following problems arise:

- Keys of destroyed caches continue to take up memory.
- The `keys()` method return incorrect results after calling `removeAll()`
  (i.e. it continues to return the old keys, although they are not present
  in the cache any more).

This commit fixes these issues, by overriding the `removeAll` and
`destroy` methods as well.
Previously, the `updateAll()` method relied on `$timeout` for scheduling a
digest. This made the possible changes apply after the next rendering and
could possibly lead to extra digests.

This commit, replaces the call to `$timeout` with a call to `$evalAsync()`,
which avoids an extra digest if one is already in progress and also applies
the changes before the next rendering.
In commit 6397040 `mdMedia` was moved out of the `sidenav` component and into
`core`. The .spec file remained in the original folder though.

This commit moves the 'media.spec.js` file to the correct location and
adds more tests to more thoroughly cover `mdMedia`'s functionality.
@gkalpak
Copy link
Member Author

gkalpak commented Dec 24, 2014

/ping @ThomasBurleson (or anyone)


cache._destroy = cache.destroy;
cache.destroy = function() {
keys = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkalpak - think this is dangerous. Perhaps:

cache._destroy = cache.destroy;
    cache.destroy = function() {
      keys = {};
      return cache._destroy();
    };

is safer and allows subsequent calls to cache.put() to work properly.

Even though semantically the cache.put( ) should not be called after cache.destroy( )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subsequent calls to put() will not work correctly anyway, because the original put() will fail after having called destroy(). (put() is not supposed to be called after destroy().)

var cache = $mdUtil.cacheFactory('test');
cache.destroy();
cache.put('key', 'value');   // Throws an error regardless of the value of `keys`

So, having keys = null helps GC (instead of having to keep a reference to an empty object around). But, it's impact should be minimal (even more so considering destroy() is rarely used currently).

@gkalpak gkalpak deleted the mdMedia-improvements branch December 29, 2014 08:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants