Closed Bug 1280743 Opened 4 years ago Closed 4 years ago
.cpp fails to compile with --disable-eme
dom/media/gmp/GMPParent.cpp:43:10: fatal error: 'mozilla/dom/WidevineCDMManifestBinding.h' file not found #include "mozilla/dom/WidevineCDMManifestBinding.h" ^
This works for me, but I don't know this code at all...
Attachment #8763317 - Flags: review?(cpearce)
4 years ago
Assignee: nobody → mats
Attachment #8763317 - Flags: review?(cpearce) → review+
Comment on attachment 8763317 [details] [diff] [review] fix? Review of attachment 8763317 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/gmp/GMPParent.cpp @@ +819,5 @@ > manifestFile->AppendRelativePath(NS_LITERAL_STRING("manifest.json")); > return ReadChromiumManifestFile(manifestFile); > +#endif > + > + return GenericPromise::CreateAndReject(NS_ERROR_FAILURE, __func__); Fly-by comment: When MOZ_EME is #defined, this last 'return' will be dead code after the previous 'return'! I would suggest using #else...#endif around it instead.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/24999edead4e Put Widevine stuff under #ifdef MOZ_EME to make this file compile under --disable-eme. r=cpearce
Good catch Gerald, I pushed with the #else as suggested.
Chris -- Is Fx 49 affected? If so, does it make sense to uplift this to Fx 49?
(In reply to Maire Reavy (On PTO July 23rd to 31st) from comment #6) > Chris -- Is Fx 49 affected? If so, does it make sense to uplift this to Fx > 49? Gerald: Can you uplift this if the build is broken without it please?
Flags: needinfo?(cpearce) → needinfo?(gsquelart)
49 is not affected, as the Widevine code there hides behind '#ifdef MOZ_WIDEVINE_EME', including the #include line that caused problem in 50.
Actually the include guard has been removed: https://hg.mozilla.org/releases/mozilla-beta/diff/7b8879ab0e02/dom/media/gmp/GMPParent.cpp
I had put up a patch for beta 49 in bug 1296627 (not fully tested yet). Is it useful here?
(In reply to kevink9876543 from comment #11) > I had put up a patch for beta 49 in bug 1296627 (not fully tested yet). Is > it useful here? Tested my local beta 49 builds w/ attachment 8783123 [details] [diff] [review] and all is working as expected. I'm confused though because the patch in this bug is much smaller. What effect are the "extra" parts in my patch have, if any? Is it Gecko-49-specific?
Gerald, can you check v49 again and uplift if necessary, thanks!
Flags: needinfo?(mats) → needinfo?(gsquelart)
Comment on attachment 8763317 [details] [diff] [review] fix? Approval Request Comment [Feature/regressing bug #]: --disable-eme [User impact if declined]: Not possible to build Firefox when EME is not wanted [Describe test coverage new/current, TreeHerder]: In 50 for 2 months [Risks and why]: No risk that I can see, as this is just disabling the code dealing with Widevine [String/UUID change made/needed]: None Note that this was initially (end July) not needed in 49, but then bug 1276132 uplifted some EME code to 49 on August 10, including the part that was failing to build with --disable-eme.
Attachment #8763317 - Flags: approval-mozilla-beta?
(In reply to kevink9876543 from comment #12) > I'm confused though because the > patch in this bug [compared to bug 1296627] is much smaller. What effect are the "extra" parts in my > patch have, if any? The patch in bug 1296627 also #ifdef'd-out the full Widevine-related function declarations and definitions, instead of just not calling them. I'm choosing the patch in this bug here for uplift, because it is touching less code, and it has been in the codebase for 2 months already so it carries lower risk.
Hi :gerald, Since there is no feature/regression bug associated with this bug, if this is not in beta, will it be a critical issue?
I suspect there are some downstream consumers (the Tor browser?) that use --disable-eme. Not breaking them might be a reason to uplift.
(In reply to Gerry Chang [:gchang] from comment #16) > Since there is no feature/regression bug associated with this bug, Maybe it wasn't quite made clear. There *is* a regressing bug here, that would be bug 1276132.
Thank you kevink, and :emk (comment 10). There is indeed a regression from bug 1276132 when it was uplifted to beta on August 10 (as I implied in my uplift request in comment 14).
Comment on attachment 8763317 [details] [diff] [review] fix? We want to be able to disable this for non-eme builds, please uplift to beta.
Attachment #8763317 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.