Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Updated to use memory.notify rather than memory.log for Firefox 16 and above. Fixes issue #134 #141

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants
Member

davehunt commented Jul 16, 2012

For some reasons the tests are not working when run against Aurora or Nightly. Possibly an Addons SDK issue?

@whimboo whimboo commented on the diff Jul 16, 2012

extension/lib/garbage-collector.js
@@ -27,9 +27,16 @@ const reporter = EventEmitter.compose({
// Make sure we clean-up correctly
unload.ensure(this, 'unload');
- // For now the logger preference has to be enabled to be able to
- // parse the GC / CC information from the console service messages
- this._isEnabled = prefs.get(config.preferences.memory_log);
+ if (config.application.branch >= 16) {
@whimboo

whimboo Jul 16, 2012

Contributor

I don't think we should maintain those different paths multiple times in our code. Simply assign the right pref to an internal property like _pref_gc_notifications or similar. That way we don't need another if/else in _enable.

@whimboo whimboo commented on an outdated diff Jul 16, 2012

extension/lib/garbage-collector.js
@@ -27,9 +27,16 @@ const reporter = EventEmitter.compose({
// Make sure we clean-up correctly
unload.ensure(this, 'unload');
- // For now the logger preference has to be enabled to be able to
- // parse the GC / CC information from the console service messages
- this._isEnabled = prefs.get(config.preferences.memory_log);
+ if (config.application.branch >= 16) {
+ // The notify preference has to be enabled to be able to parse
+ // the GC / CC information from the notifications
+ this._isEnabled = prefs.get(config.preferences.memory_notify);
+ }
+ else {
+ // The logger preference has to be enabled to be able to parse
+ // the GC / CC information from the console service messages
@whimboo

whimboo Jul 16, 2012

Contributor

The comment is not fully true for Firefox 14 and 15 because we also use the GC observer notifications. I think we can make both a single line comment like "Preference to enable GC/CC observer and console messages".

Member

davehunt commented Jul 17, 2012

Updated based of feedback.

@whimboo whimboo commented on the diff Jul 23, 2012

extension/lib/garbage-collector.js
@@ -27,9 +27,15 @@ const reporter = EventEmitter.compose({
// Make sure we clean-up correctly
unload.ensure(this, 'unload');
- // For now the logger preference has to be enabled to be able to
- // parse the GC / CC information from the console service messages
- this._isEnabled = prefs.get(config.preferences.memory_log);
+ if (config.application.branch >= 16) {
+ this._pref_gc_notifications = config.preferences.memory_notify;
@whimboo

whimboo Jul 23, 2012

Contributor

Missed that we have the same code in tests. I would say lets make this pref a public member so tests can access its value.

@whimboo whimboo commented on an outdated diff Jul 23, 2012

extension/test/test-garbage-collector.js
// We have to require the garbage collector to initialize the module
require("memchaser/garbage-collector");
- test.assert(prefs.get(config.preferences.memory_log));
-}
+ var memory_pref;
+ if (config.application.branch >= 16) {
+ memory_pref = config.preferences.memory_notify;
+ }
+ else {
+ memory_pref = config.preferences.memory_log;
+ }
+ test.assert(prefs.get(memory_pref));
+}
@whimboo

whimboo Jul 23, 2012

Contributor

please add the new line.

Contributor

whimboo commented Jul 23, 2012

Otherwise looks good and we should get it landed.

@whimboo whimboo closed this Jul 23, 2012

@whimboo whimboo reopened this Jul 23, 2012

Member

davehunt commented Jul 23, 2012

Updated as requested.

@whimboo whimboo and 1 other commented on an outdated diff Jul 23, 2012

extension/lib/garbage-collector.js
@@ -19,6 +19,9 @@ const unload = require("api-utils/unload");
const config = require("config");
+var pref_gc_notifications;
@whimboo

whimboo Jul 23, 2012

Contributor

This variable should still be on the reporter object and not stay in the global scope.

@davehunt

davehunt Jul 25, 2012

Member

As discussed on IRC, I have attempted to move this out of global scope, but this causes the test to fail.

Member

davehunt commented Jul 25, 2012

Updated.

@whimboo whimboo commented on an outdated diff Aug 7, 2012

extension/lib/garbage-collector.js
@@ -20,6 +20,10 @@ const config = require("config");
const reporter = EventEmitter.compose({
+ _pref_gc_notifications: null,
+
+ get pref_gc_notifications() this._pref_gc_notifications,
@whimboo

whimboo Aug 7, 2012

Contributor

Would you mind to move the getter down so it comes after the constructor?

Also please make it a real function. Those short declarations are hard to read.

@whimboo whimboo referenced this pull request Aug 7, 2012

Closed

Release memchaser v0.4 #146

Member

davehunt commented Aug 7, 2012

Updated as suggested. If you need further changes and want to release I would suggest fetching my branch and making the necessary changes yourself as I will have limited access tomorrow and will be on PTO for the following two days.

Contributor

whimboo commented Aug 10, 2012

@whimboo whimboo closed this Aug 10, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment