> 0:16.10 In file included from /home/nchen/gecko-dev/media/webrtc/trunk/webrtc/system_wrappers/interface/aligned_malloc.h:22:0, > 0:16.10 from /home/nchen/gecko-dev/media/webrtc/trunk/webrtc/common_video/plane.h:14, > 0:16.10 from /home/nchen/gecko-dev/media/webrtc/trunk/webrtc/common_video/interface/i420_video_frame.h:18, > 0:16.10 from /home/nchen/gecko-dev/media/webrtc/trunk/webrtc/video_engine/include/vie_capture.h:22, > 0:16.10 from ../../dist/include/MediaEngineWebRTC.h:51, > 0:16.10 from /home/nchen/gecko-dev/dom/media/MediaManager.cpp:49, > 0:16.10 from /home/nchen/gecko-dev/objdir-android/dom/media/Unified_cpp_dom_media0.cpp:15: > 0:16.10 /home/nchen/gecko-dev/media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h: In constructor 'webrtc::DefaultDeleter<T>::DefaultDeleter(const webrtc::DefaultDeleter<U>&)': > 0:16.10 /home/nchen/gecko-dev/media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h:136:76: error: typedef 'U_ptr_must_implicitly_convert_to_T_ptr' locally defined but not used [-Werror=unused-local-typedefs] > 0:16.10 COMPILE_ASSERT((webrtc::is_convertible<U*, T*>::value), One of many warnings being treated as errors in dom/media.

Simplest fix to disable the warning. This patch applies to both GCC and clang.

Comment on attachment 8432606 [details] [diff] [review] Fix unused-local-typedefs warning/error after WebRTC update (v1) Review of attachment 8432606 [details] [diff] [review]: ----------------------------------------------------------------- r+, though I'd welcome a patch against upstream trunk (not 3.50) to resolve any issues like this so we can upstream it.

Try: https://tbpl.mozilla.org/?tree=Try&rev=8b3ad8f2d301

We should really fix the build warnings. I've filed bug 1020643 and bug 1020661 on that. (Have a patch for the former; not yet sure what the right fix is for the latter, since it's in upstream webrtc code.)

(Ideally we should add a comment alongside the warning-disabling-chunk, saying that it's a hackaround for bug 1020661 & we can drop that chunk once that bug is fixed.)

(I'm not objecting to landing the patch as-is, though I'd suggest at least pushing a DONTBUILD followup to add a comment along the lines of Comment 5.)

Added a comment.

We should back this out after bug 1020661 is fixed. Not sure what the right dependency is in this case, but at least it'll generate a bugmail once bug 1020661 is fixed :)

Actually, a few thoughts on this patch: (1) Sheriffs are now requiring Try pushes before they'll honor 'checkin-needed', per a recent newsgroup thread. Hence, removing keyword for the moment (a sheriff would've done so when they got around to triaging checkin-needed anyway). (2) I suspect this will cause bustage in old versions of GCC that don't support this warning -- GCC errors out if you pass it command-line args about warnings that it doesn't recognize. (This is why we have the MOZ_CXX_SUPPORTS_WARNING() macro in configure.in) However, this may not be an issue since we already require GCC versions recent enough to have C++11 features like static_cast and 'auto', which may be more recent than when this warning was added to GCC. So, tl;dr, if this passes a Try run that includes B2G, probably no need to worry about this. (3) It may be a little better to use -Wno-error=unused-local-typedefs (note the "Wno-error=", instead of "Wno-"), which should make us still display the warnings but not treat them as errors. (This will let people still see other useful instances of this warning in dom/media and have a chance to address them, and prevent us (hopefully) from accidentally introducing additional instances.)

There's a try push from comment 5 that looks okay, https://tbpl.mozilla.org/?tree=Try&rev=8b3ad8f2d301 I do remember -Wno- being better supported/tolerated than -Wno-error= though (for clang I think).

Oops comment 3, not comment 5

Ah, apologies - I totally overlooked that Try push. (I should trust my eyes less, and lean on find-in-page more. :)) Given that the try push (particularly the B2G section w/ old GCC versions) was all green, I guess the "this may not be an issue" in comment 9 part (2) is probably the case. Hooray! And I don't really care about -Wno- vs -Wno-error (part (3)) for this particular warning. So, feel free to add back checkin-needed -- sorry for mistakenly clearing it.

No worries :)