1288320 - Define each keysystem string as specific identifier instead of placing "some.keysystem" everywhere.

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Kilik Kuo [:kikuo] (inactive)]

Per Bug 1136707 Comment 10, the code would be clearer and easier to remember.
e.g.
#define EME_KEYSYSTEM_WIDEVINE    "com.widevine.alpha"
#define EME_KEYSYSTEM_PLAYREADY   "com.microsoft.playready"
#define EME_KEYSYSTEM_CLEARKEY    "org.w3.clearkey"
#define EME_KEYSYSTEM_PRIMETIME   "com.adobe.primetime"

Comment 1

[James Cheng[:JamesCheng]]

Is gecko programmer encouraged to use const instead of macro?

such as static const char* const EME_KEYSYSTEM_PLAYREADY = "com.microsoft.playready";

Comment 2

[James Cheng[:JamesCheng]]

Attachment: Bug 1288320 - Define each keysystem string as specific identifier instead of placing some.keysystem everywhere.

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

Comment 3

[James Cheng[:JamesCheng]]

Hi Chris,

I just tried to unified the literal string into const string. 

I searched from DXR and cannot find the united naming rule for |static const string|.

So I used prefix 'k', camel case and put then in EMEUtils.h.

Please address me of the naming and if there is a more appropriate header to declare the string.

Thank you very much.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Why not using macros as suggested by comment 0?

Comment 5

[James Cheng[:JamesCheng]]

Intuitively, I think macro is not a good coding style and refer to "Item 2 : Prefer consts, enums, and inlines to #defines" from Scott Meyers". 

So I just ask comment1 that is there any policy or guideline to determine we should use macro or not.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

I can't think of a way to define literal strings without macro.

Comment 7

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

Since we're quoting Meyers:

http://flylib.com/books/en/2.887.1.13/1/

<quote>
Because constant definitions are typically put in header files (where many different source files will include them), it's important that the pointer be declared const, usually in addition to what the pointer points to. To define a constant char*-based string in a header file, for example, you have to write const twice:

 const char * const authorName = "Scott Meyers";

</quote>

You could probably instead also define the string as "extern" in the header, and define it in a .cpp file somewhere. I'd rather just have the "const char* const" literal in the header I think.

And our coding style guide says constant literals start with a 'k' prefix, though its not consistent and some use all caps.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Literals

So please use the pattern:

  const char * const kName = "literal";

Comment 8

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

Comment on attachment 8775066 [details]
Bug 1288320 - Define each keysystem string as specific identifier instead of placing some.keysystem everywhere.

https://reviewboard.mozilla.org/r/67392/#review64616

Comment 9

[James Cheng[:JamesCheng]]

Thanks for the reviewing!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f98e30cc66e
Define each keysystem string as specific identifier instead of placing some.keysystem everywhere. r=cpearce

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5f98e30cc66e