Fennec 1.0 will disable all plugins and remove the UI to re-enable plugins. Support for plugins will remain in the application and platform. It would be possible for an add-on to re-enable plugins, for example. This patch depends on patch in bug 189378

Comment on attachment 423613 [details] [diff] [review] patch >-/** Call obj.start() or stop() based on boolean pref. */ >-function PreferenceToggle(pref, obj) { >- // Make weak reference just to be sure there are no cycles >- gPrefService.addObserver(pref, this, true); >- this._obj = obj; >- this._pref = pref; >- this._check(); >-} I liked preference toggle because it did one thing and did it well. I had hoped there may be other uses for it. Any reason you merged the two? >+ checkState: function checkState() { checkState is generic. checkPreference? >+ else if (topic == "plugin-change-event") plugin-changed-event >+ <setting pref="plugin.load_plugins" type="bool" title="&enablePlugins.title;" hidden="true"/> Not that I disagree with this, but why hide instead of remove? r+ with nits, assuming you have a good reason for removing preference toggle object.

Comment on attachment 423613 [details] [diff] [review] patch >diff --git a/chrome/content/browser.js b/chrome/content/browser.js > function PluginObserver(bv) { >+ gPrefService.addObserver("plugin.load_plugins", this, true); This isn't live on the backend, so doesn't need to be live here, right? >+ this.checkState(); Similarly I think that means this can just be an unconditional call to start(). > observe: function observe(subject, topic, data) { >+ if (topic == "nsPref:changed" && data == "plugin.load_plugins") >+ this.checkState(); >+ else if (topic == "plugin-change-event") Shouldn't this be "plugin-changed-event"? >+ this.updateCurrentBrowser(); > }, >+ QueryInterface: function QueryInterface(iid) { Need nsIEventHandler too, I think? Though this change goes away if you get rid of the observer.

* removes the <setting> (no need for it) * removes the preference observer code (no need for it either) * "plugin-changed-event"

(In reply to comment #1) > (From update of attachment 423613 [details] [diff] [review]) > I liked preference toggle because it did one thing and did it well. I had > hoped there may be other uses for it. Any reason you merged the two? Yeah but we have no users anymore. Even now, we don't need to watch the preference. It will be nice code to add to Util someday, when we need it. > > >+ checkState: function checkState() { > > checkState is generic. checkPreference? This is gone now anyway. > >+ else if (topic == "plugin-change-event") > > plugin-changed-event Done > >+ <setting pref="plugin.load_plugins" type="bool" title="&enablePlugins.title;" hidden="true"/> > > Not that I disagree with this, but why hide instead of remove? I removed this too, for the same reasons as the preference toggle - no need and it only slows us down.

(In reply to comment #2) > (From update of attachment 423613 [details] [diff] [review]) > >+ gPrefService.addObserver("plugin.load_plugins", this, true); > > This isn't live on the backend, so doesn't need to be live here, right? Removed > >+ this.checkState(); > > Similarly I think that means this can just be an unconditional call to start(). Well, I still check the pref, but only once. No sense firing code that does nothing at the end. > >+ else if (topic == "plugin-change-event") > > Shouldn't this be "plugin-changed-event"? Yep > >+ QueryInterface: function QueryInterface(iid) { > > Need nsIEventHandler too, I think? Though this change goes away if you get rid > of the observer. Gone. nsIEventHanlder seems way more forgiving for some reason.

Comment on attachment 423617 [details] [diff] [review] patch 2 >diff --git a/chrome/content/browser.js b/chrome/content/browser.js >- setPluginState: function(enabled, nameMatch) { HURRAY! > initNewProfile: function initNewProfile() { Remove this given that it's now unused? I guess I don't mind leaving it either. > observe: function observe(subject, topic, data) { >- this.updateCurrentBrowser(); >+ if (topic == "plugin-changed-event") >+ this.updateCurrentBrowser(); Not strictly necessary if you're only registering for that topic, but I suppose it doesn't hurt. >diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul >- <setting pref="plugins.enabled" type="bool" title="&enablePlugins.title;" oninputchanged="Browser.setPluginState(this.value);"/> File a bug on removing the string later? Or I guess we might actually want to re-use it later, so maybe just file a bug on that and note that this string is here to be used when needed?

Comment on attachment 423617 [details] [diff] [review] patch 2 >+ if (topic == "plugin-changed-event") >+ this.updateCurrentBrowser(); If we aren't listening for pref change, don't need this.

pushed: http://hg.mozilla.org/mobile-browser/rev/5f45b121890a

(In reply to comment #9) > Created an attachment (id=423682) [details] > followup to change pref name https://hg.mozilla.org/mobile-browser/rev/0c44494ad715 https://hg.mozilla.org/mobile-browser/rev/269af0364214 (followup to followup)

verified FIXED on build: Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.2pre) Gecko/20100216 Namoroka/3.6.2pre Fennec/1.1a2pre

litmus testcase https://litmus.mozilla.org/show_test.cgi?id=11789 created to regression test this bug.