1448222 - Remove MediaPrefs

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P2)

Description

[Nicholas Nethercote [inactive]]

The new StaticPrefList mechanism subsumes MediaPrefs.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1448222 - Remove MediaPrefs.

This patch converts all the prefs in MediaPrefs to the new StaticPrefs system.

Note that the "media.wmf.skip-blacklist" pref was present in both MediaPrefs
and gfxPrefs. The copy in MediaPrefs was never used; this explains why this
patch does not add an entry for it to StaticPrefList.h

Review commit: https://reviewboard.mozilla.org/r/233210/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233210/

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8964463 [details]
Bug 1448222 - Remove MediaPrefs.

https://reviewboard.mozilla.org/r/233210/#review238846

::: dom/html/HTMLMediaElement.cpp:4448
(Diff revision 1)
>      // Already started, just keep it running.
>      return;
>    }
>    NS_NewTimerWithFuncCallback(getter_AddRefs(mVideoDecodeSuspendTimer),
>                                VideoDecodeSuspendTimerCallback, this,
> -                              MediaPrefs::MDSMSuspendBackgroundVideoDelay(), nsITimer::TYPE_ONE_SHOT,
> +                              StaticPrefs::media_suspend_bkgnd_video_delay_ms(), nsITimer::TYPE_ONE_SHOT,

the name of each method goes against Mozilla coding style.
"In C/C++, method names should be capitalized and use camelCase.  Typenames, and the names of arguments, should be separated with a single space character."

Why that naming choice? Why can't we keep the same naming convention as before.

Would make the review easier too.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8964463 [details]
Bug 1448222 - Remove MediaPrefs.

https://reviewboard.mozilla.org/r/233210/#review239000

I was going to say the same thing as jya; Mozilla C++ coding style is CamelCase, not snake_case.

Also, given that I'm moving onto layout, I think it's best that jya review this, as he'll be the one who has to live with the consequences of taking this patch.

Comment 4

[Nicholas Nethercote [inactive]]

> Why that naming choice? Why can't we keep the same naming convention as
> before.

A couple of reasons.

- It makes it easier to search for all the uses of a pref. If you search for the regexp /media.cache.size/ you will find all the occurrences of the string "media.cache_size" and also all occurrences of the identifier `media_cache_size`. This is very useful.

- The new names are in a global namespace, just like prefs are. If shorter identifiers are used, there is a chance of collisions. This avoids that possibility.

- It saves people from having to come up with an identifier.

This naming scheme has received r+ from multiple other reviewers prior to now.

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> > Why that naming choice? Why can't we keep the same naming convention as
> > before.
> 
> A couple of reasons.
> 
> - It makes it easier to search for all the uses of a pref. If you search for
> the regexp /media.cache.size/ you will find all the occurrences of the
> string "media.cache_size" and also all occurrences of the identifier
> `media_cache_size`. This is very useful.

Using camel case doesn't exclude that logical argument:
MediaCacheSize 
remove all the dots/underscore and capitalise the next letter.

As it is it makes the patch difficult to review. Not only were the preference function moved, but they also got renamed.

> 
> - The new names are in a global namespace, just like prefs are. If shorter
> identifiers are used, there is a chance of collisions. This avoids that
> possibility.
> 
> - It saves people from having to come up with an identifier.
> 
> This naming scheme has received r+ from multiple other reviewers prior to
> now.

I don't believe that people ignoring the few rules we've put in place is a strong argument :)

Comment 6

[Nicholas Nethercote [inactive]]

> Using camel case doesn't exclude that logical argument:
> MediaCacheSize 
> remove all the dots/underscore and capitalise the next letter.

The snake case form means a single regexp will match both forms.

Comment 7

[Nicholas Nethercote [inactive]]

An example, to clarify your objections:

MediaPrefs::VideoOrientationLockEnabled() changed to StaticPrefs::media_videocontrols_lock_video_orientation().

Would you be happy if the new name was StaticPrefs::MediaVideocontrolsLockVideoOrientation()? That would avoid the snake case, but still involves a renaming.

Comment 8

[Jean-Yves Avenard [:jya]]

Yes, the naming with camel case is what I was inferring. Sorry if that wasn't clear.
I'm okay with the renaming, it adds consistency too.

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8964463 [details]
Bug 1448222 - Remove MediaPrefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233210/diff/1-2/

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8964463 [details]
Bug 1448222 - Remove MediaPrefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233210/diff/2-3/

Comment 11

[Nicholas Nethercote [inactive]]

To summarize the arguments in favour of snake case getters:

- The getters look more like the pref names. E.g. media_av1_enabled() looks more like "media.av1.enabled" than MediaAv1Enabled() does. This makes it clearer that a pref is being accessed.

- Snake case gives nice vertical alignment between pref name and getter name for the definitions in StaticPrefList.h.

- Snake case makes it easy to search for all uses of a pref with a single regexp (e.g. /media.av1.enabled/).

Arguments against snake case getters:

- It's contrary to usual Mozilla style.

- The names are slightly longer, due to the additional underscores.

I think that occasional deviations from standard style are reasonable. Even rustc -- which is *highly* opinionated about naming things -- has mechanisms such as #![allow(non_snake_case] to allow such deviations. And I think that the benefits in this case are large enough to justify the deviation.

Furthermore, multiple other people have deemed this acceptable: glandium in bug 1436655, nwgh in bug 1448220, emilio in bug 1448225, hsivonen in bug 1449064, and sfink in bug 1449833.

But this patch is too much of an improvement to let a style disagreement block it, and I want to land it before it bitrots again. So I have changed the media getters to use camel case. jya, please review.

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8964463 [details]
Bug 1448222 - Remove MediaPrefs.

https://reviewboard.mozilla.org/r/233210/#review242566

Comment 13

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba5089a967b993f911896e1f787fb9c7c9b406c3
Bug 1448222 - Remove MediaPrefs. r=jya

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/ba5089a967b9

Comment 15

[Frank-Rainer Grahl (:frg)]

How could the comm-central products change the default prefs now? 

SeeMonkey sets some differently e.g. in 
suite\browser\browser-prefs.js

// Digital Rights Management, Encrypted Media Extensions
pref("media.eme.enabled", false);
 
// Turn off WebRTC by default (bug 1419507)
pref("media.navigator.enabled", false);
pref("media.peerconnection.enabled", false);

Comment 16

[Nicholas Nethercote [inactive]]

(In reply to Frank-Rainer Grahl (:frg) from comment #15)
> How could the comm-central products change the default prefs now? 
> 
> SeeMonkey sets some differently e.g. in 
> suite\browser\browser-prefs.js

Values in .js files should continue to override the static values, so there should be no need to change anything. Please let me know if that's not the case.

Comment 17

[Frank-Rainer Grahl (:frg)]

Nicholas thanks for the explanation and glad that it still works.