Closed Bug 1470082 Opened 6 years ago Closed 6 years ago

Change autoplay checkbox to combobox

Categories

(Firefox :: Settings UI, enhancement, P1)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: daleharvey, Assigned: daleharvey)

References

Details

Attachments

(1 file)

Bug 1470082 - Change autoplay checkbox to combobox.
59 bytes, text/x-review-board-request
cpearce
: review+
johannh
: review+
flod
: review+
Details 
Currently we can only turn on / off block autoplay, we want to add the ability to set it to "Always Allow" "Always Deny" "Always Ask"
Assignee: nobody → dharvey
Blocks: 1435083
Priority: -- → P1
Hey Bryant, as mentioned on Slack I am blocked on a spec here, mostly implemented it to look like https://i.imgur.com/0IpEGKx.png, but thats just a guess that I am not certain is best
Flags: needinfo?(bmao)
Spec is updated @ https://mozilla.invisionapp.com/share/SCMD6ZY6D79#/screens/306583368 (via slack)
Flags: needinfo?(bmao)
Comment on attachment 8990488 [details]
Bug 1470082 - Change autoplay checkbox to combobox.

So made a new pref for this, |media.autoplay.enabled.state|, reusing .enabled seemed confusing since it indicated a boolean and since we havent released figured it wasnt too much of a backwards compat issue to use a new pref

One change I would like to make here is to use constants for ALLOW, ASK, BLOCK similiar to https://dxr.mozilla.org/mozilla-central/source/browser/modules/SitePermissions.jsm#626, but wasnt sure if there was anywhere sensible to put them or whether making a whole new service for them seemed too much

Otherwise I think I probably need to add a new test for the BLOCK path and should be good?
Attachment #8990488 - Flags: feedback?(cpearce)
This patch seems to work for ALLOW / DENY but not ASK, looking into it
A clause got missed during rebase, fixed now and didnt realise about |media.autoplay.ask-permission|

Now that I see |media.autoplay.ask-permission|, it would have been possible to use that combined with .enabled, however having 2 prefs being used to indicate what is essentially a single value seems not as clear as possible, especially with the interaction with user-gestures-needed. 

I would say to remove the ask-permission pref as its covered by media.autoplay.enabled.state=ASK in this patch, but happy to defer to what you think is best

(and didnt mean to remove the feedback)
Flags: needinfo?(cpearce)
(In reply to Dale Harvey (:daleharvey) from comment #8)
> A clause got missed during rebase, fixed now and didnt realise about
> |media.autoplay.ask-permission|
> 
> Now that I see |media.autoplay.ask-permission|, it would have been possible
> to use that combined with .enabled, however having 2 prefs being used to
> indicate what is essentially a single value seems not as clear as possible,
> especially with the interaction with user-gestures-needed. 
> 
> I would say to remove the ask-permission pref as its covered by
> media.autoplay.enabled.state=ASK in this patch, but happy to defer to what
> you think is best
> 
> (and didnt mean to remove the feedback)

My intention with media.autoplay.ask-permission was to give us a mechanism to disable Firefox's asking permission to autoplay functionality, without affecting the rest of the autoplay behaviour.

So if the shield study shows that asking permission is not liked, or for whatever reason we decide we don't like the prompt, we could just flip the pref, and Firefox would never ask for permission to autoplay, and the front end UI code wouldn't mention anything about asking permission.

So media.autoplay.ask-permission is intended to be whether Firefox *can* be configured to ask for permission, not whether it *is* configured to ask for permission.

I was going to remove that pref once we were convinced we wanted to be asking for permission.
Flags: needinfo?(cpearce)
Comment on attachment 8990488 [details]
Bug 1470082 - Change autoplay checkbox to combobox.

https://reviewboard.mozilla.org/r/255562/#review262402

::: browser/components/preferences/in-content/privacy.js:80
(Diff revision 3)
>    { id: "privacy.sanitize.timeSpan", type: "int" },
>    // Do not track
>    { id: "privacy.donottrackheader.enabled", type: "bool" },
>  
>    // Media
> -  { id: "media.autoplay.enabled", type: "bool" },
> +  { id: "media.autoplay.enabled.state", type: "int" },

I a more appropriate name would be media.autoplay.default, as what you're configuring here is what what happens in the case where a document isn't allowed to play by any other measure.

::: browser/components/preferences/in-content/privacy.js:80
(Diff revision 3)
>    { id: "privacy.sanitize.timeSpan", type: "int" },
>    // Do not track
>    { id: "privacy.donottrackheader.enabled", type: "bool" },
>  
>    // Media
> -  { id: "media.autoplay.enabled", type: "bool" },
> +  { id: "media.autoplay.enabled.state", type: "int" },

Note the Android UI also sets this media.autoplay.enabled, but we're not going to have UI for prompting on Android in the MVP, so android will need to have media.autoplay.ask-permission=false and will need to set this pref to Allow or Block.

::: dom/media/AutoplayPolicy.cpp:96
(Diff revision 3)
>  /* static */ Authorization
>  AutoplayPolicy::IsAllowedToPlay(const HTMLMediaElement& aElement)
>  {
> -  if (Preferences::GetBool("media.autoplay.enabled")) {
> +  int autoplayState = Preferences::GetInt("media.autoplay.enabled.state", 0);
> +
> +  if (autoplayState == 1) {

The 1 and 2 here should be defined constants somewhere.

::: dom/media/AutoplayPolicy.cpp:101
(Diff revision 3)
> +  if (autoplayState == 1) {
>      return Authorization::Allowed;
>    }
>  
> -  // TODO : this old way would be removed when user-gestures-needed becomes
> -  // as a default option to block autoplay.
> +  if (autoplayState == 2) {
> +    return Authorization::Blocked;

Note that we check *all* plays against the autoplay policy.

Even plays that occur while the user is clicking the  "play" button.

So if the user has set their autoplay  default to "block", with this logic change here they've actually disabled media playback entirely in their browser, which is not what we want here. The user should always be able to play if they click the play button, and whitelisted sites should be able to play too.

So remove this block.

::: dom/media/AutoplayPolicy.cpp:120
(Diff revision 3)
>  
>    if (IsWindowAllowedToPlay(aElement.OwnerDoc()->GetInnerWindow())) {
>      return Authorization::Allowed;
>    }
>  
>    if (Preferences::GetBool("media.autoplay.ask-permission", false)) {

This block could just check if (autoPlayState==0), and inside that you could assert that media.autoplay.ask-permission is true.

::: dom/media/AutoplayPolicy.cpp:124
(Diff revision 3)
>  
>    if (Preferences::GetBool("media.autoplay.ask-permission", false)) {
>      return Authorization::Prompt;
>    }
>  
>    return Authorization::Blocked;

You should be able to assert here that autoPlayState==2.

::: modules/libpref/init/all.js:568
(Diff revision 3)
>  // to keep up under load. Useful for tests but beware of memory consumption!
>  pref("media.recorder.video.frame_drops", true);
>  
> -// Whether to autostart a media element with an |autoplay| attribute
> -pref("media.autoplay.enabled", true);
> -
> +// Whether to autostart a media element with an |autoplay| attribute.
> +// ASK=0, ALLOW=1, DENY=2
> +pref("media.autoplay.enabled.state", 1);

Best to call this pref media.autoplay.default, as that's more representative of how this pref is used.

I also think you should make the front end UI hide the option to prompt if media.autoplay.ask-permission is false, so we can turn off prompting if we decide based on the results of the shield study that we don't want to ship this.
> The 1 and 2 here should be defined constants somewhere.

Yeh was asking in the previous comment (https://bugzilla.mozilla.org/show_bug.cgi?id=1470082#c5) where do you think they should be defined? in a new service so we can use them in c++ and js, or in Autoplay.cpp and maybe privacy.js?
Flags: needinfo?(cpearce)
> This block could just check if (autoPlayState==0), and inside that you could 
> assert that media.autoplay.ask-permission is true.

asserting the value of a pref the user could configure seems wrong, in my patch I just check both values, but ifs thats wrong I can change it. Otherwise its just a question of where the constants are defined
Comment on attachment 8990488 [details]
Bug 1470082 - Change autoplay checkbox to combobox.

https://reviewboard.mozilla.org/r/255562/#review262686


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/preferences/in-content/privacy.js:991
(Diff revision 4)
>     * Show the controls for the new media autoplay behaviour behind a pref for now
>     */
>    updateAutoplayMediaControlsVisibility() {
>      document.getElementById("autoplayMediaBox").hidden =
> -      !Services.prefs.getBoolPref("media.autoplay.enabled.user-gestures-needed", false);
> +      !(Services.prefs.getBoolPref("media.autoplay.ask-permission", false) &&
> +        Services.prefs.getBoolPref("media.autoplay.enabled.user-gestures-needed", false);

Error: Parsing error: Unexpected token ; [eslint]
Comment on attachment 8990488 [details]
Bug 1470082 - Change autoplay checkbox to combobox.

https://reviewboard.mozilla.org/r/255562/#review262722

::: dom/media/AutoplayPolicy.cpp
(Diff revision 4)
>  }
>  
>  /* static */ Authorization
>  AutoplayPolicy::IsAllowedToPlay(const HTMLMediaElement& aElement)
>  {
> -  if (Preferences::GetBool("media.autoplay.enabled")) {

You still need to handle the media.autoplay.default==allowed case, otherwise people who have set the autoplay default always allowed won't always have their autoplay be allowed.

media.autoplay.default specifies what to do in the case where a document isn't otherwise allowed to play by gesture activation etc. I think it would be clearer to convert the value of media.autoplay.default to the corresponding value of the Authorization enum (which is defined in AutoplayPolicy.h, and we'll need to ensure its values align with the pref values).

But we also need to handle the case where autoplay is disabled in the case where media.autoplay.enabled.user-gestures-needed=false (the legacy block autoplay behaviour).

So please make this:

    static Authorization
    DefaultAutoplayBehaviour()
    {
      int prefValue = Preferences::GetInt("media.autoplay.default", 1);
      if (prefValue <= Authorization::Allowed || prefValue >= Authorization::Prompt) {
        // Invalid enum values are just converted to Allowed.
        return Authorization::Allowed;
      }
      return Authorization(prefValue);
    }

    /* static */ Authorization
    AutoplayPolicy::IsAllowedToPlay(const HTMLMediaElement& aElement)
    {
      const Authorization autoplayDefault = DefaultAutoplayBehaviour();
      if (!Preferences::GetBool("media.autoplay.enabled.user-gestures-needed", false)) {
        // If element is blessed, it would always be allowed to play().
        return (autoplayDefault == Authorization::Allowed ||
                aElement.IsBlessed() ||
                EventStateManager::IsHandlingUserInput())
                  ? Authorization::Allowed : Authorization::Blocked;
      }

      // Muted content
      if (aElement.Volume() == 0.0 || aElement.Muted()) {
        return Authorization::Allowed;
      }

      if (IsWindowAllowedToPlay(aElement.OwnerDoc()->GetInnerWindow())) {
        return Authorization::Allowed;
      }

      return autoplayDefault;
    }

::: dom/media/AutoplayPolicy.cpp:130
(Diff revision 4)
>  }
>  
>  /* static */ bool
>  AutoplayPolicy::IsAudioContextAllowedToPlay(NotNull<AudioContext*> aContext)
>  {
> -  if (Preferences::GetBool("media.autoplay.enabled")) {
> +  int autoplayState = Preferences::GetInt("media.autoplay.default", ASK);

We've not converted WebAudio to asking for permission yet, so we can just check whether DefaultAutoplayBehaviour()==Authorization::Allowed and return true if so, otherwise continue with the remainder of the function.

::: dom/media/AutoplayPolicy.cpp:136
(Diff revision 4)
> +
> +  if (autoplayState == ALLOW) {
>      return true;
>    }
>  
> +  if (autoplayState == DENY) {

Again, if you return false in this case, you'll prevent web audio plays that were gesture activated or in whitelisted documents.

::: dom/media/webaudio/test/test_notAllowedToStartAudioContextGC.html:28
(Diff revision 4)
>  SpecialPowers.addAsyncObserver(observer, "webaudio-node-demise", false);
>  SimpleTest.registerCleanupFunction(function() {
>    SpecialPowers.removeAsyncObserver(observer, "webaudio-node-demise");
>  });
>  
> -SpecialPowers.pushPrefEnv({"set": [["media.autoplay.enabled", false],
> +SpecialPowers.pushPrefEnv({"set": [["media.autoplay.default", 0],

All the tests that set media.autoplay.enabled to false should be setting media.autoplay.default to 3, which is the corresponding enum value.

::: modules/libpref/init/all.js:567
(Diff revision 4)
>  // Whether MediaRecorder's video encoder should allow dropping frames in order
>  // to keep up under load. Useful for tests but beware of memory consumption!
>  pref("media.recorder.video.frame_drops", true);
>  
> -// Whether to autostart a media element with an |autoplay| attribute
> -pref("media.autoplay.enabled", true);
> +// Whether to autostart a media element with an |autoplay| attribute.
> +// ASK=0, ALLOW=1, DENY=2

s/DENY/BLOCK/

The permissions manager uses the "block" terminology rather than "deny".
(In reply to Dale Harvey (:daleharvey) from comment #13)
> > This block could just check if (autoPlayState==0), and inside that you could 
> > assert that media.autoplay.ask-permission is true.
> 
> asserting the value of a pref the user could configure seems wrong, in my
> patch I just check both values, but ifs thats wrong I can change it.
> Otherwise its just a question of where the constants are defined

I see your point; it's not an invariant of the code, but the environment we created for the code. My review comments I suggest converting the default to the enum value we're using in the C++ code which we can validate the perf value and return Allowed if the pref value is invalid.
Flags: needinfo?(cpearce)
Comment on attachment 8990488 [details]
Bug 1470082 - Change autoplay checkbox to combobox.

https://reviewboard.mozilla.org/r/255562/#review263576

::: browser/components/preferences/in-content/privacy.js
(Diff revision 6)
>  
>  
>    // MEDIA
>  
>    /**
> -   * media.autoplay.enabled works the opposite to most of the other preferences.

I think if media.autoplay.enabled.user-gestures-needed is true, we should show the autoplay media UI here (i.e. not check ask-permission in the visibility function here).

If ask-permission is true, we should show the combo-box like you've implemented.

If ask-permission is false, we should show the old check box, and that toggles media.autoplay.default between BLOCK and ALLOW.

We'd like to be able to disable asking for permission without disabling the rest of the gestures-needed block autoplay behaviour.

::: dom/media/test/test_autoplay_policy.html:17
(Diff revision 6)
>  
>  <script>
>  
>  let manager = new MediaTestManager;
>  
> -gTestPrefs.push(["media.autoplay.enabled", false],
> +gTestPrefs.push(["media.autoplay.default", Ci.nsIAutoplay.PROMPT],

Please convert ["media.autoplay.enabled", false] into ["media.autoplay.default", Ci.nsIAutoplay.BLOCKED], unless the test also sets media.autoplay.ask-permission=true, whereupon you should set ["media.autoplay.default", Ci.nsIAutoplay.PROMPT] instead (and you still need to set ask-permission in that case).

Tests that were written before added asking for permission assume that we don't ask for permission, and tests written afterwards assume we don't ask for permision unless they set media.autoplay.ask-permission=true.

Once we're happy with the asking permission experience, we can remove that pref, but in the meantime we need to support both modes of asking and not asking.

::: dom/media/test/test_autoplay_policy.html:74
(Diff revision 6)
>  }
>  
>  /**
>   * Main test function for different autoplay cases without user interaction.
>   *
> - * When the pref "media.autoplay.enabled" is false and the pref
> + * When the pref "media.autoplay.default" is 2 and the pref

'When the pref "media.autoplay.default" is 1'

This test pre-dates asking for permission, so you may invalidate its underlying assumptions if you change the autoplay behaviour here.

::: dom/media/tests/mochitest/head.js:442
(Diff revision 6)
>    if (isAndroid) {
>      defaultMochitestPrefs.set.push(
>        ["media.navigator.video.default_width", 320],
>        ["media.navigator.video.default_height", 240],
>        ["media.navigator.video.max_fr", 10],
> -      ["media.autoplay.enabled", true]
> +      ["media.autoplay.default", 0]

Is it possible to use nsIAutoplay.ALLOWED instead of 0?

::: modules/libpref/init/all.js:572
(Diff revision 6)
> -pref("media.autoplay.enabled", true);
> -
> -// If "media.autoplay.enabled" is false, and this pref is true, then audible media
> -// would only be allowed to autoplay after website has been activated by specific
> -// user gestures, but the non-audible media won't be restricted.
> +// ALLOWED=0, BLOCKED=1, PROMPT=2, defined in dom/media/Autoplay.idl
> +pref("media.autoplay.default", 0);
> +
> +// If "media.autoplay.enabled.state" is not ALLOW, and this pref is true,
> +// then audible media would only be allowed to autoplay after website has
> +// been activated by specific user gestures, but the non-audible

s/but the non-audible/but non-audible/
Attachment #8990488 - Flags: review?(cpearce) → review-
Comment on attachment 8990488 [details]
Bug 1470082 - Change autoplay checkbox to combobox.

https://reviewboard.mozilla.org/r/255562/#review263810


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/preferences/in-content/privacy.js:1012
(Diff revision 7)
> +   * We will be performing a shield study to determine the behaviour to be
> +   * shipped, at which point we can remove these pref switches.
>     */
>    updateAutoplayMediaControlsVisibility() {
> -    document.getElementById("autoplayMediaBox").hidden =
> -      !Services.prefs.getBoolPref("media.autoplay.enabled.user-gestures-needed", false);
> +    let askPermission =
> +        Services.prefs.getBoolPref("media.autoplay.ask-permission",false);

Error: A space is required after ','. [eslint: comma-spacing]
Comment on attachment 8990488 [details]
Bug 1470082 - Change autoplay checkbox to combobox.

https://reviewboard.mozilla.org/r/255562/#review263898

Thanks for reworking this. Looks good.

::: browser/components/preferences/in-content/privacy.js:994
(Diff revision 8)
>  
>    // MEDIA
>  
>    /**
> -   * media.autoplay.enabled works the opposite to most of the other preferences.
> +   * media.autoplay.default works the opposite to most of the other preferences.
>     * The checkbox enabled sets the pref to false

Comment here mentions booleans, but media.autoplay.default is an int.

::: dom/media/AutoplayPolicy.h:28
(Diff revision 8)
>  /**
>   * AutoplayPolicy is used to manage autoplay logic for all kinds of media,
>   * including MediaElement, Web Audio and Web Speech.
>   *
> - * Autoplay could be disable by turn off the pref "media.autoplay.enabled".
> - * Once user disable autoplay, media could only be played if one of following
> + * Autoplay could be disable by setting the pref "media.autoplay.default"
> + * to anything but 0. Once user disables autoplay, media could only be played

Prefer to use nsIAutoplay::Allowed here instead of 0.

::: dom/media/AutoplayPolicy.cpp:95
(Diff revision 8)
>  }
>  
> -/* static */ Authorization
> -AutoplayPolicy::IsAllowedToPlay(const HTMLMediaElement& aElement)
> +static uint32_t
> +DefaultAutoplayBehaviour()
>  {
> -  if (Preferences::GetBool("media.autoplay.enabled")) {
> +  int prefValue = Preferences::GetInt("media.autoplay.default", 0);

Prefer nsIAutoplay::ALLOWED over raw 0 here.

::: dom/media/test/test_autoplay_policy.html:74
(Diff revision 8)
>  }
>  
>  /**
>   * Main test function for different autoplay cases without user interaction.
>   *
> - * When the pref "media.autoplay.enabled" is false and the pref
> + * When the pref "media.autoplay.default" is 1 and the pref

s/1/nsIAutoplay.BLOCKED/
Attachment #8990488 - Flags: review?(cpearce) → review+
Comment on attachment 8990488 [details]
Bug 1470082 - Change autoplay checkbox to combobox.

https://reviewboard.mozilla.org/r/255562/#review263956

::: dom/media/test/test_autoplay_policy.html:17
(Diff revision 8)
>  
>  <script>
>  
>  let manager = new MediaTestManager;
>  
> -gTestPrefs.push(["media.autoplay.enabled", false],
> +gTestPrefs.push(["media.autoplay.default", SpecialPowers.Ci.nsIAutoplay.BLOCKED],

There's also some prefs in toolkit/content/tests/browser/ that need to change:

     grep "media\.autoplay\.enabled" -r toolkit/*
    toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:Services.prefs.setBoolPref("media.autoplay.enabled", true);
    toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:    ["media.autoplay.enabled", false],
    toolkit/content/tests/browser/browser_autoplay_policy_request_permission.js:    ["media.autoplay.enabled.user-gestures-needed", true],
    toolkit/content/tests/browser/browser_autoplay_policy_play_twice.js:    ["media.autoplay.enabled", false],
    toolkit/content/tests/browser/browser_autoplay_policy_play_twice.js:    ["media.autoplay.enabled.user-gestures-needed", enableUserGesture]
    toolkit/content/tests/browser/browser_autoplay_policy_iframe_hierarchy.js:    ["media.autoplay.enabled", false],
    toolkit/content/tests/browser/browser_autoplay_policy_iframe_hierarchy.js:    ["media.autoplay.enabled.user-gestures-needed", true]
    toolkit/content/tests/browser/browser_block_autoplay_media.js:    ["media.autoplay.enabled", false],
    toolkit/content/tests/browser/browser_block_autoplay_media.js:    ["media.autoplay.enabled.user-gestures-needed", true],
    toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:    ["media.autoplay.enabled", false],
    toolkit/content/tests/browser/browser_autoplay_policy_user_gestures.js:    ["media.autoplay.enabled.user-gestures-needed", true],
Comment on attachment 8990488 [details]
Bug 1470082 - Change autoplay checkbox to combobox.

https://reviewboard.mozilla.org/r/255562/#review264344

This looks good to me, please see nits.

Also note that the latest OSX update busted my build so I didn't test this patch locally.

Thanks!

::: browser/components/preferences/in-content/privacy.js:1002
(Diff revision 8)
> -    document.getElementById("autoplayMediaPolicy").checked = !autoPlayEnabled;
> -    document.getElementById("autoplayMediaPolicyButton").disabled = autoPlayEnabled;
>    },
>  
>    /**
> -   * Show the controls for the new media autoplay behaviour behind a pref for now
> +   * If user-gestures-needed is false we don not show any UI for configuring autoplay,

typo: don not show

::: browser/components/preferences/in-content/privacy.js:1005
(Diff revision 8)
>  
>    /**
> -   * Show the controls for the new media autoplay behaviour behind a pref for now
> +   * If user-gestures-needed is false we don not show any UI for configuring autoplay,
> +   * if user-gestures-needed is false and ask-permission is false we show a checkbox
> +   * which only allows the user to block autoplay
> +   * if user-gestures-needed and ask-permission are true we should a combobox that

typo: we should a combobox

::: browser/components/preferences/in-content/privacy.js:1008
(Diff revision 8)
> +   * if user-gestures-needed is false and ask-permission is false we show a checkbox
> +   * which only allows the user to block autoplay
> +   * if user-gestures-needed and ask-permission are true we should a combobox that
> +   * allows the user to block / allow or prompt for autoplay
> +   * We will be performing a shield study to determine the behaviour to be
> +   * shipped, at which point we can remove these pref switches.

Can you please reference bug 1475099 here?

::: browser/components/preferences/in-content/privacy.js:1012
(Diff revision 8)
> +   * We will be performing a shield study to determine the behaviour to be
> +   * shipped, at which point we can remove these pref switches.
>     */
>    updateAutoplayMediaControlsVisibility() {
> -    document.getElementById("autoplayMediaBox").hidden =
> -      !Services.prefs.getBoolPref("media.autoplay.enabled.user-gestures-needed", false);
> +    let askPermission =
> +        Services.prefs.getBoolPref("media.autoplay.ask-permission", false);

nit: why does this have 4 spaces indent?

::: browser/components/preferences/in-content/privacy.js:1014
(Diff revision 8)
>     */
>    updateAutoplayMediaControlsVisibility() {
> -    document.getElementById("autoplayMediaBox").hidden =
> -      !Services.prefs.getBoolPref("media.autoplay.enabled.user-gestures-needed", false);
> +    let askPermission =
> +        Services.prefs.getBoolPref("media.autoplay.ask-permission", false);
> +    let userGestures =
> +        Services.prefs.getBoolPref("media.autoplay.enabled.user-gestures-needed", false);

nit: 4 spaces indent

::: browser/components/preferences/in-content/privacy.js:1018
(Diff revision 8)
> +    let userGestures =
> +        Services.prefs.getBoolPref("media.autoplay.enabled.user-gestures-needed", false);
> +    document.getElementById("autoplayMediaComboboxWrapper").hidden =
> +      !(userGestures && askPermission);
> +    document.getElementById("autoplayMediaCheckboxWrapper").hidden =
> +      !(userGestures && !askPermission);

nit: maybe it's just me but I feel like this would read a little easier:

// Hide the combobox if we don't let the user ask for permission.
document.getElementById("autoplayMediaComboboxWrapper").hidden = !userGestures || !askPermission;

// If the user may ask for permission, hide the checkbox instead.   document.getElementById("autoplayMediaCheckboxWrapper").hidden = !userGestures || askPermission;

What do you think? :)
Attachment #8990488 - Flags: review?(jhofmann) → review+
Comment on attachment 8990488 [details]
Bug 1470082 - Change autoplay checkbox to combobox.

https://reviewboard.mozilla.org/r/255562/#review264344

> nit: maybe it's just me but I feel like this would read a little easier:
> 
> // Hide the combobox if we don't let the user ask for permission.
> document.getElementById("autoplayMediaComboboxWrapper").hidden = !userGestures || !askPermission;
> 
> // If the user may ask for permission, hide the checkbox instead.   document.getElementById("autoplayMediaCheckboxWrapper").hidden = !userGestures || askPermission;
> 
> What do you think? :)

Heh yeh I went back and forwards on this on ways to make it easier to read without repeating everything but yeh I do think this is a little clearer, cheers
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 30 changes to 30 files
remote: 
remote: ************************ ERROR *************************
remote: You are trying to commit a change to an FTL file.
remote: At the moment modifying FTL files requires a review from
remote: one of the L10n Drivers.
remote: Please, request review from either:
remote:     - Francesco Lodolo (:flod)
remote:     - Zibi Braniecki (:gandalf)
remote:     - Axel Hecht (:pike)
remote:     - Stas Malolepszy (:stas)
remote: ********************************************************
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.mozhooks hook failed
abort: push failed on remote
Attachment #8990488 - Flags: review?(francesco.lodolo)
Comment on attachment 8990488 [details]
Bug 1470082 - Change autoplay checkbox to combobox.

https://reviewboard.mozilla.org/r/255562/#review264614
Attachment #8990488 - Flags: review?(francesco.lodolo) → review+
Pushed by francesco.lodolo@mozillaitalia.org:
https://hg.mozilla.org/integration/autoland/rev/36ddc886d33b
Change autoplay checkbox to combobox. r=cpearce,flod,johannh
https://hg.mozilla.org/mozilla-central/rev/36ddc886d33b
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
This does not work.

In my about:config, the default for media.autoplay.default is 0 and yet autoplay is enabled on every website.
Never mind.  I see the numbers were rearranged, as present in comment 19.
Depends on: 1516582
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: