Last Comment Bug 620776 - Add UI for new plugin.disable preference
: Add UI for new plugin.disable preference
Status: RESOLVED FIXED
[good first bug]
: helpwanted
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Edmund Wong (:ewong)
:
:
Mentors:
Depends on:
Blocks: 718302 725973
  Show dependency treegraph
 
Reported: 2010-12-21 13:26 PST by neil@parkwaycc.co.uk
Modified: 2012-02-10 05:52 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Added UI for plugins.disabled preference. (v1) (2.35 KB, patch)
2011-12-30 01:05 PST, Edmund Wong (:ewong)
iann_bugzilla: review-
Details | Diff | Splinter Review
Add UI for new plugins.disable preference. (v2) (3.91 KB, patch)
2011-12-31 08:22 PST, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Add UI for new plugins.disable preference. (v3) (3.90 KB, patch)
2011-12-31 21:53 PST, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Add UI for new plugins.disable preference. (v4) (3.90 KB, patch)
2012-01-01 20:01 PST, Edmund Wong (:ewong)
iann_bugzilla: review+
Details | Diff | Splinter Review
Add UI for new plugins.disable preference. (v5) (3.98 KB, patch)
2012-01-02 06:22 PST, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
What I was thinking of (5.38 KB, patch)
2012-01-02 11:08 PST, neil@parkwaycc.co.uk
iann_bugzilla: feedback+
Details | Diff | Splinter Review
Add UI for new plugins.disable preference. (v6) (5.70 KB, patch)
2012-01-07 19:32 PST, Edmund Wong (:ewong)
iann_bugzilla: review+
Details | Diff | Splinter Review
Add UI for new plugin.disable preference. (v7) (5.67 KB, text/plain)
2012-01-14 05:25 PST, Edmund Wong (:ewong)
ewong: review+
neil: ui‑review+
Details

Description neil@parkwaycc.co.uk 2010-12-21 13:26:11 PST
Bug 189378 added a preference to disable all preferences globally.
We could add UI for this to the Scripts & Plugins preference panel.
Comment 1 Jens Hatlak (:InvisibleSmiley) 2010-12-21 13:31:43 PST
I'd rather not have something disable all preferences globally, but otherwise confirming valid RFE. ;-)
Comment 2 Edmund Wong (:ewong) 2011-05-22 05:27:44 PDT
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"...)
Comment 3 Jens Hatlak (:InvisibleSmiley) 2011-05-22 05:32:59 PDT
(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.
Comment 4 neil@parkwaycc.co.uk 2011-05-22 06:50:53 PDT
(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!
Comment 5 Edmund Wong (:ewong) 2011-12-30 01:05:39 PST
Created attachment 584930 [details] [diff] [review]
Added UI for plugins.disabled preference. (v1)
Comment 6 Ian Neal 2011-12-30 06:49:41 PST
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.
Comment 7 Philip Chee 2011-12-30 08:05:40 PST
> +  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 8 neil@parkwaycc.co.uk 2011-12-30 08:55:06 PST
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.
Comment 9 Edmund Wong (:ewong) 2011-12-31 08:22:16 PST
Created attachment 585145 [details] [diff] [review]
Add UI for new plugins.disable preference. (v2)
Comment 10 Edmund Wong (:ewong) 2011-12-31 21:53:03 PST
Created attachment 585176 [details] [diff] [review]
Add UI for new plugins.disable preference. (v3)
Comment 11 Edmund Wong (:ewong) 2012-01-01 20:01:06 PST
Created attachment 585249 [details] [diff] [review]
Add UI for new plugins.disable preference. (v4)
Comment 12 Ian Neal 2012-01-01 21:24:53 PST
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.
Comment 13 Philip Chee 2012-01-02 04:37:28 PST
Both return NodeList(s)
Comment 15 Philip Chee 2012-01-02 04:56:26 PST
> Both return NodeList(s)
Documentation bug. I've fixed DevMo.
Comment 16 Edmund Wong (:ewong) 2012-01-02 06:22:34 PST
Created attachment 585279 [details] [diff] [review]
Add UI for new plugins.disable preference. (v5)

used getElementsByTagName()
Comment 17 neil@parkwaycc.co.uk 2012-01-02 11:08:10 PST
Created attachment 585313 [details] [diff] [review]
What I was thinking of
Comment 18 Ian Neal 2012-01-02 14:58:13 PST
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?
Comment 19 Philip Chee 2012-01-03 11:09:38 PST
> 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.
Comment 20 neil@parkwaycc.co.uk 2012-01-04 04:02:32 PST
(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...
Comment 21 Philip Chee 2012-01-05 11:28:25 PST
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?
Comment 22 Edmund Wong (:ewong) 2012-01-07 19:32:54 PST
Created attachment 586750 [details] [diff] [review]
Add UI for new plugins.disable preference. (v6)
Comment 23 Ian Neal 2012-01-08 13:08:50 PST
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
Comment 24 Edmund Wong (:ewong) 2012-01-14 05:25:27 PST
Created attachment 588629 [details]
Add UI for new plugin.disable preference. (v7)

nits fixed.
Comment 25 Edmund Wong (:ewong) 2012-01-15 07:24:27 PST
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/600face6bd77

Note You need to log in before you can comment on or make changes to this bug.