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)

Product:
Core
Component:
WebRTC: Audio/Video
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: mchiang, Assigned: pehrsons)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1430993 - Replace RefPtr with StaticRefPtr to avoid the static constructor.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1430993 - Clean up minor issues in CamerasParent.
59 bytes, text/x-review-board-request
jib
: review+
Details
Assignee: nobody → mchiang
Summary: Replace RefPtr with StaticRefPtr to to avoid the static constructor → Replace RefPtr with StaticRefPtr to avoid the static constructor
Status: NEW → ASSIGNED
Rank: 15
Priority: -- → P2
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 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();
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> *'"
I can take that one and finish the review.
Assignee: bonchiang → achronop
Sorry for stealing this. I have a half-done followup and coordinated with achronop on IRC.
Assignee: achronop → apehrson
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.
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.
Attachment #8943145 - Attachment is obsolete: true
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+
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 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+
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: