For now, the DevTools add-on only cleans up when pressing Ctrl+Alt+R key shortcut. But it should also cleanup DevTools when the add-on is disabled. Cleanup means, closing all toolboxes and remove menu as well as key shortuts.

DevTools bug triage (filter on CLIMBING SHOES).

Comment on attachment 8865921 [details] Bug 1363419 - Unregister DevTools on Add-on disabling. https://reviewboard.mozilla.org/r/137508/#review141336 The code change itself looks good, I just have a doubt regarding calling unload() from the shudown() method. Not sure if my concerns are grounded, so I'll let you have a look and decide if it's relevant or not. ::: devtools/bootstrap.js:155 (Diff revision 1) > obs.notifyObservers(null, "message-manager-flush-caches"); > > /* Also purge cached modules in child processes, we do it a few lines after > in the parent process */ > if (Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT) { > Services.obs.notifyObservers(null, "devtools-unload", "reload"); I tried to track how this "reload" data parameter had any impact on the unload. I think the only "special" value is "shutdown" [1] ? So any value other that "shutdown" should have the same effect. For total correctness, maybe we should have different data parameters when unload() is called from reload() or from shutdown(). [1] http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/system/events.js#165-167 ::: devtools/bootstrap.js:313 (Diff revision 1) > if (userValue == prefs[name]) { > Services.prefs.setBoolPref(name, originalPrefValues[name]); > } > } > + > + unload(); shutdown() is called in 3 situations, according to MDN: - When the extension is uninstalled, if it's currently enabled. - When the extension becomes disabled. - When the user quits the application, if the extension is enabled. The two first cases are fine, but what is the impact of calling the addon unload() when quitting Firefox? Today, I don't think we run explicitly a similar shutdown code when Firefox closes? Will this represent additional processing? Could it slow down the closing of FF? Normally the shutdown method receives a reason as 2nd argument. Should we avoid calling our unload() if the reason is APP_SHUTDOWN?

(In reply to Julian Descottes [:jdescottes] from comment #4) > Comment on attachment 8865921 [details] > > ::: devtools/bootstrap.js:155 > (Diff revision 1) > > obs.notifyObservers(null, "message-manager-flush-caches"); > > > > /* Also purge cached modules in child processes, we do it a few lines after > > in the parent process */ > > if (Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT) { > > Services.obs.notifyObservers(null, "devtools-unload", "reload"); > > I tried to track how this "reload" data parameter had any impact on the > unload. I think the only "special" value is "shutdown" [1] ? So any value > other that "shutdown" should have the same effect. > > For total correctness, maybe we should have different data parameters when > unload() is called from reload() or from shutdown(). > > [1] > http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/system/ > events.js#165-167 Yes. But we could use it here to make a useful distinction: http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#205-209 I'll at least pass a meaningful reason here and not always "reload", even if listening code doesn't use it. > Today, I don't think we run explicitly a similar shutdown code when Firefox > closes? Will this represent additional processing? Could it slow down the > closing of FF? We do run some cleanup, similar one: http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#201-202 forgetBrowserWindow is going to unregister menus for ex. > > Normally the shutdown method receives a reason as 2nd argument. Should we > avoid calling our unload() if the reason is APP_SHUTDOWN? But I agree, this existing cleanup in devtools-browser is historical, I'll try to avoid doing redundant/unecessary cleanup here on shutdown.

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/20977f40cc82 Unregister DevTools on Add-on disabling. r=jdescottes