Status

()

enhancement
P2
normal
Rank:
19
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
The new StaticPrefList mechanism subsumes MediaPrefs.
(Assignee)

Updated

a year ago
Blocks: 1448220
(Assignee)

Updated

a year ago
Blocks: 1448219
No longer blocks: 1448220
Priority: -- → P2
Rank: 19
Component: Audio/Video → Audio/Video: Playback
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
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.
Attachment #8964463 - Flags: review?(cpearce)

Comment 3

a year ago
mozreview-review
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.
(Assignee)

Comment 4

a year ago
> 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.
(Assignee)

Updated

a year ago
Attachment #8964463 - Flags: review?(jyavenard)
Attachment #8964463 - Flags: review?(jyavenard)
(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 :)
(Assignee)

Comment 6

a year ago
> 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.
(Assignee)

Comment 7

a year ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

a year ago
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

a year ago
mozreview-review
Comment on attachment 8964463 [details]
Bug 1448222 - Remove MediaPrefs.

https://reviewboard.mozilla.org/r/233210/#review242566
Attachment #8964463 - Flags: review?(jyavenard) → review+
https://hg.mozilla.org/mozilla-central/rev/ba5089a967b9
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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);
Flags: needinfo?(n.nethercote)
(Assignee)

Comment 16

a year ago
(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.
Flags: needinfo?(n.nethercote)
Nicholas thanks for the explanation and glad that it still works.
Depends on: 1466569
Depends on: 1477253
Depends on: 1479631
You need to log in before you can comment on or make changes to this bug.