Closed
Bug 1430993
Opened 6 years ago
Closed 6 years ago
Replace RefPtr with StaticRefPtr to avoid the static constructor
Categories
(Core :: WebRTC: Audio/Video, enhancement, P2)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: mchiang, Assigned: pehrsons)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
Please see more detail in bug 1430534 comment 13
Reporter | ||
Updated•6 years ago
|
Assignee: nobody → mchiang
Reporter | ||
Updated•6 years ago
|
Summary: Replace RefPtr with StaticRefPtr to to avoid the static constructor → Replace RefPtr with StaticRefPtr to avoid the static constructor
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Status: NEW → ASSIGNED
Rank: 15
Priority: -- → P2
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8943145 [details] Bug 1430993 - Replace RefPtr with StaticRefPtr to avoid the static constructor. https://reviewboard.mozilla.org/r/213442/#review219370 r- for returning raw pointer. Please return already_AddRefed. Also made a code cleanup suggestion, so I'd like to see it again. ::: dom/media/systemservices/CamerasParent.cpp:341 (Diff revision 1) > > bool > CamerasParent::SetupEngine(CaptureEngine aCapEngine) > { > LOG((__PRETTY_FUNCTION__)); > - RefPtr<mozilla::camera::VideoEngine>* engine = &sEngines[aCapEngine]; > + StaticRefPtr<mozilla::camera::VideoEngine>* engine = &sEngines[aCapEngine]; Nit: Since you're in here. I see no need for a pointer here for this temporary. I'd recommend: auto& engine = sEngines[aCapEngine]; and then instead of if (!engine->get()) { do if (!engine) That should simplify the invariant in this code. All these get() calls seem redundant to me. ::: dom/media/systemservices/CamerasParent.cpp:375 (Diff revision 1) > MOZ_CRASH(); > break; > } > > config->Set<webrtc::CaptureDeviceInfo>(captureDeviceInfo); > *engine = mozilla::camera::VideoEngine::Create(UniquePtr<const webrtc::Config>(config.release())); engine = ::: dom/media/systemservices/VideoEngine.h:38 (Diff revision 1) > > public: > VideoEngine (){}; > NS_INLINE_DECL_REFCOUNTING(VideoEngine) > > - static RefPtr<VideoEngine> Create(UniquePtr<const webrtc::Config>&& aConfig); > + static VideoEngine* Create(UniquePtr<const webrtc::Config>&& aConfig); Please have it return already_AddRefed<VideoEngine> to make it leak-safe. ::: dom/media/systemservices/VideoEngine.cpp:185 (Diff revision 1) > > -RefPtr<VideoEngine> VideoEngine::Create(UniquePtr<const webrtc::Config>&& aConfig) { > +VideoEngine* VideoEngine::Create(UniquePtr<const webrtc::Config>&& aConfig) { > LOG((__PRETTY_FUNCTION__)); > LOG(("Creating new VideoEngine with CaptureDeviceType %s", > aConfig->Get<webrtc::CaptureDeviceInfo>().TypeName())); > - RefPtr<VideoEngine> engine(new VideoEngine(std::move(aConfig))); > + return new VideoEngine(std::move(aConfig)); Here we can use: return MakeRefPtr<VideoEngine>(std::move(aConfig));
Attachment #8943145 -
Flags: review?(jib) → review-
Comment 3•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8943145 [details] Bug 1430993 - Replace RefPtr with StaticRefPtr to avoid the static constructor. https://reviewboard.mozilla.org/r/213442/#review219370 > Here we can use: > > return MakeRefPtr<VideoEngine>(std::move(aConfig)); Sorry I meant: return MakeRefPtr<VideoEngine>(std::move(aConfig)).forget();
Reporter | ||
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8943145 [details] Bug 1430993 - Replace RefPtr with StaticRefPtr to avoid the static constructor. https://reviewboard.mozilla.org/r/213442/#review219604 ::: dom/media/systemservices/CamerasParent.cpp:341 (Diff revision 1) > > bool > CamerasParent::SetupEngine(CaptureEngine aCapEngine) > { > LOG((__PRETTY_FUNCTION__)); > - RefPtr<mozilla::camera::VideoEngine>* engine = &sEngines[aCapEngine]; > + StaticRefPtr<mozilla::camera::VideoEngine>* engine = &sEngines[aCapEngine]; Changing to auto& lead to this build error "error: non-const lvalue reference to type 'mozilla::StaticRefPtr<mozilla::camera::VideoEngine> *' cannot bind to a temporary of type 'StaticRefPtr<mozilla::camera::VideoEngine> *'"
Assignee | ||
Comment 6•6 years ago
|
||
Sorry for stealing this. I have a half-done followup and coordinated with achronop on IRC.
Assignee: achronop → apehrson
Assignee | ||
Comment 7•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8943145 [details] Bug 1430993 - Replace RefPtr with StaticRefPtr to avoid the static constructor. https://reviewboard.mozilla.org/r/213442/#review219370 > Sorry I meant: > > return MakeRefPtr<VideoEngine>(std::move(aConfig)).forget(); MakeAndAddRef would be even shorter, but no, it doesn't have access to that private ctor.
Assignee | ||
Comment 8•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8943145 [details] Bug 1430993 - Replace RefPtr with StaticRefPtr to avoid the static constructor. https://reviewboard.mozilla.org/r/213442/#review219604 > Changing to auto& lead to this build error "error: non-const lvalue reference to type 'mozilla::StaticRefPtr<mozilla::camera::VideoEngine> *' cannot bind to a temporary of type 'StaticRefPtr<mozilla::camera::VideoEngine> *'" Not sure what your line looked like exactly, but `StaticRefPtr<VideoEngine>& engine = sEngines[aCapEngine];` seems to work.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8943145 -
Attachment is obsolete: true
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8945372 [details] Bug 1430993 - Replace RefPtr with StaticRefPtr to avoid the static constructor. https://reviewboard.mozilla.org/r/215562/#review221438 ::: dom/media/systemservices/CamerasParent.cpp (Diff revision 1) > - if (!engine->get()) { > webrtc::CaptureDeviceInfo *captureDeviceInfo = nullptr; > UniquePtr<webrtc::Config> config(new webrtc::Config); > + if (!engine) { Did you mean to move these definitions out? The indent is off now. They're only used inside the if() block, so if anything, I'd move `config` down to where it's used.
Attachment #8945372 -
Flags: review?(jib) → review+
Assignee | ||
Comment 12•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8945372 [details] Bug 1430993 - Replace RefPtr with StaticRefPtr to avoid the static constructor. https://reviewboard.mozilla.org/r/215562/#review221438 > Did you mean to move these definitions out? The indent is off now. > > They're only used inside the if() block, so if anything, I'd move `config` down to where it's used. An interactive commit gone wrong. It get's fixed in the other patch but I'll move it here.
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8945373 [details] Bug 1430993 - Clean up minor issues in CamerasParent. https://reviewboard.mozilla.org/r/215564/#review222234
Attachment #8945373 -
Flags: review?(jib) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•6 years ago
|
||
Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8980da245d08 Replace RefPtr with StaticRefPtr to avoid the static constructor. r=jib https://hg.mozilla.org/integration/mozilla-inbound/rev/a32e2a20cb41 Clean up minor issues in CamerasParent. r=jib
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8980da245d08 https://hg.mozilla.org/mozilla-central/rev/a32e2a20cb41
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•