Closed Bug 986787 Opened 6 years ago Closed 6 years ago

Fix -Wreorder warning and other cleanups in MediaEngineTabVideoSource.cpp

Categories

(Core :: WebRTC, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox30 --- wontfix
firefox31 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch MediaEngineTabVideoSource.patch (obsolete) — Splinter Review
1. Fix -Wreorder warning:

content/media/webrtc/MediaEngineTabVideoSource.cpp:27:3 [-Wreorder] field 'mUuid' will be initialized after field 'mData'

2. Add missing MPL2 license.
3. Replace mName and mUuid member variables with local string literals.
4. Change 0 pointer to nullptr.
5. Remove unnecessary null check before call free().
Attachment #8395167 - Flags: review?(rjesup)
Comment on attachment 8395167 [details] [diff] [review]
MediaEngineTabVideoSource.patch

Review of attachment 8395167 [details] [diff] [review]:
-----------------------------------------------------------------

r+, but I think it would be smart to avoid manual lifetime management

::: content/media/webrtc/MediaEngineTabVideoSource.cpp
@@ -32,5 @@
>  
>  MediaEngineTabVideoSource::~MediaEngineTabVideoSource()
>  {
> -  if (mData)
> -      free(mData);

Can we instead move mData to something like an nsAutoPtr, and drop the free and mData(0)?
Attachment #8395167 - Flags: review?(rjesup) → review+
Summary: Fix -Wreorder warning and other cleanups in MediaEngineTabeVideoSource.cpp → Fix -Wreorder warning and other cleanups in MediaEngineTabVideoSource.cpp
patch v2 changes mData from a bare pointer to a mozilla::ScopedFreePtr.
Attachment #8395167 - Attachment is obsolete: true
Attachment #8396924 - Flags: review?(rjesup)
Comment on attachment 8396924 [details] [diff] [review]
MediaEngineTabVideoSource-v2.patch

Review of attachment 8396924 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +37,1 @@
>  }

Optional: this can now go away/move to the .h file
Attachment #8396924 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/c4c3f686b728
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.