1201590 - Implement DOM Bindings for WebMIDI API

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P3)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Implement WebIDL Bindings and backing code to support WebMIDI.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - WIP: WebMIDI DOM Implementation

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - WIP: WebMIDI Mochitests

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - WebMIDI DOM Implementation

Welp. Here we go. DOM Implementation of WebMIDI, ready for the first round of reviews. Note that this is only DOM and the testing class, no actual platform specifics.

I realize this is a huge patch, so if there's some way I can split this up that would help, or if you want it in reviewboard instead of splinter, lemme know.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v2) - Mochitests for WebMIDI

Comment 5

[Paul Adenot (:padenot)]

Kyle, I'd rather review this on mozreview, there is probably going to be a couple back and forth and having a working interdiff is going to be important.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1201590 - WebMIDI WebIDL files;

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

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1201590 - WebMIDI Mochitests

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

Comment 8

[Paul Adenot (:padenot)]

I think I'll start by re-reading the spec, so I'm not going to start reviewing the actual implementation just now.

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

https://reviewboard.mozilla.org/r/54682/#review52852

These are the first comments. Do you mind if I continue the review when these comments are applied?

::: browser/components/nsBrowserGlue.js:2620
(Diff revision 1)
>  
>      this._showPrompt(aRequest, message, "geo", actions, "geolocation",
>                       "geo-notification-icon", options);
>    },
>  
> +  _promptMIDI : function(aRequest) {

I ignore all the browser/* changes. Ask somebody else to review this code, I'm not a peer for it.

::: dom/midi/MIDIAccess.h:19
(Diff revision 1)
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsIObserver.h"
> +#include "nsWrapperCache.h"
> +
> +
> +

3 lines here? What about 1 or 2 only? :)

::: dom/midi/MIDIAccess.h:30
(Diff revision 1)
> +class Promise;
> +class MIDIInputMap;
> +class MIDIOutputMap;
> +class MIDIPort;
> +class MIDIPortInfo;
> +class MIDIAccessManager;

alphabetic order here.

::: dom/midi/MIDIAccess.h:67
(Diff revision 1)
> +
> +  virtual JSObject* WrapObject(JSContext* aCx,
> +                               JS::Handle<JSObject*> aGivenProto) override;
> +
> +  // Return map of MIDI Input Ports
> +  MIDIInputMap* Inputs()

MIDIInputMap* Inputs() const

::: dom/midi/MIDIAccess.h:73
(Diff revision 1)
> +  {
> +    return mInputMap;
> +  }
> +
> +  // Return map of MIDI Output Ports
> +  MIDIOutputMap* Outputs()

MIDIOutputMap* Outputs() const

::: dom/midi/MIDIAccess.h:101
(Diff revision 1)
> +  // Notify all MIDIPorts that were created by this MIDIAccess and are still
> +  // alive, and detach from the MIDIAccessManager.
> +  void Shutdown();
> +  IMPL_EVENT_HANDLER(statechange);
> +private:
> +  explicit MIDIAccess(nsPIDOMWindowInner* aWindow, bool aSysexEnabled,

1. remove explicit here.
2. RefPtr<Promise> ? Why not Promise*?
3. mAccessPromise -> aAccessPromise

::: dom/midi/MIDIAccess.h:103
(Diff revision 1)
> +  void Shutdown();
> +  IMPL_EVENT_HANDLER(statechange);
> +private:
> +  explicit MIDIAccess(nsPIDOMWindowInner* aWindow, bool aSysexEnabled,
> +                      RefPtr<Promise> mAccessPromise);
> +  virtual ~MIDIAccess();

final class. Remove 'virtual'

::: dom/midi/MIDIAccess.cpp:59
(Diff revision 1)
> +NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)
> +
> +NS_IMPL_ADDREF_INHERITED(MIDIAccess, DOMEventTargetHelper)
> +NS_IMPL_RELEASE_INHERITED(MIDIAccess, DOMEventTargetHelper)
> +
> +MIDIAccess::MIDIAccess(nsPIDOMWindowInner* aWindow, bool aSysexEnabled, RefPtr<Promise> aPromise) :

Promise* aPromise

::: dom/midi/MIDIAccess.cpp:67
(Diff revision 1)
> +  mOutputMap(new MIDIOutputMap(aWindow)),
> +  mSysexEnabled(aSysexEnabled),
> +  mAccessPromise(aPromise),
> +  mHasShutdown(false)
> +{
> +  MOZ_COUNT_CTOR(MIDIAccess);

We use these macros only for non CCed/Refcnt classes. Remove them from here.

::: dom/midi/MIDIAccess.cpp:84
(Diff revision 1)
> +MIDIAccess::Shutdown()
> +{
> +  if (mHasShutdown) {
> +    return;
> +  }
> +  mDestructionObservers.Broadcast(true);

Add a comment about this 'true'

::: dom/midi/MIDIAccess.cpp:93
(Diff revision 1)
> +  mHasShutdown = true;
> +}
> +
> +void
> +MIDIAccess::FireConnectionEvent(MIDIPort* aPort)
> +{

MOZ_ASSERT(aPort)

::: dom/midi/MIDIAccess.cpp:96
(Diff revision 1)
> +void
> +MIDIAccess::FireConnectionEvent(MIDIPort* aPort)
> +{
> +  MIDIConnectionEventInit init;
> +  init.mPort = aPort;
> +  nsString id;

nsAutoString

::: dom/midi/MIDIAccess.cpp:100
(Diff revision 1)
> +  init.mPort = aPort;
> +  nsString id;
> +  aPort->GetId(id);
> +  ErrorResult rv;
> +  if (aPort->State() == MIDIPortDeviceState::Disconnected) {
> +    MIDIInputMapBinding::MaplikeHelpers::Delete(mInputMap, id, rv);

This is wrong. if this Delete throws an exception, then, reusing the same ErrorResult for the second rv calls a MOZ_CRASH.

What about if we check if these delete  calls fail?

...::Delete(..., rv);
if (NS_WARN_IF(rv.Failed())) { ...

...::Delete(..., rv);
if (NS_WARN_IF(rv.Failed())) { ...

or write a MOZ_ASSERT(!rv.Failed(), "With a good comment here");

::: dom/midi/MIDIAccess.cpp:105
(Diff revision 1)
> +    MIDIInputMapBinding::MaplikeHelpers::Delete(mInputMap, id, rv);
> +    MIDIOutputMapBinding::MaplikeHelpers::Delete(mOutputMap, id, rv);
> +  } else {
> +    // If we receive an event from a port that is not in one of our port maps,
> +    // this means a port that was disconnected has been reconnected, with the port
> +    // owner holding the object during that time, and we should readd that port

readd ?

::: dom/midi/MIDIAccess.cpp:107
(Diff revision 1)
> +  } else {
> +    // If we receive an event from a port that is not in one of our port maps,
> +    // this means a port that was disconnected has been reconnected, with the port
> +    // owner holding the object during that time, and we should readd that port
> +    // object to our maps.
> +    if (aPort->Type() == MIDIPortType::Input) {

} else if { ... instead 

} else {
  if (...

::: dom/midi/MIDIAccess.cpp:108
(Diff revision 1)
> +    // If we receive an event from a port that is not in one of our port maps,
> +    // this means a port that was disconnected has been reconnected, with the port
> +    // owner holding the object during that time, and we should readd that port
> +    // object to our maps.
> +    if (aPort->Type() == MIDIPortType::Input) {
> +      if (!MIDIInputMapBinding::MaplikeHelpers::Has(mInputMap, id, rv)) {

same problem with rv here. Everywhere in this method.

::: dom/midi/MIDIAccess.cpp:111
(Diff revision 1)
> +    // object to our maps.
> +    if (aPort->Type() == MIDIPortType::Input) {
> +      if (!MIDIInputMapBinding::MaplikeHelpers::Has(mInputMap, id, rv)) {
> +        MIDIInputMapBinding::MaplikeHelpers::Set(mInputMap, id, *(static_cast<MIDIInput*>(aPort)), rv);
> +      }
> +    } else {

MOZ_ASSERT(aPort->Type() == MIDIPortType::Output) ?

::: dom/midi/MIDIAccess.cpp:116
(Diff revision 1)
> +    } else {
> +      if (!MIDIOutputMapBinding::MaplikeHelpers::Has(mOutputMap, id, rv)) {
> +        MIDIOutputMapBinding::MaplikeHelpers::Set(mOutputMap, id, *(static_cast<MIDIOutput*>(aPort)), rv);
> +      }
> +    }
> +  }

You must call:  rv.SuppressException() somewhere.

::: dom/midi/MIDIAccess.cpp:122
(Diff revision 1)
> +  RefPtr<MIDIConnectionEvent> event =
> +    MIDIConnectionEvent::Constructor(this, NS_LITERAL_STRING("statechange"), init);
> +  DispatchTrustedEvent(event);
> +}
> +
> +void

maybe it a boolean return value to stop the notification loop. Read Notify() comments.

::: dom/midi/MIDIAccess.cpp:128
(Diff revision 1)
> +MIDIAccess::MaybeCreateMIDIPort(const MIDIPortInfo& aInfo)
> +{
> +  ErrorResult rv;
> +  nsString id = aInfo.id();
> +  MIDIPortType type = static_cast<MIDIPortType>(aInfo.type());
> +  // TODO Should probably unduplicate this code somehow.

any follow up?

::: dom/midi/MIDIAccess.cpp:131
(Diff revision 1)
> +  nsString id = aInfo.id();
> +  MIDIPortType type = static_cast<MIDIPortType>(aInfo.type());
> +  // TODO Should probably unduplicate this code somehow.
> +  if (type == MIDIPortType::Input) {
> +    // TODO Do something with return value here
> +    if (MIDIInputMapBinding::MaplikeHelpers::Has(mInputMap, id, rv)) {

if (NS_WARN_IF(rv.Failed())) {
  mAccessPromise->MaybeReject(rv);
  mAccessPromise = nullptr;
  return false;
}

::: dom/midi/MIDIAccess.cpp:135
(Diff revision 1)
> +    // TODO Do something with return value here
> +    if (MIDIInputMapBinding::MaplikeHelpers::Has(mInputMap, id, rv)) {
> +      return;
> +    }
> +    RefPtr<MIDIInput> port(new MIDIInput(GetOwner(), this, aInfo, mSysexEnabled));
> +    mDestructionObservers.AddObserver(port);

Move this after ::Set() in order to avoid 'dirty' data if something goes wrong.

::: dom/midi/MIDIAccess.cpp:140
(Diff revision 1)
> +    mDestructionObservers.AddObserver(port);
> +    // TODO Do something with return value here
> +    MIDIInputMapBinding::MaplikeHelpers::Set(mInputMap,
> +                                             id,
> +                                             *port,
> +                                             rv);

Do something with rv.

::: dom/midi/MIDIAccess.cpp:146
(Diff revision 1)
> +    // If we haven't resolved the promise for handing the MIDIAccess object to
> +    // content, this means we're still populating the list of already connected
> +    // devices. Don't fire events yet.
> +    if (!mAccessPromise) {
> +      FireConnectionEvent(port);
> +    }

return;

::: dom/midi/MIDIAccess.cpp:147
(Diff revision 1)
> +    // content, this means we're still populating the list of already connected
> +    // devices. Don't fire events yet.
> +    if (!mAccessPromise) {
> +      FireConnectionEvent(port);
> +    }
> +  } else if (type == MIDIPortType::Output) {

if (type == MIDIPortType::Ooutput) {
...

same comments about 'rv' and the promise.

::: dom/midi/MIDIAccess.cpp:153
(Diff revision 1)
> +    // TODO Do something with return value here
> +    if (MIDIOutputMapBinding::MaplikeHelpers::Has(mOutputMap, id, rv)) {
> +      return;
> +    }
> +    RefPtr<MIDIOutput> port(new MIDIOutput(GetOwner(), this, aInfo, mSysexEnabled));
> +    mDestructionObservers.AddObserver(port);

same comment here.

::: dom/midi/MIDIAccess.cpp:164
(Diff revision 1)
> +    // If we haven't resolved the promise for handing the MIDIAccess object to
> +    // content, this means we're still populating the list of already connected
> +    // devices. Don't fire events yet.
> +    if (!mAccessPromise) {
> +      FireConnectionEvent(port);
> +    }

return;

::: dom/midi/MIDIAccess.cpp:168
(Diff revision 1)
> +      FireConnectionEvent(port);
> +    }
> +  } else {
> +    // If we hit this, then we have some port that is neither input nor output.
> +    // That is bad.
> +    MOZ_CRASH("We shouldn't be here!");

remove } else {

::: dom/midi/MIDIAccess.cpp:170
(Diff revision 1)
> +  } else {
> +    // If we hit this, then we have some port that is neither input nor output.
> +    // That is bad.
> +    MOZ_CRASH("We shouldn't be here!");
> +  }
> +}

Just because this method is quite complex, I would pass a ErrorResult as param and use it everywhere instead 'rv'. If something fails, do a quick return.

And in Notify() do:

ErrorResultrv;
MaybeCreateDOMPort(foo, rv);
if (NS_WARN_IF(rv.Failed())) {
  mAccessPromise->MaybeReject(rv);
  mAccesSPromise = nullptr;
  return;
}

::: dom/midi/MIDIAccess.cpp:177
(Diff revision 1)
> +// For the MIDIAccess object, only worry about new connections, where we create
> +// MIDIPort objects. When a port is removed and the TMIDIPortRemove event is
> +// received, that will be handled by the MIDIPort object itself, and it will
> +// request removal from MIDIAccess's maps.
> +void
> +MIDIAccess::Notify(const MIDIPortList &aEvent)

If something goes wrong in MaybeCreateDOMMIDIPort() we want to reject the promise, right?

So what about if we do:

// Something went wrong...
if (!MaybeCreateMIDIPort(port)) {
  return;
}

::: dom/midi/MIDIAccess.cpp:183
(Diff revision 1)
> +{
> +  for (auto& port : aEvent.ports()) {
> +    MaybeCreateMIDIPort(port);
> +  }
> +  if (mAccessPromise) {
> +    // TODO According to the spec we should return both this /and/ the

follow up?

::: dom/midi/MIDIAccess.cpp:198
(Diff revision 1)
> +  return MIDIAccessBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +void
> +MIDIAccess::RemovePortListener(Observer<bool>* aObs)
> +{

Do we want to assert that this aObs is not already contained in mDestructionObservers ?

::: dom/midi/MIDIAccessManager.h:50
(Diff revision 1)
> +  NS_DECL_NSIOBSERVER
> +public:
> +  // Handles requests from Navigator for MIDI permissions and MIDIAccess
> +  // creation.
> +  already_AddRefed<Promise>
> +    RequestMIDIAccess(nsPIDOMWindowInner* aWindow,

Usually we indent:

already_AddRefed<Promise>
RequesTMIDIAccess(...

::: dom/midi/MIDIAccessManager.h:54
(Diff revision 1)
> +  already_AddRefed<Promise>
> +    RequestMIDIAccess(nsPIDOMWindowInner* aWindow,
> +                      const MIDIOptions& aOptions,
> +                      ErrorResult& aRv);
> +  // Creates a new MIDIAccess object
> +  void CreateMIDIAccess(nsPIDOMWindowInner* aWindow, bool aNeedsSysex, RefPtr<Promise> aPromise);

Promise* aPromise

::: dom/midi/MIDIAccessManager.h:62
(Diff revision 1)
> +  // True if manager singleton has been created
> +  static bool IsRunning();
> +  // Send device connection updates to all known MIDIAccess objects.
> +  void Update(const MIDIPortList& aEvent);
> +  // Adds a device update observer (usually a MIDIAccess object)
> +  void AddObserver(Observer<MIDIPortList>* o);

o => aObserver

::: dom/midi/MIDIAccessManager.h:64
(Diff revision 1)
> +  // Send device connection updates to all known MIDIAccess objects.
> +  void Update(const MIDIPortList& aEvent);
> +  // Adds a device update observer (usually a MIDIAccess object)
> +  void AddObserver(Observer<MIDIPortList>* o);
> +  // Removes a device update observer (usually a MIDIAccess object)
> +  void RemoveObserver(Observer<MIDIPortList>* o);

o => aObserver

::: dom/midi/MIDIAccessManager.cpp:35
(Diff revision 1)
> +class MIDIPermissionRequest final
> +  : public nsIContentPermissionRequest,
> +    public nsIRunnable
> +{
> +public:
> +  MIDIPermissionRequest(nsPIDOMWindowInner* aWindow, RefPtr<Promise> aPromise, const MIDIOptions& aOptions);

Promise* aPromise

::: dom/midi/MIDIAccessManager.cpp:43
(Diff revision 1)
> +  NS_DECL_NSICONTENTPERMISSIONREQUEST
> +  NS_DECL_NSIRUNNABLE
> +  NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(MIDIPermissionRequest,
> +                                           nsIContentPermissionRequest)
> +private:
> +  virtual

no virtual for final classes.

::: dom/midi/MIDIAccessManager.cpp:70
(Diff revision 1)
> +
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(MIDIPermissionRequest)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(MIDIPermissionRequest)
> +
> +MIDIPermissionRequest::MIDIPermissionRequest(nsPIDOMWindowInner* aWindow,
> +                                             RefPtr<Promise> aPromise,

ditto

::: dom/midi/MIDIAccessManager.cpp:76
(Diff revision 1)
> +                                             const MIDIOptions& aOptions)
> +: mWindow(aWindow),
> +  mPromise(aPromise),
> +  mNeedsSysex(aOptions.mSysex),
> +  mRequester(new nsContentPermissionRequester(mWindow))
> +{

MOZ_ASSERT(aWindow)

::: dom/midi/MIDIAccessManager.cpp:79
(Diff revision 1)
> +  mNeedsSysex(aOptions.mSysex),
> +  mRequester(new nsContentPermissionRequester(mWindow))
> +{
> +  MOZ_ASSERT(aPromise, "aPromise should not be null!");
> +  nsCOMPtr<nsIDocument> doc = mWindow->GetDoc();
> +  if (!doc) {

So... this is bad, right?
What about if you move this check when MIDIPermissionRequest is created?

Or you implement a ::Init(ErrorResult& aRv) where you do this check and in case, aRv.Throw(NS_ERROR_FAILURE);

::: dom/midi/MIDIAccessManager.cpp:100
(Diff revision 1)
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +MIDIPermissionRequest::GetTypes(nsIArray** aTypes)
> +{

NS_ENSURE_ARG_POINTER(aTypes)

::: dom/midi/MIDIAccessManager.cpp:113
(Diff revision 1)
> +                                                         aTypes);
> +}
> +
> +NS_IMETHODIMP
> +MIDIPermissionRequest::GetPrincipal(nsIPrincipal** aRequestingPrincipal)
> +{

NS_ENSURE_ARG_POINTER(aRequestingPrincipal)

::: dom/midi/MIDIAccessManager.cpp:120
(Diff revision 1)
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +MIDIPermissionRequest::GetWindow(mozIDOMWindow** aRequestingWindow)
> +{

NS_ENSURE_ARG_POINTER(aRequestingWindow)

::: dom/midi/MIDIAccessManager.cpp:127
(Diff revision 1)
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +MIDIPermissionRequest::GetElement(nsIDOMElement** aRequestingElement)
> +{

NS_ENSURE_ARG_POINTER(aRequestingElement)

::: dom/midi/MIDIAccessManager.cpp:135
(Diff revision 1)
> +}
> +
> +NS_IMETHODIMP
> +MIDIPermissionRequest::Cancel()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());

Well... all of this is main-thread only, right?
Remove this assertion.

::: dom/midi/MIDIAccessManager.cpp:143
(Diff revision 1)
> +}
> +
> +NS_IMETHODIMP
> +MIDIPermissionRequest::Allow(JS::HandleValue aChoices)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());

ditto.

::: dom/midi/MIDIAccessManager.cpp:153
(Diff revision 1)
> +}
> +
> +NS_IMETHODIMP
> +MIDIPermissionRequest::Run()
> +{
> +  if (!mWindow) {

This cannot really happen, right?
How can it be? mWindow has been received by the CTOR...

::: dom/midi/MIDIAccessManager.cpp:186
(Diff revision 1)
> +//-------------------------------------------------
> +// MIDI Access Manager
> +//-------------------------------------------------
> +
> +// Singleton object for MIDIAccessManager
> +StaticRefPtr<MIDIAccessManager> gMIDIAccessManager;

Move this to an anonymous namespace.

::: dom/midi/MIDIAccessManager.cpp:193
(Diff revision 1)
> +
> +MIDIAccessManager::MIDIAccessManager() :
> +  mHasPortList(false),
> +  mChild(nullptr)
> +{
> +  MOZ_COUNT_CTOR(MIDIAccessManager);

remove

::: dom/midi/MIDIAccessManager.cpp:198
(Diff revision 1)
> +  MOZ_COUNT_CTOR(MIDIAccessManager);
> +}
> +
> +MIDIAccessManager::~MIDIAccessManager()
> +{
> +  MOZ_COUNT_DTOR(MIDIAccessManager);

remove

::: dom/midi/MIDIAccessManager.cpp:216
(Diff revision 1)
> +
> +NS_IMETHODIMP
> +MIDIAccessManager::Observe(nsISupports* aSubject,
> +                           const char* aTopic,
> +                           const char16_t* aData)
> +{

MOZ_ASSERT(!strcmp(aTopic, ...))

::: dom/midi/MIDIAccessManager.cpp:218
(Diff revision 1)
> +MIDIAccessManager::Observe(nsISupports* aSubject,
> +                           const char* aTopic,
> +                           const char16_t* aData)
> +{
> +  nsCOMPtr<nsIObserverService> observerService =
> +    mozilla::services::GetObserverService();

This can be null.

if (observerService) {
  observerServcice->removeObserver(..

::: dom/midi/MIDIAccessManager.cpp:238
(Diff revision 1)
> +                                     ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(aWindow);
> +  RefPtr<Promise> p = Promise::Create(go, aRv);
> +  if (aRv.Failed()) {

NS_WARN_IF

::: dom/midi/MIDIAccessManager.cpp:247
(Diff revision 1)
> +  NS_DispatchToMainThread(permRunnable);
> +  return p.forget();
> +}
> +
> +void
> +MIDIAccessManager::AddObserver(Observer<MIDIPortList>* o)

o => aObserver

::: dom/midi/MIDIAccessManager.cpp:274
(Diff revision 1)
> +    }
> +  }
> +}
> +
> +void
> +MIDIAccessManager::RemoveObserver(Observer<MIDIPortList>* o)

o => aObserver

::: dom/midi/MIDIAccessManager.cpp:277
(Diff revision 1)
> +
> +void
> +MIDIAccessManager::RemoveObserver(Observer<MIDIPortList>* o)
> +{
> +  mChangeObservers.RemoveObserver(o);
> +  if (mChangeObservers.Length() == 0) {

Does it have IsEmpty() ?

::: dom/midi/MIDIAccessManager.cpp:279
(Diff revision 1)
> +MIDIAccessManager::RemoveObserver(Observer<MIDIPortList>* o)
> +{
> +  mChangeObservers.RemoveObserver(o);
> +  if (mChangeObservers.Length() == 0) {
> +    // If we're out of listeners, go ahead and shut down.
> +    gMIDIAccessManager = nullptr;

So, here you don't delete the actor.
But when a new MIDIAccessManager is created, the second one will create a new actor as well...

This seems a memory leak, plus a wrong IPC actor management. Am I wrong?

::: dom/midi/MIDIAccessManager.cpp:284
(Diff revision 1)
> +    gMIDIAccessManager = nullptr;
> +  }
> +}
> +
> +void
> +MIDIAccessManager::CreateMIDIAccess(nsPIDOMWindowInner* aWindow,

Promise* aPromise

::: dom/midi/MIDIAccessManager.cpp:286
(Diff revision 1)
> +}
> +
> +void
> +MIDIAccessManager::CreateMIDIAccess(nsPIDOMWindowInner* aWindow,
> +                                    bool aNeedsSysex,
> +                                    RefPtr<Promise> aPromise)

Promise* aPromise

::: dom/midi/MIDIAccessManager.cpp:287
(Diff revision 1)
> +
> +void
> +MIDIAccessManager::CreateMIDIAccess(nsPIDOMWindowInner* aWindow,
> +                                    bool aNeedsSysex,
> +                                    RefPtr<Promise> aPromise)
> +{

MOZ_ASSERT(aWindow)

::: dom/midi/MIDIAccessManager.cpp:333
(Diff revision 1)
> +  mChild = mgr;
> +}
> +
> +void
> +MIDIAccessManager::ActorFailed()
> +{

MOZ_CRASH(...)

::: dom/midi/MIDIAccessManager.cpp:334
(Diff revision 1)
> +}
> +
> +void
> +MIDIAccessManager::ActorFailed()
> +{
> +}

\n
} // dom namespace
} // mozilla namespace

::: dom/midi/MIDIInput.h:39
(Diff revision 1)
> +  // functions ourselves.
> +
> +  // Getter for the event handler callback
> +  EventHandlerNonNull* GetOnmidimessage();
> +  // Setter for the event handler callback
> +  void SetOnmidimessage(EventHandlerNonNull* aCallback);

IMPL_EVENT_HANDLER(midimessage)

::: dom/midi/MIDIInput.cpp:42
(Diff revision 1)
> +  nsCOMPtr<nsIDocument> doc = GetOwner()->GetDoc();
> +  if (!doc) {
> +    NS_WARNING("No document available to send MIDIMessageEvent to!");
> +    return;
> +  }
> +  for (auto& msg: aMsgs) {

<space>:<space>

::: dom/midi/MIDIInputMap.h:37
(Diff revision 1)
> +  }
> +
> +  explicit MIDIInputMap(nsPIDOMWindowInner* aParent);
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
> +private:
> +  virtual ~MIDIInputMap() {}

no virtual

::: dom/midi/MIDIManagerChild.cpp:25
(Diff revision 1)
> +bool
> +MIDIManagerChild::RecvMIDIPortListUpdate(const MIDIPortList& aPortList)
> +{
> +  // If the access manager isn't running, how are we even getting updates?
> +  MOZ_ASSERT(MIDIAccessManager::IsRunning());
> +  MIDIAccessManager::Get()->Update(aPortList);

In order to improve what your comment says about memory management of the actor,
What about if you pass the MiniAccessManager as a pointer to this child? Then here you just do: mManager->Update(aPortList);

I say this because ::Get() could create a new manager. But this is a bug that has been discussed in a previous comment.

::: dom/midi/MIDIMessageEvent.h:47
(Diff revision 1)
> +    Constructor(EventTarget* aOwner,
> +                const class TimeStamp& aReceivedTime,
> +                const nsTArray<uint8_t>& aData);
> +
> +  static already_AddRefed<MIDIMessageEvent>
> +    Constructor(const GlobalObject& aGlobal, const nsAString& aType,

ditto about the indentation.

::: dom/midi/MIDIMessageEvent.h:55
(Diff revision 1)
> +  // Getter for message data
> +  void GetData(JSContext* cx,
> +               JS::MutableHandle<JSObject*> aData,
> +               ErrorResult& aRv);
> +private:
> +  virtual ~MIDIMessageEvent();

no virtual

::: dom/midi/MIDIMessageEvent.cpp:71
(Diff revision 1)
> +
> +already_AddRefed<MIDIMessageEvent>
> +MIDIMessageEvent::Constructor(EventTarget* aOwner,
> +                              const class TimeStamp& aReceivedTime,
> +                              const nsTArray<uint8_t>& aData)
> +{

MOZ_ASSERT(aOwner)

::: dom/midi/MIDIMessageEvent.cpp:95
(Diff revision 1)
> +  // Set data for event. Timestamp will always be set to Now() (default for
> +  // event) using this constructor.
> +  const auto& a = aEventInitDict.mData.Value();
> +  a.ComputeLengthAndData();
> +  e->mData = Uint8Array::Create(aGlobal.Context(), owner, a.Length(), a.Data());
> +  if (!e->mData) {

NS_WARN_IF

::: dom/midi/MIDIMessageEvent.cpp:115
(Diff revision 1)
> +  if (!mData) {
> +    mData = Uint8Array::Create(cx,
> +                               this,
> +                               mRawData.Length(),
> +                               mRawData.Elements());
> +    if (!mData) {

NS_WARN_IF

::: dom/midi/MIDIMessageQueue.cpp:53
(Diff revision 1)
> +    if (aTimestamp < msg.timestamp()) {
> +      break;
> +    }
> +    aMsgQueue.AppendElement(msg);
> +    i++;
> +  }

if (i > 0) {
  ...

::: dom/midi/MIDIMessageQueue.cpp:83
(Diff revision 1)
> +  for (auto msg : mMessageQueue) {
> +    if (now < msg.timestamp()) {
> +      break;
> +    }
> +    i++;
> +  }

if (i > 0

::: dom/midi/MIDIOutput.cpp:51
(Diff revision 1)
> +  //
> +  // If timestamp is either not set or zero, set timestamp to now and send the
> +  // message ASAP.
> +  TimeStamp timestamp;
> +  if (aTimestamp.WasPassed() && aTimestamp.Value() != 0) {
> +    nsCOMPtr<nsIDocument> doc = GetOwner()->GetDoc();

This can be null. Throw an exception.

::: dom/midi/MIDIOutput.cpp:61
(Diff revision 1)
> +    timestamp = TimeStamp::Now();
> +  }
> +
> +  nsTArray<MIDIMessage> msgArray;
> +  MIDIUtils::ParseMessages(aData, timestamp, msgArray);
> +  if (msgArray.Length() == 0) {

IsEmpty()

::: dom/midi/MIDIOutputMap.h:39
(Diff revision 1)
> +    return mParent;
> +  }
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
> +private:
> +  virtual ~MIDIOutputMap() {}

remove virtual

::: dom/midi/MIDIOutputMap.cpp:34
(Diff revision 1)
> +MIDIOutputMap::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
> +{
> +  return MIDIOutputMapBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +}

// dom namespace
// mozilla namespace

::: dom/midi/MIDIPlatformRunnables.h:23
(Diff revision 1)
> +
> +/**
> + * Base class for runnables to be fired to the platform-specific MIDI service
> + * thread in PBackground.
> + */
> +class MIDIBackgroundRunnable : public mozilla::Runnable

remove 'mozilla::'

::: dom/midi/MIDIPlatformRunnables.h:28
(Diff revision 1)
> +class MIDIBackgroundRunnable : public mozilla::Runnable
> +{
> +public:
> +  MIDIBackgroundRunnable() {}
> +  ~MIDIBackgroundRunnable() {}
> +  NS_IMETHOD Run();

override

::: dom/moz.build:114
(Diff revision 1)
>      'resourcestats',
>      'manifest',
>      'vr',
>      'newapps',
>      'u2f',
> +    'midi'

why don't we have an alphabetic order here?

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

https://reviewboard.mozilla.org/r/54682/#review52440

So, I was going to review the lifetime today, but baku know more about IPDL and he's done it, so here are my first comments. It would be great if the items noted "TODO" would be addressed, it does not seems super hard.

The big problem we have is normalizing the message parser I think.

::: browser/components/nsBrowserGlue.js:2620
(Diff revision 1)
>  
>      this._showPrompt(aRequest, message, "geo", actions, "geolocation",
>                       "geo-notification-icon", options);
>    },
>  
> +  _promptMIDI : function(aRequest) {

We should get "someone who knows" review this. Poking around, MattN looks like an appropriate person.

::: browser/locales/en-US/chrome/browser/browser.properties:767
(Diff revision 1)
> +midi.alwaysShareDevices.accesskey = A
> +midi.neverShareDevices = Never share devices?
> +midi.neverShareDevices.accesskey = N
> +midi.shareDevices = Share devices?
> +midi.shareDevices.accesskey = Q
> +midi.shareDevicesWithSysex = Always share devices sysex?

"SysEx" vs. "sysex"? I vote for the former, but I don't really know if it's more correct. I just thinks it looks better.

::: dom/base/Navigator.cpp:2049
(Diff revision 1)
>    mVRGetDevicesPromises.Clear();
>  }
>  
> +already_AddRefed<Promise>
> +Navigator::RequestMIDIAccess(const MIDIOptions& aOptions,
> +                             ErrorResult& aRv) {

nit: { on its line.

::: dom/midi/MIDIAccess.cpp:173
(Diff revision 1)
> +    MOZ_CRASH("We shouldn't be here!");
> +  }
> +}
> +
> +// For the MIDIAccess object, only worry about new connections, where we create
> +// MIDIPort objects. When a port is removed and the TMIDIPortRemove event is

s/TMidiPortRemove/MidiPortRemove/

::: dom/midi/MIDIAccess.cpp:184
(Diff revision 1)
> +  for (auto& port : aEvent.ports()) {
> +    MaybeCreateMIDIPort(port);
> +  }
> +  if (mAccessPromise) {
> +    // TODO According to the spec we should return both this /and/ the
> +    // MIDIOptions object we started the request with.

I don't see that in the spec ?

::: dom/midi/MIDIAccessManager.h:62
(Diff revision 1)
> +  // True if manager singleton has been created
> +  static bool IsRunning();
> +  // Send device connection updates to all known MIDIAccess objects.
> +  void Update(const MIDIPortList& aEvent);
> +  // Adds a device update observer (usually a MIDIAccess object)
> +  void AddObserver(Observer<MIDIPortList>* o);

Nit: aArgument

::: dom/midi/MIDIAccessManager.h:83
(Diff revision 1)
> +  nsTArray<RefPtr<MIDIAccess>> mAccessHolder;
> +  // Device state update observers (usually MIDIAccess objects)
> +  ObserverList<MIDIPortList> mChangeObservers;
> +  // IPC Object for MIDIManager. Lifetime of MIDIAccessManager depends on
> +  // lifetime of MIDIManagerChild in parent process.
> +  MIDIManagerChild* mChild;

Sure we want a raw ptr here ?

::: dom/midi/MIDIAccessManager.cpp:159
(Diff revision 1)
> +    Cancel();
> +    return NS_OK;
> +  }
> +  // If the testing flag is true, skip dialog
> +  if (Preferences::GetBool("midi.prompt.testing", false)) {
> +    bool allow = Preferences::GetBool("midi.prompt.testing.allow", false);

We have `media.navigator.permission.disabled` for gUM, maybe we could reuse it ?

::: dom/midi/MIDIInput.cpp:19
(Diff revision 1)
> +namespace mozilla {
> +namespace dom {
> +
> +MIDIInput::MIDIInput(nsPIDOMWindowInner* aWindow, MIDIAccess* aMIDIAccessParent,
> +                     const MIDIPortInfo& aPortInfo, const bool aSysexEnabled) :
> +  MIDIPort(aWindow, aMIDIAccessParent, aPortInfo, aSysexEnabled)

nit: colon on the same line as the ctor init list:

> : MidiPort

::: dom/midi/MIDIMessageQueue.cpp:13
(Diff revision 1)
> +
> +namespace mozilla {
> +namespace dom {
> +
> +MIDIMessageQueue::MIDIMessageQueue() :
> +  mMutex("MIDIMessageQueue::mMutex")

Nit, : on the same line.

::: dom/midi/MIDIOutput.cpp:61
(Diff revision 1)
> +    timestamp = TimeStamp::Now();
> +  }
> +
> +  nsTArray<MIDIMessage> msgArray;
> +  MIDIUtils::ParseMessages(aData, timestamp, msgArray);
> +  if (msgArray.Length() == 0) {

What if the sequence contained a valid message and an invalid message ? What is the behaviour expected ?

Reading the spec:

> The data contains one or more valid, complete 
> MIDI messages. Running status is not allowed in
> the data, as underlying systems may not support
> it.
> 
> If data is not a valid sequence or does not
> contain a valid MIDI message, throw a TypeError
> exception. 

"a valid sequence" is not defined, it could be defined as "a sequence that contains at least a valid MIDI message" or "a sequence that parses every message correctly as a valid MIDI message".

This needs to be agreed upon and specced.

::: dom/midi/MIDIPlatformRunnables.h:118
(Diff revision 1)
> +    mConnection(aConnection)
> +  {}
> +  ~SetStatusRunnable() {}
> +  virtual void RunInternal() override;
> +private:
> +  // This shouldn't die while we're waiting to run.

Raw pointers and threads make me nervous. Can you RefPtr everything ? Or use some utility or ways to enforce life time ?

::: dom/midi/MIDIPlatformService.h:132
(Diff revision 1)
> +  // it is populated. This is set to True when that is done, so we don't
> +  // constantly spam MIDIManagers with port lists.
> +  bool mHasSentPortList;
> +
> +  // Array of MIDIManager IPC objects.
> +  nsTArray<PMIDIManagerParent*> mManagers;

What is the life time of this ?

::: dom/midi/MIDIPlatformService.h:146
(Diff revision 1)
> +  nsTArray<MIDIPortParent*> mPorts;
> +
> +  // Per-port message queue hashtable. Handles scheduling messages for sending.
> +  // Protected instead of private so that it can be access as needed by platform
> +  // specific libraries, but requires lock.
> +  nsClassHashtable<nsStringHashKey, MIDIMessageQueue> mMessageQueues;

Make a private getter for this that asserts that the lock is taken.

::: dom/midi/MIDIPlatformService.cpp:208
(Diff revision 1)
> +    bool useTestService = false;
> +    //rv = Preferences::GetRootBranch()->GetBoolPref("midi.testing", &useTestService);
> +    if (useTestService) {
> +      gMIDIPlatformService = new TestMIDIPlatformService();
> +    } else {
> +    }

This looks weird.

::: dom/midi/MIDIPlatformService.cpp:223
(Diff revision 1)
> +  if (!IsRunning()) {
> +    // Service already stopped or never started. Exit.
> +    return;
> +  }
> +  // If we have any ports or managers left, we should still be alive.
> +  if (mPorts.Length() != 0 ||

IsEmpty()

::: dom/midi/MIDIPort.h:78
(Diff revision 1)
> +
> +  virtual void Receive(const nsTArray<MIDIMessage>& aMsg);
> +
> +  // Removes self from parent MIDIAccess object, and destroys IPC actor.
> +  void Shutdown();
> +  // This object holds a point to its corresponding IPC MIDIPortChild actor. If

s/a point/a pointer/

::: dom/midi/MIDIPort.h:90
(Diff revision 1)
> +  MIDIPortChild* mPort;
> +private:
> +  // MIDIAccess object that created this MIDIPort object. There is a chance the
> +  // MIDIPort object can outlive the MIDIAccess object, so this is a weak
> +  // reference that must be handled properly.
> +  MIDIAccess* mMIDIAccessParent;

Document the life time of this: where is it set, where is it cleared, etc.

::: dom/midi/MIDIPort.cpp:191
(Diff revision 1)
> +  return p.forget();
> +}
> +
> +void
> +MIDIPort::Notify(const bool& aParentDead)
> +{

Is the parameter useful ? If not, put it in between /* */.

::: dom/midi/MIDIPortChild.cpp:60
(Diff revision 1)
> +
> +bool
> +MIDIPortChild::RecvUpdateStatus(const uint32_t& aDeviceState, const uint32_t& aConnectionState)
> +{
> +  mDeviceState = static_cast<MIDIPortDeviceState>(aDeviceState);
> +  mConnectionState = static_cast<MIDIPortConnectionState>(aConnectionState);

Are all transitions from all states legal here ? If not, can you MOZ_ASSERT that the transition is legal ?

::: dom/midi/MIDIUtils.cpp:19
(Diff revision 1)
> +static const uint8_t kSystemRealtimeMessage = 0xF8;
> +// Represents the length of all possible command messages.
> +static const uint8_t kCommandLengths[] = {3, 3, 3, 3, 2, 2, 3};
> +// Represents the length of all possible system messages. The length of sysex
> +// messages is variable, so we just put zero since it won't be checked anyways.
> +static const uint8_t kSystemLengths[] = {0, 2, 3, 2, 1, 1, 1, 1};

This looks sensible, but we really need some serious normative reference and normalize this parser.

I've filed https://github.com/WebAudio/web-midi-api/issues/164 for this

::: dom/midi/linux/LinuxMIDIService.cpp:13
(Diff revision 1)
> +#include <alsa/asoundlib.h>
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class LinuxMIDIService {

I was thinking that maybe there would be some value in having an little independant library that implement a common interface that we could use in Gecko ? We could have tests for it and all that. We do that for audio input and output, and it's very nice.

Maybe something like this already exists with a license that works for us ?

::: dom/midi/linux/LinuxMIDIService.cpp:52
(Diff revision 1)
> +  if (err != 0) {
> +    NS_WARNING("MIDI Output open failed!");
> +    return false;
> +  }
> +  //int out_client_id_ = snd_seq_client_id(mOut);
> +  NS_WARNING("YAAAAAAAAAAAAAAY");

YAAAAAAY !

::: dom/midi/moz.build:74
(Diff revision 1)
> +#         'linux/LinuxMIDIService.cpp'
> +#     ]
> +# elif CONFIG['MOZ_GAMEPAD_BACKEND'] == 'android':
> +#     UNIFIED_SOURCES += [
> +#         'android/AndroidGamepad.cpp'
> +#     ]

Remember to remove this.

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8755611 [details]
Bug 1201590 - WebMIDI Mochitests

https://reviewboard.mozilla.org/r/54684/#review52896

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yeah, chunking this into smaller reivews due to the size of the patch is fine with me, I'll start taking a look at these tomorrow, thanks!

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

> I was thinking that maybe there would be some value in having an little
> independant library that implement a common interface that we could use in
> Gecko ? We could have tests for it and all that. We do that for audio input
> and output, and it's very nice.
> 
> Maybe something like this already exists with a license that works for us ?

There's rtmidi:

https://github.com/thestk/rtmidi

MIT licensed, and but I've only used it via python. Possibly worth a look though. Pinging agoode to see if they evaluated it for chromium

Comment 14

[Adam Goode]

I started working on Chromium Web MIDI after a few backends were implemented, but from personal experience I found that implementing the Web MIDI device and event model is a lot of work and rtmidi doesn't help you there (it doesn't support hotplug events).

One useful idea might be first to create libwebmidi (a standalone C or C++ library), either by extracting from chromium or writing from scratch. This would help with using Web MIDI from Node and other things. You could imagine extending rtmidi to implement the Web MIDI event and properties model.

In general, Web MIDI is a good API (it takes good inspiration from existing OS implementations) and I could see benefits to applications using it in non-web applications.

Comment 15

[Paul Adenot (:padenot)]

What Adam said, it would be a good idea, and I reiterate that having separate libraries for leaf modules is often a good idea !

Comment 16

[Paul Adenot (:padenot)]

Comment on attachment 8755611 [details]
Bug 1201590 - WebMIDI Mochitests

https://reviewboard.mozilla.org/r/54684/#review53152

::: dom/midi/test/mochitest/MIDITestUtils.js:34
(Diff revision 1)
> +  alwaysClosedTestOutputInfo: {
> +    id: "f87d0c76-3c68-49a9-a44f-700f1125c07a",
> +    name: "Always Closed MIDI Device Output Port",
> +    manufacturer: "Test Manufacturer",
> +    version: "1.0.0"
> +  },

Put a comment in both the C++ and here saying that those lists should be synchronized.

::: dom/midi/test/mochitest/mochitest.ini:15
(Diff revision 1)
> +[test_midi_device_explicit_open_close.html]
> +[test_midi_device_connect_disconnect.html]
> +[test_midi_device_pending.html]
> +[test_midi_device_sysex.html]
> +[test_midi_device_system_rt.html]
> +[test_midi_packet_timing_sorting.html]

I feel bad asking about this because it's all already written, but if you have tests that are not too gecko specific, you could make them be web-platform-tests.

I don't know how feasible that is, considering we have this nice fake midi backend thing.

Comment 17

[Paul Adenot (:padenot)]

(In reply to Paul Adenot (:padenot) from comment #15)
> What Adam said, it would be a good idea, and I reiterate that having
> separate libraries for leaf modules is often a good idea !

Also, it would be 100% more awesome to write it in Rust.

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Paul Adenot (:padenot) from comment #16)
> ::: dom/midi/test/mochitest/mochitest.ini:15
> (Diff revision 1)
> > +[test_midi_device_explicit_open_close.html]
> > +[test_midi_device_connect_disconnect.html]
> > +[test_midi_device_pending.html]
> > +[test_midi_device_sysex.html]
> > +[test_midi_device_system_rt.html]
> > +[test_midi_packet_timing_sorting.html]
> 
> I feel bad asking about this because it's all already written, but if you
> have tests that are not too gecko specific, you could make them be
> web-platform-tests.
> 
> I don't know how feasible that is, considering we have this nice fake midi
> backend thing.

Some of the chromium people are also doing w-p-t's. I'll check with them to see the status on it and maybe start moving them that direction.

Comment 19

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Paul Adenot (:padenot) from comment #17)
> (In reply to Paul Adenot (:padenot) from comment #15)
> > What Adam said, it would be a good idea, and I reiterate that having
> > separate libraries for leaf modules is often a good idea !
> 
> Also, it would be 100% more awesome to write it in Rust.

I'd actually talked to a couple of people about writing our platform specific MIDI libraries in Rust instead of C++, since we can probably just make all of the bindings C anyways. I'd definitely be interested in doing this as I've been working on picking up Rust lately anyways. 

Seems like Rust would kind of hinder the "Oh then Node could use it too!" thing, but I guess parallel implementations could happen.

Comment 20

[Adam Goode]

Hah! Rust would be a really fun thing to write this in. As long as you can create reasonable C bindings, I don't see why Node would have trouble.

Comment 21

[Paul Adenot (:padenot)]

Yes node and rust can work together just fine, along with a number of other script language. There are even libraries to make it easier.

Comment 22

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ah, ok, wasn't considering the C FFI when I said that, heh. So I'm totally on board with a webmidi rust library. Working on contacting the RustAudio team to see if they've got any interest in helping out or know of any related projects too.

Comment 23

[Adam Goode]

Let us know when there is a Mozilla Rust libwebmidi project people can start contributing to :)

Comment 24

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Just a heads up, I have the review comments mostly addressed, but I'm having to refactor some stuff in order to plan ahead for Rust-based platform specific backends. That plus working on other projects that are completely outside of the context of this (private browsing IDB, etc...) are why things are stalled a bit right now. 

In terms of the Rust stuff: the MIDIPlatformService class was to be a base class that the platform specific classes would derive from, and now needs to stand by itself while calling out into whatever the C bindings for Rust will be. This won't be 100% by the next review round, but I'm trying to get it as close as possible now to avoid more churn later.

Comment 25

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Now have can call into rust from gecko, and vice versa, compiling on all platforms. (https://treeherder.mozilla.org/#/jobs?repo=try&revision=e40b706ea2a8f4414907e2b1dbce00d4c2353b63) and working on my local machine. Will be overhauling the TestMIDIPlatform to be in rust, then submitting for review round 2.

Comment 26

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Note to self: Permissions UI got rewritten in but 1297475. We need to update our permissions prompt code to live in browser/modules/PermissionUI.jsm

Comment 27

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1201590 - WebMIDI IPDL files;

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

Comment 28

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

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

Comment 29

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755611 [details]
Bug 1201590 - WebMIDI Mochitests

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

Comment 30

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I've just submitted patches with everything I'd gotten fixed from the first review set, plus a new extra patch that shows the rather hacky start of writing the platform specific services in Rust, starting with rewriting the TestPlatformService.

My github repo for work on this is at https://github.com/qdot/gecko-hg/tree/1201590-webmidi-dom

There's not really much in the way of comments in the Rust code yet because I've just been trying to throw the idea together when I've had time, so other people can start helping out. Basic gist is that rust will manage threads that run the platform specific functions for MIDI. Going from Gecko -> Rust will happen via Rust C-extern'd functions. Rust will keep channels to the various threads it needs, and forward data over those. Going from Rust Platform Threads -> Gecko C++ happens via a set of C extern'd runnable creation functions in Gecko. These are exposed in dom/MIDI/MIDIPlatformRunnables.[h|cpp]. The thread local MANAGER variable in the rust library just works as a global compilation unit singleton would in Gecko.

What's left to be done:

- Currently, *PlatformService (Mac/Test are the only ones done, and Mac is crufty. See bug 1201593.) are children of the MIDIPlatformService class. We can't do this across languages, so we'll need to clean up the MIDIPlatformService class to be a standalone class that just communicates with the Rust library via the provided functions. We'll still need MIDIPlatformService around to manage the MIDIPortParent/MIDIManagerParent objects, if nothing else.
- All of the platform specific stuff needs to be implemented in Rust. There's already udev rust bindings for linux (I'm using these for U2F device detection so I have some experience with them), as well as Cocoa and CoreAudio rust bindings. I'm not sure about the status of Win32 functions we'll need in winapi-rs yet, so that's an open question. I've worked with platform specific bindings across all the platforms we're planning on supporting now, if someone wants to start on any of those and has questions, just ni? me.
- There's still some bug work that needed to happen, mainly related to the MIDI packet parser. I tried to mark everything I'd finished in reviewboard, so the bug lists there should be up to date.
- There's still some open questions around how we're going to move Vec<u8>'s or whatever back and forth between C++/Rust, since we don't have a nice TArray translator yet (see bug 1302213).

I'm gonna try to continue helping out where I can, as a lot of the work I'm having to do for U2F will overlap with this. However, I certainly don't want this blocked waiting for prioritization that I don't think is going to happen any time soon.

Comment 31

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Kyle Machulis [:qdot] from comment #26)
> Note to self: Permissions UI got rewritten in bug 1297475. We need to update
> our permissions prompt code to live in browser/modules/PermissionUI.jsm

Also: I went ahead and fixed this, moving the UI to PermissionUI.jsm. Seems to work, but could probably use more testing.

Comment 32

[Andrea Marchesini [:baku]]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

https://reviewboard.mozilla.org/r/54682/#review89166

do you realize that this is a lot of code to review? :)
Do you think it's possible to split it in smaller logically-independent patches?
I need to review this is several steps, but in the meantime, can you fix the IPC issue and the other little nits?

::: dom/midi/MIDIAccess.h:61
(Diff revision 2)
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(MIDIAccess,
> +                                                         DOMEventTargetHelper)
> +public:
> +  nsPIDOMWindowInner*
> +  GetParentObject() const

you don't need this if the class is DOMEventTargetHelper

::: dom/midi/MIDIAccess.cpp:104
(Diff revision 2)
> +  ErrorResult rv;
> +  if (aPort->State() == MIDIPortDeviceState::Disconnected) {
> +    if (aPort->Type() == MIDIPortType::Input &&
> +        MIDIInputMapBinding::MaplikeHelpers::Has(mInputMap, id, rv)) {
> +      MIDIInputMapBinding::MaplikeHelpers::Delete(mInputMap, id, rv);
> +      if(NS_WARN_IF(rv.Failed())) {

you can remove this and make the following if() stmt to manage the errorResult.

::: dom/midi/MIDIAccess.cpp:110
(Diff revision 2)
> +        return;
> +      }
> +    } else if (aPort->Type() == MIDIPortType::Output &&
> +               MIDIOutputMapBinding::MaplikeHelpers::Has(mOutputMap, id, rv)) {
> +      MIDIOutputMapBinding::MaplikeHelpers::Delete(mOutputMap, id, rv);
> +      if(NS_WARN_IF(rv.Failed())) {

same here.

::: dom/midi/MIDIAccess.cpp:115
(Diff revision 2)
> +      if(NS_WARN_IF(rv.Failed())) {
> +        return;
> +      }
> +    }
> +    // Check to make sure the Has() calls haven't failed.
> +    if(NS_WARN_IF(rv.Failed())) {

this works also for delete.

::: dom/midi/MIDIAccess.cpp:157
(Diff revision 2)
> +}
> +
> +void
> +MIDIAccess::MaybeCreateMIDIPort(const MIDIPortInfo& aInfo, ErrorResult& aRv)
> +{
> +  nsString id = aInfo.id();

nsAutoString

::: dom/midi/MIDIAccess.cpp:209
(Diff revision 2)
> +// For the MIDIAccess object, only worry about new connections, where we create
> +// MIDIPort objects. When a port is removed and the MIDIPortRemove event is
> +// received, that will be handled by the MIDIPort object itself, and it will
> +// request removal from MIDIAccess's maps.
> +void
> +MIDIAccess::Notify(const MIDIPortList &aEvent)

& next to MIDIPortList

::: dom/midi/MIDIAccessManager.cpp:219
(Diff revision 2)
> +{
> +  MOZ_ASSERT(!strcmp(aTopic, "xpcom-shutdown"));
> +  nsCOMPtr<nsIObserverService> observerService =
> +    mozilla::services::GetObserverService();
> +  if (observerService) {
> +    observerService->RemoveObserver(this, NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID);

this observer doesn't sound very useful.

I guess you want to clean something here, and delete the observers.

::: dom/midi/MIDIAccessManager.cpp:236
(Diff revision 2)
> +already_AddRefed<Promise>
> +MIDIAccessManager::RequestMIDIAccess(nsPIDOMWindowInner* aWindow,
> +                                     const MIDIOptions& aOptions,
> +                                     ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());

MOZ_ASSERT(aWindow);

::: dom/midi/MIDIAccessManager.cpp:247
(Diff revision 2)
> +  nsCOMPtr<nsIDocument> doc = aWindow->GetDoc();
> +  if (!doc) {
> +    return nullptr;
> +  }
> +  nsCOMPtr<nsIRunnable> permRunnable = new MIDIPermissionRequest(aWindow, p, aOptions);
> +  NS_DispatchToMainThread(permRunnable);

In theory, this can fail.

::: dom/midi/MIDIAccessManager.cpp:286
(Diff revision 2)
> +  mChangeObservers.RemoveObserver(aObserver);
> +  if (mChangeObservers.Length() == 0) {
> +    // If we're out of listeners, go ahead and shut down. Make sure to cleanup
> +    // the IPDL protocol also.
> +    if (mChild) {
> +      mChild->Send__delete__(mChild);

What about if the parent is sending a receive() message at the sametime? We should avoid this inverting the order of the message:

child->SendClose()
parent::RecvClose() { ... child->Send__delete__

::: dom/midi/MIDIInputMap.h:20
(Diff revision 2)
> +namespace dom {
> +
> +/**
> + * Maplike DOM object that holds a list of all MIDI input ports available for
> + * access. Almost all functions are implemented automatically by WebIDL.
> + *

extra *\n

::: dom/midi/MIDIInputMap.cpp:34
(Diff revision 2)
> +MIDIInputMap::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
> +{
> +  return MIDIInputMapBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +}

// dom namesapace
// mozilla namespace

::: dom/midi/MIDIManagerChild.cpp:13
(Diff revision 2)
> +#include "mozilla/dom/MIDIAccessManager.h"
> +
> +using namespace mozilla::dom;
> +
> +MIDIManagerChild::MIDIManagerChild() :
> +  mActorWasAlive(false)

I don't see why having this boolean

::: dom/midi/MIDIManagerChild.cpp:23
(Diff revision 2)
> +{
> +}
> +
> +bool
> +MIDIManagerChild::RecvMIDIPortListUpdate(const MIDIPortList& aPortList)
> +{

MOZ_ASSERT(NS_IsMainThread())

::: dom/midi/MIDIManagerChild.cpp:37
(Diff revision 2)
> +void
> +MIDIManagerChild::SetActorAlive()
> +{
> +  MOZ_ASSERT(!mActorWasAlive);
> +  mActorWasAlive = true;
> +  AddRef();

Why do you need this?

The manager keeps this object alive. Right?

::: dom/midi/MIDIMessageEvent.h:56
(Diff revision 2)
> +  void GetData(JSContext* cx,
> +               JS::MutableHandle<JSObject*> aData,
> +               ErrorResult& aRv);
> +private:
> +  ~MIDIMessageEvent();
> +  nsTArray<uint8_t> mRawData;

Check if this event can be generated code in dom/webidl/moz.build

::: dom/midi/MIDIMessageQueue.cpp:21
(Diff revision 2)
> +
> +MIDIMessageQueue::~MIDIMessageQueue()
> +{
> +}
> +
> +class MIDIMessageTimestampComparator {

{
in a new line

::: dom/midi/MIDIPlatformService.h:29
(Diff revision 2)
> + * Base class for platform specific MIDI implementations. Handles aggregation of
> + * IPC service objects, as well as sending/receiving updates about port
> + * connection events and messages.
> + *
> + */
> +class MIDIPlatformService {

{ in a new line

::: dom/midi/MIDIPlatformService.cpp:232
(Diff revision 2)
> +  Stop();
> +  gMIDIPlatformService = nullptr;
> +}
> +
> +void
> +MIDIPlatformService::AddManager(MIDIManagerParent* p)

give it a proper name. also in other methods.

::: dom/midi/MIDITypes.ipdlh:30
(Diff revision 2)
> +
> +struct MIDIPortList {
> +    MIDIPortInfo[] ports;
> +};
> +
> +}

comments :)

::: dom/midi/MIDIUtils.h:33
(Diff revision 2)
> +                       const TimeStamp& aTimestamp,
> +                       nsTArray<MIDIMessage>& aMsgArray);
> +// Returns true if a message is a sysex message.
> +bool IsSysexMessage(const MIDIMessage& a);
> +}
> +}

comment please // dom // mozilla

::: dom/midi/MIDIUtils.cpp:134
(Diff revision 2)
> +  if (aMsg.data()[0] == kSysexMessageStart) {
> +    return true;
> +  }
> +  return false;
> +}
> +}

Add some comment in this list if '}'

::: dom/webidl/MIDIPort.webidl:6
(Diff revision 2)
> +/* -*- 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/.
> + */
> +

Can you add the URL of the spec here?
and in any other .webidl file?

::: modules/libpref/init/all.js:209
(Diff revision 2)
>  // even during composition (keypress events are never fired during composition
>  // even if this is true).
>  pref("dom.keyboardevent.dispatch_during_composition", false);
>  
> +// Whether the WebMIDI API is enabled
> +pref("dom.webmidi.enabled", true);

Are we enabling it by default immediately?

Comment 33

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Andrea Marchesini [:baku] from comment #32)
> Comment on attachment 8755610 [details]
> Bug 1201590 - WebMIDI DOM Implementation
> 
> https://reviewboard.mozilla.org/r/54682/#review89166
> 
> do you realize that this is a lot of code to review? :)
> Do you think it's possible to split it in smaller logically-independent
> patches?
> I need to review this is several steps, but in the meantime, can you fix the
> IPC issue and the other little nits?
> 

Oops. This wasn't supposed to be in review, I was just updating the patch for people that had mentioned they wanted to help out too. That said, I'll try to get this stuff fixed up, and divide things down into more patches.

Comment 34

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

https://reviewboard.mozilla.org/r/54682/#review52440

> We should get "someone who knows" review this. Poking around, MattN looks like an appropriate person.

Going to have :florian look at this on the next round.

> I don't see that in the spec ?

Yeah, that's changed since I first implemented this. Removed.

> Sure we want a raw ptr here ?

It's somewhat common to let IPDL handle raw pointers since the lifetime is managed by the protocols anyways. That said, changed this to a RefPtr just to be safe.

> Is the parameter useful ? If not, put it in between /* */.

Changed to a void_t. Can't be commented out due to the way mfbt Observer/ObserverList works, but void_t is about the cleanest way to do it.

> I was thinking that maybe there would be some value in having an little independant library that implement a common interface that we could use in Gecko ? We could have tests for it and all that. We do that for audio input and output, and it's very nice.
> 
> Maybe something like this already exists with a license that works for us ?

Moving toward combined Rust library as discussed in further comments.

> YAAAAAAY !

Yeah this shouldn't of even made it into the review in the first place. That was an early test class I forgot to remove. :)

Comment 35

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

https://reviewboard.mozilla.org/r/54682/#review52852

Yup, that's fine.

> I ignore all the browser/* changes. Ask somebody else to review this code, I'm not a peer for it.

Going to have :florian look at this on the next round.

> Add a comment about this 'true'

Moved this to actually be a Observer<void_t>. Makes life easier.

> You must call:  rv.SuppressException() somewhere.

Cleaned up the ErrorResult handling, so I think this should be ok now?

> maybe it a boolean return value to stop the notification loop. Read Notify() comments.

Went with passing ErrorResult from Notify() and checking that.

> if (NS_WARN_IF(rv.Failed())) {
>   mAccessPromise->MaybeReject(rv);
>   mAccessPromise = nullptr;
>   return false;
> }

Promise rejection happens in Notify, since that's the only place this is called from anyways.

> If something goes wrong in MaybeCreateDOMMIDIPort() we want to reject the promise, right?
> 
> So what about if we do:
> 
> // Something went wrong...
> if (!MaybeCreateMIDIPort(port)) {
>   return;
> }

Handled using the ErrorResult value.

> follow up?

Spec changed since I wrote this, what it does now is fine.

> Do we want to assert that this aObs is not already contained in mDestructionObservers ?

ObserverList doesn't really give us a nice way to do that, and if it's not in the ObserverList it's basically a no-op, so I don't think it's a big deal?

> So... this is bad, right?
> What about if you move this check when MIDIPermissionRequest is created?
> 
> Or you implement a ::Init(ErrorResult& aRv) where you do this check and in case, aRv.Throw(NS_ERROR_FAILURE);

Now assert on validity of aWindow->GetDoc(), and also check and return early at MIDIPermissionRequest creation.

> Does it have IsEmpty() ?

Nope, just length. Worth adding IsEmpty?

> So, here you don't delete the actor.
> But when a new MIDIAccessManager is created, the second one will create a new actor as well...
> 
> This seems a memory leak, plus a wrong IPC actor management. Am I wrong?

You're right. It would leak until the channel was closed by the super-protocol.

> IMPL_EVENT_HANDLER(midimessage)

See comment above this about explicitly implementing what IMPL_EVENT_HANDLER does since we need extra operations in it.

> In order to improve what your comment says about memory management of the actor,
> What about if you pass the MiniAccessManager as a pointer to this child? Then here you just do: mManager->Update(aPortList);
> 
> I say this because ::Get() could create a new manager. But this is a bug that has been discussed in a previous comment.

I can't pass a pointer here because this is part of an IPC Send/Recv function, so it's not coming from the right place to pass the MIDIAccessManager pointer. However, I can check MIDIAccessManager::IsRunning() and return false instead of just asserting, which will cause an IPC error, which is what we want. If we're receiving a port list without a live MIDIAccessManager, something has gone very wrong anyways.

> why don't we have an alphabetic order here?

Neither INTERFACES nor DIRS requires alphabetical order. I guess I can fix it for this as it's like, half sorted?

Comment 36

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

https://reviewboard.mozilla.org/r/54682/#review89166

> Check if this event can be generated code in dom/webidl/moz.build

It can't. Setting the Uint8Array can throw in the initializer, and we can't generate events that throw.

Comment 37

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

https://reviewboard.mozilla.org/r/54682/#review89166

> this observer doesn't sound very useful.
> 
> I guess you want to clean something here, and delete the observers.

Yeah this was a holdover from when I was having some problem with ClearOnShutdown. Removed.

> What about if the parent is sending a receive() message at the sametime? We should avoid this inverting the order of the message:
> 
> child->SendClose()
> parent::RecvClose() { ... child->Send__delete__

Ok, unfortunately there's been a year between this comment being made and me addressing it, so I may be missing something. mChild here is a MIDIManagerChild, not a MIDIPortChild, so even if the parent is calling Receive, that has no bearing on this. MIDIPort objects are separate, and can outlive the MIDIAccess objects that made them.

Comment 38

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1201590 - WebMIDI Utility classes

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

Comment 39

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1201590 - WebMIDI Content implementation

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

Comment 40

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1201590 - WebMIDI Parent/PBackground implementation

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

Comment 41

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1201590 - WebMIDI device access permissions prompt implementation

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

Comment 42

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

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

Comment 43

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8805211 [details]
Bug 1201590 - WebMIDI IPDL files;

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

Comment 44

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755611 [details]
Bug 1201590 - WebMIDI Mochitests

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

Comment 45

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

One year later, patch updates. \o/

First off, I've moved this from being 1 patch to 7. There was way too much code for one patch in the first round of this, and a lot of this, like the WebIDL and IPDL files, will rarely if ever change, so this should make review iteration easier on the big child/parent implementation parts. I haven't made this so that each patch compiles standalone, and some of the moz.build changes haven't been redistributed between patches yet, but with the whole set applied, everything does build and at least starts up.

That said, test fails catastrophically, so I'm not putting in for new review requests until those are fixed up and passing on try.

The goal is to get this reviewed and landed soon, along with a minimal OS X layer, also in C++ (see bug 1201593). Everything will be pref'd off on first landing, but this will at least get us into the code base. After that, I'm hoping to convert the test service to rust, then do platform implementations in rust also. Haven't had a chance to research MIDI in the rust ecosystem lately, but if there's a crate out that does what we need, happy to discuss with authors and pull that in.

We also most likely won't be spec feature complete on first landing. Device identification and ID reuse per domain will definitely be a followup, and I need to go through the FAQ, spec repo issues, and current chrome implementation status to see what other followups may need to happen too.

Comment 46

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Back from PTO. Went through and checked tests, most succeed when run separately, but fail when run with other tests, meaning we've got some manager cleanup issues to handle. Going to be working those out then trying to get another review round started.

Comment 47

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

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

Comment 48

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8805211 [details]
Bug 1201590 - WebMIDI IPDL files;

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

Comment 49

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929646 [details]
Bug 1201590 - WebMIDI Utility classes

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

Comment 50

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929647 [details]
Bug 1201590 - WebMIDI Content implementation

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

Comment 51

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929648 [details]
Bug 1201590 - WebMIDI Parent/PBackground implementation

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

Comment 52

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

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

Comment 53

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755611 [details]
Bug 1201590 - WebMIDI Mochitests

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

Comment 54

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Patches are back in for review. I covered the review comments from a year+ ago, and rewrote the tests to mostly use async/await instead of counters and god knows what else. My js skills have improved significantly since I first wrote these. :|

For some reason reviewboard keeps clearing my r? for :baku on the WebIDL patch. :baku, if you could do that review, that'd be great. It should mirror the current state of the github WebMIDI spec version.

Finally, while I'd like reviews on these, they're still stuck on try. They'll pass as a single test pass, but fail in either test-verify, or locally while running with verify counts long enough to have a gc/cc pass trigger. Something is trying to send a PMIDIPort::UpdateStatus from parent -> child after the IPC channel dies, and it's only happening during CC. Still tracking that down.

Comment 55

[Andrea Marchesini [:baku]]

Comment on attachment 8929646 [details]
Bug 1201590 - WebMIDI Utility classes

https://reviewboard.mozilla.org/r/200924/#review211002

::: dom/midi/MIDIPortInterface.h:25
(Diff revision 2)
> + */
> +class MIDIPortInterface
> +{
> +public:
> +  MIDIPortInterface(const MIDIPortInfo& aPortInfo, bool aSysexEnabled);
> +  nsString Id() const { return mId; }

const nsString& as return value? Here and elsewhere.

::: dom/midi/MIDIPortInterface.h:34
(Diff revision 2)
> +  bool SysexEnabled() const { return mSysexEnabled; }
> +  MIDIPortType Type() const { return mType; }
> +  MIDIPortDeviceState DeviceState() const { return mDeviceState; }
> +  MIDIPortConnectionState ConnectionState() const { return mConnectionState; }
> +  virtual void Shutdown();
> +  bool IsShutdown() { return mShuttingDown; }

bool IsShutdown() const { ...

::: dom/midi/MIDIUtils.cpp:29
(Diff revision 2)
> +
> +// Checks validity of MIDIMessage passed to it. Throws debug warnings and
> +// returns false if message is not valid.
> +bool
> +IsValidMessage(const MIDIMessage* aMsg)
> +{

if (NS_WARN_IF(!aMsg)) {
  return false;
}

::: dom/midi/MIDIUtils.cpp:117
(Diff revision 2)
> +uint32_t
> +ParseMessages(const uint8_t* aByteBuffer,
> +              const size_t aLength,
> +              const TimeStamp& aTimestamp,
> +              nsTArray<MIDIMessage>& aMsgArray)
> +{

check if aByteBuffer is not null.

::: modules/libpref/init/all.js:204
(Diff revision 2)
>  // even during composition (keypress events are never fired during composition
>  // even if this is true).
>  pref("dom.keyboardevent.dispatch_during_composition", false);
>  
> +// Whether the WebMIDI API is enabled
> +pref("dom.webmidi.enabled", false);

do we want to enable it in nightly only?

Comment 56

[Andrea Marchesini [:baku]]

Comment on attachment 8805211 [details]
Bug 1201590 - WebMIDI IPDL files;

https://reviewboard.mozilla.org/r/88986/#review211000

::: dom/midi/PMIDIManager.ipdl:20
(Diff revision 3)
> +  async __delete__();
> +child:
> +  /*
> +   * Send an updated list of MIDI ports to the child
> +   */
> +  async MIDIPortListUpdate(MIDIPortList aPortList);

What about if the parent wants to send an update and the child calls __delete__ ? Better to make this logic stronger.

I prefer to add a: Close() method running on the parent side. The parent calls __delete__ when this is received. In this way we are sure that the parent doesn't send update after a Close().

::: dom/midi/PMIDIPort.ipdl:26
(Diff revision 3)
> +  async Clear();
> +child:
> +  async Receive(MIDIMessage[] msg);
> +  // Actually takes a MIDIDeviceConnectionState and MIDIPortConnectionState
> +  // respectively.
> +  async UpdateStatus(uint32_t deviceState, uint32_t connectionState);

same here. Maybe add a Shutdown() method on the parent side. The parent sends a __delete__ after.

::: ipc/glue/PBackground.ipdl:136
(Diff revision 3)
>  
>    async PQuota();
>  
>    async PFileSystemRequest(FileSystemParams params);
>  
> +

no extra lines.

Comment 57

[Florian Quèze [:florian]]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

https://reviewboard.mozilla.org/r/200930/#review211006

I added some simple comments but didn't do a full review, I would like to redirect this request to :johannh

::: browser/locales/en-US/chrome/browser/browser.properties:930
(Diff revision 2)
> +midi.shareDevices.accesskey = Q
> +midi.shareDevicesWithSysex = Always share devices SysEx?
> +midi.shareDevicesWithSysex.accesskey = A
> +midi.neverShareDevicesWithSysex = Never share devices with SysEx?
> +midi.neverShareDevicesWithSysex.accesskey = N
> +midi.shareWithFile = Share MIDI with file %S?

These 4 strings should have a localization note explaining what %S will be replaced with.

::: browser/modules/PermissionUI.jsm:687
(Diff revision 2)
> +  this.request = request;
> +  let types = request.types.QueryInterface(Ci.nsIArray);
> +  let perm = types.queryElementAt(0, Ci.nsIContentPermissionType);
> +  this.isSysexPerm = (perm.options.length > 0 &&
> +                      perm.options.queryElementAt(0, Ci.nsISupportsString) == "sysex");
> +  let permName = 'midi';

nit: please use double quotes.

::: browser/modules/PermissionUI.jsm:689
(Diff revision 2)
> +  let perm = types.queryElementAt(0, Ci.nsIContentPermissionType);
> +  this.isSysexPerm = (perm.options.length > 0 &&
> +                      perm.options.queryElementAt(0, Ci.nsISupportsString) == "sysex");
> +  let permName = 'midi';
> +  if (this.isSysexPerm) {
> +    permName = 'midi-sysex';

here too.

Comment 58

[Andrea Marchesini [:baku]]

Comment on attachment 8929647 [details]
Bug 1201590 - WebMIDI Content implementation

https://reviewboard.mozilla.org/r/200926/#review211004

It looks OK except for the shutting down of the actors.

::: dom/base/Navigator.cpp:57
(Diff revision 2)
>  #include "mozilla/SSE.h"
>  #include "mozilla/StaticPtr.h"
>  #include "Connection.h"
>  #include "mozilla/dom/Event.h" // for nsIDOMEvent::InternalDOMEvent()
>  #include "nsGlobalWindow.h"
> +#include "mozilla/dom/MIDIAccessManager.h"

alphabetic order.

::: dom/base/Navigator.cpp:1527
(Diff revision 2)
>  
> +already_AddRefed<Promise>
> +Navigator::RequestMIDIAccess(const MIDIOptions& aOptions,
> +                             ErrorResult& aRv)
> +{
> +  if (!mWindow || !mWindow->GetDocShell()) {

why this GetDocShell() check?

::: dom/midi/MIDIAccessManager.h:18
(Diff revision 2)
> +#include "mozilla/Observer.h"
> +#include "nsWeakPtr.h"
> +
> +namespace mozilla {
> +namespace ipc {
> +class PBackgroundChild;

you don't need this.

::: dom/midi/MIDIAccessManager.cpp:68
(Diff revision 2)
> +  RefPtr<Promise> p = Promise::Create(go, aRv);
> +  if (NS_WARN_IF(aRv.Failed())) {
> +    return nullptr;
> +  }
> +  nsCOMPtr<nsIDocument> doc = aWindow->GetDoc();
> +  if (!doc) {

you must return an error here.
aRv.Throw(something);

::: dom/midi/MIDIAccessManager.cpp:90
(Diff revision 2)
> +    // Otherwise we must begin the PBackground initialization process and
> +    // wait for the async ActorCreated() callback.
> +    MOZ_ASSERT(NS_IsMainThread());
> +    ::mozilla::ipc::PBackgroundChild* actor = BackgroundChild::GetOrCreateForCurrentThread();
> +    if (NS_WARN_IF(!actor)) {
> +      MOZ_CRASH("We shouldn't be here!");

We don't crash anymore when this happens. Do nothing instead.

::: dom/midi/MIDIAccessManager.cpp:98
(Diff revision 2)
> +    RefPtr<MIDIManagerChild> mgr(new MIDIManagerChild());
> +    PMIDIManagerChild* constructedMgr =
> +      actor->SendPMIDIManagerConstructor(mgr);
> +
> +    if (NS_WARN_IF(!constructedMgr)) {
> +      MOZ_CRASH("We shouldn't be here!");

same here.
If you make this method to return a true/false, you can reject promises.

::: dom/midi/MIDIAccessManager.cpp:117
(Diff revision 2)
> +  mChangeObservers.RemoveObserver(aObserver);
> +  if (mChangeObservers.Length() == 0) {
> +    // If we're out of listeners, go ahead and shut down. Make sure to cleanup
> +    // the IPDL protocol also.
> +    if (mChild) {
> +      mChild->Send__delete__(mChild);

This can cause a crash. Better if you ask the parent side to delete the actor. What about if the parent is sending a message? But I have to finish the review yet, maybe this cannot happen for other reasons...

::: dom/midi/MIDIAccessManager.cpp:131
(Diff revision 2)
> +                                    bool aNeedsSysex,
> +                                    Promise* aPromise)
> +{
> +  MOZ_ASSERT(aWindow);
> +  RefPtr<MIDIAccess> a(new MIDIAccess(aWindow, aNeedsSysex, aPromise));
> +  AddObserver(a);

if (NS_WARN_IF(!AddObjserver(a))) {
... reject the prmise

::: dom/midi/MIDIInput.h:30
(Diff revision 2)
> +public:
> +  MIDIInput(nsPIDOMWindowInner* aWindow, MIDIAccess* aMIDIAccessParent,
> +            const MIDIPortInfo& aPortInfo, const bool aSysexEnabled);
> +  ~MIDIInput();
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;

no virtual for final classes.

::: dom/midi/MIDIInput.h:40
(Diff revision 2)
> +
> +  // Getter for the event handler callback
> +  EventHandlerNonNull* GetOnmidimessage();
> +  // Setter for the event handler callback
> +  void SetOnmidimessage(EventHandlerNonNull* aCallback);
> +protected:

private:

this is a final class.

::: dom/midi/MIDIInput.h:43
(Diff revision 2)
> +  // Setter for the event handler callback
> +  void SetOnmidimessage(EventHandlerNonNull* aCallback);
> +protected:
> +  // Takes an array of IPC MIDIMessage objects and turns them into
> +  // MIDIMessageEvents, which it then fires.
> +  virtual void Receive(const nsTArray<MIDIMessage>& aMsgs) override;

no virtual for final classes.

::: dom/midi/MIDIInputMap.h:34
(Diff revision 2)
> +  {
> +    return mParent;
> +  }
> +
> +  explicit MIDIInputMap(nsPIDOMWindowInner* aParent);
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;

no virtual for final classes.

::: dom/midi/MIDIManagerChild.h:28
(Diff revision 2)
> +{
> +public:
> +  NS_INLINE_DECL_REFCOUNTING(MIDIManagerChild)
> +
> +  MIDIManagerChild();
> +  virtual mozilla::ipc::IPCResult RecvMIDIPortListUpdate(const MIDIPortList& aPortList);

no virtual for final classes.

::: dom/midi/MIDIManagerChild.cpp:25
(Diff revision 2)
> +mozilla::ipc::IPCResult
> +MIDIManagerChild::RecvMIDIPortListUpdate(const MIDIPortList& aPortList)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  // If the access manager isn't running, how are we even getting updates?
> +  if (!MIDIAccessManager::IsRunning()) {

can you assert this?

::: dom/midi/MIDIManagerChild.cpp:34
(Diff revision 2)
> +  return IPC_OK();
> +}
> +
> +void
> +MIDIManagerChild::SetActorAlive()
> +{

Add a comment here as well.

::: dom/midi/MIDIMessageEvent.h:37
(Diff revision 2)
> +  explicit MIDIMessageEvent(mozilla::dom::EventTarget* aOwner);
> +
> +  JS::Heap<JSObject*> mData;
> +
> +public:
> +  virtual MIDIMessageEvent* AsMIDIMessageEvent();

no virtual for final classes. everywhere here.

::: dom/midi/MIDIMessageEvent.cpp:125
(Diff revision 2)
> +    mRawData.Clear();
> +    JS::ExposeObjectToActiveJS(mData);
> +  }
> +  aData.set(mData);
> +}
> +

Are you sure this cannot be a generated event?
What prevents this to be one of them?

::: dom/midi/MIDIOutput.h:34
(Diff revision 2)
> +public:
> +  MIDIOutput(nsPIDOMWindowInner* aWindow, MIDIAccess* aMIDIAccessParent,
> +             const MIDIPortInfo& aPortInfo, const bool aSysexEnabled);
> +  ~MIDIOutput();
> +
> +  virtual JSObject* WrapObject(JSContext* aCx,

ditto.

::: dom/midi/MIDIOutput.cpp:26
(Diff revision 2)
> +  MIDIPort(aWindow, aMIDIAccessParent, aPortInfo, aSysexEnabled)
> +{
> +  MOZ_ASSERT(static_cast<MIDIPortType>(aPortInfo.type()) == MIDIPortType::Output);
> +}
> +
> +MIDIOutput::~MIDIOutput()

= default;

::: dom/midi/MIDIOutputMap.h:37
(Diff revision 2)
> +  GetParentObject() const
> +  {
> +    return mParent;
> +  }
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;

ditto

::: dom/midi/MIDIPort.cpp:49
(Diff revision 2)
> +  MOZ_ASSERT(aWindow);
> +  MOZ_ASSERT(aMIDIAccessParent);
> +  RefPtr<MIDIPortChild> port = new MIDIPortChild(aPortInfo, aSysexEnabled, this);
> +  PBackgroundChild* b = BackgroundChild::GetForCurrentThread();
> +  MOZ_ASSERT(b, "Should always have a valid BackgroundChild when creating a port object!");
> +  Unused << b->SendPMIDIPortConstructor(port, aPortInfo, aSysexEnabled);

this can fail. Maybe make a ::Create() and in case, handle the error.

::: dom/midi/MIDIPortChild.cpp:33
(Diff revision 2)
> +  if (mDOMPort) {
> +    mDOMPort->UnsetIPCPort();
> +    mDOMPort = nullptr;
> +  }
> +  if (!mShuttingDown) {
> +    Send__delete__(this);

this is fragile. If the parent calls SendReceive() we crash.

Better if you do:

SendShutdown();
mShuttingDown = true;

if mShuttingDown you don't call any Send*() method.
And at some point you will receive a ActorDestroy().

The parent will do:

MIDIPortParent::RecvShutdown()
{
  ...
  Send__delete__()
}

::: dom/midi/moz.build:36
(Diff revision 2)
> +    'MIDIPlatformRunnables.h',
> +    'MIDIPlatformService.h',
> +    'MIDIPort.h',
> +    'MIDIPortChild.h',
> +    'MIDIPortInterface.h',
> +    'MIDIPortParent.h',

this is not included.

::: dom/midi/moz.build:56
(Diff revision 2)
> +    'MIDIPlatformRunnables.cpp',
> +    'MIDIPlatformService.cpp',
> +    'MIDIPort.cpp',
> +    'MIDIPortChild.cpp',
> +    'MIDIPortInterface.cpp',
> +    'MIDIPortParent.cpp',

this is not included..

Comment 59

[Andrea Marchesini [:baku]]

Comment on attachment 8929648 [details]
Bug 1201590 - WebMIDI Parent/PBackground implementation

https://reviewboard.mozilla.org/r/200928/#review211012

good. the shutdown part in interdiff?

::: dom/midi/MIDIMessageQueue.h:21
(Diff revision 2)
> +
> +/**
> + * Since some MIDI Messages can be scheduled to be sent in the future, the
> + * MIDIMessageQueue is responsible for making sure all MIDI messages are
> + * scheduled and sent in order.
> + *

extra line

::: dom/midi/MIDIPlatformRunnables.h:29
(Diff revision 2)
> +{
> +public:
> +  MIDIBackgroundRunnable(const char* aName) :
> +    Runnable(aName)
> +  {}
> +  ~MIDIBackgroundRunnable() {}

= default;

::: dom/midi/MIDIPlatformRunnables.h:29
(Diff revision 2)
> +{
> +public:
> +  MIDIBackgroundRunnable(const char* aName) :
> +    Runnable(aName)
> +  {}
> +  ~MIDIBackgroundRunnable() {}

this should be virtual

::: dom/midi/MIDIPlatformRunnables.h:39
(Diff revision 2)
> +/**
> + * Runnable fired from platform-specific MIDI service thread to PBackground
> + * Thread whenever messages need to be sent to a MIDI device.
> + *
> + */
> +class ReceiveRunnable : public MIDIBackgroundRunnable

final?

::: dom/midi/MIDIPlatformRunnables.h:54
(Diff revision 2)
> +    MIDIBackgroundRunnable("ReceiveRunnable"),
> +    mPortId(aPortId)
> +  {
> +    mMsgs.AppendElement(aMsgs);
> +  }
> +  ~ReceiveRunnable() {}

= default

::: dom/midi/MIDIPlatformRunnables.h:55
(Diff revision 2)
> +    mPortId(aPortId)
> +  {
> +    mMsgs.AppendElement(aMsgs);
> +  }
> +  ~ReceiveRunnable() {}
> +  virtual void RunInternal() override;

no virtual

::: dom/midi/MIDIPlatformRunnables.h:66
(Diff revision 2)
> +/**
> + * Runnable fired from platform-specific MIDI service thread to PBackground
> + * Thread whenever a device is connected.
> + *
> + */
> +class AddPortRunnable : public MIDIBackgroundRunnable

same comments as before

::: dom/midi/MIDIPlatformRunnables.h:83
(Diff revision 2)
> +/**
> + * Runnable fired from platform-specific MIDI service thread to PBackground
> + * Thread whenever a device is disconnected.
> + *
> + */
> +class RemovePortRunnable : public MIDIBackgroundRunnable

same comments as before

::: dom/midi/MIDIPlatformRunnables.h:101
(Diff revision 2)
> +/**
> + * Runnable used to delay calls to SendPortList, which is requires to make sure
> + * MIDIManager actor initialization happens correctly. Also used for testing.
> + *
> + */
> +class SendPortListRunnable : public MIDIBackgroundRunnable

same comments as before

::: dom/midi/MIDIPlatformRunnables.h:116
(Diff revision 2)
> +/**
> + * Runnable fired from platform-specific MIDI service thread to PBackground
> + * Thread whenever a device is disconnected.
> + *
> + */
> +class SetStatusRunnable : public MIDIBackgroundRunnable

same comments as before

::: dom/midi/MIDIPlatformService.cpp:205
(Diff revision 2)
> +  AssertIsOnBackgroundThread();
> +  if (!IsRunning()) {
> +    ErrorResult rv;
> +    // Uncomment once we have an actual platform library to test.
> +    //
> +    // bool useTestService = false;

this is going to be implemented by other patches?

::: dom/midi/MIDIPortParent.h:28
(Diff revision 2)
> +    public PMIDIPortParent,
> +    public MIDIPortInterface
> +{
> +public:
> +  NS_INLINE_DECL_REFCOUNTING(MIDIPortParent);
> +  virtual void ActorDestroy(ActorDestroyReason) override;

no virtual for final classes.

::: dom/midi/MIDIPortParent.h:38
(Diff revision 2)
> +  MOZ_IMPLICIT MIDIPortParent(const MIDIPortInfo& aPortInfo, const bool aSysexEnabled);
> +  // Sends the current port status to the child actor. May also send message
> +  // buffer if required.
> +  bool SendUpdateStatus(const MIDIPortDeviceState& aState,
> +                        const MIDIPortConnectionState& aConnection);
> +  uint32_t GetInternalId() { return mInternalId; }

uint32_t GetInternalId() const

::: dom/midi/MIDIPortParent.cpp:65
(Diff revision 2)
> +  }
> +  return IPC_OK();
> +}
> +
> +void
> +MIDIPortParent::ActorDestroy(ActorDestroyReason)

If you follow the comments of the other patch, here you will have:

MIDIPortParent::RecvShutdown()
{
  ActorDestroy(whatever);
}

::: dom/midi/MIDIPortParent.cpp:88
(Diff revision 2)
> +  if (aConnectionState == MIDIPortConnectionState::Open &&
> +      aDeviceState == MIDIPortDeviceState::Disconnected) {
> +    mConnectionState = MIDIPortConnectionState::Pending;
> +  } else if (aConnectionState == MIDIPortConnectionState::Open &&
> +             aDeviceState == MIDIPortDeviceState::Connected &&
> +             mMessageQueue.Length() > 0) {

!mMEssageQueue.IsEmpty()

::: dom/midi/MIDIPortParent.cpp:101
(Diff revision 2)
> +}
> +
> +MIDIPortParent::MIDIPortParent(const MIDIPortInfo& aPortInfo,
> +                               const bool aSysexEnabled) :
> +  MIDIPortInterface(aPortInfo, aSysexEnabled),
> +  mInternalId(gId)

In this way, 0 is a valid internalId. Better to do:

mInternalId(++gId) instead

::: dom/midi/MIDIPortParent.cpp:109
(Diff revision 2)
> +             "Shouldn't be able to add MIDI port without MIDI service!");
> +  MIDIPlatformService::Get()->AddPort(this);
> +  ++gId;
> +}
> +
> +MIDIPortParent::~MIDIPortParent()

= default;

::: ipc/glue/BackgroundParentImpl.cpp:969
(Diff revision 2)
>  bool
>  BackgroundParentImpl::DeallocPHttpBackgroundChannelParent(
>                                        net::PHttpBackgroundChannelParent *aActor)
>  {
> +  // release extra refcount hold by AllocPHttpBackgroundChannelParent
> +  RefPtr<net::HttpBackgroundChannelParent> actor =

this is the magic of the diff...

Comment 60

[Andrea Marchesini [:baku]]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

https://reviewboard.mozilla.org/r/200930/#review211016

r+ for the DOM part.

Comment 61

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929646 [details]
Bug 1201590 - WebMIDI Utility classes

https://reviewboard.mozilla.org/r/200924/#review211002

> do we want to enable it in nightly only?

Yeah, until we get platform support for at least one platform landed, this should only be on for nightly.

Comment 62

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929647 [details]
Bug 1201590 - WebMIDI Content implementation

https://reviewboard.mozilla.org/r/200926/#review211004

> Are you sure this cannot be a generated event?
> What prevents this to be one of them?

The problem is that the event holds a Uint8Array, and so far seems to be the only Event in the codebase that does so. Creating Uint8Arrays is fallible on out of memory instances, in which case, we throw. Marking the getter as [Throws] means we can't generate it. If we removed the [Throws] and just acted as if it was infallible, the event generator produces code with compile errors, due to trying to use JS::Head<JSObject*>.set() in a way that's not really supported. Not sure if it's worth filing a followup on that.

Comment 63

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929648 [details]
Bug 1201590 - WebMIDI Parent/PBackground implementation

https://reviewboard.mozilla.org/r/200928/#review211012

> this is going to be implemented by other patches?

Yeah. Right now ALL we have is the test service, so it's just the default. As we start implementing platforms, this will probably become pref triggered. I'll add a TODO for that.

> If you follow the comments of the other patch, here you will have:
> 
> MIDIPortParent::RecvShutdown()
> {
>   ActorDestroy(whatever);
> }

The way I implemented this, I call Send__delete__() there, since we'll be destroying the IPC channel. That ends up eventually calling ActorDestroy anyways. It seems to work, but is that incorrect?

Comment 64

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

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

Comment 65

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8805211 [details]
Bug 1201590 - WebMIDI IPDL files;

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

Comment 66

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929646 [details]
Bug 1201590 - WebMIDI Utility classes

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

Comment 67

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929647 [details]
Bug 1201590 - WebMIDI Content implementation

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

Comment 68

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929648 [details]
Bug 1201590 - WebMIDI Parent/PBackground implementation

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

Comment 69

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

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

Comment 70

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755611 [details]
Bug 1201590 - WebMIDI Mochitests

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

Comment 71

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755611 [details]
Bug 1201590 - WebMIDI Mochitests

https://reviewboard.mozilla.org/r/54684/#review53152

> I feel bad asking about this because it's all already written, but if you have tests that are not too gecko specific, you could make them be web-platform-tests.
> 
> I don't know how feasible that is, considering we have this nice fake midi backend thing.

I'm really not sure how we're going to handle web platform tests here yet, since it requires emulation of the devices. I'm not even seeing mention of the idea on the spec repo, so I'll bring it up there and can maybe start reworking some of our tests if we come to an agreement on how it should be done.

Comment 72

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

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

Comment 73

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8805211 [details]
Bug 1201590 - WebMIDI IPDL files;

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

Comment 74

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929646 [details]
Bug 1201590 - WebMIDI Utility classes

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

Comment 75

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929647 [details]
Bug 1201590 - WebMIDI Content implementation

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

Comment 76

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929648 [details]
Bug 1201590 - WebMIDI Parent/PBackground implementation

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

Comment 77

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

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

Comment 78

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755611 [details]
Bug 1201590 - WebMIDI Mochitests

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

Comment 79

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, patches are back in view review comments from :baku and :florian addressed. 

A heads up on WebIDL changes here. Having talked to :overhold and :annevk, I've changed the WebMIDI IDL interfaces to require SecureContext, following the "new APIs are secure context only" policy that's in discussion. Right now, we'll only be shipping on Nightly anyways, so this would be the similar criteria for Origin Trials on Chrome. We can continue to discuss this deviation from the spec while this is in Nightly.

Comment 80

[Andrea Marchesini [:baku]]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

https://reviewboard.mozilla.org/r/54682/#review211802

Comment 81

[Andrea Marchesini [:baku]]

Comment on attachment 8805211 [details]
Bug 1201590 - WebMIDI IPDL files;

https://reviewboard.mozilla.org/r/88986/#review211804

Comment 82

[Andrea Marchesini [:baku]]

Comment on attachment 8929647 [details]
Bug 1201590 - WebMIDI Content implementation

https://reviewboard.mozilla.org/r/200926/#review211806

Several unrelated things.

::: dom/base/Navigator.h
(Diff revisions 2 - 4)
>    bool CookieEnabled();
>    void GetBuildID(nsAString& aBuildID, CallerType aCallerType,
>                    ErrorResult& aRv) const;
>    bool JavaEnabled(CallerType aCallerType, ErrorResult& aRv);
>    uint64_t HardwareConcurrency();
> -  bool CpuHasSSE2();

Unrelated?

::: dom/base/Navigator.cpp
(Diff revisions 2 - 4)
>    }
>  
>    return rts->ClampedHardwareConcurrency();
>  }
>  
> -bool

Unrelated. Separate patch, separate bug?

::: dom/midi/MIDIAccessManager.cpp:68
(Diff revisions 2 - 4)
>    RefPtr<Promise> p = Promise::Create(go, aRv);
>    if (NS_WARN_IF(aRv.Failed())) {
>      return nullptr;
>    }
>    nsCOMPtr<nsIDocument> doc = aWindow->GetDoc();
>    if (!doc) {

NS_WARN_IF

::: dom/midi/MIDIManagerChild.cpp
(Diff revisions 2 - 4)
>  mozilla::ipc::IPCResult
>  MIDIManagerChild::RecvMIDIPortListUpdate(const MIDIPortList& aPortList)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> -  // If the access manager isn't running, how are we even getting updates?
> +  MOZ_ASSERT(MIDIAccessManager::IsRunning());
> -  if (!MIDIAccessManager::IsRunning()) {

In order to avoid calling Update() if we are shutting down, probably you can do:

void
MIDIManagerChild::Shutdown()
{
  MOZ_ASSERT(!mShutdown);
  mShutdown = true;
  SendShutdown();
}

and here you can do:

if (mShutdown) {
  return IPC_OK();
}

MOZ_ASSERT(MIDIAccessManager::IsRunning());
...

And of course, call Shutdown() instead of SendShutdown() in the manager.

::: dom/midi/MIDIOutput.cpp:32
(Diff revisions 2 - 4)
> +MIDIOutput::Create(nsPIDOMWindowInner* aWindow, MIDIAccess* aMIDIAccessParent,
> +                   const MIDIPortInfo& aPortInfo, const bool aSysexEnabled)
>  {
> +  MOZ_ASSERT(static_cast<MIDIPortType>(aPortInfo.type()) == MIDIPortType::Output);
> +  auto port = new MIDIOutput(aWindow, aMIDIAccessParent);
> +  if (!port->Initialize(aPortInfo, aSysexEnabled)) {

NS_WARN_IF

::: ipc/glue/BackgroundChildImpl.cpp:45
(Diff revisions 2 - 4)
>  #include "mozilla/ipc/PParentToChildStreamChild.h"
>  #include "mozilla/layout/VsyncChild.h"
>  #include "mozilla/net/HttpBackgroundChannelChild.h"
>  #include "mozilla/net/PUDPSocketChild.h"
>  #include "mozilla/dom/network/UDPSocketChild.h"
> -#include "mozilla/dom/WebAuthnTransactionChildBase.h"
> +#include "mozilla/dom/WebAuthnTransactionChild.h"

unrelated

::: ipc/glue/BackgroundChildImpl.cpp:91
(Diff revisions 2 - 4)
>  using mozilla::dom::cache::PCacheStorageChild;
>  using mozilla::dom::cache::PCacheStreamControlChild;
>  using mozilla::dom::LocalStorage;
>  using mozilla::dom::StorageDBChild;
>  
> -using mozilla::dom::WebAuthnTransactionChildBase;
> +using mozilla::dom::WebAuthnTransactionChild;

unrelated

::: ipc/glue/BackgroundChildImpl.cpp:655
(Diff revisions 2 - 4)
>  
>  bool
>  BackgroundChildImpl::DeallocPWebAuthnTransactionChild(PWebAuthnTransactionChild* aActor)
>  {
>    MOZ_ASSERT(aActor);
> -  RefPtr<dom::WebAuthnTransactionChildBase> child =
> +  RefPtr<dom::WebAuthnTransactionChild> child =

unrelated?

::: ipc/glue/BackgroundChildImpl.cpp:710
(Diff revisions 2 - 4)
>  BackgroundChildImpl::GetMessageSchedulerGroups(const Message& aMsg, SchedulerGroupSet& aGroups)
>  {
>    if (aMsg.type() == layout::PVsync::MessageType::Msg_Notify__ID) {
>      MOZ_ASSERT(NS_IsMainThread());
>      aGroups.Clear();
> -    if (dom::TabChild::HasActiveTabs()) {
> +    if (dom::TabChild::HasVisibleTabs()) {

why this?

Comment 83

[Andrea Marchesini [:baku]]

Comment on attachment 8929648 [details]
Bug 1201590 - WebMIDI Parent/PBackground implementation

https://reviewboard.mozilla.org/r/200928/#review211810

::: dom/midi/MIDIPlatformRunnables.h:44
(Diff revisions 2 - 4)
>  {
>  public:
>    ReceiveRunnable(const nsAString& aPortId, const nsTArray<MIDIMessage>& aMsgs) :
>      MIDIBackgroundRunnable("ReceiveRunnable"),
>      mMsgs(aMsgs),
> -    mPortId(aPortId)
> +    mPortId(aPortId) {}

why this change?

Comment 84

[Francesco Lodolo [:flod]]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

https://reviewboard.mozilla.org/r/200930/#review211828

While our current files are all over the place, it would be better to use

XXXX.label
XXXX.accesskey

instead of

XXXX
XXXX.accesskey

::: browser/locales/en-US/chrome/browser/browser.properties:938
(Diff revision 4)
> +midi.alwaysShareDevices = Always share devices?
> +midi.alwaysShareDevices.accesskey = A
> +midi.neverShareDevices = Never share devices?
> +midi.neverShareDevices.accesskey = N
> +midi.shareDevices = Share devices?
> +midi.shareDevices.accesskey = Q

It's an accesskey, it should be a character available in the label.

::: browser/locales/en-US/chrome/browser/browser.properties:939
(Diff revision 4)
> +midi.alwaysShareDevices.accesskey = A
> +midi.neverShareDevices = Never share devices?
> +midi.neverShareDevices.accesskey = N
> +midi.shareDevices = Share devices?
> +midi.shareDevices.accesskey = Q
> +midi.shareDevicesWithSysex = Always share devices SysEx?

Should this be "SysEx devices"? For example, you normally talk about "SysEx messages", not "messages SysEx".

::: browser/locales/en-US/chrome/browser/browser.properties:941
(Diff revision 4)
> +midi.neverShareDevices.accesskey = N
> +midi.shareDevices = Share devices?
> +midi.shareDevices.accesskey = Q
> +midi.shareDevicesWithSysex = Always share devices SysEx?
> +midi.shareDevicesWithSysex.accesskey = A
> +midi.neverShareDevicesWithSysex = Never share devices with SysEx?

Why one string says "share devices SysEx", and the next "never share devices with SysEx"? I assume both should be "share SysEx devices".

::: browser/locales/en-US/chrome/browser/browser.properties:943
(Diff revision 4)
> +midi.shareDevices.accesskey = Q
> +midi.shareDevicesWithSysex = Always share devices SysEx?
> +midi.shareDevicesWithSysex.accesskey = A
> +midi.neverShareDevicesWithSysex = Never share devices with SysEx?
> +midi.neverShareDevicesWithSysex.accesskey = N
> +# LOCALIZATION NOTE  - %S is the name of the file URL (file://...) requesting MIDI access

Please stick with the format (also for the next strings)

# LOCALIZATION NOTE (STRING):

::: browser/locales/en-US/chrome/browser/browser.properties:948
(Diff revision 4)
> +# LOCALIZATION NOTE  - %S is the name of the file URL (file://...) requesting MIDI access
> +midi.shareWithFile = Share MIDI with file %S?
> +# LOCALIZATION NOTE  - %S is the name of the site URL (https://...) requesting MIDI access
> +midi.shareWithSite = Share MIDI with site %S?
> +# LOCALIZATION NOTE  - %S is the name of the file URL (file://...) requesting MIDI access
> +midi.shareSysexWithFile = Share MIDI with file %S?

What's the difference between this string and midi.shareWithFile? I assume you should be using the same string here. Same question for the site string.

Comment 85

[Paul Adenot (:padenot)]

Comment on attachment 8929646 [details]
Bug 1201590 - WebMIDI Utility classes

https://reviewboard.mozilla.org/r/200924/#review211820

::: dom/midi/MIDIUtils.cpp:10
(Diff revision 4)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/dom/MIDITypes.h"
> +#include "mozilla/dom/MIDIUtils.h"
> +
> +static const uint8_t kCommandByte = 0x80;

I think we want to reference "MIDI IMPLEMENTATION CHART INSTRUCTIONS", page 97 of the spec.

::: dom/midi/MIDIUtils.cpp:16
(Diff revision 4)
> +static const uint8_t kSysexMessageStart = 0xF0;
> +static const uint8_t kSystemMessage = 0xF0;
> +static const uint8_t kSysexMessageEnd = 0xF7;
> +static const uint8_t kSystemRealtimeMessage = 0xF8;
> +// Represents the length of all possible command messages.
> +static const uint8_t kCommandLengths[] = {3, 3, 3, 3, 2, 2, 3};

Maybe say this is table 2, page 101 or something.

::: dom/midi/MIDIUtils.cpp:19
(Diff revision 4)
> +static const uint8_t kSystemRealtimeMessage = 0xF8;
> +// Represents the length of all possible command messages.
> +static const uint8_t kCommandLengths[] = {3, 3, 3, 3, 2, 2, 3};
> +// Represents the length of all possible system messages. The length of sysex
> +// messages is variable, so we just put zero since it won't be checked anyways.
> +static const uint8_t kSystemLengths[] = {0, 2, 3, 2, 1, 1, 1, 1};

Maybe say this is page 105 or table 5 in the MIDI 1.0 spec.

::: dom/midi/MIDIUtils.cpp:56
(Diff revision 4)
> +    return aMsg->data().Length() == 1;
> +  }
> +  // Otherwise, just use the correct array for testing lengths. We can't tell
> +  // much about message validity other than that.
> +  if ((cmd & kSystemMessage) == kSystemMessage) {
> +    if (cmd - kSystemMessage >= (uint8_t)ArrayLength(kSystemLengths)) {

nit: c++ style cast.

::: dom/midi/MIDIUtils.cpp:123
(Diff revision 4)
> +              const TimeStamp& aTimestamp,
> +              nsTArray<MIDIMessage>& aMsgArray)
> +{
> +  MOZ_ASSERT(aByteBuffer);
> +  nsTArray<uint8_t> bytes;
> +  bytes.AppendElements(aByteBuffer, aLength);

Do we really need to copy here? It's all synchronous.

Comment 86

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929646 [details]
Bug 1201590 - WebMIDI Utility classes

https://reviewboard.mozilla.org/r/200924/#review211820

> Do we really need to copy here? It's all synchronous.

Not even sure why that was there. It's dead code. Just removed it for now, though I have a feeling it was probably for one of the platform specific implementation patches not in this set, and it may come back later. Will fix then.

Comment 87

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929647 [details]
Bug 1201590 - WebMIDI Content implementation

https://reviewboard.mozilla.org/r/200926/#review211806

> Unrelated?

Ok I have no idea what happened here. This isn't even in my diff?

> Unrelated. Separate patch, separate bug?

Not seeing this in my local patches either?

> unrelated

I'm not seeing any of these unrelated changes in my local patch set. Is there something weird going on with interdiff or something?

Comment 88

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

https://reviewboard.mozilla.org/r/200930/#review211828

Ok, will do. Also, do I still need review from :johannh, or should I reassign to you?

> It's an accesskey, it should be a character available in the label.

Changed to S, since we don't have a Y in there for "Yes".

> Should this be "SysEx devices"? For example, you normally talk about "SysEx messages", not "messages SysEx".

I went with "devices with SysEx message permissions", since devices can use both regular and SysEx messages.

> What's the difference between this string and midi.shareWithFile? I assume you should be using the same string here. Same question for the site string.

That was an oversight on my part. Those should be two different strings.

Comment 89

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

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

Comment 90

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8805211 [details]
Bug 1201590 - WebMIDI IPDL files;

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

Comment 91

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929646 [details]
Bug 1201590 - WebMIDI Utility classes

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

Comment 92

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929647 [details]
Bug 1201590 - WebMIDI Content implementation

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

Comment 93

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929648 [details]
Bug 1201590 - WebMIDI Parent/PBackground implementation

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

Comment 94

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

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

Comment 95

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1201590 - WebMIDI Mochitests

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

Comment 96

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

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

Comment 97

[Andrea Marchesini [:baku]]

Comment on attachment 8935660 [details]
Bug 1201590 - WebMIDI Mochitests

https://reviewboard.mozilla.org/r/206560/#review212242

::: dom/midi/TestMIDIPlatformService.cpp
(Diff revision 1)
>          }
>          // Cause control test ports to connect
>          case 0x01:
>          {
>            nsCOMPtr<nsIRunnable> r1(new AddPortRunnable(mStateTestInputPort));
> -          nsCOMPtr<nsIRunnable> r2(new AddPortRunnable(mStateTestOutputPort));

why this change?

Comment 98

[Paul Adenot (:padenot)]

Comment on attachment 8805211 [details]
Bug 1201590 - WebMIDI IPDL files;

https://reviewboard.mozilla.org/r/88986/#review212408

::: dom/midi/moz.build:11
(Diff revision 6)
> +
> +# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> +# vim: set filetype=python:
> +# 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/.

You've put in the license and editor config and stuff twice it seems.

Comment 99

[Paul Adenot (:padenot)]

Comment on attachment 8805211 [details]
Bug 1201590 - WebMIDI IPDL files;

https://reviewboard.mozilla.org/r/88986/#review212412

Comment 100

[Paul Adenot (:padenot)]

Comment on attachment 8929647 [details]
Bug 1201590 - WebMIDI Content implementation

https://reviewboard.mozilla.org/r/200926/#review212432

::: dom/midi/MIDIMessageEvent.cpp:7
(Diff revision 5)
> +/* vim:set ts=2 sw=2 sts=2 et cindent: */
> +/* 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/. */
> +
> +/* THIS FILE IS AUTOGENERATED - DO NOT EDIT */

!?

::: dom/midi/MIDIOutput.cpp:76
(Diff revision 5)
> +  nsTArray<MIDIMessage> msgArray;
> +  MIDIUtils::ParseMessages(aData, timestamp, msgArray);
> +  // Our translation of the spec is that invalid messages in a multi-message
> +  // sequence will be thrown out, but that valid messages will still be used.
> +  if (msgArray.IsEmpty()) {
> +    aRv.Throw(NS_ERROR_DOM_TYPE_ERR);

We still don't have an agreement on [0]. Let's land this and complain louder.

[0]: https://github.com/WebAudio/web-midi-api/issues/164

Comment 101

[Paul Adenot (:padenot)]

Comment on attachment 8929648 [details]
Bug 1201590 - WebMIDI Parent/PBackground implementation

https://reviewboard.mozilla.org/r/200928/#review212472

Don't have much to contribute to the IPC parts, and the testing parts look good.

Comment 102

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929647 [details]
Bug 1201590 - WebMIDI Content implementation

https://reviewboard.mozilla.org/r/200926/#review212432

> !?

Ugh, this patch set is such a mess. That probably got added while I was trying to turn this into a generated event. Fixed.

Comment 103

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8935660 [details]
Bug 1201590 - WebMIDI Mochitests

https://reviewboard.mozilla.org/r/206560/#review212242

> why this change?

This was causing more harm than good in the tests. Right now we're still failing TV on try, which means we'll get backed right out if we land. Expect more changes here before landing.

Comment 104

[Johann Hofmann [:johannh]]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

https://reviewboard.mozilla.org/r/200930/#review214268

Thanks for the patch and thanks for using the content permission integration properly!

Since this permission will not be hidden behind a pref AFAIU, we'll need to do a little more UI work before this can land. I made some suggestion to make the doorhanger copy consistent below, but we will also need:

- A permission icon (I'll needinfo UX)
- The midi permissions to show up in the site identity panel and page info (you'll only need to add the permission keys to browser/modules/SitePermissions.jsm, see below)
- Tests for the permission prompt (really easy to add, see below)

You can find a great example changeset that introduced the persistent-storage permission in the patch here: https://reviewboard.mozilla.org/r/91652/diff/10#index_header

Please let me know if there's anything I can help you with!

::: browser/locales/en-US/chrome/browser/browser.properties:933
(Diff revision 6)
>  # LOCALIZATION NOTE (certImminentDistrust.message):
>  # Shown in the browser console when visiting a website that is trusted today,
>  # but won't be in the future unless the site operator makes a change.
>  certImminentDistrust.message = The security certificate in use on this website will no longer be trusted in a future release. For more information, visit https://wiki.mozilla.org/CA/Upcoming_Distrust_Actions
> +
> +midi.alwaysShareDevices.label = Always share devices?

I think we'll need to rework the prompt copy to be consistent with the other doorhangers (and make it look less stretched). My suggestion would be

Will you allow %s to access MIDI devices \{with SysEx permissions\}?

\[x\] Remember this decision

\[Allow MIDI Access\] \[Don't Allow\]

That would also include adding a checkbox, which should be relatively easy by looking at the other permission prompts in the file (e.g. here: https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/browser/modules/PermissionUI.jsm#620)

::: browser/modules/PermissionUI.jsm:689
(Diff revision 6)
> +  let perm = types.queryElementAt(0, Ci.nsIContentPermissionType);
> +  this.isSysexPerm = (perm.options.length > 0 &&
> +                      perm.options.queryElementAt(0, Ci.nsISupportsString) == "sysex");
> +  let permName = "midi";
> +  if (this.isSysexPerm) {
> +    permName = "midi-sysex";

What is this used for? Was this supposed to live in the permissionKey getter?

::: browser/modules/PermissionUI.jsm:701
(Diff revision 6)
> +  get permissionKey() {
> +    return "midi-sysex";
> +  },
> +
> +  get popupOptions() {
> +    // TODO We need a security/permissions explanation URL for this

Can you file a bug with SUMO -> Knowledge Base Content for this and add it to the comment?

::: security/manager/ssl/nsISecurityUITelemetry.idl:145
(Diff revision 6)
> + */
> +
> +const uint32_t WARNING_MIDI_REQUEST = 77;
> +const uint32_t WARNING_MIDI_REQUEST_SHARE_DEVICE = 78;
> +const uint32_t WARNING_MIDI_REQUEST_ALWAYS_SHARE = 79;
> +const uint32_t WARNING_MIDI_REQUEST_NEVER_SHARE = 80;

Where are these used? Do they have to be in this commit?

Comment 105

[Johann Hofmann [:johannh]]

(In reply to Johann Hofmann [:johannh] from comment #104)
> Will you allow %s to access MIDI devices \{with SysEx permissions\}?
> 
> \[x\] Remember this decision
> 
> \[Allow MIDI Access\] \[Don't Allow\]
> 

Ugh, this got destroyed by reviewboard...

What I meant was:

Will you allow %s to access MIDI devices {with SysEx permissions}?
 
[x] Remember this decision

[Allow MIDI Access] [Don't Allow]

Comment 106

[Johann Hofmann [:johannh]]

Bryan, can you point us to someone who can make a Photon looking MIDI (music themed?) permission icon? Thanks!

Comment 107

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

This particular set of patches will be hidden behind a pref for the moment. We'll have dom.webmidi.enabled set to false until we get an actual platform working, and then it will live on nightly for a bit before riding trains. This bug only lands a test server that doesn't really do much.

That said, I'd rather get the permissions stuff hammered out now rather than later so we can continue to land easily after this, so I'm happy to take care of the permissions work in this bug. I'm on vacation until January but will follow up on the review comments and whatever UX work gets done before I get back then. Thanks!

Comment 108

[Amin Al Hazwani [:amin] (Firefox UX)]

Hey Johann, here the midi icon https://design.firefox.com/icons/viewer/#midi

Comment 109

[Johann Hofmann [:johannh]]

Yaaay! Thank you for taking this, Amin!

Kyle, do you think that icon works? If so, please include it in your patch! (Again see https://reviewboard.mozilla.org/r/91652/diff/10#index_header for an example)

Thanks :)

Comment 110

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Looks good, thanks for making that! I'll add that when I get back.

Comment 111

[Paul Adenot (:padenot)]

Comment on attachment 8929648 [details]
Bug 1201590 - WebMIDI Parent/PBackground implementation

https://reviewboard.mozilla.org/r/200928/#review215818

::: dom/midi/MIDIPlatformService.h:128
(Diff revision 5)
> +  void GetMessagesBefore(const nsAString& aPortId,
> +                         const TimeStamp& aTimeStamp,
> +                         nsTArray<MIDIMessage>& aMsgs);
> +
> +  // Convenience object for sending runnables to the background thread.
> +  nsCOMPtr<nsIThread> mBackgroundThread;

Can you document on which threads the attributes are being used and how (if applicable) those accesses are synchronized?

::: dom/midi/TestMIDIPlatformService.cpp:44
(Diff revision 5)
> +    mSrv->ProcessMessages(mPortID);
> +    return NS_OK;
> +  }
> +private:
> +  nsString mPortID;
> +  TestMIDIPlatformService* mSrv;

Why is it ok to have a raw ptr here?

Comment 112

[Paul Adenot (:padenot)]

Comment on attachment 8929648 [details]
Bug 1201590 - WebMIDI Parent/PBackground implementation

https://reviewboard.mozilla.org/r/200928/#review215822

Comment 113

[Paul Adenot (:padenot)]

Comment on attachment 8935660 [details]
Bug 1201590 - WebMIDI Mochitests

https://reviewboard.mozilla.org/r/206560/#review215828

Looks good as a first set of tests. Are there any web platform tests for this? Could we contribute some?

::: dom/midi/tests/test_midi_device_connect_disconnect.html:19
(Diff revision 1)
> +     async function runTests() {
> +       await MIDITestUtils.permissionSetup(true);
> +       let output;
> +
> +       let midi_access = await navigator.requestMIDIAccess({ "sysex": false });
> +       ok(true, "MIDI Access Request successful");

Shouldn't we check that the promise has been resolved here?

Comment 114

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929648 [details]
Bug 1201590 - WebMIDI Parent/PBackground implementation

https://reviewboard.mozilla.org/r/200928/#review215818

> Can you document on which threads the attributes are being used and how (if applicable) those accesses are synchronized?

Moved this down to TestMIDIPlatformService for the time being, since that's the only place it is currently used. Every runnable sent has checks to make sure the MIDIPlatformManager is alive before doing anything, added that explanation in the member comments.

> Why is it ok to have a raw ptr here?

Turned this into a static cast of the pointer we retrieve from MIDIPlatformService::Get(). That's safer than storing and checking.

Comment 115

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Paul Adenot (:padenot) from comment #113)
> Comment on attachment 8935660 [details]
> Bug 1201590 - WebMIDI Mochitests
> 
> https://reviewboard.mozilla.org/r/206560/#review215828
> 
> Looks good as a first set of tests. Are there any web platform tests for
> this? Could we contribute some?
> 

There aren't webplatform tests yet, nor is there even a bug to implement some, surprisingly. I'll add a bug in the spec repo, but I have a feeling we're going to be following WebBluetooth's lead on creating fake devices for WPTs, since they can't really be run against real hardware.

Comment 116

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Working on the patches for the permission request, I just realized we've got a situation that's different from the rest of the permissions.

WebMIDI has 2 levels of permissions: midi and midi-sysex. If midi is granted, but a sysex message needs to be sent, we need to query the user again for additional privileges. However, if midi-sysex is granted, midi should be granted too. Right now, we only allow one PermissionKey to be set per permission prompt class. I could make two different classes to fix that, but even then, I still need to figure out how to grant 2 permissions from one prompt choice (in the case of midi-sysex and midi). I think I see how I could manage that in PermissionsUI.jsm, but before I do that work, I was wondering if this already happens anywhere else that you're aware of?

Comment 117

[Glenn Linderman]

Re: comment 116
There seems to be a parallel here between page activity permission granting in a browser, and smartphone app permission granting. On my phone, when I install an app, I get a whole list of what it wants to do, or might ever want to do, in one dialog to accept or refuse.

You make it sound like doing similar things in a web page will request one permission at a time... if the list is long, that could be annoying. I applaud your attempt to do both at once, instead of each sequentially. But it raises a more general issue of web page or site permission granting: shouldn't all the permissions be collected, and the user queried once?

On the other hand, one thing I dislike about the single query approach, is that if you want an app to do some things, but never intend to use it for some of its capabilities, you still have to grant permission for those capabilities, in order for it to function at all.

A better system would list, in a single dialog, all the permissions requested, which ones are mandatory, and which features would be disabled by not granting optional permissions. While that might make apps/sites more complex up-front, it might also make them more acceptable to more users. For example, why does my calendar program need access to my contact list, if I never intend to put contacts into my scheduled events? In that scenario, I'd be OK not granting that permission, and then if I ever try to use such a feature, it would be disabled, or, for clever apps/sites, I'd be prompted at time of attempted use to grant the additional permission.

A lot of sites now seem to be asking for location sharing... I get annoyed by the repeated asking... I'd rather not be asked until I request the site to do something that requires knowing my location. I guess that opinion is somewhat contrary to the "all at once, up front" opinion I just got done stating, though :)  It would sure be nice to have a permission that would prevent playing of audio or video without my explicit direction, etc. but that seems to happen without my granting permissions... why should MIDI be different?

So it is probably guaranteed that any general solution will not please some of the people any of the time :)  Of course, I visit a far larger set of sites than the number of apps I install, so maybe that is the key difference. I'd like a blanket "don't ask me for permissions unless you can't function at all without them" for sites, but for apps, I'd like an up-front list of permissions needed and optional permissions to enable certain features. This comment probably belongs in some other bug, or feature than here, but it is related: if the general features don't exist for handling multiple permissions, you can't use them, or have to invent them, but there should be general features of various sorts, as the number of permissions that exist increase.

Comment 118

[Johann Hofmann [:johannh]]

Glenn, thanks for your thoughts, but, as you noted, it's slightly off-topic to this bug. The change you're proposing is rather fundamental and should be discussed elsewhere.

Comment 119

[Johann Hofmann [:johannh]]

(In reply to Kyle Machulis [:qdot] [:kmachulis] (if a patch has no decent commit message, automatic r-) from comment #116)
> Working on the patches for the permission request, I just realized we've got
> a situation that's different from the rest of the permissions.
> 
> WebMIDI has 2 levels of permissions: midi and midi-sysex. If midi is
> granted, but a sysex message needs to be sent, we need to query the user
> again for additional privileges. However, if midi-sysex is granted, midi
> should be granted too. Right now, we only allow one PermissionKey to be set
> per permission prompt class. I could make two different classes to fix that,
> but even then, I still need to figure out how to grant 2 permissions from
> one prompt choice (in the case of midi-sysex and midi). I think I see how I
> could manage that in PermissionsUI.jsm, but before I do that work, I was
> wondering if this already happens anywhere else that you're aware of?

Hm, yeah, that's an interesting problem. Spontaneously I'd say you have two choices then:

You could query the permission manager for midi-sysex in MIDIAccessManager or MIDIPermissionRequest and don't even start the permission request if it's set to ALLOW. It would make the permanent BLOCK case slightly awkward because websites could prompt twice when blocked (once for midi and once for midi-syntex), though I guess we could add mitigations for that by also checking whether the "midi" permission is set to BLOCK.

Alternatively you could use the callback function you can set on actions to automatically set the "midi" permission when "midi-sysex" was granted. The callback is called here: https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/browser/modules/PermissionUI.jsm#298.

Intuitively, the former sounds like the better solution to me.

In any case you will have to make the permissionKey getter variably return "midi" or "midi-sysex" based on whether this.isSysexPerm is true.

I hope that helps, but maybe I'm missing something. What do you think?

Comment 120

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Johann Hofmann [:johannh] from comment #119)
> I hope that helps, but maybe I'm missing something. What do you think?

The first solution sounds fine. I just wanted to make sure there wasn't something integrated into the permissions UI that I might be missing. The getter is already fixed up in my local patches. I'll get to work on that, thanks!

Comment 121

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok. This permissions stuff is just the gift that keeps on taking.

Since our internal permissions implementation is basically binary (i.e. ALLOW or BLOCK keyed off a string), we're going to have both "midi" and "midi-sysex" permissions internally. While both permissions are related to the same API, "midi-sysex" is considered stronger than "midi". We don't have a good way to grant 2 permissions at once, so the logic surrounding "midi"/"midi-sysex" will have to happen before the permissions prompt, as mentioned in Comment 119 and Comment 120.

However, the actual W3C Permissions API considers "midi" to be a single permission with a "sysex" option, so on the Permission API side, we're going to have to make some special cases to spit out a MidiPermissionDescriptor (https://w3c.github.io/permissions/#midi) with the correct boolean value based on whether we have "midi" or "midi-sysex" internally.

I /think/ that's the total scope of the permission implementation though, so hopefully I can use that as a checklist for getting this done and landed finally.

Comment 122

[Johann Hofmann [:johannh]]

Sorry for the late suggestion, but you can of course also add another permission state for sysex to the "midi" permission (like ALLOW but rather ALLOW_SYSEX). The permission manager can store any integer value. You might have to do a little adjusting in the UI code, but I'm happy to help with that. Does that sound like a better option to you?

Comment 123

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I've got the dual midi and midi-sysex permissions working (hopefully back into review today!), and I'd rather not start adding states others could accidentally use in their permissions code. I think we'll be fine with what we've got, that was more just an exasperated "how did I miss this?" post to remind myself to take care of it. :)

Comment 124

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Since Permission API stuff will require another round of WebIDL reviews and some serious test updates, and won't be a huge issue since it'll only be on nightly, I'm moving that to Bug 1437171. That way we can at least get this landed first.

Comment 125

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1201590 - Add icon for WebMIDI Permissions;

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

Comment 126

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

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

Comment 127

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8805211 [details]
Bug 1201590 - WebMIDI IPDL files;

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

Comment 128

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929646 [details]
Bug 1201590 - WebMIDI Utility classes

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

Comment 129

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929647 [details]
Bug 1201590 - WebMIDI Content implementation

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

Comment 130

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929648 [details]
Bug 1201590 - WebMIDI Parent/PBackground implementation

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

Comment 131

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

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

Comment 132

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8935660 [details]
Bug 1201590 - WebMIDI Mochitests

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

Comment 133

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

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

Comment 134

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8935660 [details]
Bug 1201590 - WebMIDI Mochitests

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

Comment 135

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8950442 [details]
Bug 1201590 - Add icon for WebMIDI Permissions;

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

Comment 136

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

https://reviewboard.mozilla.org/r/200930/#review214268

> Where are these used? Do they have to be in this commit?

Removed. They were brought in a long time ago but never hooked up, still need to figure out if/where we actually need security telemetry on this.

Comment 137

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

:johannh, do you have time to look at this review soon? Just want to get this landed before I get pulled off onto site isolation stuff. :)

Comment 138

[Johann Hofmann [:johannh]]

Yup, sorry, it's been on my list the entire week. I'll look at it now.

Comment 139

[Johann Hofmann [:johannh]]

Comment on attachment 8950442 [details]
Bug 1201590 - Add icon for WebMIDI Permissions;

https://reviewboard.mozilla.org/r/219702/#review226728

r=me with the comments addressed. Thank you!

::: browser/themes/shared/jar.inc.mn:77
(Diff revision 2)
>    skin/classic/browser/notification-icons/popup.svg                         (../shared/notification-icons/popup.svg)
>    skin/classic/browser/notification-icons/popup-subitem.svg                 (../shared/notification-icons/popup-subitem.svg)
>    skin/classic/browser/notification-icons/screen-blocked.svg                (../shared/notification-icons/screen-blocked.svg)
>    skin/classic/browser/notification-icons/screen.svg                        (../shared/notification-icons/screen.svg)
>    skin/classic/browser/notification-icons/update.svg                        (../shared/notification-icons/update.svg)
> +  skin/classic/browser/notification-icons/midi.svg                        (../shared/notification-icons/midi.svg)

tiny nit: align the "(../shared/..." string with the ones above

::: browser/themes/shared/notification-icons.inc.css:120
(Diff revision 2)
>  
> +.midi-icon {
> +  list-style-image: url(chrome://browser/skin/notification-icons/midi.svg);
> +}
> +
> +.popup-notification-icon[popupid="midi"] {

nit: just combine this selector with the one above

::: browser/themes/shared/notification-icons/midi.svg:4
(Diff revision 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/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16">

Please add these attributes:

fill="context-fill" fill-opacity="context-fill-opacity"

to the <svg> tag. This is for coloring the icon correctly depending on its environment, e.g. dark theme.

Comment 140

[Johann Hofmann [:johannh]]

Comment on attachment 8950442 [details]
Bug 1201590 - Add icon for WebMIDI Permissions;

https://reviewboard.mozilla.org/r/219702/#review226756

::: browser/themes/shared/notification-icons.inc.css:116
(Diff revision 2)
>  
>  .screen-icon.blocked-permission-icon {
>    list-style-image: url(chrome://browser/skin/notification-icons/screen-blocked.svg);
>  }
>  
> +.midi-icon {

I just noticed this while trying out your patch: You need to add another selector for .midi-sysex-icon here to have the icon correctly show up in the identity popup for midi-sysex permissions. :)

Comment 141

[Johann Hofmann [:johannh]]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

https://reviewboard.mozilla.org/r/200930/#review226730

Tentative r=me with comments fixed, there's still some stuff to do but I feel like you don't necessarily need another review. Feel free to flag me for review again if you're unsure about anything.

Note that these prompts have super convenient tests, you just have to add your prompt to https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/browser/modules/test/browser/browser_PermissionUI_prompts.js#24 the same way storage does it, and it will automatically apply the tests we have for our permission prompts to the one you just added.

There's one catch: because we're mocking the permission requests you will need to add the "types" field to the mocking code:
https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/browser/modules/test/browser/head.js#172

It should only be a matter of adding somethink like:

  let type = {
    options: [],
    QueryInterface: XPCOMUtils.generateQI([Ci.nsIContentPermissionType]),
  };
  let types = Cc["@mozilla.org/array;1"].createInstance(Components.interfaces.nsIMutableArray);
  types.appendElement(type);

to makeMockPermissionRequest. (This way you could also add testing for the sysex permission by conditionally including it in the options array).

I will not block review on the test, but if you don't add one here please file a follow-up bug.

Thank your for all the work, this looks great already!

::: browser/locales/en-US/chrome/browser/browser.properties:942
(Diff revision 8)
>  certImminentDistrust.message = The security certificate in use on this website will no longer be trusted in a future release. For more information, visit https://wiki.mozilla.org/CA/Upcoming_Distrust_Actions
> +
> +midi.Allow.label = Allow
> +midi.Allow.accesskey = A
> +midi.DontAllow.label = Don’t Allow
> +midi.DontAllow.accesskey = D

For some reason the usual accesskey for this label seems to be "n", so I'd suggest using that instead.

::: browser/locales/en-US/chrome/browser/browser.properties:944
(Diff revision 8)
> +midi.Allow.label = Allow
> +midi.Allow.accesskey = A
> +midi.DontAllow.label = Don’t Allow
> +midi.DontAllow.accesskey = D
> +midi.remember=Remember this decision
> +# LOCALIZATION NOTE (STRING): %S is the name of the file URL (file://...) requesting MIDI access

I think you need to replace (STRING) with (midi.shareWithFile.message). This obviously applies to the notes below, too :)

::: browser/locales/en-US/chrome/browser/browser.properties:947
(Diff revision 8)
> +midi.DontAllow.accesskey = D
> +midi.remember=Remember this decision
> +# LOCALIZATION NOTE (STRING): %S is the name of the file URL (file://...) requesting MIDI access
> +midi.shareWithFile.message = Share MIDI devices with file %S?
> +# LOCALIZATION NOTE (STRING): %S is the name of the site URL (https://...) requesting MIDI access
> +midi.shareWithSite.message = Share MIDI devices with site %S?

"Share MIDI devices with site mozilla.org?" doesn't sound particularly great to me, mostly because of the kind of superflous "site". Is the phrasing we usually use not suitable for your use case?

https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/browser/locales/en-US/chrome/browser/browser.properties#583

"Will you allow %S to access your MIDI devices?"

If it's not, can we at least drop the "site" from that string? :)

For files, you could go with what geolocation does and just say "this local file".

::: browser/modules/PermissionUI.jsm:720
(Diff revision 8)
> +  get permissionKey() {
> +    return this.permName;
> +  },
> +
> +  get popupOptions() {
> +    // TODO We need a security/permissions explanation URL for this

Please reference bug 1433235 here.

::: browser/modules/PermissionUI.jsm:749
(Diff revision 8)
> +
> +  get anchorID() {
> +    return "";
> +  },
> +
> +  get message() {

Sorry, in the meantime we introduced a new way to format these messages to make the hostname show up bold (which is helpful in case iframes request permissions). So the standard way of formatting permission prompt messages looks like this now:

https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/browser/modules/PermissionUI.jsm#463

You basically split up your translation string and pass three parts to PopupNotifications, the start, the bold part (the hostname) and the end.

It's a little verbose and arcane and we're working on making a utility function for that in bug 1435160.

I'm not requiring you to change it in this bug, but if you don't please make a follow-up bug for it.

::: browser/modules/PermissionUI.jsm:776
(Diff revision 8)
> +      },
> +      {
> +        label: gBrowserBundle.GetStringFromName("midi.DontAllow.label"),
> +        accessKey: gBrowserBundle.GetStringFromName("midi.DontAllow.accesskey"),
> +        action: Ci.nsIPermissionManager.DENY_ACTION
> +      }];

nit: indent by -1 level

::: browser/modules/SitePermissions.jsm:654
(Diff revision 8)
>    "canvas": {
>    },
> +
> +  "midi": {
> +    exactHostMatch: true,
> +    states: [ SitePermissions.ALLOW, SitePermissions.BLOCK ],

I don't think including this field is correct, there's an UNKNOWN/PROMPT state for midi (called "Always Ask" in the UI). Including this field makes the default show up as "Allow" in page info, which is incorrect. It should work correctly if you just remove "states".

::: browser/modules/SitePermissions.jsm:659
(Diff revision 8)
> +    states: [ SitePermissions.ALLOW, SitePermissions.BLOCK ],
> +  },
> +
> +  "midi-sysex": {
> +    exactHostMatch: true,
> +    states: [ SitePermissions.ALLOW, SitePermissions.BLOCK ],

See above, please remove this field :)

Comment 142

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

https://reviewboard.mozilla.org/r/200930/#review226730

> "Share MIDI devices with site mozilla.org?" doesn't sound particularly great to me, mostly because of the kind of superflous "site". Is the phrasing we usually use not suitable for your use case?
> 
> https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/browser/locales/en-US/chrome/browser/browser.properties#583
> 
> "Will you allow %S to access your MIDI devices?"
> 
> If it's not, can we at least drop the "site" from that string? :)
> 
> For files, you could go with what geolocation does and just say "this local file".

Oh, yeah, that wording is fine, I'll change it to be in line with the rest of the permissions. I copied these strings when I first started the project in late 2014-early 2015, so they're really dated and I just hadn't checked to update the wording.

> Sorry, in the meantime we introduced a new way to format these messages to make the hostname show up bold (which is helpful in case iframes request permissions). So the standard way of formatting permission prompt messages looks like this now:
> 
> https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/browser/modules/PermissionUI.jsm#463
> 
> You basically split up your translation string and pass three parts to PopupNotifications, the start, the bold part (the hostname) and the end.
> 
> It's a little verbose and arcane and we're working on making a utility function for that in bug 1435160.
> 
> I'm not requiring you to change it in this bug, but if you don't please make a follow-up bug for it.

No prob, I fixed it up to look like geolocation, with the extra parsing for the Sysex permission difference.

Comment 143

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8755610 [details]
Bug 1201590 - WebMIDI WebIDL files;

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

Comment 144

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8805211 [details]
Bug 1201590 - WebMIDI IPDL files;

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

Comment 145

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929646 [details]
Bug 1201590 - WebMIDI Utility classes

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

Comment 146

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929647 [details]
Bug 1201590 - WebMIDI Content implementation

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

Comment 147

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929648 [details]
Bug 1201590 - WebMIDI Parent/PBackground implementation

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

Comment 148

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8929649 [details]
Bug 1201590 - WebMIDI device access permissions prompt implementation

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

Comment 149

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8935660 [details]
Bug 1201590 - WebMIDI Mochitests

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

Comment 150

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8950442 [details]
Bug 1201590 - Add icon for WebMIDI Permissions;

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

Comment 151

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8935660 [details]
Bug 1201590 - WebMIDI Mochitests

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

Comment 152

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8950442 [details]
Bug 1201590 - Add icon for WebMIDI Permissions;

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

Comment 153

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba8fc92e202f
WebMIDI WebIDL files; r=baku
https://hg.mozilla.org/integration/autoland/rev/608f52e1da69
WebMIDI IPDL files; r=baku
https://hg.mozilla.org/integration/autoland/rev/6464e4c6f27f
WebMIDI Utility classes; r=baku,padenot
https://hg.mozilla.org/integration/autoland/rev/63de8ef246cc
WebMIDI Content implementation; r=baku,padenot
https://hg.mozilla.org/integration/autoland/rev/e908708fda6b
WebMIDI Parent/PBackground implementation; r=baku,padenot
https://hg.mozilla.org/integration/autoland/rev/a84ef21b6b36
WebMIDI device access permissions prompt implementation; r=baku,johannh
https://hg.mozilla.org/integration/autoland/rev/21fc5c9ae628
WebMIDI Mochitests; r=baku,padenot
https://hg.mozilla.org/integration/autoland/rev/8b156da986ee
Add icon for WebMIDI Permissions; r=johannh

Comment 154

[Cosmin Sabou [:CosminS]]

Follow up for the CSS issues:
https://hg.mozilla.org/integration/autoland/rev/e81cd3e834c34338e60ee8f6af3981cd04fb503e

Comment 155

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ba8fc92e202f
https://hg.mozilla.org/mozilla-central/rev/608f52e1da69
https://hg.mozilla.org/mozilla-central/rev/6464e4c6f27f
https://hg.mozilla.org/mozilla-central/rev/63de8ef246cc
https://hg.mozilla.org/mozilla-central/rev/e908708fda6b
https://hg.mozilla.org/mozilla-central/rev/a84ef21b6b36
https://hg.mozilla.org/mozilla-central/rev/21fc5c9ae628
https://hg.mozilla.org/mozilla-central/rev/8b156da986ee

Comment 156

[Daniele "Mte90" Scasciafratte]

There is a demo page for test this API?
Actually I see a lot of stuff merged but not a page to test it in the browser.

Comment 157

[Paul Adenot (:padenot)]

This is just the DOM bindings, it's not really usable for now.