The new StaticPrefList mechanism subsumes MediaPrefs.

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 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.

> 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.

(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 :)

> 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.

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.

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.

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 on attachment 8964463 [details] Bug 1448222 - Remove MediaPrefs. https://reviewboard.mozilla.org/r/233210/#review242566

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

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);

(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.

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