1286429 - Implement mediaDevices.ondevicechange for Mac OSX

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Comment 1

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

Review commit: https://reviewboard.mozilla.org/r/63824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63824/

Comment 2

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63824/diff/1-2/

Comment 3

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63824/diff/2-3/

Comment 4

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63824/diff/3-4/

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/63824/#review61130

Yay works!! I tested it with https://jsfiddle.net/Lqo4paed/ though I get devicechange events fired twice!

Twice on removing, and twice on reinserting my USB camera.

Misc comments and nits. Main concern is creation of MediaManager on pageload, redundant webidl, and use of naked pointers in a couple of places that hopefully can be avoided.

::: dom/base/nsGkAtomList.h:1946
(Diff revision 4)
> +// MediaDevices device change event
> +GK_ATOM(ondevicechange, "ondevicechange")

Why is this needed? I'm not familiar with gklayout...

::: dom/media/MediaDevices.h:46
(Diff revision 4)
>    GetUserMedia(const MediaStreamConstraints& aConstraints, ErrorResult &aRv);
>  
>    already_AddRefed<Promise>
>    EnumerateDevices(ErrorResult &aRv);
>  
> +  // Post an devicechange event to script.

a devicechange event

::: dom/media/MediaDevices.cpp:145
(Diff revision 4)
> +MediaDevices::MediaDevices(nsPIDOMWindowInner* aWindow) : DOMEventTargetHelper(aWindow)
> +{
> +  MediaManager::Get()->AddDeviceChangeCallback(this);
> +}

This will create MediaManager on pageload, which is bad (most pages don't use gUM and shouldn't load MediaManager at all).

e.g. console.log(navigator.mediaDevices.getUserMedia); shouldn't launch MediaManager. 

Ideally, we only need to call AddDeviceChangeCallback when someone calls navigator.mediaDevices.addEventListener or sets navigator.mediaDevices.ondevicechange.

::: dom/media/MediaDevices.cpp:194
(Diff revision 4)
> +  class PostEventRunnable : public Runnable {
> +  public:
> +    explicit PostEventRunnable(MediaDevices* aMediaDevices)
> +      : mMediaDevices(aMediaDevices)
> +    {
> +    }
> +    NS_IMETHOD Run()
> +    {
> +      if (mMediaDevices)
> +        mMediaDevices->PostDeviceChangeEvent();
> +      return NS_OK;
> +    }
> +  private:
> +    MediaDevices* mMediaDevices;
> +  };
> +
> +  NS_DispatchToMainThread(new PostEventRunnable(this));

This uses a raw pointer. What keeps mediaDevices alive here during shutdown?

How about:

    RefPtr<MediaDevices> self(this);

    NS_DispatchToMainThread(NewRunnableFrom([this, self]() mutable {
      PostDeviceChangeEvent();
      return NS_OK;
    }));

?

::: dom/media/MediaDevices.cpp:220
(Diff revision 4)
> +  if (MediaManager::Get()->IsActivelyCapturingOrHasAPermission(GetOwner()->WindowID()) ||
> +    Preferences::GetBool("media.navigator.permission.disabled", false)) {

Nit: I would inverse this if-statement and use an early return pattern, to reduce indentation.

::: dom/media/MediaDevices.cpp:227
(Diff revision 4)
> +    nsCOMPtr<nsIDOMEvent> event =
> +      DeviceChangeEvent::Constructor(this, NS_LITERAL_STRING("devicechange"), init);

According to the spec [1] this should be a plain Event.

[1] https://w3c.github.io/mediacapture-main/getusermedia.html#event-mediadevices-devicechange

::: dom/media/MediaManager.cpp:1812
(Diff revision 4)
> +    MediaManager::PostTask(NewTaskFrom([]() mutable {
> +      RefPtr<MediaManager> manager = MediaManager_GetInstance();
> +      manager->GetBackend(0);

Please add a comment on why this is needed.

Also, maybe it's not needed until navigator.mediaDevices.addEventListener is called or navigator.mediaDevices.ondevicechange is set?

::: dom/media/systemservices/CamerasChild.h:141
(Diff revision 4)
> +template <class MEM_FUN, class... ARGS>
> +int GetChildAndCall_NoCreate(MEM_FUN&& f, ARGS&&... args)

Ugh, I think we can get rid of this one (see other comments).

::: dom/media/systemservices/CamerasChild.cpp:79
(Diff revision 4)
> -GetCamerasChild() {
> +GetCamerasChild(bool aCreateChildIfNull) {
>    CamerasSingleton::Mutex().AssertCurrentThreadOwns();
> -  if (!CamerasSingleton::Child()) {
> +  if (!CamerasSingleton::Child() && aCreateChildIfNull) {

Not crazy about boolean method args. Why not add a simple:

CamerasChild*
GetCamerasChildIfExists() {
  CamerasSingleton::Mutex().AssertCurrentThreadOwns();
  return CamerasSingleton::Child();
}

instead, and call that first in the one place we have this question?

::: dom/media/systemservices/CamerasParent.h:88
(Diff revision 4)
> +private:
> +  CamerasParent* mParent;
> +};

Naked pointer member. Would it be safer to use RefPtr here?

::: dom/media/systemservices/CamerasParent.h:169
(Diff revision 4)
>    bool mChildIsAlive;
>    bool mDestroyed;
>    // Above 2 are PBackground only, but this is potentially
>    // read cross-thread.
>    mozilla::Atomic<bool> mWebRTCAlive;
> +  nsTArray<InputObserver*> mObservers;

I suppose we have no choice but to use raw pointer here, since InputObserver is not refcountable?

::: dom/media/systemservices/CamerasParent.cpp:42
(Diff revision 4)
> +class DeviceChangeRunnable : public Runnable {
> +public:
> +  explicit DeviceChangeRunnable(CamerasParent *aParent)
> +    : mParent(aParent) {}

The predominant pattern in this file seems to be to use media::NewRunnableFrom. Any reason not to use it here as well?

::: dom/media/systemservices/CamerasParent.cpp:440
(Diff revision 4)
> +  RefPtr<CamerasParent> self(this);
> +  InputObserver** observer = self->mObservers.AppendElement(
> +          new InputObserver(self));

This RefPtr doesn't appear to do anything and should be removed (instead InputObserver should use RefPtr<Parent> mParent).

::: dom/media/systemservices/CamerasParent.cpp:444
(Diff revision 4)
>  
> +  RefPtr<CamerasParent> self(this);
> +  InputObserver** observer = self->mObservers.AppendElement(
> +          new InputObserver(self));
> +
> +  helper->mPtrViECapture->RegisterInputObserver(*observer);

Either check return value here or change RegisterInputObserver to assert it succeeds.

::: dom/media/systemservices/CamerasParent.cpp:496
(Diff revision 4)
> +  for (size_t i = 0; i < mObservers.Length(); i++) {
> +    delete mObservers[i];
> +  }

nit:

    for (observer : mObservers) {
      delete observer;
    }

::: dom/media/systemservices/DeviceChangeCallback.h:24
(Diff revision 4)
> +  virtual int AddDeviceChangeCallback(DeviceChangeCallback* aCallback)
> +  {
> +    MutexAutoLock lock(mCallbackMutex);
> +    if (mDeviceChangeCallbackList.IndexOf(aCallback) == mDeviceChangeCallbackList.NoIndex) {
> +      mDeviceChangeCallbackList.AppendElement(aCallback);
> +      return 0;
> +    } else {
> +      return -1;

Can we assert this and return void rather than -1? You're not checking return values anywhere this is called anyway.

When I test this with https://jsfiddle.net/jib1/Lqo4paed/ I get events fired twice. Could multiple listeners be a reason why?

::: dom/media/systemservices/DeviceChangeCallback.h:35
(Diff revision 4)
> +  virtual int RemoveDeviceChangeCallback(DeviceChangeCallback* aCallback)
> +  {
> +    MutexAutoLock lock(mCallbackMutex);
> +    if (mDeviceChangeCallbackList.IndexOf(aCallback) != mDeviceChangeCallbackList.NoIndex) {
> +      mDeviceChangeCallbackList.RemoveElement(aCallback);
> +      return 0;
> +    } else {
> +      return -1;

Same here.

::: dom/media/systemservices/DeviceChangeCallback.h:46
(Diff revision 4)
> +    } else {
> +      return -1;
> +    }
> +  }
> +
> +  DeviceChangeCallback():mCallbackMutex("mozilla::media::DeviceChangeCallback::mCallbackMutex")

nit: spaces around colon

::: dom/media/systemservices/DeviceChangeCallback.h:51
(Diff revision 4)
> +protected:
> +  nsTArray<DeviceChangeCallback*> mDeviceChangeCallbackList;

Maybe nsTArray<RefPtr<DeviceChangeCallback>> ?

Or will that create a refcount cycle?

::: dom/media/webrtc/MediaEngine.h:45
(Diff revision 4)
>    kStarted,
>    kStopped,
>    kReleased
>  };
>  
> -class MediaEngine
> +class MediaEngine : public DeviceChangeCallback

I'm a little lost. Why does MediaDevices, MediaManager, CamerasChild, and every MediaEngine (cameras, microphones, screensharing, tabsharing, fake cam and fake mic) all need DeviceChange callbacks? That seems... excessive.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:430
(Diff revision 4)
> +  // Since Shutdown() will be called twice. We don't want to create child again in the 2nd calling.
> +  mozilla::camera::GetChildAndCall_NoCreate(
> +    &mozilla::camera::CamerasChild::RemoveDeviceChangeCallback,
> +    this);

Why is shutdown called twice??? That seems, wrong.

In any case, it seems simpler to do:

    if (camera::GetCamerasChildIfExists()) {
      camera::GetChildAndCall(
          &mcamera::CamerasChild::RemoveDeviceChangeCallback, this);
    }

and lose the GetChildAndCall_NoCreate template.

::: dom/webidl/DeviceChangeEvent.webidl:1
(Diff revision 4)
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[Constructor(DOMString type, optional DeviceChangeEventInit eventInitDict)]
> +interface DeviceChangeEvent : Event
> +{
> +};
> +
> +dictionary DeviceChangeEventInit : EventInit
> +{
> +};

The spec [1] says to use a plain Event, so we shouldn't add this (should help your try run as well).

[1] https://w3c.github.io/mediacapture-main/getusermedia.html#event-mediadevices-devicechange

::: media/webrtc/trunk/webrtc/modules/video_capture/include/video_capture.h:27
(Diff revision 4)
>  namespace webrtc {
>  
> +class VideoInputFeedBack
> +{
> +public:
> +    virtual void OnDeviceChange(){}

Maybe = 0; to root out superfluous use?

::: media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_info_objc.mm:245
(Diff revision 4)
> +    id deviceWasConnectedObserver = [notificationCenter addObserverForName:AVCaptureDeviceWasConnectedNotification
> +                                                                    object:nil
> +                                                                     queue:[NSOperationQueue mainQueue]
> +                                                                usingBlock:^(NSNotification *note) {
> +                                                                    [_lock lock];
> +                                                                    if(_owner)
> +                                                                        _owner->DeviceChange();
> +                                                                    [_lock unlock];
> +                                                                }];

nit: extreme indentation.

::: media/webrtc/trunk/webrtc/video_engine/vie_capture_impl.cc:403
(Diff revision 4)
> +int ViECaptureImpl::DeregisterInputObserver() {
> +  if (shared_data_->input_manager()->DeRegisterObserver() != 0) {
> +    shared_data_->SetLastError(kViECaptureDeviceUnknownError);
> +    return -1;
> +  }
> +  return 0;
> +}

DeregisterInputObserver() appears unused? If so, should we remove it?

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

https://reviewboard.mozilla.org/r/63824/#review61386

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

With a rank of 27, did you mean P2? Ondevicechange is on for Q3, so I think so.

Comment 8

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63824/diff/4-5/

Comment 9

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://reviewboard.mozilla.org/r/63824/#review61130

Twice events are expected because Logitec webcam consists both camera and microphone.
When it is inserted, there are two devices plugged in.

> Why is this needed? I'm not familiar with gklayout...

It is needed when we call IMPL_EVENT_HANDLER. https://dxr.mozilla.org/mozilla-central/source/dom/events/DOMEventTargetHelper.h#236

> Please add a comment on why this is needed.
> 
> Also, maybe it's not needed until navigator.mediaDevices.addEventListener is called or navigator.mediaDevices.ondevicechange is set?

manager->GetBackend() is used to create MediaEngineWebRTC.
I will add a comment and call this snippet when navigator.mediaDevices.ondevicechange is set.

> The predominant pattern in this file seems to be to use media::NewRunnableFrom. Any reason not to use it here as well?

I just mimic FrameSizeChangeRunnable.
Will replace it with media::NewRunnableFrom though.

> Can we assert this and return void rather than -1? You're not checking return values anywhere this is called anyway.
> 
> When I test this with https://jsfiddle.net/jib1/Lqo4paed/ I get events fired twice. Could multiple listeners be a reason why?

Ｉwill replace -1 with assert.
Twice events are not caused by this.
I indeed observe two events sent from AVFoundation framework each time I plug in/out my webcam.
I think it is because there are two devices (microphone and camera) on Logitech webcam.

> Maybe nsTArray<RefPtr<DeviceChangeCallback>> ?
> 
> Or will that create a refcount cycle?

We don't need RefPtr to hold the owner.
Before any owner being destroyed, it will call RemoveDeviceChangeCallback() to clear the callback.
A mutex mCallbackMutex is used to prevent race condition.

> I'm a little lost. Why does MediaDevices, MediaManager, CamerasChild, and every MediaEngine (cameras, microphones, screensharing, tabsharing, fake cam and fake mic) all need DeviceChange callbacks? That seems... excessive.

The purpose is to minimize the code change.
Otherwise I need to add these API interface & implemenation (duplicate codes) one by one.

The reason MediaEngine need to inherit DeviceChangeCallback is MediaManager controls MediaEngineWebRTC via MediaEngine interface (MediaEngine::GetBackend()). MediaManager doens't know which device (camera , microphones, screensharing, etc.) is behind the MediaEngine interface. If we want to limit the change to MediaEngineWebRTC only, then MediaManager need to know if MediaEngine is actually MediaEngineWebRTC, that may break the polymorphism pattern.

> Why is shutdown called twice??? That seems, wrong.
> 
> In any case, it seems simpler to do:
> 
>     if (camera::GetCamerasChildIfExists()) {
>       camera::GetChildAndCall(
>           &mcamera::CamerasChild::RemoveDeviceChangeCallback, this);
>     }
> 
> and lose the GetChildAndCall_NoCreate template.

It is called at two places:
https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#2892
https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#2897

I will use your simpler version.

> DeregisterInputObserver() appears unused? If so, should we remove it?

I added a snippet to call this function in CamerasParent::CloseEngines()

Comment 10

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63824/diff/5-6/

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/63824/#review61130

I've opened https://github.com/w3c/mediacapture-main/issues/375 to seek clarification on this. I think it would make sense to look into only firing once in this case, given how most people would react to these events (which carry no information), but we can come back to that once the spec is clear.

> We don't need RefPtr to hold the owner.
> Before any owner being destroyed, it will call RemoveDeviceChangeCallback() to clear the callback.
> A mutex mCallbackMutex is used to prevent race condition.

Understood, but if some code ever forgets to call RemoveDeviceChangeCallback() in some codepath, then we're looking at a sec bug rather than just a leak. Raw pointers are frowned on in the tree for that reason, so smart pointers are preferable whenever we can use them.

Deregistering in destructors may be a problem actually, because for reference-counted objects, there's a 30 second window where unreferenced expected-to-be-dead objects are waiting to be garbage collected, and they'll still get executed here the rare time a user yanks their USB camera at the wrong time, which would be surprising and undesirable, maybe even dangerous.

I suggest we call RemoveDeviceChangeCallback() from explicit stop and shutdown methods only. Sorry I missed that initially. Re-opening.

Also, we can then switch this to a RefPtr (though it'll leak if ever called from a destructor, which I think is fine actually, given that we want to avoid that pattern). If not, I think we need to add some serious comments and asserts around this raw pointer.

> The purpose is to minimize the code change.
> Otherwise I need to add these API interface & implemenation (duplicate codes) one by one.
> 
> The reason MediaEngine need to inherit DeviceChangeCallback is MediaManager controls MediaEngineWebRTC via MediaEngine interface (MediaEngine::GetBackend()). MediaManager doens't know which device (camera , microphones, screensharing, etc.) is behind the MediaEngine interface. If we want to limit the change to MediaEngineWebRTC only, then MediaManager need to know if MediaEngine is actually MediaEngineWebRTC, that may break the polymorphism pattern.

If you put the call to RemoveDeviceChangeCallback inside Shutdown, then does MediaManager need to know? Could we then move this inheritance to MediaEngineWebRTC instead? I should have the double shutdown licked in bug 1286096.

> It is called at two places:
> https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#2892
> https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#2897
> 
> I will use your simpler version.

I should have fixed the double shutdown in bug 1286096.

Comment 12

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://reviewboard.mozilla.org/r/63824/#review61130

I just confirmed that twice events are fired because there are two devices on the webcam.
When I inserted a Logitech h340 usb headset (with microphone), there is only one event pop up.

Here are some challenges if we want to fire only one event for webcam with two devices.
We can add a timer to drop the second device change event if it comes after the first one within x miniseconds.
But it will be a problem to assign a value to x.

> Understood, but if some code ever forgets to call RemoveDeviceChangeCallback() in some codepath, then we're looking at a sec bug rather than just a leak. Raw pointers are frowned on in the tree for that reason, so smart pointers are preferable whenever we can use them.
> 
> Deregistering in destructors may be a problem actually, because for reference-counted objects, there's a 30 second window where unreferenced expected-to-be-dead objects are waiting to be garbage collected, and they'll still get executed here the rare time a user yanks their USB camera at the wrong time, which would be surprising and undesirable, maybe even dangerous.
> 
> I suggest we call RemoveDeviceChangeCallback() from explicit stop and shutdown methods only. Sorry I missed that initially. Re-opening.
> 
> Also, we can then switch this to a RefPtr (though it'll leak if ever called from a destructor, which I think is fine actually, given that we want to avoid that pattern). If not, I think we need to add some serious comments and asserts around this raw pointer.

OK, I will fix via RefPtr and submit a new patch later.

> If you put the call to RemoveDeviceChangeCallback inside Shutdown, then does MediaManager need to know? Could we then move this inheritance to MediaEngineWebRTC instead? I should have the double shutdown licked in bug 1286096.

OK, I will fix this and submit a new patch later

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

https://reviewboard.mozilla.org/r/63824/#review63848

Sorry for the delay in reviewing this. This looks good, and is almost there.

But we need to cache whether AddDeviceChangeCallback has already been called or this will quickly assert.

Also, I'm still fairly sure deregistering in the destructor is too late, and that we want to deregister in an explicit shutdown method. I suggest MediaManager::Shutdown (should be doable by since mDeviceChangeCallbackList is protected).

::: dom/media/MediaDevices.cpp:208
(Diff revisions 4 - 6)
> +MediaDevices::SetOndevicechange(mozilla::dom::EventHandlerNonNull* aCallback)
> +{
> +  if (NS_IsMainThread()) {
> +    SetEventHandler(nsGkAtoms::ondevicechange, EmptyString(), aCallback);
> +  } else {
> +    SetEventHandler(nullptr, NS_LITERAL_STRING("devicechange"), aCallback);
> +  }
> +
> +  MediaManager::Get()->AddDeviceChangeCallback(this);

Won't this assert if I do:

    navigator.mediaDevices.ondevicechange = () => {};
    navigator.mediaDevices.ondevicechange = () => {};

?

::: dom/media/MediaDevices.cpp:220
(Diff revisions 4 - 6)
> -MediaDevices::PostDeviceChangeEvent()
> +MediaDevices::AddEventListener(const nsAString& aType,
> +  nsIDOMEventListener* aListener,
> +  bool aUseCapture, bool aWantsUntrusted,
> +  uint8_t optional_argc)
>  {
> -  MOZ_ASSERT(NS_IsMainThread());
> +  MediaManager::Get()->AddDeviceChangeCallback(this);

Ditto here.

::: dom/media/MediaDevices.cpp:248
(Diff revisions 4 - 6)
> +MediaDevices::~MediaDevices()
> +{
> +  MediaManager::Get()->RemoveDeviceChangeCallback(this);

I think we need to move this out of a destructor, as I mentioned in the last review.

Can we move it to MediaManager::Shutdown?

::: dom/media/MediaManager.cpp:1973
(Diff revisions 4 - 6)
>    return loadContext && loadContext->UsePrivateBrowsing();
>  }
>  
> +int MediaManager::AddDeviceChangeCallback(DeviceChangeCallback* aCallback)
> +{
> +  MediaManager::PostTask(NewTaskFrom([]() mutable {

nit: mutable keyword is redundant when no vars are captured.

::: dom/media/systemservices/DeviceChangeCallback.h:24
(Diff revisions 4 - 6)
>      {
>        observer->OnDeviceChange();
>      }
>    }
>  
>    virtual int AddDeviceChangeCallback(DeviceChangeCallback* aCallback)

Why not void?

::: dom/media/systemservices/DeviceChangeCallback.h:32
(Diff revisions 4 - 6)
> -    } else {
> -      return -1;
> -    }
>    }
>  
>    virtual int RemoveDeviceChangeCallback(DeviceChangeCallback* aCallback)

Why not void?

::: dom/media/systemservices/DeviceChangeCallback.h:53
(Diff revisions 4 - 6)
>    }
>  
>  protected:
>    nsTArray<DeviceChangeCallback*> mDeviceChangeCallbackList;
>    Mutex mCallbackMutex;
> +  int32_t mRef_count;

mRef_count seems unused.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:430
(Diff revisions 4 - 6)
> -  // Since Shutdown() will be called twice. We don't want to create child again in the 2nd calling.
> -  mozilla::camera::GetChildAndCall_NoCreate(
> -    &mozilla::camera::CamerasChild::RemoveDeviceChangeCallback,
> -    this);
> +  if (mozilla::camera::GetCamerasChildIfExists()) {
> +    mozilla::camera::GetChildAndCall(
> +      &mozilla::camera::CamerasChild::RemoveDeviceChangeCallback, this);
> +  }

nit: we're technically inside the mozilla namespace here so camera:: would suffice, unless you prefer full namespace paths on purpose? Either way.

Comment 14

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://reviewboard.mozilla.org/r/63824/#review63848

> Won't this assert if I do:
> 
>     navigator.mediaDevices.ondevicechange = () => {};
>     navigator.mediaDevices.ondevicechange = () => {};
> 
> ?

I will remove the assert in AddDeviceChangeCallback() and RemoveDeviceChangeCallback()

> I think we need to move this out of a destructor, as I mentioned in the last review.
> 
> Can we move it to MediaManager::Shutdown?

There are multiple MediaDevices' mapping to one MediaManager.
So MediaManager wouldn't have idea which MediaDevices to remove from its callback list.

I have found an api which can fulfill our requirement.
https://dxr.mozilla.org/mozilla-central/source/dom/events/DOMEventTargetHelper.h#149
I will call RemoveDeviceChangeCallback in this function instead of destructor.

> Why not void?

return void will break at this function https://dxr.mozilla.org/mozilla-central/source/dom/media/systemservices/CamerasChild.h#129

> Why not void?

return void will break at this function https://dxr.mozilla.org/mozilla-central/source/dom/media/systemservices/CamerasChild.h#129

Comment 15

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63824/diff/6-7/

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/63824/#review63848

> I will remove the assert in AddDeviceChangeCallback() and RemoveDeviceChangeCallback()

No, I think the assert is correct. We don't want to add it if it's already there. That causes bloat and would also fire events twice as many times, yes?

Instead we could use a boolean to check whether we're already added.

> There are multiple MediaDevices' mapping to one MediaManager.
> So MediaManager wouldn't have idea which MediaDevices to remove from its callback list.
> 
> I have found an api which can fulfill our requirement.
> https://dxr.mozilla.org/mozilla-central/source/dom/events/DOMEventTargetHelper.h#149
> I will call RemoveDeviceChangeCallback in this function instead of destructor.

It should remove all of them, yes?

> return void will break at this function https://dxr.mozilla.org/mozilla-central/source/dom/media/systemservices/CamerasChild.h#129

Hmm, that template means those calls can fail, shouldn't we handle failure or at least log it?

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

https://reviewboard.mozilla.org/r/63824/#review64000

::: dom/media/MediaDevices.cpp:141
(Diff revisions 6 - 7)
> +MediaDevices::DisconnectFromOwner()
> +{
> +  MediaManager::Get()->RemoveDeviceChangeCallback(this);
> +  DOMEventTargetHelper::DisconnectFromOwner();
> +}

I could be wrong but I don't think this is the right API. I still contend we want to tear this down explicitly, not when objects are destructed, which could be much later, and is when I suspect this will be called (when is this called?)

If I remember correctly, we do a lot of per page teardown in MediaManager::OnNavigation

https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#2690

Comment 18

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://reviewboard.mozilla.org/r/63824/#review64000

> I could be wrong but I don't think this is the right API. I still contend we want to tear this down explicitly, not when objects are destructed, which could be much later, and is when I suspect this will be called (when is this called?)
> 
> If I remember correctly, we do a lot of per page teardown in MediaManager::OnNavigation
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#2690

DisconnectFromOwner is not called when the dom object is destroyed. It is called exactly when we tear down the page. In my test, it is called immediately after I close the tab, after several seconds, the MediaDevices is destroyed.

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

That may be ok then. TIL.

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/63824/#review64016

::: dom/media/MediaDevices.cpp:141
(Diff revisions 6 - 7)
> +MediaDevices::DisconnectFromOwner()
> +{
> +  MediaManager::Get()->RemoveDeviceChangeCallback(this);
> +  DOMEventTargetHelper::DisconnectFromOwner();
> +}

That may be ok then. TIL.

But another problem is that MediaManager::Get() will create MediaManager if it doesn't exist, which would be bad if it was never created (which is most of the time). Use MediaManager::GetIfExists() to avoid this.

I think I'd still be happier with a boolean here, e.g.

    if (mHasDeviceChangeCallback) {
      MediaManager::Get()->RemoveDeviceChangeCallback(this)
      mHasDeviceChangeCallback = false;
    }

That way the logic is cleaner, as methods named Add/Remove are typically never idempotent, and people expect them to either throw or compound (i.e. that things are not removed until the same number of Removes are called as were added). I would put back the asserts as well. I think you had those right.

Comment 21

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Also, now that we have this working, we should add a mochitest for this. I know of no easy way to test the low lever code, but we could exercise the API code at least, perhaps by putting in some fake code in MediaEngineDefault, similar to what we do here:

https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/dom/media/webrtc/MediaEngineDefault.cpp#98-102

E.g. Use a fake trigger like a "new device" deviceId string similar to that, and have that fire a devicechange event, and check that in a mochitest. We should check both that it fires when gUM is in use and that it doesn't otherwise. WDYT?

Also, we'll need a DOM review. Since this only works on OSX for now, I'm a bit concerned that perhaps we shouldn't expose this new API on other platforms until it works? I'll ask in #content what common practice is here.

Comment 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Added smaug for DOM review (one .webidl file).

See also outstanding issue https://github.com/w3c/mediacapture-main/issues/375.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #21)

> Also, we'll need a DOM review. Since this only works on OSX for now, I'm a
> bit concerned that perhaps we shouldn't expose this new API on other
> platforms until it works? I'll ask in #content what common practice is here.

Yes, better to have it behind a pref for now.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

And given that this is about event handler, also the event should be dispatched only if the pref is enabled. (and it should be disabled by default for now)

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

https://reviewboard.mozilla.org/r/63824/#review64030

::: dom/webidl/MediaDevices.webidl:15
(Diff revision 7)
>   * liability, trademark and document use rules apply.
>   */
>  
>  [Func="Navigator::HasUserMediaSupport"]
>  interface MediaDevices : EventTarget {
> -//  attribute EventHandler ondevicechange;
> +  attribute EventHandler ondevicechange;

If the event is dispatched only on some platforms for now, better to keep this behind a Pref.
With that, r+

Comment 26

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Thanks smaug!

OK, so if we want to land this before we have the other platforms done, it means we should add something like [Pref="media.ondevicechange.enabled"] to the ondevicechange attribute in the webidl file, add that pref, and default it to false for now.

Comment 27

[Blake Wu [:bwu][:blakewu]]

Munro, 
Since this bug is to implement new web API, it would be required[1] to send your "Intent to Implement" to [e-mail]. 

[1]https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Suggested_implementation_process

Comment 28

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Thanks Blake. Munro, I found Google's for the same feature, if it helps for wording:

https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/Intent$20to$20Implement$3A$20MediaDevices$20devicechange$20event./blink-dev/709H0911zqM/PqDAvxy696wJ

This reminds me I should have done that for bug 1213517 as well. :-/ Will do now. Better late than never.

Comment 29

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Sorry, here's the link I meant: https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/Intent$20to$20implement$20devicechange/blink-dev/wdSV0zUuvJQ/Jd7p_mAIDgAJ

Comment 30

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 1286429 - Fire fake devicechange event in Camera IPC thread for mochitest;

Review commit: https://reviewboard.mozilla.org/r/68792/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68792/

Comment 31

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 1286429 - MediaDevices ondevicechange mochitest;

Review commit: https://reviewboard.mozilla.org/r/68794/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68794/

Comment 32

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63824/diff/7-8/

Comment 33

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8777189 [details]
Bug 1286429 - MediaDevices ondevicechange mochitest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68794/diff/1-2/

Comment 34

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

https://reviewboard.mozilla.org/r/63824/#review66254

Lgtm with Get()/nullptr fix.

::: dom/media/MediaDevices.cpp:143
(Diff revisions 7 - 8)
> -  MediaManager::Get()->RemoveDeviceChangeCallback(this);
> +  if (mHasDeviceChangeCallback) {
> +    MediaManager::GetIfExists()->RemoveDeviceChangeCallback(this);

GetIfExists() can return nullptr, so please either test for nullptr, ASSERT !nullptr (or use Get() again now that it's logically protected by a boolean that ensures it is only called after it's been created).

Comment 35

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8777189 [details]
Bug 1286429 - MediaDevices ondevicechange mochitest;

https://reviewboard.mozilla.org/r/68794/#review66256

I think this test needs a bit of work still, and we may have a problem with gUM use in the tree that is tripping us up...

::: dom/media/tests/mochitest/test_ondevicechange.html:26
(Diff revision 2)
> +function testOnDeviceChangeWithGUM() {
> +  return new Promise(function(resolve, reject) {
> +    navigator.mediaDevices.ondevicechange = function() {
> +      ok(true, "expect devicechange event is fired when gUM is in use");
> +      resolve();
> +    };
> +  });
> +}

Wrapping navigator.mediaDevices.ondevicechange in a promise is a good idea, but always "promisify" at the lowest possible level, to streamline error handling by avoiding app code inside promise wrappers as much as possible. e.g. better:

    new Promise(resolve => navigator.mediaDevices.ondevicechange = resolve)
      .then(e => ok(true, "expect devicechange event is fired when gUM is in use"))

In JS, there's no need to mention unused args, and we're allowed arrow functions, so we might as well use them.

Also, and this may be a style thing I confess, I'm not a fan of functions for functions' sake unless there's reuse or change in encapsulation level (i.e. if it just relocates code to elsewhere in the file). I'd rather read code (with a comment if needed) than exposition in function names. :) Feel free to disagree, this is your test and YMMV.

Some people like a function for each test, but this function isn't a complete test, since there's no ok(false) or timeout, so this function only ever completes if the event fires, so it seems misnamed at best.

::: dom/media/tests/mochitest/test_ondevicechange.html:35
(Diff revision 2)
> +function getusermedia() {
> +  return new Promise(function(resolve, reject) {
> +    var constraints;
> +    constraints = {video: true, fake: true}
> +
> +    navigator.mediaDevices.getUserMedia(constraints)
> +    .then(function(mediaStream) {
> +        ok(true, "getUserMedia is working");
> +        resolve();
> +    }).catch(function(error) {
> +        ok(false, "getUserMedia is working");
> +        reject();
> +    })
> +  });
> +}

Promises need only be explicitly created when wrapping non-promise APIs.

When dealing with APIs that already return promises, wrapping them is considered the "Promise constructor anti-pattern" [1].

This wrapper also seems superfluous so I would just remove it, and inline the call. gUM is tested elsewhere.

[1] http://stackoverflow.com/q/23803743/918910

::: dom/media/tests/mochitest/test_ondevicechange.html:51
(Diff revision 2)
> +function start() {
> +  info("check if devicechange event is fired when gUM is NOT in use");
> +  testOnDeviceChangeWithoutGUM().then(function() {
> +    info("launch gUM");

I'm concerned if this test works.

testOnDeviceChangeWithoutGUM() only fulfills its promise if navigator.mediaDevices.ondevicechange fired, which shouldn't happenz since gUM hasn't been called (right?), so we should never get past this point.

Lets come back to why it may be working (I'm assuming you tried it?), and first look at some problems with the test first instead:

You have no ok(false), which means this test only succeeds or times out, which isn't great. It also seems wrong ATM. Since you've implemented the fake devicechange with a 5 second interval, you'll need to change testOnDeviceChangeWithoutGUM() to wait ~6 seconds at which point you want to do ok(true) if NO event fired, and ok(false) otherwise, and proceed to the next test. You can use Promise.race to create a timeout.

See [1] for a similar non-event test using race (with a 0 second wait, for an event guaranteed to fire on next cycle, but you'd need a longer wait obviously). It's in mediaStreamPlayback.js, which has a ton of helper functions, most of which you wont need so unsure if you'd want to pull that in just for this one, up to you. We also added the handy haveEvent() function recently (in head.js) if you're interested. It goes a little nuts with promises, so don't hesitate to ask if you have questions, or prefer to roll your own. It reminds me though, should we test addEventListener too? We may also want to test clearing of ondevicechange.

[1] https://dxr.mozilla.org/mozilla-central/rev/331c4166a3a2df2d3a037107addef5d85cdc31b5/dom/media/tests/mochitest/mediaStreamPlayback.js#60

::: dom/media/tests/mochitest/test_ondevicechange.html:55
(Diff revision 2)
> +    return getusermedia();
> +  }).then(function() {

Back to why it works...

You're ignoring the stream returned from getUserMedia here. We need to do a:

    stream.getTracks().forEach(track => track.stop())

on the stream from gUM, otherwise the stream stays active until garbage collection happens about 30 seconds later.

And that's a problem for us...

I suspect a lot of other tests in our tree do this as well, which really is a problem for our particular test, and is why, I suspect, that we're seeing devicechange fired without (our) gUM call. :/

Even the web-platform-tests which we import from w3c do this (Bug 1290854 comment 5) which stinks.

Anyway, take this opportunity to test the failure path, and maybe do the test in isolation to confirm this is the problem? I suspect we'll have to go through and clean up some of the other gUM tests here.

I already found one in bug 1088621 - https://reviewboard.mozilla.org/r/67556/diff/2#index_header

::: dom/media/tests/mochitest/test_ondevicechange.html:59
(Diff revision 2)
> +    info("launch gUM");
> +    return getusermedia();
> +  }).then(function() {
> +    info("check if devicechange event is fired when gUM is in use");
> +    return testOnDeviceChangeWithGUM();
> +  }).then(SimpleTest.finish);;

You'll want a catch here, otherwise all kinds of errors won't get reported promptly. Also passing SimpleTest.finish like this will use the wrong 'this' pointer, which may be a problem (though turns out it's not, but ugh *). Try:

    })
    .catch(e => ok(false, "Unhandled error " + e))
    .then(() => SimpleTest.finish());

*) Looking in the tree, I see SimpleTest doesn't use 'this', and this is apparently a pattern, but I'm still not a fan of such assumptions, up to you.

Comment 36

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8777188 [details]
Bug 1286429 - Fire fake devicechange event in Camera IPC thread for mochitest;

https://reviewboard.mozilla.org/r/68792/#review66278

lgtm with nits

::: dom/media/systemservices/CamerasChild.h:163
(Diff revision 1)
>                                  const int64_t&) override;
>    virtual bool RecvFrameSizeChange(const int&, const int&,
>                                     const int& w, const int& h) override;
>  
>    virtual bool RecvDeviceChange() override;
> +  virtual int AddDeviceChangeCallback_(DeviceChangeCallback* aCallback, bool aFakeDeviceChangeEventOn);

Piggybacking here seems a bit odd, and unclear with the _.

Can we maybe make this an explicit separate function instead? E.g SetFakeDeviceChangeEvents(bool)?

::: dom/media/systemservices/CamerasChild.cpp:44
(Diff revision 1)
>  
>  CamerasSingleton::~CamerasSingleton() {
>    LOG(("~CamerasSingleton: %p", this));
>  }
>  
> +class FakeOnDeviceChangeEvent: public Runnable

FakeOnDeviceChangeEventRunnable, or it looks like an event.

Space before :

::: dom/media/systemservices/CamerasChild.cpp:55
(Diff revision 1)
> +      RefPtr<FakeOnDeviceChangeEvent> evt = new FakeOnDeviceChangeEvent();
> +      CamerasSingleton::Thread()->DelayedDispatch(evt.forget(), FAKE_ONDEVICECHANGE_EVENT_PERIOD_IN_MS);

I always wonder why we don't just do:

CamerasSingleton::Thread()->DelayedDispatch(new FakeOnDeviceChangeEventRunnable(),

::: dom/media/systemservices/CamerasChild.cpp:56
(Diff revision 1)
> +    CamerasChild* child = CamerasSingleton::Child();
> +    if (child) {
> +      child->OnDeviceChange();
> +
> +      RefPtr<FakeOnDeviceChangeEvent> evt = new FakeOnDeviceChangeEvent();
> +      CamerasSingleton::Thread()->DelayedDispatch(evt.forget(), FAKE_ONDEVICECHANGE_EVENT_PERIOD_IN_MS);

Does this tear down cleanly? (It might, I'm not familiar).

Also, what turns it off? Just concerned if it keeps beating after the test that enabled it has ended...

Nit: maybe wordwrap.

::: dom/media/systemservices/CamerasChild.cpp:626
(Diff revision 1)
> +  // To simulate the devicechange event in mochitest,
> +  // we fire a fake devicechange event in Camera IPC thread periodically
> +  if (aFakeDeviceChangeEventOn)
> +  {
> +    RefPtr<FakeOnDeviceChangeEvent> evt = new FakeOnDeviceChangeEvent();
> +    CamerasSingleton::Thread()->Dispatch(evt, NS_DISPATCH_NORMAL);

not .forget()?

Comment 37

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://reviewboard.mozilla.org/r/68794/#review66256

> I'm concerned if this test works.
> 
> testOnDeviceChangeWithoutGUM() only fulfills its promise if navigator.mediaDevices.ondevicechange fired, which shouldn't happenz since gUM hasn't been called (right?), so we should never get past this point.
> 
> Lets come back to why it may be working (I'm assuming you tried it?), and first look at some problems with the test first instead:
> 
> You have no ok(false), which means this test only succeeds or times out, which isn't great. It also seems wrong ATM. Since you've implemented the fake devicechange with a 5 second interval, you'll need to change testOnDeviceChangeWithoutGUM() to wait ~6 seconds at which point you want to do ok(true) if NO event fired, and ok(false) otherwise, and proceed to the next test. You can use Promise.race to create a timeout.
> 
> See [1] for a similar non-event test using race (with a 0 second wait, for an event guaranteed to fire on next cycle, but you'd need a longer wait obviously). It's in mediaStreamPlayback.js, which has a ton of helper functions, most of which you wont need so unsure if you'd want to pull that in just for this one, up to you. We also added the handy haveEvent() function recently (in head.js) if you're interested. It goes a little nuts with promises, so don't hesitate to ask if you have questions, or prefer to roll your own. It reminds me though, should we test addEventListener too? We may also want to test clearing of ondevicechange.
> 
> [1] https://dxr.mozilla.org/mozilla-central/rev/331c4166a3a2df2d3a037107addef5d85cdc31b5/dom/media/tests/mochitest/mediaStreamPlayback.js#60

In this test case, I mimic the behavior that user grant "always share" to gUM via setting ["media.navigator.permission.disabled", true]. According to the spec, when user grant perminent permission, ondevicechange event should be fired even gUM hasn't been called. And yes, I indeed tried it and it is working.

Comment 38

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://reviewboard.mozilla.org/r/68794/#review66256

> In this test case, I mimic the behavior that user grant "always share" to gUM via setting ["media.navigator.permission.disabled", true]. According to the spec, when user grant perminent permission, ondevicechange event should be fired even gUM hasn't been called. And yes, I indeed tried it and it is working.

https://reviewboard-hg.mozilla.org/gecko/rev/4afef0038b82#l2.33

Comment 39

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/68794/#review66256

> https://reviewboard-hg.mozilla.org/gecko/rev/4afef0038b82#l2.33

Ah, of course. That's a relief. We should test with it set to false as well then, to make sure we don't fire always.

Comment 40

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63824/diff/8-9/

Comment 41

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8777188 [details]
Bug 1286429 - Fire fake devicechange event in Camera IPC thread for mochitest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68792/diff/1-2/

Comment 42

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8777189 [details]
Bug 1286429 - MediaDevices ondevicechange mochitest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68794/diff/2-3/

Comment 43

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cd919967e7f

Comment 44

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

https://reviewboard.mozilla.org/r/63824/#review68038

::: dom/media/MediaDevices.cpp:155
(Diff revisions 8 - 9)
> +  //In normal case, we should remove the callback in DisconnectFromOwner(),
> +  //However, to be fail-safe, we also need to remove the callback in dtor
> +  //in case of any unexpected flow.
> +  if (mHasDeviceChangeCallback) {
> +    MediaManager* mediamanager = MediaManager::GetIfExists();
> +    MOZ_ASSERT(mediamanager != nullptr);

Can this actually happen? Code that never runs is untested code.

Note that GC may happen up to 30 seconds later, so MediaManager may be long gone if this ever happens, so this'll likely assert in debug, which is what I'd recommend doing here anyway.

Comment 45

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8777189 [details]
Bug 1286429 - MediaDevices ondevicechange mochitest;

https://reviewboard.mozilla.org/r/68794/#review68044

Lgtm with nits! We want to avoid mixing promises and callbacks because it is hard to follow, and has poor error handling characteristics to boot.

Ideally we want a single promise chain with a single

    .catch(e => whatever)
    .then(() => SimpleTest.finish());

at the very end. FWIW Andreas Pehrsons uses an indentation style I like for breaking up tests logically:

https://dxr.mozilla.org/mozilla-central/rev/6cf0089510fad8deb866136f5b92bbced9498447/dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html#16

::: dom/media/tests/mochitest/test_ondevicechange.html:20
(Diff revision 3)
> +
> +const RESPONSE_WAIT_TIME_MS = 10000;
> +
> +// devicechange event should be fired when gum is in use
> +function OnDeviceChangeEvent() {
> +  return new Promise((resolve, reject) => navigator.mediaDevices.ondevicechange = resolve);

Nit: reject is unused and can be removed. e.g.

    new Promise(resolve => navigator.mediaDevices.ondevicechange = resolve);

is shorter and simpler with fewer parenthesis. Applies throughout.

::: dom/media/tests/mochitest/test_ondevicechange.html:26
(Diff revision 3)
> +}
> +
> +function OnDeviceChangeEventReceived() {
> +  return Promise.race([
> +    OnDeviceChangeEvent(),
> +    new Promise((resolve, reject) => setTimeout(reject, RESPONSE_WAIT_TIME_MS, "Timed out while waiting for devicechange event")),

Nit: setTimeout is a callback API with terrible error handling characteristics, so we want to wrap it tightly in a promise-returning wait() helper, like this one:

https://dxr.mozilla.org/mozilla-central/rev/6cf0089510fad8deb866136f5b92bbced9498447/dom/media/tests/mochitest/head.js#445

::: dom/media/tests/mochitest/test_ondevicechange.html:41
(Diff revision 3)
> +    SpecialPowers.pushPrefEnv({"set": [
> +      ["media.ondevicechange.fakeDeviceChangeEvent.enabled", true],
> +      ['media.navigator.permission.disabled', true],
> +      ["media.ondevicechange.enabled", true]
> +      ]}, test2);

Use a helper like this to avoid mixing callbacks and promises:

https://dxr.mozilla.org/mozilla-central/rev/6cf0089510fad8deb866136f5b92bbced9498447/dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html#18

::: dom/media/tests/mochitest/test_ondevicechange.html:70
(Diff revision 3)
> +}
> +
> +function test3() {
> +  info("assure devicechange event is fired when gUM is in use");
> +  var videoTracks;
> +  navigator.mediaDevices.getUserMedia({video: true, fake: true}).then((stream) => {

Nit: redundant parens around stream.

::: dom/media/tests/mochitest/test_ondevicechange.html:73
(Diff revision 3)
> +  }).then(() => {
> +    ok(true, "devicechange event is fired when gUM is in use");
> +  }).catch(e => {
> +    ok(false, "Error: " + e);
> +  }).then(() => {

Nit: redundant {}s around one liners. Prefer:

    .then(() => ok(true, "..."))
    .catch(e => ok(false, "Error: " + e))

Comment 46

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8777189 [details]
Bug 1286429 - MediaDevices ondevicechange mochitest;

https://reviewboard.mozilla.org/r/68794/#review68044

(To clarify: you'd still use the individual catches you have for each test today of course, I just mean have one SimpleTest.finish(), and that it should come after any final catch so it never gets skipped even when there's an error).

Comment 47

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Also, https://github.com/w3c/mediacapture-main/pull/376 from comment 11 was merged, so it might make sense to make it so that we only fire one event when a device with both camera and mic is inserted.

Comment 48

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #47)
> Also, https://github.com/w3c/mediacapture-main/pull/376 from comment 11 was
> merged, so it might make sense to make it so that we only fire one event
> when a device with both camera and mic is inserted.
OK, I have file a extension bug 1294316 to trace this.

Comment 49

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63824/diff/9-10/

Comment 50

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8777188 [details]
Bug 1286429 - Fire fake devicechange event in Camera IPC thread for mochitest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68792/diff/2-3/

Comment 51

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8777189 [details]
Bug 1286429 - MediaDevices ondevicechange mochitest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68794/diff/3-4/

Comment 52

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

https://reviewboard.mozilla.org/r/63824/#review68038

> Can this actually happen? Code that never runs is untested code.
> 
> Note that GC may happen up to 30 seconds later, so MediaManager may be long gone if this ever happens, so this'll likely assert in debug, which is what I'd recommend doing here anyway.

I found DisconnectFromOwner() is not called when the mochitest test_ondevicechange.html ends.
After reviewing the design, a better place to remove the callback should be in MediaManager::OnNavigation.

Comment 53

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8777189 [details]
Bug 1286429 - MediaDevices ondevicechange mochitest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68794/diff/4-5/

Comment 54

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8777189 [details]
Bug 1286429 - MediaDevices ondevicechange mochitest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68794/diff/5-6/

Comment 55

[Ryan VanderMeulen [:RyanVM]]

These patches need rebasing. Also, can you please adjust your commit messages to use the more-standard "Bug XXX - blah blah blah" format? Putting the bug # in parens at the end of the commit message makes it harder to find at a glance.

Comment 56

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: WIP-01.patch

Comment 57

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: WIP-02.patch

Comment 58

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: WIP-03.patch

Comment 59

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Carry r+ from jib

Comment 60

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8770354 [details]
Bug 1286429 - implement mediaDevices.ondevicechange for Mac OSX;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63824/diff/10-11/

Comment 61

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8777188 [details]
Bug 1286429 - Fire fake devicechange event in Camera IPC thread for mochitest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68792/diff/3-4/

Comment 62

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8777189 [details]
Bug 1286429 - MediaDevices ondevicechange mochitest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68794/diff/6-7/

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df5bd6813b75
implement mediaDevices.ondevicechange for Mac OSX; r=jib,smaug
https://hg.mozilla.org/integration/autoland/rev/5022a33fd3e9
Fire fake devicechange event in Camera IPC thread for mochitest; r=jib
https://hg.mozilla.org/integration/autoland/rev/5ff0ce53fccf
MediaDevices ondevicechange mochitest; r=jib

Comment 64

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/df5bd6813b75
https://hg.mozilla.org/mozilla-central/rev/5022a33fd3e9
https://hg.mozilla.org/mozilla-central/rev/5ff0ce53fccf

Comment 65

[Nicolas Silva [:nical]]

This if block needs to be braced: http://hg.mozilla.org/integration/mozilla-inbound/rev/df5bd6813b75#l4.80

Either this is a bug, or it looks like one. Either way, it is best to not write if blocks without braces. Could you fix this in a followup? Thanks!

Comment 66

[Eric Shepherd [:sheppy]]

Tracking all the pieces of this since it's being implemented on different platforms on different schedules, plus the pref. All of that will affect the developer docs and release notes in various ways.

Comment 67

[Eric Shepherd [:sheppy]]

The devicechange event was already documented, but I updated it and added compatibility information for Firefox: https://developer.mozilla.org/en-US/docs/Web/Events/devicechange

I then added this page for MediaDevices.ondevicechange: https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/ondevicechange. It needs an example added and that should happen tomorrow. I will track that work over on bug 1152383.

I updated the information about the event handler on the main MediaDevices page, and added compat information there as well: https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices

I then noted on Firefox 51 for developers that this arrived on OS X only, and that it's preffed off; I also added appropriate information to these overview pages:

https://developer.mozilla.org/en-US/Firefox/Experimental_features
https://developer.mozilla.org/en-US/Firefox/Releases/51

Leaving this doc needed until I get that example done tomorrow, however.

Comment 68

[Eric Shepherd [:sheppy]]

Actually, I'll track adding the example on bug 1152383 instead.