1133000 - Expose the protected-mode toggle in the Firefox UI

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Benjamin Smedberg]

Attachment: Flash-protectedmode-UI.png

Currently we have a hook which disabled Flash protected mode, but no UI to expose this to users.

Since turning protected mode off for all users may be a security risk we are unwilling to bear, we are considering offering users a way to opt into this option if/when they experience a Flash hang. It may also be a useful option for users with screen readers or other accessibility tools, since those often cause Flash issues.

Once we do that, we still want to have a UI-exposed way to reverse that decision. That is what this bug tracks.

I am looking for a fast turnaround for this, since it may be something we will want to uplift into 37.

Comment 1

[Benjamin Smedberg]

Attachment: bug1133000-flash-toggle.patch

Matt, this is intended as a code review while UX review of the strings is still pending.

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8564313 [details] [diff] [review]
bug1133000-flash-toggle.patch

Review of attachment 8564313 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but I'm not that familiar with this code so you may want Blair to also take a look.

::: toolkit/mozapps/extensions/internal/PluginProvider.jsm
@@ +89,5 @@
> +        // Protected mode is win32-only, not win64
> +        let showProtectedModePref = plugin.isFlashPlugin &&
> +          Services.appinfo.XPCOMABI == "x86-msvc";
> +        aSubject.getElementById("pluginDisableProtectedMode")
> +          .setAttribute("collapsed", showProtectedModePref ? "" : "true");

I initially wondered if this could cause unintentionally changing the pref while viewing a non-Flash plugin and having the pref flipped from other code. I thought that maybe we save the pref upon navigation but it seems like the pref changes live with the checkbox so this shouldn't be a problem.

Nit: I'm unsure if this is the preferred indentation style for the module as I don't see another line that had to do similar wrapping and I'm new to the code.

@@ +494,5 @@
>      }
>      return permissions;
>    });
> +
> +  this.isFlashPlugin = false;

I'm fine with this custom property if Blair is.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8564313 [details] [diff] [review]
bug1133000-flash-toggle.patch

Review of attachment 8564313 [details] [diff] [review]:
-----------------------------------------------------------------

Sadly, I don't think we have a reliable way to test this.... normally we'd mock nsPluginHost to test PluginProvider, but we can't do that when we need a browser-chrome test. And normally for browser-chrome tests we use the test-plugin, which doesn't help here :\

::: toolkit/mozapps/extensions/content/pluginPrefs.xul
@@ +13,5 @@
>    <setting type="control" title="&plugin.mimeTypes;">
>      <label class="text-list" id="pluginMimeTypes"/>
>    </setting>
> +  <setting type="bool" pref="dom.ipc.plugins.flash.disable-protected-mode"
> +           title="&plugin.flashProtectedMode.label;" id="pluginDisableProtectedMode">

As a general rule, checking a checkbox to disable something confuses people. It's not how people expect checkboxes to work. So the label here should be "Enable XYZ". And adding inverted="true" to the <setting> will invert how the pref is read/set.

::: toolkit/mozapps/extensions/internal/PluginProvider.jsm
@@ +494,5 @@
>      }
>      return permissions;
>    });
> +
> +  this.isFlashPlugin = false;

I'm not fine with a custom property :)

Can use a closure for optionsType, and observe() is already iterating over pluginMimeTypes.

@@ +496,5 @@
>    });
> +
> +  this.isFlashPlugin = false;
> +  for (let type of this.pluginMimeTypes) {
> +    if (type.type == "application/x-shockwave-flash") {

Make this a const at the top of the file? Makes it more obvious that we're special-casing Flash - going to be too easy to miss otherwise.

Comment 4

[Benjamin Smedberg]

Attachment: bug1133000-flash-toggle.patch

Blair, I don't have final strings from UX, and so I haven't yet made the change about an inverted pref, but I believe this covers your other concerns. Please review the rest and I'll get final strings with UX tomorrow for landing.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8566692 [details] [diff] [review]
bug1133000-flash-toggle.patch

Review of attachment 8566692 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with pref inversion/strings proviso.

Comment 6

[Madhava Enros [:madhava]]

re: the issue of this making a positive checkbox state represent a negative state (i.e. check to disable" -- why don't we change the string to "Enable Adobe Flash protected mode" and make the corresponding inversion when we check the box.

Comment 7

[Madhava Enros [:madhava]]

Also - can we strike the grey explanatory text and replace it with a "Learn More" link to the same content we use from the infobar? The trade-off needs more context in its explanation, and that's what we're providing on the Learn More page.

Comment 8

[Madhava Enros [:madhava]]

(In reply to Madhava Enros [:madhava] from comment #6)
> re: the issue of this making a positive checkbox state represent a negative
> state (i.e. check to disable" -- why don't we change the string to "Enable
> Adobe Flash protected mode" and make the corresponding inversion when we
> check the box.

sorry - that's ambiguous -- we should just make the checkbox do the right thing, given the new string.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/886bc136c20b

Comment 10

[Jim Jeffery not reading bug-mail 1/2/11]

Clicking 'Learn More' which is shown in 'Blue' as if its a link goes to Mozilla Page not Found error.

Intended or ?

Comment 11

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #10)
> Clicking 'Learn More' which is shown in 'Blue' as if its a link goes to
> Mozilla Page not Found error.
> 
> Intended or ?

forgot to include URL: https://support.mozilla.org/en-US/kb/flash-protected-mode-settings

Comment 12

[Benjamin Smedberg]

Yes, the page doesn't exist yet but it will.