344982 - camino.enable_plugins doesn't enable/disable properly

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[froodian (Ian Leue)]

If about:config is the only tab open, setting camino.enable_plugins works as expected (on next page load, plugins are disabled, which is reflected in about:plugins).  However, if there's another tab open (even if its only about:blank), this doesn't work.  The prefs.js file is still updated correctly (so the pref is respected upon Camino's relaunch), but plugins are never actually disabled.  This is reproducible for both enabling and disabling the pref. STR:

1. Open about:config
2. In a new tab, open about:blank
3. Change camino.enable_plugins value
4. Load a page with plugins (http://zefrank.com/theshow/archives/2006/06/061606.html)

What happens: Plugins are still on the setting that they were prior to changing the pref
What should happen: Plugin settings should reverse

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Odd; I'm pretty sure this worked correctly when I QA'd timeless's patch in bug 210724.  /me sighs :(

Is there somewhere else active plugins are getting cached?

Comment 2

[timeless]

the current code is initialized once per window and doesn't listen to pref change notifications.

Comment 3

[Chris Lawson (gone)]

(In reply to comment #1)
> Odd; I'm pretty sure this worked correctly when I QA'd timeless's patch in bug
> 210724.

Based on CVS logs and the discussion in bug 167423, it appears this has never worked correctly ("correct" being defined as "how this bug thinks it should work").

The guilty code is 

http://lxr.mozilla.org/mozilla/source/camino/src/browser/BrowserWrapper.mm#160

et seq.; we only check the pref upon initialising a new BrowserWrapper (basically, any time we open a new tab or window).

What we need to be doing is broadcasting a notification whenever the pref is toggled, watching for that notification, and setting the Gecko pref accordingly at that time.

I'll look into this.

cl

Comment 4

[Stuart Morgan]

(In reply to comment #0)
> If about:config is the only tab open, setting camino.enable_plugins works as
> expected (on next page load, plugins are disabled, which is reflected in
> about:plugins).  However, if there's another tab open (even if its only
> about:blank), this doesn't work.

This description is a bit misleading. It works regardless of the number of tabs/windows open, but it doesn't apply to anything that is currently open.

(In reply to comment #2)
> the current code is initialized once per window and doesn't listen to pref
> change notifications.

It's actually once per tab, not once per window.

Comment 5

[Chris Lawson (gone)]

Attachment: Code fixes

Also fixes the dependent bug. Nib coming in a second.

Comment 6

[Chris Lawson (gone)]

Attachment: nib

Adds the checkbox to the prefpane.

Comment 7

[Chris Lawson (gone)]

Targeting back to 1.1 now that we have a fix.

Comment 8

[Chris Lawson (gone)]

Attachment: uses BrowserWrapper to handle everything

Per smorgan's suggestions on IRC this afternoon, make BrowserWrapper do the work on itself rather than using BWC as a notification center.

Comment 9

[Nick Kreeger]

Comment on attachment 247634 [details] [diff] [review]
uses BrowserWrapper to handle everything

+- (void)updatePluginsEnabledState
+{
+  BOOL gotPref;
+  BOOL pluginsEnabled = [[PreferenceManager sharedInstance] getBooleanPref:"camino.enable_plugins" withSuccess:&gotPref];
+  
+  [mBrowserView setProperty:nsIWebBrowserSetup::SETUP_ALLOW_PLUGINS
+                    toValue:((gotPref && !pluginsEnabled) ? PR_FALSE : PR_TRUE)];
+}

First, reverse this so that you aren't checking the negative first. Next, you don't need a conditional to return the PR boolean:
  toValue:(gotPref && pluginsEnabled)];

It seems like you are missing a bunch of your code here, specifically from the Pref-panes. Please post that in a new patch (all together).

Comment 10

[Chris Lawson (gone)]

(In reply to comment #9)

> It seems like you are missing a bunch of your code here, specifically from the
> Pref-panes. Please post that in a new patch (all together).

Hah. Yeah, when I respun the patch, I forgot to diff against WebFeatures. Oops.

The code changes for that are in the original patch, though. Just ignore anything that isn't WebFeatures.h or WebFeatures.mm ;)

cl

Comment 11

[Chris Lawson (gone)]

Attachment: consolidated changes

Comment 12

[Chris Lawson (gone)]

Comment on attachment 247721 [details] [diff] [review]
consolidated changes

pink for sr, anticipating r+ from kreeger

Comment 13

[Nick Kreeger]

Comment on attachment 247721 [details] [diff] [review]
consolidated changes

+// for camino.enable_plugins; needs to match string in WebFeatures.mm
+static NSString* const EnablePluginsChangedNotificationName = @"EnablePluginsChanged";

Use the proper prefix, 'k': kEnablePluginsChangedNotificationName

Comment 14

[froodian (Ian Leue)]

Since we want to give this UI for 1.1, flagging 1.1b1?

Comment 15

[Chris Lawson (gone)]

Comment on attachment 247721 [details] [diff] [review]
consolidated changes

Hrm, something ate the sr request, I think. At any rate, really targeting pink.

Comment 16

[Nick Kreeger]

Comment on attachment 247721 [details] [diff] [review]
consolidated changes

+// for camino.enable_plugins; needs to match string in WebFeatures.mm
+static NSString* const EnablePluginsChangedNotificationName = @"EnablePluginsChanged";
+

Use the 'k' prefix.

 -(void)willResignActiveBrowser
 {
   [mToolTip closeToolTip];
 
-  [[NSNotificationCenter defaultCenter] removeObserver:self name:kOfflineNotificationName object:nil];
+  [[NSNotificationCenter defaultCenter] removeObserver:self
+                                                  name:kOfflineNotificationName
+                                                object:nil];
+                                                
+  [[NSNotificationCenter defaultCenter] removeObserver:self
+                                                  name:EnablePluginsChangedNotificationName
+                                                object:nil];
   [mBrowserView setActive:NO];
 }

 +- (void)updatePluginsEnabledState
 +{
 +  BOOL gotPref;
 +  BOOL pluginsEnabled = [[PreferenceManager sharedInstance] getBooleanPref:"camino.enable_plugins" withSuccess:&gotPref];
 +  
 +  [mBrowserView setProperty:nsIWebBrowserSetup::SETUP_ALLOW_PLUGINS toValue:(!gotPref || pluginsEnabled)];
 +}
 
 Problem with your logic here in |(!gotPref || pluginsEnabled):
   1. If |gotPref| and |pluginsEnabled| both are false, the plugin value is set to true.
   2. If |gotPref| is NO, and |pluginsEnabled| is YES, the plugin value is set to true.
   
I would write this in an easier to read state:
  // you can substitute 'NO' for 'gotPref'
  [mBrowserView setProperty:nsIWebBrowserSetup::SETUP_ALLOW_PLUGINS toValue:(gotPref ? pluginsEnabled : NO)];


   - (void)registerNotificationListener
   {
  -  [[NSNotificationCenter defaultCenter] addObserver:	self
  -                                        selector:     @selector(imageLoadedNotification:)
  -                                        name:         SiteIconLoadNotificationName
  -                                        object:				self];
  +  [[NSNotificationCenter defaultCenter] addObserver:self
  +                                           selector:@selector(imageLoadedNotification:)
  +                                               name:SiteIconLoadNotificationName
  +                                             object:self];

  +  [[NSNotificationCenter defaultCenter] addObserver:self
  +                                           selector:@selector(enablePluginsChanged:)
  +                                               name:EnablePluginsChangedNotificationName
  +                                             object:self];                                           object:self];
  
No need to add |self| to your |enablePluginsChanged:| call, you don't do anything with it.


 // needs to match the string in PreferenceManager.mm
 static NSString* const AdBlockingChangedNotificationName = @"AdBlockingChanged";
 
+// for camino.enable_plugins; needs to match string in BrowserWindowController.mm
+static NSString* const EnablePluginsChangedNotificationName = @"EnablePluginsChanged";
+

It would be nice to see these both change get a 'k' prefix as well.

Comment 17

[Chris Lawson (gone)]

(In reply to comment #16)

>  +- (void)updatePluginsEnabledState
>  +{
>  +  BOOL gotPref;
>  +  BOOL pluginsEnabled = [[PreferenceManager sharedInstance]
> getBooleanPref:"camino.enable_plugins" withSuccess:&gotPref];
>  +  
>  +  [mBrowserView setProperty:nsIWebBrowserSetup::SETUP_ALLOW_PLUGINS
> toValue:(!gotPref || pluginsEnabled)];
>  +}
> 
>  Problem with your logic here in |(!gotPref || pluginsEnabled):
>    1. If |gotPref| and |pluginsEnabled| both are false, the plugin value is set
> to true.
>    2. If |gotPref| is NO, and |pluginsEnabled| is YES, the plugin value is set
> to true.

This is a correct evaluation, but the choice was made to do this for a reason -- if we fail to get the pref for some reason, we should fall back on the default behaviour of allowing plugins. Note that ((pluginsEnabled == NO) && (gotPref == NO)) is an impossible situation.

> No need to add |self| to your |enablePluginsChanged:| call, you don't do
> anything with it.

Again, true, but this is what we do elsewhere in the code. Is there a compelling reason *not* to pass an object with this call?

I'll fix up the constants when I respin.

cl

Comment 18

[Chris Lawson (gone)]

Attachment: fixed

Here we go. This should do it. Nib shouldn't need any changing since the code changes were all back-end stuff.

Comment 19

[Nick Kreeger]

Comment on attachment 251656 [details] [diff] [review]
fixed

>+  // if we can't get the pref, fall back on default of allowing plugins

Please me a little bit clearer on this.
r=me

Comment 20

[Chris Lawson (gone)]

Attachment: addresses nits

transferring r+ from other patch, targeting smorgan for sr per pink via AIM.

Comment 21

[Stuart Morgan]

Comment on attachment 251661 [details] [diff] [review]
addresses nits

> - (void)didBecomeActiveBrowser
> {
...
>+  [self updatePluginsEnabledState];
...
>+  [[NSNotificationCenter defaultCenter] addObserver:self
>+                                           selector:@selector(enablePluginsChanged:)
>+                                               name:kEnablePluginsChangedNotificationName
>+                                             object:nil];
> }
> 
> -(void)willResignActiveBrowser
> {
...
>+  [[NSNotificationCenter defaultCenter] removeObserver:self
>+                                                  name:kEnablePluginsChangedNotificationName
>+                                                object:nil];
>   [mBrowserView setActive:NO];
> }

You set up to listen for this notification at init, through registerNotificationListener; why keep registering and unregistering? Why check the pref and set the plugin enabled state every time a tab is made active instead of only when the pref changes, when the expectation is that it will change rarely?

(If the answer to the latter is just that it's what offline mode did, I think the reason there was probably that it needs to manipulate window titles, which only makes sense for the foreground tabs.)

>+  // If we can't get the pref, ensure we don't leave plugins disabled.

s/don't leave plugins disabled/leave plugins enabled/

>+  [mBrowserView setProperty:nsIWebBrowserSetup::SETUP_ALLOW_PLUGINS toValue:(!gotPref || pluginsEnabled)];

I agree with kreeger on the style here; (gotPref ? pluginsEnabled : YES) is easier to read.


>+// for camino.enable_plugins; needs to match string in BrowserWindowController.mm

BrowserWrapper.mm

>+  BOOL pluginsEnabled = [self getBooleanPref:"camino.enable_plugins" withSuccess:&gotPref] && gotPref;
>+  [mEnablePlugins setState:pluginsEnabled];

This doesn't match the logic in BrowserWrapper. If somehow getting the pref started failing everywhere, plugins would be on, but the checkbox would be off.

>+// Set pref if plugins are enabled

This needs to be reworded; this sounds like it only sets the pref if it's changing it to on.

>+  [self setPref:"camino.enable_plugins" toBoolean:[sender state] == NSOnState];

() around |[sender state] == NSOnState]|

Comment 22

[Chris Lawson (gone)]

Attachment: fixed

Moving r+ from other patch.

This addresses all sr comments.

>>+// Set pref if plugins are enabled

>This needs to be reworded; this sounds like it only sets
> the pref if it's changing it to on.

I took the liberty of fixing the description of the similar methods too, since we had three methods in there with the same problem.

>>+  [self setPref:"camino.enable_plugins" toBoolean:[sender state] == NSOnState];

>() around |[sender state] == NSOnState]|

Likewise, I took the liberty of fixing this throughout the file. There were several other places that needed it too.

cl

Comment 23

[Stuart Morgan]

Comment on attachment 251875 [details] [diff] [review]
fixed


> - (void)didBecomeActiveBrowser
> {
>   [self ensureContentClickListeners];
>   [self updateOfflineStatus];
>+  [self updatePluginsEnabledState];

This change shouldn't be here any more; sr=smorgan with that removed.

Comment 24

[Stuart Morgan]

Attachment: sr version

Version with that changed removed, since I don't know if cl would have a chance to respin anytime soon.

Comment 25

[froodian (Ian Leue)]

Checked in on trunk and MOZILLA_1_8_BRANCH.