542316 - Disable all plugins by default

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

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 1

[Benjamin Stover (:stechz)]

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 2

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

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

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(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 6

[:Gavin Sharp [email: [e-mail]]]

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 7

[Benjamin Stover (:stechz)]

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.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: followup to change pref name

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(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)

Comment 11

[Aakash Desai [:aakashd]]

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

Comment 12

[Aakash Desai [:aakashd]]

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