Closed Bug 620776 Opened 9 years ago Closed 8 years ago

Add UI for new plugin.disable preference

Categories

(SeaMonkey :: Preferences, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: neil, Assigned: ewong)

References

Details

(Keywords: helpwanted, Whiteboard: [good first bug])

Attachments

(2 files, 6 obsolete files)

Bug 189378 added a preference to disable all preferences globally.
We could add UI for this to the Scripts & Plugins preference panel.
I'd rather not have something disable all preferences globally, but otherwise confirming valid RFE. ;-)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Where should this new plugin.disabled UI be?  

Under Advanced->Scripts&Plugins?  But then it looks pretty full unless
we decrease the height of the enable/disable list of prefs box (the
one starting with "Move or resize existing windows"...)
(In reply to comment #2)
> Where should this new plugin.disabled UI be?  

Several options:
a) directly on the Advanced pane, below "Enable Java" (same groupbox)
b) on the Scripts & Plugins pane (which already has it in the name, so this should be preferred), as the general case of "Enable Plugins for" (disabling that if the pref discussed here is true). I'd expect the box containing the checkboxes below "Allow scripts to" to reduce itself in vertical size itself.
(In reply to comment #3)
> (In reply to comment #2)
> > Where should this new plugin.disabled UI be?  
> b) on the Scripts & Plugins pane (which already has it in the name, so this
> should be preferred)
SeaMonkey 1.x's preference pane had a placeholder for it there (added by bug 147877). See also bug 189378 comment 0!
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #584930 - Flags: review?(iann_bugzilla)
Comment on attachment 584930 [details] [diff] [review]
Added UI for plugins.disabled preference. (v1)

>+function updatePluginPreferences() {
>+  var test = document.getElementById("pluginPreferences");
Could do with a better variable name here. Perhaps it might be better to get the relevant children by TagName as well.
>+  let i;
Just put this within the for declaration
>+  for (i=1; i <= test.childElementCount-1; i++){
Nit: need spacing around = and use < then don't need the -1
>+    let loopChild = test.children[i]
>+    loopChild.disabled = document.getElementById("pluginsDisabled").checked;
Would probably be better to get the checked status outside of the loop and then use it within the loop.
r- for the moment as I would like to see the new patch.
Attachment #584930 - Flags: review?(iann_bugzilla) → review-
> +  var test = document.getElementById("pluginPreferences");
"test" isn't terribly edifying. how about "plugins"?

> +  let i;
> +  
> +  for (i=1; i <= test.childElementCount-1; i++){

Use for (let i = 1; ... also spaces around the "="

https://developer.mozilla.org/en/DOM/Element.childElementCount
> Warning: Don't use it. Support might be dropped in future versions of Gecko. See bug 673790.
Use .children instead. e.g.

for (let i = 0; i < plugins.children.length -1; i++)

Also did you also intend to disable the <caption> inside the groupbox?

> +    let loopChild = test.children[i]
suggest: |let child =| loopChild is redundant since you are already inside a loop.

> +    loopChild.disabled = document.getElementById("pluginsDisabled").checked;

Move |document.getElementById("pluginsDisabled").checked| out of the loop.

> +  }

> +      <preference id="plugins.disabled"
> +                  name="plugins.disabled"
> +                  type="bool"

As far as I can tell it's |plugin.disable| singular without any "s", and disable rather than disableD.

http://hg.mozilla.org/mozilla-central/annotate/0d684c34d1e4/dom/plugins/base/nsPluginHost.cpp#l402
Comment on attachment 584930 [details] [diff] [review]
Added UI for plugins.disabled preference. (v1)

>+  
Nit: trailing whitespace

>+  for (i=1; i <= test.childElementCount-1; i++){
[Because this is XUL you don't have to worry about non-elements; just use childCount and childNodes everywhere.]

> <!ENTITY enablePlugin.label              "Enable Plugins for">
As I understand it, the original prefpane was to look something like this:
+- Enable Plugins for --+
| [X] Browser           |
| [ ] Mail & Newsgroups |
+-----------------------+
The Mail & Newsgroups checkbox disabling would still apply of course, but maybe it belongs in mailPrefsOverlay instead.
Attachment #584930 - Attachment is obsolete: true
Attachment #585145 - Flags: review?(iann_bugzilla)
Attachment #585145 - Attachment is obsolete: true
Attachment #585176 - Flags: review?(iann_bugzilla)
Attachment #585145 - Flags: review?(iann_bugzilla)
Attachment #585176 - Attachment is obsolete: true
Attachment #585249 - Flags: review?(iann_bugzilla)
Attachment #585176 - Flags: review?(iann_bugzilla)
Comment on attachment 585249 [details] [diff] [review]
Add UI for new plugins.disable preference. (v4)

>+  for (let i = 1; i < pluginGroup.children.length; i++){
Nit: space between ) and { please.
>+    let child = pluginGroup.children[i];
>+    child.disabled = pluginDisableChecked;
It still seems readable with these combined into a single line (then you won't need {} either)
>+  }
I presume caption will always be pluginGroup.children[0]

Neil did suggest using childNodes so might be worth checking he is happy for you to use children instead, both give a list of XULElements - one is a NodeList the other HTMLCollection.
Attachment #585249 - Flags: review?(iann_bugzilla) → review+
Both return NodeList(s)
> Both return NodeList(s)
Documentation bug. I've fixed DevMo.
used getElementsByTagName()
Attachment #585249 - Attachment is obsolete: true
Attachment #585279 - Flags: review?(iann_bugzilla)
Attachment #585313 - Flags: feedback?(iann_bugzilla)
Comment on attachment 585313 [details] [diff] [review]
What I was thinking of

>+++ b/suite/common/pref/pref-scripts.xul
>+    <groupbox id="pluginPreferences">
>       <caption label="&enablePlugin.label;"/>
>+
>+      <checkbox id="pluginAllowNavigator"
>+                label="&enablePluginNavigator.label;"
>+                accesskey="&enablePluginNavigator.accesskey;"
>+                preference="plugin.disable"/>
Hmmm, this doesn't just control Browser, so perhaps Suite or SeaMonkey or something like "Browser and potentially other components listed below:"
Then anything overlayed should be indented as is usually the case when there is a dependency.

Generally f+ though if the above can be sorted.

Does Lightning or anything else overlay this?
Attachment #585313 - Flags: feedback?(iann_bugzilla) → feedback+
> Does Lightning or anything else overlay this?
Lightning: No, it adds it's own prefpanes to the tree.
Anything else: only the SHADOW knows! But otherwise highly unlikely.
(In reply to Ian Neal from comment #18)
> Hmmm, this doesn't just control Browser, so perhaps Suite or SeaMonkey
Well, that could be said of the Scripts preference too...
Both patches work fine. I didn't detect any peculiarities.
Edmund's patch can be improved by adding
+pref("plugin.disable", false);
to browser-prefs.js and inverting the preference as in Neil's patch. However I prefer Edmunds UI.

>> Hmmm, this doesn't just control Browser, so perhaps Suite or SeaMonkey
Agreed.
> Well, that could be said of the Scripts preference too...
Separate bug?
Attachment #585279 - Attachment is obsolete: true
Attachment #586750 - Flags: review?(iann_bugzilla)
Attachment #585279 - Flags: review?(iann_bugzilla)
Comment on attachment 586750 [details] [diff] [review]
Add UI for new plugins.disable preference. (v6)

>+    <groupbox id="pluginPreferences">
>       <caption label="&enablePlugin.label;"/>
>+      <checkbox id="pluginForSuite"
>+                class="indent"
You don't need to indent the suite checkbox, only the mailnews one.
>+                label="&enablePluginForSuite.label;"
>+                accesskey="&enablePluginForSuite.accesskey;"
>+                preference="plugin.disable"/>
>+
r=me with that fixed
Attachment #586750 - Flags: review?(iann_bugzilla) → review+
nits fixed.
Attachment #586750 - Attachment is obsolete: true
Attachment #588629 - Flags: ui-review?(neil)
Attachment #588629 - Flags: review+
Attachment #588629 - Flags: ui-review?(neil) → ui-review+
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/600face6bd77
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Summary: Add UI for new plugins.disabled preference → Add UI for new plugin.disable preference
Attachment #588629 - Attachment description: Add UI for new plugins.disable preference. (v7) → Add UI for new plugin.disable preference. (v7)
Blocks: 725973
You need to log in before you can comment on or make changes to this bug.