1026350 - [Stingray] InputPort API

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Joe Cheng [:jcheng] (please needinfo)]

This is a user story tracking additional input ports support on the webAPI level for TVs
- Select input sources
- HDMI input support
- Composite video/audio input
- Display port (DVI)
- Support renaming inputs

Comment 1

[Marco Chen [:mchen]]

Currently we sent a proposal to a small group for discussion first and will not aim to V2.1 so put to backlog .

Comment 2

[Shelly Lin [:shelly]]

Attachment: Part1: Webidls and files of InputPort API

Hi Ehsan, (here comes the review process...)
I think this InputPort API would be much simpler than the TV Manager API, and most of the hardware-dependent works will be taken care by partner. This patch is so far a basic skeleton of InputPort API, next would be the access of InputPortManager in Navigator and test cases.

Speaking of test case, should I create InputPort services like TV Manager does? (I'd assume no since the scale of InputPort API is relative small) Or simply just let partner implement the real work in each InputPort component?

If we go with later, I think we can create some FakeInputPortManager, FakeAVInputPort,...for testing purpose?

Comment 3

[Shelly Lin [:shelly]]

Attachment: All changes

Comment 4

[Shelly Lin [:shelly]]

Attachment: [All-In-One] Implementation of InputPort API

This is the first runnable version of InputPort API. Including implementation of interfaces, input port service, and events handling.

I've also created a fake input port service, which creates three input ports, and simulates a disconnect event on the first one in the later attached test case (test_inputports.html).

* Please note that this version hasn't implement any permission check yet.

Comment 5

[Shelly Lin [:shelly]]

Attachment: test_inputports.html

Comment 7

[Tatsuya Fujimoto]

- current patch doesn't include IPC between b2g process and content process
 -> we have implemented current patch to our platform and it is basically working well. 
(our daemon handles multiple connections from b2g/content processes, not preferred solution).
We would like to get updated patch if possible.

- We would like to have a way to know hardware port number.
  e.g. portId is 0 for 'HDMI1' input, 1 for 'HDMI2' input...
  
  attribute id seems not represent simply hardware port number which is our expectation. Correct?

Comment 8

[Shelly Lin [:shelly]]

(In reply to Tatsuya Fujimoto from comment #7)
> - current patch doesn't include IPC between b2g process and content process
>  -> we have implemented current patch to our platform and it is basically
> working well. 
> (our daemon handles multiple connections from b2g/content processes, not
> preferred solution).
> We would like to get updated patch if possible.
> 
Allow me to confirm with the request, are you expecting the patch to be implemented with IPC or without IPC?

> - We would like to have a way to know hardware port number.
>   e.g. portId is 0 for 'HDMI1' input, 1 for 'HDMI2' input...
Are you saying this portId comes from driver, doesn't equal to the "number" users see from the Settings menu?
>   
>   attribute id seems not represent simply hardware port number which is our
> expectation. Correct?
If the purpose is just for displaying available ports to users in Settings menu, we would suggest you managing your own entries of ports and their associated names (e.g. a map of key=id, value=name), this is for the capability of displaying something like "HDMI-Chromecast" (if this port is currently connected to a Chromecast) in Settings menu.
However, you can simply use the attribute id to be the hardware port number, as long as they are unique and consistent to applications. Which says, the port name to display would be port.type + port.id.

Comment 9

[Shelly Lin [:shelly]]

Attachment: [All-In-One] Implementation of InputPort API (clean out whitespaces)

Comment 10

[Tatsuya Fujimoto]

(In reply to Shelly Lin [:shelly] from comment #8)
> (In reply to Tatsuya Fujimoto from comment #7)
> > - current patch doesn't include IPC between b2g process and content process
> >  -> we have implemented current patch to our platform and it is basically
> > working well. 
> > (our daemon handles multiple connections from b2g/content processes, not
> > preferred solution).
> > We would like to get updated patch if possible.
> > 
> Allow me to confirm with the request, are you expecting the patch to be
> implemented with IPC or without IPC?
> 

We are expecting the patch with IPC between b2g and content processes.

> > - We would like to have a way to know hardware port number.
> >   e.g. portId is 0 for 'HDMI1' input, 1 for 'HDMI2' input...
> Are you saying this portId comes from driver, doesn't equal to the "number"
> users see from the Settings menu?
> >   
> >   attribute id seems not represent simply hardware port number which is our
> > expectation. Correct?
> If the purpose is just for displaying available ports to users in Settings
> menu, we would suggest you managing your own entries of ports and their
> associated names (e.g. a map of key=id, value=name), this is for the
> capability of displaying something like "HDMI-Chromecast" (if this port is
> currently connected to a Chromecast) in Settings menu.
> However, you can simply use the attribute id to be the hardware port number,
> as long as they are unique and consistent to applications. Which says, the
> port name to display would be port.type + port.id.

OK, now I understand we can use 'id' for our purpose (just to show port number to customers).

Comment 11

[Shelly Lin [:shelly]]

(In reply to Tatsuya Fujimoto from comment #10)
> (In reply to Shelly Lin [:shelly] from comment #8)
> > (In reply to Tatsuya Fujimoto from comment #7)
> > > - current patch doesn't include IPC between b2g process and content process
> > >  -> we have implemented current patch to our platform and it is basically
> > > working well. 
> > > (our daemon handles multiple connections from b2g/content processes, not
> > > preferred solution).
> > > We would like to get updated patch if possible.
> > > 
> > Allow me to confirm with the request, are you expecting the patch to be
> > implemented with IPC or without IPC?
> > 
> 
> We are expecting the patch with IPC between b2g and content processes.
> 
After consulting with the lead of our API team, for the first phase, we will stay at the non-IPC version, but you are welcome to extend it with IPC implementation, just like how we work out for the TV Manager API, and please feel free to post your questions or patches during the implementation, thank you :)

Comment 12

[saegusa.fumiaki.dev]

(In reply to Shelly Lin [:shelly] from comment #8)
> However, you can simply use the attribute id to be the hardware port number,
> as long as they are unique and consistent to applications. Which says, the
> port name to display would be port.type + port.id.

Your implementation seems to expect that port.id is unique against all other port.
For example, following method don't use port.type and use only port.id
------------------------------------------------------------------
/* virtual */ NS_IMETHODIMP
InputPortListener::NotifyConnectionChanged(const nsAString& aPortId,
                                           bool aIsConnected)
------------------------------------------------------------------
It means that we can't use port.id as port number. (to distinguish AV1 / HDMI1)
Could you please consider to add new properties for port number? 

Or do we have to always count up same input.type?, like
------------------------------------------------------
function handleHDMI2() {
　　　inputPortMgr.getInputPorts().then(
      function(aPorts) {
        var portNum = 0;
        for (var i=0; i < aPorts.length; i++) {
          if (aPorts.type == 'hdmi') {
             if (++portNum == 2){
               // handle aPorts[i]
             }
          }
      },
    );
------------------------------------------------------

Comment 13

[Shelly Lin [:shelly]]

Hi Fumiaki-san,

Oh I see the problem here, and yes you're correct, the port.id should be unique against all other ports.
However, I would suggest not to add a new attribute for the port number, because from the users' point of view, the port number is just for display, so the "query" and "mapping" of port and its number should leave to system app.

For example, query and store your own dictionary of ports in GetPort(), and do
function handleHDMI2() {
  var port = GetPort("hdmi", 2);on handleHDMI2() {on handleHDMI2() {
  var port = GetPort("hdmi", 2);
}
  var port = GetPort("hdmi", 2);
}
  // 
}

Comment 14

[Shelly Lin [:shelly]]

Gosh the layout has somehow messed up!

It should be

function handleHDMI2() {
  var port = GetPort("hdmi", 2);
  // Do something to the port.
}

Comment 15

[saegusa.fumiaki.dev]

(In reply to Shelly Lin [:shelly] from comment #13)
> Hi Fumiaki-san,
> 
> Oh I see the problem here, and yes you're correct, the port.id should be
> unique against all other ports.
> However, I would suggest not to add a new attribute for the port number,
> because from the users' point of view, the port number is just for display,
> so the "query" and "mapping" of port and its number should leave to system
> app.
> 

OK, I understood your points.
We'll count up the same port.type to know port number.

Comment 16

[James Cheng[:JamesCheng]]

Comment on attachment 8545714 [details] [diff] [review]
[All-In-One] Implementation of InputPort API (clean out whitespaces)

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

::: dom/inputport/InputPortServiceCallbacks.cpp
@@ +61,5 @@
> +    if (NS_WARN_IF(!portData)) {
> +      continue;
> +    }
> +
> +    InputPortData* data = static_cast<InputPortData*>(portData.get());

use data var for below code.

::: dom/webidl/InputPort.webidl
@@ +8,5 @@
> +
> + interface InputPort : EventTarget {
> +   readonly attribute DOMString id;
> +   readonly attribute InputPortType type;
> +   readonly attribute MediaStream stream;

use "MediaStream?" since "MediaStream" cannot pass null for return value

Comment 17

[James Cheng[:JamesCheng]]

Attachment: Part1 - InputPort API.patch

Rebase the previous patch and add permission check and only allow certified app to use this API.

Comment 18

[James Cheng[:JamesCheng]]

Attachment: [WIP] Test-Case.patch

Add mochitest and xpcshell test, still work on the test case.

Comment 19

[James Cheng[:JamesCheng]]

Comment on attachment 8573069 [details] [diff] [review]
Part1 - InputPort API.patch

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

Hi Ehsan,
Could you please help to review this patch for InputPort API?
Thanks.

Comment 20

[Shelly Lin [:shelly]]

(In reply to James Cheng[:JamesCheng] from comment #16)
> Comment on attachment 8545714 [details] [diff] [review]
> [All-In-One] Implementation of InputPort API (clean out whitespaces)
> 
> Review of attachment 8545714 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/inputport/InputPortServiceCallbacks.cpp
> @@ +61,5 @@
> > +    if (NS_WARN_IF(!portData)) {
> > +      continue;
> > +    }
> > +
> > +    InputPortData* data = static_cast<InputPortData*>(portData.get());
> 
> use data var for below code.
> 
> ::: dom/webidl/InputPort.webidl
> @@ +8,5 @@
> > +
> > + interface InputPort : EventTarget {
> > +   readonly attribute DOMString id;
> > +   readonly attribute InputPortType type;
> > +   readonly attribute MediaStream stream;
> 
> use "MediaStream?" since "MediaStream" cannot pass null for return value
According to our previous discussion and the spec, the stream object is not nullable, please change it back.

Comment 21

[James Cheng[:JamesCheng]]

Comment on attachment 8573069 [details] [diff] [review]
Part1 - InputPort API.patch

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

cancel review for Shelly's comment.

Comment 22

[James Cheng[:JamesCheng]]

Attachment: Part1 - Inputport API.patch

According to Shelly's comment. Should not return null. 

Hi Ehsan,
Could you please help to review this patch for InputPort API?
Thanks.

Comment 23

[James Cheng[:JamesCheng]]

Attachment: Part2 - Test-Case.patch

Add test case for inputport API.

Hi Ehsan,
Please kindly help to review this patch as well.
Thanks.

Comment 24

[(no longer active)]

I will probably get to these reviews some time next week.

Comment 25

[James Cheng[:JamesCheng]]

Thanks you, and I will run the test case in parallel.

Comment 26

[James Cheng[:JamesCheng]]

Attachment: Part1 - Inputport API.patch

Update the patch for fixing the compile warning.

Comment 27

[James Cheng[:JamesCheng]]

Attachment: Part1 - Inputport API.patch

Update for test case fail.

Comment 28

[James Cheng[:JamesCheng]]

Attachment: Part1 - Inputport API.patch

Update the dom/tests/mochitest/general/test_interfaces.html since exposing new webidls.

Attach the try result as reference.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b608b072dfc3

Comment 29

[(no longer active)]

Comment on attachment 8575021 [details] [diff] [review]
Part1 - Inputport API.patch

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

I have a couple of comments on the WebIDL before I move to reviewing the patches.  Can you please address them and attach a new patch?  Thanks!

::: dom/webidl/AVInputPort.webidl
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[Pref="dom.inputport.enabled", CheckPermissions="inputport", AvailableIn=CertifiedApps]
> + interface AVInputPort : InputPort {

These three interfaces do not provide any additional methods.  Having them now is fine if you are planning to add properties to them in the future, but we should either have a type property on InputPort, or the separate interfaces, not both.  So, if you're planning to keep the separate interfaces, you should drop the type member.  Then, you can use |port instanceof AVInputPort| and similar if you need to detect what type of port a given object is.

::: dom/webidl/Navigator.webidl
@@ +413,5 @@
>  };
>  
> +partial interface Navigator {
> +  [Pref="dom.inputport.enabled", CheckPermissions="inputport", AvailableIn=CertifiedApps]
> +  readonly attribute InputPortManager? inputPortManager;

Under what circumstances can navigator.inputPortManager be null?

Comment 30

[(no longer active)]

James, I'm sorry but I will be very busy for the next two weeks or so working on a very high priority project, so I'm planning to defer the reviews here until then.  Is that OK, or is this high priority?

Thanks!

Comment 31

[James Cheng[:JamesCheng]]

Attachment: Part1 - Inputport API.patch

Hi Ehsan,
I update the patch.
I remove the "type" property and remain the separated interface for flexibility and scalability.

navigator.inputPortManager will be null when the InputPortManager::Init() fails for the error handling. 

already_AddRefed<InputPortManager>
InputPortManager::Create(nsPIDOMWindow* aWindow)
{
  nsRefPtr<InputPortManager> manager = new InputPortManager(aWindow);
  return manager->Init() ? manager.forget() : nullptr;
}
 
bool
InputPortManager::Init()
{
  mInputPortService = InputPortServiceFactory::AutoCreateInputPortService();
  NS_ENSURE_TRUE(mInputPortService, false);
 
  nsCOMPtr<nsIInputPortServiceCallback> callback =
    new InputPortServicePortGetterCallback(this);
  nsresult rv = mInputPortService->GetInputPorts(callback);
  NS_ENSURE_SUCCESS(rv, false);
 
  return true;
}


It is not high priority, so please help to review or feedback when you have free time.

Thanks.

Comment 32

[James Cheng[:JamesCheng]]

Attachment: Part2 - Test-Case.patch

Update the test case.

Comment 33

[(no longer active)]

Instead of making navigator.inputPortManager nullable, I think it's better to throw when the initialization fails.

Comment 34

[James Cheng[:JamesCheng]]

Attachment: Part1 - Inputport API.patch

Update patch for "throw" instead of "nullable" in navigator.inputPortManage.

Comment 35

[James Cheng[:JamesCheng]]

Attachment: Part1 - Inputport API.patch

Update the patch(removing the nullable "?")

Comment 36

[James Cheng[:JamesCheng]]

Attach the try result as reference.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=610d44877d46

Comment 37

[James Cheng[:JamesCheng]]

Attachment: Part-1-DOM-API-WebIDL-Inputport API

Comment 38

[James Cheng[:JamesCheng]]

Attachment: Part-2-Inputport-Service.

Comment 39

[James Cheng[:JamesCheng]]

Attachment: Part-3-Test-Case.

Comment 40

[James Cheng[:JamesCheng]]

separate the previous patches into 3 parts
- webidl part, service idl part and test case part.
update the patch with 
1. Rebase.
2. "override and final" instead of MOZ_OVERRIDE and MOZ_FINAL.
3. virtual JSObject* WrapObject(JSContext* cx, JS::Handle<JSObject*> aGivenProto) = 0;
   instead of virtual JSObject* WrapObject(JSContext* cx) = 0;  

Hi Ehsan,
please help to review these patches.

Thank you very much.

Comment 41

[James Cheng[:JamesCheng]]

Attach the try result as reference.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdcad99ccac9

Comment 42

[James Cheng[:JamesCheng]]

Hi Ehsan,
Could you please help to schedule the review process?
Or is there any suggested reviewer if you are very busy right now.

Thanks.

Comment 43

[(no longer active)]

Andrea, do you think you can help with these reviews by any chance?

Comment 44

[Andrea Marchesini [:baku]]

Comment on attachment 8582940 [details] [diff] [review]
Part-1-DOM-API-WebIDL-Inputport API

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

there are 2 main comments here:

1. are we sure we want different classes and not a enum of types?

2. reviewing this patch is almost impossible because it depends on other patches. Usually when we submit a patch just for WebIDL bindings, all the methods are stubbed. The patch must be self-contained.
So, up to you: submit a patch containing just the web bindings and no external stuff OR, I'm fine if you want to merge this patch with the second one and submit all in once.

I'm happy to review this code again. Thanks.

::: dom/base/Navigator.cpp
@@ +40,5 @@
>  #include "mozilla/dom/ServiceWorkerContainer.h"
>  #include "mozilla/dom/Telephony.h"
>  #include "mozilla/dom/Voicemail.h"
>  #include "mozilla/dom/TVManager.h"
> +#include "mozilla/dom/InputPortManager.h"

alphabetic order.

@@ +1634,5 @@
> +    if (!mWindow) {
> +      aRv.Throw(NS_ERROR_FAILURE);
> +      return nullptr;
> +    }
> +    mInputPortManager = InputPortManager::Create(mWindow);

mInputPortManager = InputPortManager::Create(mWindow, aRv);
return mInputPortManager.forget();

::: dom/inputport/InputPort.cpp
@@ +32,5 @@
> +{
> +}
> +
> +bool
> +InputPort::Init(nsIInputPortData* aData, nsIInputPortListener* aListener)

return the real nsresult and not a boolean.

@@ +50,5 @@
> +  }
> +
> +  aData->GetConnected(&mIsConnected);
> +
> +  mInputPortListener = static_cast<InputPortListener*>(aListener);

I don't like this casting... who calls Init()? Can it pass a InputPortListener instead?

@@ +80,5 @@
> +}
> +
> +void
> +InputPort::NotifyConnectionChanged(bool aIsConnected)
> +{

MOZ_ASSERT(mIsConnected != aIsConnected);

@@ +96,5 @@
> +{
> +  aId = mId;
> +}
> +
> +already_AddRefed<DOMMediaStream>

DOMMediaStream*

@@ +99,5 @@
> +
> +already_AddRefed<DOMMediaStream>
> +InputPort::Stream() const
> +{
> +  nsRefPtr<DOMMediaStream> stream = mStream;

return mStream;

::: dom/inputport/InputPort.h
@@ +43,5 @@
> +
> +protected:
> +  explicit InputPort(nsPIDOMWindow* aWindow);
> +
> +  ~InputPort();

virtual

::: dom/inputport/InputPortManager.cpp
@@ +48,5 @@
> +
> +bool
> +InputPortManager::Init()
> +{
> +  mInputPortService = InputPortServiceFactory::AutoCreateInputPortService();

if (NS_WARN_IF(!mInputPortService)) {
  return NS_ERROR_FAILURE:
}

@@ +53,5 @@
> +  NS_ENSURE_TRUE(mInputPortService, false);
> +
> +  nsCOMPtr<nsIInputPortServiceCallback> callback =
> +    new InputPortServicePortGetterCallback(this);
> +  nsresult rv = mInputPortService->GetInputPorts(callback);

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +56,5 @@
> +    new InputPortServicePortGetterCallback(this);
> +  nsresult rv = mInputPortService->GetInputPorts(callback);
> +  NS_ENSURE_SUCCESS(rv, false);
> +
> +  return true;

return NS_OK;

@@ +72,5 @@
> +  return InputPortManagerBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +void
> +InputPortManager::RejectPendingGetInputPortsPromises(nsresult aRv)

where do you use this method?

@@ +91,5 @@
> +
> +
> +nsresult
> +InputPortManager::SetInputPorts(const nsTArray<nsRefPtr<InputPort>>& aPorts)
> +{

It's a bit hard to review this part without InputPortService, but can you assert this?

::: dom/inputport/InputPortManager.h
@@ +9,5 @@
> +
> +#include "nsWrapperCache.h"
> +
> +class nsIInputPortService;
> +

class nsPIDOMWindow;

@@ +17,5 @@
> +class Promise;
> +class InputPort;
> +
> +class InputPortManager final : public nsISupports
> +                                 , public nsWrapperCache

indentation

@@ +23,5 @@
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(InputPortManager)
> +
> +  static already_AddRefed<InputPortManager> Create(nsPIDOMWindow* aWindow);

static already_AddRefed<InputPortManager> Create(nsPIDOMWindow* aWindow, ErrorResult& aRv);

and use aRv in Navigator.cpp.

@@ +44,5 @@
> +  explicit InputPortManager(nsPIDOMWindow* aWindow);
> +
> +  ~InputPortManager();
> +
> +  bool Init();

return the proper error code.

::: dom/inputport/moz.build
@@ +12,5 @@
> +    'InputPortManager.h',
> +]
> +
> +EXPORTS += [
> +    'FakeInputPortService.h',

where is this file?

@@ +30,5 @@
> +    'InputPortData.cpp',
> +    'InputPortListeners.cpp',
> +    'InputPortManager.cpp',
> +    'InputPortServiceCallbacks.cpp',
> +    'InputPortServiceFactory.cpp',

A lot of these files are missing from this patch.

::: dom/webidl/HDMIInputPort.webidl
@@ +5,5 @@
> + */
> +
> +[Pref="dom.inputport.enabled", CheckPermissions="inputport", AvailableIn=CertifiedApps]
> + interface HDMIInputPort : InputPort {
> + };

instead having HDMIInputPort and DisplayPortInputPort and AVInputPort, can you add an enum and a type?

Something like:

enum InputPortType {
 "displayPort",
 "AVI",
 "HDMI"
};

interface InputPort {
  readonly attribute InputPortType type;
  ...
}

::: dom/webidl/InputPort.webidl
@@ +2,5 @@
> +/* 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 some URL of the spec? or a wiki page? or something useful.

Comment 45

[James Cheng[:JamesCheng]]

Attachment: Part-1-Inputport-API-implementation.

Comment 46

[James Cheng[:JamesCheng]]

Attachment: Part-2-Test-Case.patch

Comment 47

[James Cheng[:JamesCheng]]

(In reply to Andrea Marchesini (:baku) from comment #44)
> Comment on attachment 8582940 [details] [diff] [review]
> Part-1-DOM-API-WebIDL-Inputport API
> 
> Review of attachment 8582940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> there are 2 main comments here:
> 
> 1. are we sure we want different classes and not a enum of types?
> 
> 2. reviewing this patch is almost impossible because it depends on other
> patches. Usually when we submit a patch just for WebIDL bindings, all the
> methods are stubbed. The patch must be self-contained.
> So, up to you: submit a patch containing just the web bindings and no
> external stuff OR, I'm fine if you want to merge this patch with the second
> one and submit all in once.
> 
> I'm happy to review this code again. Thanks.
> 
Sorry for that, I've merged the patches.
Please help to review again and see my comments below.
Thanks.
> ::: dom/base/Navigator.cpp
> @@ +40,5 @@
> >  #include "mozilla/dom/ServiceWorkerContainer.h"
> >  #include "mozilla/dom/Telephony.h"
> >  #include "mozilla/dom/Voicemail.h"
> >  #include "mozilla/dom/TVManager.h"
> > +#include "mozilla/dom/InputPortManager.h"
> 
> alphabetic order.
> 
Fixed.
> @@ +1634,5 @@
> > +    if (!mWindow) {
> > +      aRv.Throw(NS_ERROR_FAILURE);
> > +      return nullptr;
> > +    }
> > +    mInputPortManager = InputPortManager::Create(mWindow);
> 
> mInputPortManager = InputPortManager::Create(mWindow, aRv);
> return mInputPortManager.forget();
> 
Fixed.
> ::: dom/inputport/InputPort.cpp
> @@ +32,5 @@
> > +{
> > +}
> > +
> > +bool
> > +InputPort::Init(nsIInputPortData* aData, nsIInputPortListener* aListener)
> 
> return the real nsresult and not a boolean.
> 
Fixed.
> @@ +50,5 @@
> > +  }
> > +
> > +  aData->GetConnected(&mIsConnected);
> > +
> > +  mInputPortListener = static_cast<InputPortListener*>(aListener);
> 
> I don't like this casting... who calls Init()? Can it pass a
> InputPortListener instead?
> 
Since the listener is from  "attribute nsIInputPortListener inputPortListener;(idl)->GetInputPortListener"
The cast is inevitable.
> @@ +80,5 @@
> > +}
> > +
> > +void
> > +InputPort::NotifyConnectionChanged(bool aIsConnected)
> > +{
> 
> MOZ_ASSERT(mIsConnected != aIsConnected);
> 
Fixed.
> @@ +96,5 @@
> > +{
> > +  aId = mId;
> > +}
> > +
> > +already_AddRefed<DOMMediaStream>
> 
> DOMMediaStream*
> 
> @@ +99,5 @@
> > +
> > +already_AddRefed<DOMMediaStream>
> > +InputPort::Stream() const
> > +{
> > +  nsRefPtr<DOMMediaStream> stream = mStream;
> 
> return mStream;
> 
Fixed.  But I was curious why returning raw pointer is better than returning already_AddRefed<DOMMediaStream>?

> ::: dom/inputport/InputPort.h
> @@ +43,5 @@
> > +
> > +protected:
> > +  explicit InputPort(nsPIDOMWindow* aWindow);
> > +
> > +  ~InputPort();
> 
> virtual
> 
Fixed.
> ::: dom/inputport/InputPortManager.cpp
> @@ +48,5 @@
> > +
> > +bool
> > +InputPortManager::Init()
> > +{
> > +  mInputPortService = InputPortServiceFactory::AutoCreateInputPortService();
> 
> if (NS_WARN_IF(!mInputPortService)) {
>   return NS_ERROR_FAILURE:
> }
> 
Fixed.
> @@ +53,5 @@
> > +  NS_ENSURE_TRUE(mInputPortService, false);
> > +
> > +  nsCOMPtr<nsIInputPortServiceCallback> callback =
> > +    new InputPortServicePortGetterCallback(this);
> > +  nsresult rv = mInputPortService->GetInputPorts(callback);
> 
> if (NS_WARN_IF(NS_FAILED(rv))) {
>   return rv;
> }
> 
> @@ +56,5 @@
> > +    new InputPortServicePortGetterCallback(this);
> > +  nsresult rv = mInputPortService->GetInputPorts(callback);
> > +  NS_ENSURE_SUCCESS(rv, false);
> > +
> > +  return true;
> 
> return NS_OK;
> 
Fixed.
> @@ +72,5 @@
> > +  return InputPortManagerBinding::Wrap(aCx, this, aGivenProto);
> > +}
> > +
> > +void
> > +InputPortManager::RejectPendingGetInputPortsPromises(nsresult aRv)
> 
> where do you use this method?
> 
In the service code(InputPortServicePortGetterCallback).

> @@ +91,5 @@
> > +
> > +
> > +nsresult
> > +InputPortManager::SetInputPorts(const nsTArray<nsRefPtr<InputPort>>& aPorts)
> > +{
> 
> It's a bit hard to review this part without InputPortService, but can you
> assert this?
> 
Please see the new patches.
> ::: dom/inputport/InputPortManager.h
> @@ +9,5 @@
> > +
> > +#include "nsWrapperCache.h"
> > +
> > +class nsIInputPortService;
> > +
> 
> class nsPIDOMWindow;
> 
> @@ +17,5 @@
> > +class Promise;
> > +class InputPort;
> > +
> > +class InputPortManager final : public nsISupports
> > +                                 , public nsWrapperCache
> 
> indentation
> 
Fixed.
> @@ +23,5 @@
> > +public:
> > +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> > +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(InputPortManager)
> > +
> > +  static already_AddRefed<InputPortManager> Create(nsPIDOMWindow* aWindow);
> 
> static already_AddRefed<InputPortManager> Create(nsPIDOMWindow* aWindow,
> ErrorResult& aRv);
> 
> and use aRv in Navigator.cpp.
> 
Fixed.
> @@ +44,5 @@
> > +  explicit InputPortManager(nsPIDOMWindow* aWindow);
> > +
> > +  ~InputPortManager();
> > +
> > +  bool Init();
> 
> return the proper error code.
> 
Fixed.
> ::: dom/inputport/moz.build
> @@ +12,5 @@
> > +    'InputPortManager.h',
> > +]
> > +
> > +EXPORTS += [
> > +    'FakeInputPortService.h',
> 
> where is this file?
> 
I've merged the patch into one, sorry.
> @@ +30,5 @@
> > +    'InputPortData.cpp',
> > +    'InputPortListeners.cpp',
> > +    'InputPortManager.cpp',
> > +    'InputPortServiceCallbacks.cpp',
> > +    'InputPortServiceFactory.cpp',
> 
> A lot of these files are missing from this patch.
> 
> ::: dom/webidl/HDMIInputPort.webidl
> @@ +5,5 @@
> > + */
> > +
> > +[Pref="dom.inputport.enabled", CheckPermissions="inputport", AvailableIn=CertifiedApps]
> > + interface HDMIInputPort : InputPort {
> > + };
> 
> instead having HDMIInputPort and DisplayPortInputPort and AVInputPort, can
> you add an enum and a type?
> 
> Something like:
> 
> enum InputPortType {
>  "displayPort",
>  "AVI",
>  "HDMI"
> };
> 
> interface InputPort {
>   readonly attribute InputPortType type;
>   ...
> }
> 
According to comment#29, I remove the type attribute.
For keeping flexibility and scalability, I remain the derived classses for further extension.

> ::: dom/webidl/InputPort.webidl
> @@ +2,5 @@
> > +/* 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 some URL of the spec? or a wiki page? or something useful.

please refer to the following link, thanks.
https://wiki.mozilla.org/User:Shellylin/InputPort

Thank you very much.

Comment 48

[James Cheng[:JamesCheng]]

(In reply to Andrea Marchesini (:baku) from comment #44)
> Comment on attachment 8582940 [details] [diff] [review]

> > +already_AddRefed<DOMMediaStream>
> 
> DOMMediaStream*
> 
> @@ +99,5 @@
> > +
> > +already_AddRefed<DOMMediaStream>
> > +InputPort::Stream() const
> > +{
> > +  nsRefPtr<DOMMediaStream> stream = mStream;
> 
> return mStream;
> 
Hi Andrea, 
The prototype is code generation. Should I modify the prototype manually?
Thanks.

Comment 49

[Andrea Marchesini [:baku]]

> The prototype is code generation. Should I modify the prototype manually?

Yes. The generate prototype is just a guideline. In the end, InputPort keeps a mStream alive because it has a reference to it. Then you can return a raw pointer and make the code simpler.

Comment 50

[Andrea Marchesini [:baku]]

Comment on attachment 8586630 [details] [diff] [review]
Part-1-Inputport-API-implementation.

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

Can you submit a new patch with this comments fixed? Thanks
Then, why do we have this FakeInputPortService? When are we going to remove it?

::: browser/installer/package-manifest.in
@@ +231,5 @@
>  @RESPATH@/components/dom_stylesheets.xpt
>  @RESPATH@/components/dom_telephony.xpt
>  @RESPATH@/components/dom_traversal.xpt
>  @RESPATH@/components/dom_tv.xpt
> +@RESPATH@/components/dom_inputport.xpt

If inputport API is not available on desktop, this is not needed.

::: dom/base/Navigator.cpp
@@ +1649,5 @@
> +      aRv.Throw(NS_ERROR_FAILURE);
> +      return nullptr;
> +    }
> +    mInputPortManager = InputPortManager::Create(mWindow, aRv);
> +    if (!mInputPortManager) {

if (NS_WARN_IF(aRv.Failed())) {
  return nullptr;
}

::: dom/inputport/AVInputPort.cpp
@@ +21,5 @@
> +
> +/* static */ already_AddRefed<AVInputPort>
> +AVInputPort::Create(nsPIDOMWindow* aWindow,
> +                    nsIInputPortListener* aListener,
> +                    nsIInputPortData* aData)

ErrorResult& aRv)

@@ +24,5 @@
> +                    nsIInputPortListener* aListener,
> +                    nsIInputPortData* aData)
> +{
> +  nsRefPtr<AVInputPort> inputport = new AVInputPort(aWindow);
> +  nsresult rv = inputport->Init(aData, aListener);

inputport->Init(aData, aListener, aRv);
if (NS_WARN_IF(aRv.Failed())) {
  return nullptr;
}

::: dom/inputport/AVInputPort.h
@@ +11,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class AVInputPort : public InputPort

final

@@ +16,5 @@
> +{
> +public:
> +  static already_AddRefed<AVInputPort> Create(nsPIDOMWindow* aWindow,
> +                                              nsIInputPortListener* aListener,
> +                                              nsIInputPortData* aData);

...
nsIInputPortData* aData,
ErrorResult& aRv);

Propagate the errorResult everywhere. This is an useful information to keep with you all the time.

::: dom/inputport/DisplayPortInputPort.cpp
@@ +21,5 @@
> +
> +/* static */ already_AddRefed<DisplayPortInputPort>
> +DisplayPortInputPort::Create(nsPIDOMWindow* aWindow,
> +                             nsIInputPortListener* aListener,
> +                             nsIInputPortData* aData)

ErrorResult here too.

::: dom/inputport/DisplayPortInputPort.h
@@ +11,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class DisplayPortInputPort : public InputPort

final

@@ +16,5 @@
> +{
> +public:
> +  static already_AddRefed<DisplayPortInputPort> Create(nsPIDOMWindow* aWindow,
> +                                                       nsIInputPortListener* aListener,
> +                                                       nsIInputPortData* aData);

ErrorResult

::: dom/inputport/FakeInputPortService.cpp
@@ +82,5 @@
> +  Shutdown();
> +}
> +
> +void
> +FakeInputPortService::Init()

Why do we have this FakeInputPortService? Is it going to be removed at some point?

@@ +159,5 @@
> +FakeInputPortService::MockInputPort(const nsAString& aId,
> +                                    const nsAString& aType,
> +                                    bool aIsConnected)
> +{
> +  nsCOMPtr<nsIInputPortData> portData = new InputPortData();

nsCOMPtr<nsIInputPortData> portData = new InputPortData(aId, aType, aIsConnected);
return portData.forget();

::: dom/inputport/HDMIInputPort.cpp
@@ +24,5 @@
> +                      nsIInputPortListener* aListener,
> +                      nsIInputPortData* aData)
> +{
> +  nsRefPtr<HDMIInputPort> inputport = new HDMIInputPort(aWindow);
> +  nsresult rv = inputport->Init(aData, aListener);

ErrorResult propagation.

::: dom/inputport/HDMIInputPort.h
@@ +11,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class HDMIInputPort : public InputPort

final

@@ +16,5 @@
> +{
> +public:
> +  static already_AddRefed<HDMIInputPort> Create(nsPIDOMWindow* aWindow,
> +                                                nsIInputPortListener* aListener,
> +                                                nsIInputPortData* aData);

ErrorResult

::: dom/inputport/InputPort.cpp
@@ +7,5 @@
> +#include "InputPortData.h"
> +#include "InputPortListeners.h"
> +#include "mozilla/AsyncEventDispatcher.h"
> +#include "mozilla/dom/InputPort.h"
> +#include "DOMMediaStream.h"

move it on top. alphabetic order.

@@ +23,5 @@
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(InputPort)
> +NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)
> +
> +InputPort::InputPort(nsPIDOMWindow* aWindow)
> +  : DOMEventTargetHelper(aWindow)

mConnected(false) ?

@@ +33,5 @@
> +}
> +
> +nsresult
> +InputPort::Init(nsIInputPortData* aData, nsIInputPortListener* aListener)
> +{

MOZ_ASSERT(aData, aListener);

@@ +37,5 @@
> +{
> +  NS_ENSURE_TRUE(aData, NS_ERROR_FAILURE);
> +  NS_ENSURE_TRUE(aListener, NS_ERROR_FAILURE);
> +
> +  nsresult rv = aData->GetId(mId);

This will be:

aRv = aData->GetId(mId);
if (NS_WARN_IF(aRv.Failed())) {
  return;
}

@@ +42,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +  if (NS_WARN_IF(mId.IsEmpty())) {
> +    return NS_ERROR_FAILURE;

aRv.Throw(NS_ERROR_FAILURE);
return;

@@ +47,5 @@
> +  }
> +
> +  InputPortType type = static_cast<InputPortData*>(aData)->GetType();
> +  if (NS_WARN_IF(type == InputPortType::EndGuard_)) {
> +    return NS_ERROR_FAILURE;

same here.

@@ +62,5 @@
> +}
> +
> +void
> +InputPort::Shutdown()
> +{

Just to avoid double Shutdown calls, do:

MOZ_ASSERT(mInputPortListener);

if (mInputPortListener) {
  mInputPortListener->UnregisterInputPort(this);
  mInputPortListener = nullptr;
}

@@ +75,5 @@
> +  return InputPortBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +const nsString&
> +InputPort::GetId() const

remove this.

@@ +89,5 @@
> +
> +  nsRefPtr<AsyncEventDispatcher> asyncDispatcher =
> +    new AsyncEventDispatcher(
> +      this, aIsConnected ? NS_LITERAL_STRING("connect") :
> +      NS_LITERAL_STRING("disconnect"), false);

Do a better indentation:

nsRefPtr<AsyncEventDispatcher> asyncDispatcher =
  new AsyncEventDispatcher(this,
                           aIsConnected ? NS_LITERAL_STRING("connect")
                                        : NS_LITERAL_STRING("disconnect"),
                           false);

or something similar.

::: dom/inputport/InputPort.h
@@ +26,5 @@
> +
> +  // WebIDL (internal functions)
> +  virtual JSObject* WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) override;
> +
> +  const nsString& GetId() const;

remove this. You have the other GetId(). Why do you need this?

@@ +45,5 @@
> +  explicit InputPort(nsPIDOMWindow* aWindow);
> +
> +  virtual ~InputPort();
> +
> +  nsresult Init(nsIInputPortData* aData, nsIInputPortListener* aListener);

Just because you use the Init in the Create() of the InputPorts, just do:

void Init nsIInputPortData* aData, nsIInputPortListener* aListener, ErrorResult& aRv);

::: dom/inputport/InputPortData.cpp
@@ +12,5 @@
> +
> +namespace {
> +
> +InputPortType
> +ToInputPortType(const nsAString& aStr)

If you remove the attributes from the nsIInputPortData you don't need this method.

@@ +33,5 @@
> +} // namespace anonymous
> +
> +NS_IMPL_ISUPPORTS(InputPortData, nsIInputPortData)
> +
> +InputPortData::InputPortData()

: mConnected(false) ?

@@ +42,5 @@
> +{
> +}
> +
> +/* virtual */ NS_IMETHODIMP
> +InputPortData::GetId(nsAString& aId)

get rid of this.

@@ +49,5 @@
> +  return NS_OK;
> +}
> +
> +/* virtual */ NS_IMETHODIMP
> +InputPortData::SetId(const nsAString& aId)

remove this.

@@ +60,5 @@
> +  return NS_OK;
> +}
> +
> +/* virtual */ NS_IMETHODIMP
> +InputPortData::GetType(nsAString& aType)

remove this.

@@ +67,5 @@
> +  return NS_OK;
> +}
> +
> +/* virtual */ NS_IMETHODIMP
> +InputPortData::SetType(const nsAString& aType)

remove it.

@@ +82,5 @@
> +  return NS_OK;
> +}
> +
> +/* virtual */ NS_IMETHODIMP
> +InputPortData::GetConnected(bool* aIsConnected)

remove it

@@ +89,5 @@
> +  return NS_OK;
> +}
> +
> +/* virtual */ NS_IMETHODIMP
> +InputPortData::SetConnected(const bool aIsConnected)

remove it

::: dom/inputport/InputPortData.h
@@ +14,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +enum class InputPortType : uint32_t {

{ in a new line;

@@ +27,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIINPUTPORTDATA
> +
> +  InputPortData();

InputPortData(const nsAString& aId, const nsAString& aType, bool mConnected)
  : mId(aId)
  , mType(aType)
  , mConnected(aConnected)
{}

::: dom/inputport/InputPortListeners.cpp
@@ +21,5 @@
> +NS_INTERFACE_MAP_END
> +
> +void
> +InputPortListener::RegisterInputPort(InputPort* aPort)
> +{

MOZ_ASSERT(!mInputPorts.Contains(aPort));

@@ +27,5 @@
> +}
> +
> +void
> +InputPortListener::UnregisterInputPort(InputPort* aPort)
> +{

MOZ_ASSERT(mInputPorts.Contains(aPort));

::: dom/inputport/InputPortManager.cpp
@@ +42,5 @@
> +/* static */ already_AddRefed<InputPortManager>
> +InputPortManager::Create(nsPIDOMWindow* aWindow, ErrorResult& aRv)
> +{
> +  nsRefPtr<InputPortManager> manager = new InputPortManager(aWindow);
> +  nsresult rv = manager->Init();

manager->Init(aRv);
if (NS_WARN_IF(aRv.Failed())) {
  return nullptr;
}

@@ +51,5 @@
> +  return manager.forget();
> +}
> +
> +nsresult
> +InputPortManager::Init()

void
InputPortManager::Init(ErrorResult& aRv)

@@ +55,5 @@
> +InputPortManager::Init()
> +{
> +  mInputPortService = InputPortServiceFactory::AutoCreateInputPortService();
> +  if (NS_WARN_IF(!mInputPortService)) {
> +    return NS_ERROR_FAILURE;

aRv.Throw(NS_ERROR_FAILURE);
return;

@@ +59,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCOMPtr<nsIInputPortServiceCallback> callback =
> +    new InputPortServicePortGetterCallback(this);

What about if InputPortManager inherits from nsIInputPortCallback?
In this way you don't need to have an additional class and 2 additional files.

You can just do:

aRv = mInputPortService->GetInputPorts(this);

@@ +60,5 @@
> +  }
> +
> +  nsCOMPtr<nsIInputPortServiceCallback> callback =
> +    new InputPortServicePortGetterCallback(this);
> +  nsresult rv = mInputPortService->GetInputPorts(callback);

aRv = mInputPortService-> ...
if (NS_WARN_IF(aRv.Failed())) {
  return;
}

@@ +101,5 @@
> +
> +nsresult
> +InputPortManager::SetInputPorts(const nsTArray<nsRefPtr<InputPort>>& aPorts)
> +{
> +  // Should be called only when InputPortManager hasn't been ready yet.

MOZ_ASSERT(!mIsReady) is too much?

::: dom/inputport/InputPortManager.h
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_InputPortManager_h
> +#define mozilla_dom_InputPortManager_h
> +

#include "nsISupports.h"

::: dom/inputport/InputPortServiceCallbacks.cpp
@@ +39,5 @@
> +{
> +}
> +
> +/* virtual */ NS_IMETHODIMP
> +InputPortServicePortGetterCallback::NotifySuccess(nsIArray* aDataList)

move this to InputPortManager.

@@ +40,5 @@
> +}
> +
> +/* virtual */ NS_IMETHODIMP
> +InputPortServicePortGetterCallback::NotifySuccess(nsIArray* aDataList)
> +{

MOZ_ASSERT(aDataList);

@@ +48,5 @@
> +  }
> +
> +  uint32_t length;
> +  nsresult rv = aDataList->GetLength(&length);
> +  NS_ENSURE_SUCCESS(rv, rv);

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +52,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIInputPortListener> portListener;
> +  mManager->GetInputPortService()->GetInputPortListener(
> +    getter_AddRefs(portListener));

this returns a nsresult, right? check it.

@@ +56,5 @@
> +    getter_AddRefs(portListener));
> +
> +  nsTArray<nsRefPtr<InputPort>> ports(length);
> +  for (uint32_t i = 0; i < length; i++) {
> +    nsCOMPtr<nsIInputPortData> portData = do_QueryElementAt(aDataList, i);

MOZ_ASSERT(portData); This must be a nsIInputPortData, correct?

@@ +66,5 @@
> +    nsRefPtr<InputPort> port;
> +    switch (data->GetType()) {
> +    case InputPortType::Av:
> +      port = AVInputPort::Create(mManager->GetParentObject(), portListener,
> +        portData);

indent it under mManager

@@ +70,5 @@
> +        portData);
> +      break;
> +    case InputPortType::Displayport:
> +      port = DisplayPortInputPort::Create(mManager->GetParentObject(),
> +        portListener, portData);

same here.

@@ +74,5 @@
> +        portListener, portData);
> +      break;
> +    case InputPortType::Hdmi:
> +      port = HDMIInputPort::Create(mManager->GetParentObject(), portListener,
> +        portData);

same here.

@@ +76,5 @@
> +    case InputPortType::Hdmi:
> +      port = HDMIInputPort::Create(mManager->GetParentObject(), portListener,
> +        portData);
> +      break;
> +    default:

MOZ_ASSERT_UNREACHABLE("Unknown InputPort type"); Or a better message.

@@ +79,5 @@
> +      break;
> +    default:
> +      break;
> +    }
> +    NS_ENSURE_TRUE(port, NS_ERROR_DOM_ABORT_ERR);

MOZ_ASSERT(port);

::: dom/inputport/InputPortServiceCallbacks.h
@@ +14,5 @@
> +
> +class InputPortManager;
> +
> +class InputPortServicePortGetterCallback final
> +  : public nsIInputPortServiceCallback

I think you should get rid of this class.

::: dom/inputport/InputPortServiceFactory.cpp
@@ +24,5 @@
> +InputPortServiceFactory::AutoCreateInputPortService()
> +{
> +  nsresult rv;
> +  nsCOMPtr<nsIInputPortService> service =
> +    do_CreateInstance(INPUTPORT_SERVICE_CONTRACTID);

do_GetService()

@@ +27,5 @@
> +  nsCOMPtr<nsIInputPortService> service =
> +    do_CreateInstance(INPUTPORT_SERVICE_CONTRACTID);
> +  if (!service) {
> +    // Fallback to the fake service.
> +    service = do_CreateInstance(FAKE_INPUTPORT_SERVICE_CONTRACTID, &rv);

do_GetService

@@ +32,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return nullptr;
> +    }
> +  }
> +

MOZ_ASSERT(service);

::: dom/inputport/InputPortServiceFactory.h
@@ +15,5 @@
> +namespace dom {
> +
> +class FakeInputPortService;
> +
> +class InputPortServiceFactory

final

::: dom/inputport/InputPortServiceRunnables.h
@@ +22,5 @@
> + * nsCOMPtr<nsIRunnable> runnable =
> + *   new InputPortServiceNotifyRunnable(callback, dataList, optional errorCode);
> + * return NS_DispatchToCurrentThread(runnable);
> + */
> +class InputPortServiceNotifyRunnable final : public nsRunnable

We don't need to expose this class. Move it into an anonymous namespace in FakeInputPortService implementation.

@@ +32,5 @@
> +      uint16_t aErrorCode = nsIInputPortServiceCallback::INPUTPORT_ERROR_OK)
> +    : mCallback(aCallback)
> +    , mDataList(aDataList)
> +    , mErrorCode(aErrorCode)
> +  {}

MOZ_ASSERT(aCallback);
MOZ_ASSERT(aDataList);

::: dom/inputport/nsIInputPortService.idl
@@ +15,5 @@
> +
> +/**
> + * XPCOM component which acts as the container for input port data.
> + */
> +[scriptable, builtinclass, uuid(244a2b1d-aa1f-4188-a639-ddb56c554b6d)]

why scriptable?

@@ +19,5 @@
> +[scriptable, builtinclass, uuid(244a2b1d-aa1f-4188-a639-ddb56c554b6d)]
> +interface nsIInputPortData : nsISupports
> +{
> +  attribute DOMString id;
> +  attribute DOMString type;

So, actually you don't use these attributes. Everywhere you cast this object to InputPortData, so just do:

interface nsIInputPortdata : nsISupports
{
};

@@ +65,5 @@
> +#define INPUTPORT_SERVICE_CONTRACTID \
> +  "@mozilla.org/inputport/inputportservice;1"
> +%}
> +
> +[uuid(6214dae0-840e-11e4-b4a9-0800200c9a66)]

buildinclass

::: dom/tests/mochitest/general/test_interfaces.html
@@ +642,5 @@
>      "InputEvent",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    {name: "InputPortManager", b2g: true, pref: "dom.inputport.enabled", permission: ["inputport"]},
> +// IMPORTANT: Do not change this list without review from a DOM peer!
> +    {name: "InputPort", b2g: true, pref: "dom.inputport.enabled", permission: ["inputport"]},

This goes before InputPortManager

Comment 51

[Andrea Marchesini [:baku]]

Comment on attachment 8586632 [details] [diff] [review]
Part-2-Test-Case.patch

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

::: dom/inputport/test/xpcshell/test_inputport_data.js
@@ +10,5 @@
> +  var inputportId = "inputportId";
> +
> +  var data = Cc["@mozilla.org/inputport/inputportdata;1"].
> +             createInstance(Ci.nsIInputPortData);
> +  data.id = inputportId;

Hold on, do we really need to expose this nsIInputPortData?
Tell me more.

Comment 52

[James Cheng[:JamesCheng]]

(In reply to Andrea Marchesini (:baku) from comment #51)
> Comment on attachment 8586632 [details] [diff] [review]
> Part-2-Test-Case.patch
> 
> Review of attachment 8586632 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/inputport/test/xpcshell/test_inputport_data.js
> @@ +10,5 @@
> > +  var inputportId = "inputportId";
> > +
> > +  var data = Cc["@mozilla.org/inputport/inputportdata;1"].
> > +             createInstance(Ci.nsIInputPortData);
> > +  data.id = inputportId;
> 
> Hold on, do we really need to expose this nsIInputPortData?
> Tell me more.

Hi Andrea,

We assume that partner will implement the service in JS or C++,
So I did not mark "buildinclass" in service interface.
For this reason, nsIInputPortData will be exposed and remain the attributes.

For fake service,
1. A fallback mechanism if any failure case occur when initialize concrete service.
2. Easy for running test case.

For packaging into "browser/installer/package-manifest.in",
Can I keep it only because I want to do the test on desktop?
If not, I will remove it and modify the ini file to b2g only.

I will revise the patch and upload.
Thanks.

Comment 53

[James Cheng[:JamesCheng]]

Attachment: Part-1-Inputport-API-implementation.-r-baku.patch

Comment 54

[James Cheng[:JamesCheng]]

Attachment: Part-2-Test-Case.-r-baku.patch

Comment 55

[James Cheng[:JamesCheng]]

Attachment: Part-1-Inputport-API-implementation.-r-b.patch

Comment 56

[James Cheng[:JamesCheng]]

Attachment: Part-2-Test-Case.-r-baku.patch

Hi Andrea,

I remove package-manifest.in from mobile and browser. and also skip the test except b2g.

Due to comment #52, I keep the InputData for JS implementation.

Please help to review the patch, Thanks.


Attach the try test result for reference.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=557b4eec9725

Thanks.

Comment 57

[Andrea Marchesini [:baku]]

Comment on attachment 8588325 [details] [diff] [review]
Part-1-Inputport-API-implementation.-r-b.patch

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

Ok, it's good for me. But what about child processes? I guess you have to add some IPC magic. Can you write a test and then do it in a separate patch?

Plus: I would get rid of InputPortServiceCallback :)

::: dom/base/Navigator.cpp
@@ +279,5 @@
>      mTVManager = nullptr;
>    }
>  
> +  if (mInputPortManager) {
> +    mInputPortManager = nullptr;

I know that this is a common pattern here but actually we can just do:

mInputPortManager = nullptr;

without checking if it exists or not. Don't change what you did, it was just a consideration.

::: dom/inputport/InputPort.cpp
@@ +79,5 @@
> +  return InputPortBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +void
> +InputPort::NotifyConnectionChanged(bool aIsConnected)

What about if InputPort runs in a child process? How does it receive the notification?
Can you create a mochitest for this?

::: dom/inputport/InputPortListeners.h
@@ +8,5 @@
> +#define mozilla_dom_InputPortListeners_h
> +
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsIInputPortService.h"
> +#include "nsTArray.h"

alphabetic order.

::: dom/inputport/InputPortManager.cpp
@@ +90,5 @@
> +  mPendingGetInputPortsPromises.Clear();
> +}
> +
> +nsIInputPortService*
> +InputPortManager::GetInputPortService()

const

::: dom/inputport/InputPortServiceCallbacks.h
@@ +21,5 @@
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_NSIINPUTPORTSERVICECALLBACK
> +  NS_DECL_CYCLE_COLLECTION_CLASS(InputPortServicePortGetterCallback)
> +
> +  explicit InputPortServicePortGetterCallback(InputPortManager* aManager);

I suggested you to remove this class completely and to make InputPortManager to inherit nsIInputPortServiceCallback.
In this way you don't need to expose RejectPendingGetInputPortsPromises publicly.
Any reason why you don't like this approach?

::: dom/webidl/InputPort.webidl
@@ +2,5 @@
> +/* 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/.
> + */
> +

As I said, if you have any URL with documentation, add it here in a comment.
Here and in any other .webidl file.

Comment 58

[Andrea Marchesini [:baku]]

Comment on attachment 8588326 [details] [diff] [review]
Part-2-Test-Case.-r-baku.patch

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

Can you give a treeherder result?

Comment 59

[James Cheng[:JamesCheng]]

(In reply to Andrea Marchesini (:baku) from comment #58)
> Comment on attachment 8588326 [details] [diff] [review]
> Part-2-Test-Case.-r-baku.patch
> 
> Review of attachment 8588326 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you give a treeherder result?

Attach the try test result for reference.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=557b4eec9725

I will modify the patch 1 soon. Thanks for your suggestions.

Comment 60

[James Cheng[:JamesCheng]]

Attachment: Part-1-Inputport-API-implementation.patch

Carry the r+ from Andrea on comment #57 and update patch for reviewer's suggestion.

Comment 61

[James Cheng[:JamesCheng]]

Attachment: Part-2-Test-Case.patch

Hi Andrea,

I removed InputPortServiceCallback as your suggestion.

The design of InputportManager API is similar to TVManager API,

we expect that partner will implement the service.
(include IPC to parent process and callback to InputPortListener running in child(content) process if necessary).

Attach the treeherder result for the latest patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=82856fcb6f0e
Thanks.

Comment 62

[Andrea Marchesini [:baku]]

Comment on attachment 8588997 [details] [diff] [review]
Part-1-Inputport-API-implementation.patch

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

::: dom/inputport/InputPortManager.cpp
@@ +129,5 @@
> +
> +  return promise.forget();
> +}
> +
> +/* virtual */ NS_IMETHODIMP

remove this /* virtual */, we do this additional comment just for static methods.

@@ +193,5 @@
> +
> +  return erv.ErrorCode();
> +}
> +
> +/* virtual */ NS_IMETHODIMP

here too.

Comment 63

[Andrea Marchesini [:baku]]

Comment on attachment 8589000 [details] [diff] [review]
Part-2-Test-Case.patch

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

::: dom/inputport/test/mochitest/test_inputport_connection_event.html
@@ +23,5 @@
> +    var inputPortMgr = navigator.inputPortManager;
> +    inputPortMgr.getInputPorts().then(
> +      function(aPorts) {
> +        ok(aPorts.length > 0, "Got at least 1 inputport.");
> +          var port0 = aPorts[0];

indentation

Comment 64

[Andrea Marchesini [:baku]]

You still need the IPC part. You cannot land this code without it.
Check this in e10s and force FakeInputPortService in the parent process.

I guess we don't want that the child process has access to the input port directly. Do we?

Comment 65

[James Cheng[:JamesCheng]]

Attachment: Part-1-Inputport-API-implementation.patch

Carry the r+ from Andrea on comment #57 and update patch for reviewer's suggestion on comment #62.

Comment 66

[James Cheng[:JamesCheng]]

Attachment: Part-2-Test-Case.patch

Carry the r+ from Andrea on comment #63 and update patch for reviewer's suggestion.

Hi Andrea,

The Stingray project is based on FreeBSD. The content process may directly access HW related resources without help from B2G process. So IPC doesn't appear necessary for now. The requirement does not include IPC part.

We plan to create a follow-up bug for IPC and then start implementing it when it becomes necessary someday.

Would it be fine to you?

Thanks.

Comment 67

[James Cheng[:JamesCheng]]

Hi Andrea, 
Please see comment #66 for my explanation.

Thanks.

Comment 68

[Andrea Marchesini [:baku]]

> The Stingray project is based on FreeBSD. The content process may directly
> access HW related resources without help from B2G process. So IPC doesn't
> appear necessary for now. The requirement does not include IPC part.

Yes, it's ok. But, don't we want to implement any kind of permission check?
Check if the child process can actually have access to the ports. I know that some syscall is disabled in the child processes, so maybe that can be a serious limitation.

Comment 69

[James Cheng[:JamesCheng]]

Hi Andrea,
I think the permission check(even ipc scenario) should be implemented by the real service and if there is no permission to access the port, it will callback via NotifyError with error code such as permission denied.
For Stingray project, I think it is ok for current design.

Thanks.

Comment 70

[Andrea Marchesini [:baku]]

good for me.

Comment 71

[James Cheng[:JamesCheng]]

Create follow-up bug Bug 1152632.

Comment 72

[James Cheng[:JamesCheng]]

Additional for comment #66, in Stingray project,

partner implements TV daemon to access HW related resource, content process will communicate with TV daemon directly for HW access.

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/5508dd802e48
https://hg.mozilla.org/integration/b2g-inbound/rev/ded8396fa202

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5508dd802e48
https://hg.mozilla.org/mozilla-central/rev/ded8396fa202

Comment 75

[Daniel Holbert [:dholbert]]

This was missing some 'override' annotations, which causes clang 3.6 &
newer to spam a "-Winconsistent-missing-override" build warning, which
busts --enable-warnings-as-errors builds (with clang 3.6+).

I pushed a followup to add that keyword, with blanket r+ that ehsan
granted me for fixes of this sort over in bug 1126447 comment 2:
  https://hg.mozilla.org/integration/b2g-inbound/rev/7819b829942d

Comment 76

[Daniel Holbert [:dholbert]]

(Separately, I also noticed that we've incorrectly got "NS_IMETHODIMP" on the Notify method-declaration -- should be NS_IMETHOD:
>+++ b/dom/inputport/FakeInputPortService.cpp
>+class PortConnectionChangedCallback final : public nsITimerCallback
>+{
[...]
>+  NS_IMETHODIMP
>+  Notify(nsITimer* aTimer)
>+  {

That should be fixed as well, though I'm not sure if that actually causes any problems in reality, and my rs=ehsan doesn't cover that general cleanup, so I didn't fix that as part of my followup.)

Comment 77

[Daniel Holbert [:dholbert]]

The NS_IMETHODIMP typo doesn't cause any problems in reality; it's the same as NS_IMETHOD except it's missing 'virtual', which doesn't matter because 'virtual' is implied by the superclass in this case.

Still, worth fixing for consistency/confusion-avoidance, so I pushed a second followup to address correct this typo, with rs=froydnj granted over IRC:
https://hg.mozilla.org/integration/b2g-inbound/rev/740071f22e89

Comment 78

[James Cheng[:JamesCheng]]

(In reply to Daniel Holbert [:dholbert] from comment #77)
> The NS_IMETHODIMP typo doesn't cause any problems in reality; it's the same
> as NS_IMETHOD except it's missing 'virtual', which doesn't matter because
> 'virtual' is implied by the superclass in this case.
> 
> Still, worth fixing for consistency/confusion-avoidance, so I pushed a
> second followup to address correct this typo, with rs=froydnj granted over
> IRC:
> https://hg.mozilla.org/integration/b2g-inbound/rev/740071f22e89

Hi Daniel,

Thanks for your revision.

But one thing I was curious that my class PortConnectionChangedCallback marked as "final",

why I need to explicitly add virtual keyword as NS_IMETHOD?

Just a good habit?

Thank you.

Comment 79

[James Cheng[:JamesCheng]]

Hi Daniel,

Could you please give me an example when I should use 
NS_IMETHODIMP instead of NS_IMETHOD? I want to correct my concept.

Thanks.

Comment 80

[Daniel Holbert [:dholbert]]

Right, the presence/absence of 'virtual' doesn't strictly matter in this case, because we already get the virtualness from our parent class.

The distinction is pretty simple:
 - NS_IMETHOD goes on function declarations [which may or may not have an inline implementation], inside of a "class { ... };" definition.
 - NS_IMETHODIMP goes on function implementations, outside of a class { ... }; definition. These implementations correspond to method-decls that were tagged with NS_IMETHOD.

Concrete example:
 * XPCOM interface nsIFoo, which defines DoSomething() and DoSomethingElse().
 * Foo.cpp which looks like:
 ====
 class Foo : public nsIFoo {
   NS_IMETHOD DoSomething();
   NS_IMETHOD DoSomethingElse { /* one-liner */ }
 };

 NS_IMETHODIMP
 Foo::DoSomething()
 {
    ...
 }
 ====

Comment 81

[Daniel Holbert [:dholbert]]

(The only reason that NS_IMETHODIMP exists is that you can't use annotations like 'virtual' or 'static' on out-of-line function definitions -- like DoSomething() in the example above.  So, we have NS_IMETHODIMP for specifically this purpose -- out-of-line function definitions -- which still defines the calling convention just like NS_IMETHOD does, but lacks the 'virtual' keyword.

But inside a 'class { ... }' scope, where the 'virtual' keyword is allowed & appropriate, we don't need NS_IMETHODIMP.

These macros are defined here, FWIW, though there's not much documentation unfortunately:
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nscore.h?rev=7e4e5e971d95&mark=175-176#175
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nscore.h?rev=7e4e5e971d95&mark=96-97#96

One other thing:
> But one thing I was curious that my class PortConnectionChangedCallback marked as "final",
> why I need to explicitly add virtual keyword as NS_IMETHOD?

In case it's not clear: your method here *is virtual*, whether you label it as such or not. It's virtual because the parent class (nsITimerCallback) declares it as virtual.  So, setting NS_* macros aside, it's appropriate to label it as 'virtual' for clarity/consistency, even if it's technically unnecessary since it's implied by the parent class.

Comment 82

[Daniel Holbert [:dholbert]]

(and technically unnecessary since there are no further derived classes due to 'finalness', as you note)

Comment 83

[James Cheng[:JamesCheng]]

Hi Daniel,

I understood.

Thanks for your answer.

Comment 84

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7819b829942d
https://hg.mozilla.org/mozilla-central/rev/740071f22e89

Comment 85

[Sotaro Ikeda [:sotaro]]

FYI: The following is a diagram of android HdmiControlService.
  https://github.com/sotaroikeda/android-diagrams/blob/master/system/HdmiControlService_6.0.pdf
  https://github.com/sotaroikeda/android-diagrams/wiki/Android-Diagrams