Files should use GMP_LOG macro from GMPLog.h rather than spinning their own
Categories
(Core :: Audio/Video: GMP, enhancement, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox71 | --- | fixed |
People
(Reporter: bryce, Assigned: bryce)
Details
Attachments
(5 files)
GMPLog.h defines a macro to be used for logging to the GMP log. However, many files still create their own log define -- some of these include GMPLog.h and some do not. I think we should prefer minimizing the number of defines here. Benefits I see:
- Consistent usage of
GMP_LOGmakes it clearer that we're logging to the GMP log. With different defines, such asLOG, it's not as immediately obvious what log is being used. - Lowers risk of footgunning due to re-defines causing bugs. The motivating factor in creating this bug is that I'm not seeing logs where expected, and I suspect it's because
GMPService.cppdefines aLOGDGMP log, but doesn't undef it first if it's already defined. - Less boilerplate code at the top of files using the log.
- A centralised location for the log macro definition makes them easier to change and each file can define its own macro if it really needs it.
|
Assignee | |
Comment 1•5 years ago
|
||
|
|
Assignee | |
Comment 2•5 years ago
|
||
Depends on D47192
|
|
Assignee | |
Comment 3•5 years ago
|
||
This patch changes instances where the GMP MOZ_LOG is used to prefer using
macros from GMPLog.h:
- Files that don't need their own macros now just directly call
GMP_LOG_<LEVEL>as required. - Files that use their own macros for formatting have had those macro
definitions changes so that the macros have unique names and are expressed in
terms of the macros from GMPLog.h.
I've reworked the includes in most of the files I've touched since I was adding
GMPLog.h:
- Reordered the includes to better match the Google C++ style.
- Removed includes that are already included from the associated header or
GMPLog.h. I.e. if Foo.cpp includes Foo.h, and Foo.cpp includes other headers
already included in Foo.h, these were removed.
I've also made a couple of drive by edits to logs so that they log more than a
couple of words and updated some strings where the incorrect class name was
being logged.
Depends on D47193
|
|
Assignee | |
Comment 4•5 years ago
|
||
I've reworked the includes in most of the files I've touched since I was adding
GMPLog.h:
- Reordered the includes to better match the Google C++ style.
- Removed includes that are already included from the associated header or
GMPLog.h. I.e. if Foo.cpp includes Foo.h, and Foo.cpp includes other headers
already included in Foo.h, these were removed.
Depends on D47194
|
|
Assignee | |
Comment 5•5 years ago
|
||
Now that we have explicit macros for each GMP_LOG level GMP_LOG and
GMP_LOG_DEBUG are doing the same thing. This patch replaces all instances of
GMP_LOG with GMP_LOG_DEBUG. This has the benefit of all uses of the GMP macros
sharing a consistent naming convention and being explicit about the level
they're logging at.
Depends on D47373
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
Pushed by bvandyk@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ffa5b2008115 Rework GMPLog.h include guard to follow Google C++ style. r=alwu https://hg.mozilla.org/integration/autoland/rev/d2c33136e60f Add different log level macros to GMPLog.h r=alwu https://hg.mozilla.org/integration/autoland/rev/e146d1355850 Use GMPLog.h macros where applicable. r=alwu https://hg.mozilla.org/integration/autoland/rev/0a0ff7a39921 Tidy includes in files using new logs. r=alwu https://hg.mozilla.org/integration/autoland/rev/d92b15efc8b4 Replace usages of GMP_LOG with GMP_LOG_DEBUG and remove GMP_LOG. r=alwu
|
||
Comment 7•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/ffa5b2008115
https://hg.mozilla.org/mozilla-central/rev/d2c33136e60f
https://hg.mozilla.org/mozilla-central/rev/e146d1355850
https://hg.mozilla.org/mozilla-central/rev/0a0ff7a39921
https://hg.mozilla.org/mozilla-central/rev/d92b15efc8b4
Description
•