Closed Bug 1180589 Opened 4 years ago Closed 4 years ago

[Simulator] Merge TV Manager API dummy code.

Categories

(Firefox OS Graveyard :: Simulator, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(feature-b2g:2.5+)

RESOLVED FIXED
feature-b2g 2.5+

People

(Reporter: mantaroh, Assigned: mantaroh)

References

Details

Attachments

(4 files, 25 obsolete files)

26.80 KB, patch
mantaroh
: review+
Details | Diff | Splinter Review
13.88 KB, patch
mantaroh
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
mantaroh
: review+
Details | Review
1.45 KB, patch
bholley
: review+
Details | Diff | Splinter Review
Attached file WIP_tv.js (obsolete) —
Attachment file is the developed TV Manager API program.

Fake TV Manager API spec:
 - can use the TV Manager API attributes and methods[1].
 - configurable the TV Tuners / Sources / Channels / Programs.
 - can get the MediaStream from the Tuner Object. (movie is the fake)
 - can change channel and switch the tuner's stream.
 - can't notify the EIT(Event Information Table) Event.
 - can't change the emergency flag dynamically.
 - can't change the scanning method.

[1] http://seanyhlin.github.io/tv-manager-api/index.html

This file is work in progress.
I would like to clean up the code.
Blocks: 1180124
Summary: Merge Firefox OS Simulator for TV. → [Simulator] Merge TV Manager API dummy code.
The purpose of this patch is separating simulator and real-device code when build.
So, main process of tv.js is empty.

This patch referred the FXOS_SIMULATOR environment flag in order to identifying build target(simulator or device).
This patch is dom part only.
(To remove other parts.)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9760a0df5c17
Attachment #8634605 - Attachment is obsolete: true
Reattached patch.

Previous patch forgot several code(package-manifest.in).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4b5915859b5
Attachment #8638441 - Attachment is obsolete: true
Attachment #8639712 - Flags: review?(mh+mozilla)
Attachment #8639712 - Flags: review?(mh+mozilla)
mshal:
Can you review this patch?

I'm going to implement to TV Manager API[1][bug 998872] for Simulator.

This patch is consist of 3 parts.

moz.build and C++ part:
 This part is separating device and simulator code.
 There are using FXOS_SIMULATOR flag.
 This flag is using B2G Desktop build.[1]

Tv.js and Tv.manifest part: 
 This part is prototype implementation of the TV Manager API[2][bug 998872][attachment 8629250 [details]]
 Simulator code implemented by JavaScript code.
 Because it is simple way using Media Capture API.[3]

mozbuild of webidl part:
 This part is adding the simulator interface definition.

[1] http://mxr.mozilla.org/mozilla-central/source/b2g/config/mozconfigs/linux32_gecko/nightly
[2] http://seanyhlin.github.io/TV-Manager-API/
[3] http://w3c.github.io/mediacapture-fromelement/
Attachment #8639712 - Attachment is obsolete: true
Attachment #8639779 - Flags: feedback?(mshal)
Comment on attachment 8639779 [details] [diff] [review]
bug1180589_part1_separate_build_config.patch

I can review the build system bits, but you'll also want someone else to review the js/cpp changes, and maybe get a second opinion on the manifest changes.

>Bug 1180589 - Part1.Separate build config. rev=mshal

Use "r=" instead of "rev="

>diff --git a/b2g/installer/package-manifest.in b/b2g/installer/package-manifest.in
>+#ifndef FXOS_SIMULATOR
> @RESPATH@/components/dom_tv.xpt
>+#endif
...
>+#ifdef FXOS_SIMULATOR
>+@RESPATH@/components/Tv.js
>+@RESPATH@/components/Tv.manifest
>+#endif

Can we make this an #ifdef/#else/#endif instead of an #ifndef and #ifdef? Eg:

#ifdef FXOS_SIMULATOR
@RESPATH@/components/Tv.js
@RESPATH@/components/Tv.manifest
#else
@RESPATH@/components/dom_tv.xpt
#endif

I don't really know why the package manifest is organized into .xpt components and .js components, but IMO it's better to have the TV manager parts grouped together. You should get a second opinion here from someone who is more familiar with these manifests, though.

>diff --git a/dom/tv/Tv.js b/dom/tv/Tv.js
>+/* This Source Code Form is subject to the terms of the Mozilla Public^M

>diff --git a/dom/tv/Tv.manifest b/dom/tv/Tv.manifest
>+component {be177b10-f0cc-4520-89c7-cd0e829b2abd} Tv.js^M

These two files both have Windows-style line endings - please switch them to Unix-style, and configure your editor to use that as the default.

>diff --git a/dom/webidl/moz.build b/dom/webidl/moz.build
>+if not CONFIG['FXOS_SIMULATOR']:
>+    WEBIDL_FILES += [
>+        'TVChannel.webidl',
>+        'TVManager.webidl',
>+        'TVProgram.webidl',
>+        'TVSource.webidl',
>+        'TVTuner.webidl',
>+    ]
>+    

nit: random whitespace in the above line
Attachment #8639779 - Flags: feedback?(mshal) → feedback+
mshal,
Thank you for your feedback!
I've fixed your indicate.

And I'll ask manifest and implement parts.
Attachment #8639779 - Attachment is obsolete: true
Comment on attachment 8644141 [details] [diff] [review]
bug1180589_part1_separate_build_config.patch

Evelyn,

I'll implement TV Manager API as JavaScript code.
Because It is easy way using by 'Media Capture from DOM Element'[1] in order to implement the TVTuner.stream.
(There aren't way to create MediaStream instance. So I'll inject video tag when load the Tv.js, and provide MediaStream using by mozCaptureStream[2])

So my implementation include the several change( like WebIDL change / Build change / TV.js / TV.manifest).


For Example,TVManager.webidl:
----------------------------------------------------------------------------------
(For Real Device Build)
[Pref="dom.tv.enabled", CheckAnyPermissions="tv", Func="Navigator::HasTVSupport"]

(For Simulator Build)
[Pref="dom.tv.enabled", CheckAnyPermissions="tv", Func="Navigator::HasTVSupport",
 JSImplementation="@mozilla.org/tvManager;1", 
 NoInterfaceObject, NavigatorProperty="tv"]
----------------------------------------------------------------------------------
I will use FXOS_SIMULATOR flag in order to switch simulator and device build.

Could you feed back about this implementation?

[1] http://www.w3.org/TR/mediacapture-fromelement/
[2] http://mxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLMediaElement.webidl#121
Attachment #8644141 - Flags: feedback?(ehung)
Great! Thank you so much, Mantaroh. I think Sean is the best person to give you feedback. :)
Flags: needinfo?(selin)
Attachment #8644141 - Flags: feedback?(ehung) → feedback?(selin)
Actually I'm less inclined to have separate builds and still thinking to integrate TV simulator logic with nsITVService based on the following proposal:

1. Add the followings to nsITVTunerData [1]

  const unsigned short TV_STREAM_TYPE_SIMULATOR = 0;
  const unsigned short TV_STREAM_TYPE_HW = 1;
  // More types may be added in the future.

  attribute unsigned short streamType;

2. Add |uint16_t mStreamType| to |TVTuner|. In |TVTuner::InitMediaStream| [2], add simulator specific logic to create |mozCaptureStream| as mentioned in comment 7. For example,

  if (mStreamType == nsITVTunerData::TV_STREAM_TYPE_HW) {
    // Current implementation to create the HW stream.
  } else if (mStreamType == nsITVTunerData::TV_STREAM_TYPE_SIMULATOR) {
    // Create the stream for simulator use.
  }

In this way, we don't need to have different builds or maintain two TV API implementations in the code base. A |nsITVSimulatorService| interface may extend |nsITVService| and have more simulator specific methods, such as the one to provide the necessary info to create the simulated stream. And a |TVSimulatorService| class implementing that interface could be created to load simulated data. (In the long run FakeTVService might be completely replaced by TV simulator service.)

[1] https://dxr.mozilla.org/mozilla-central/source/dom/tv/nsITVService.idl#24
[2] https://dxr.mozilla.org/mozilla-central/source/dom/tv/TVTuner.cpp#198-205

Hi Ehsan,

Since you've provided lots feedback during the implementation of TV API, could you also share your opinions about this proposal?
Flags: needinfo?(selin) → needinfo?(ehsan)
I really have no idea what Mantaroh's goal here is, and why we're adding a new build type, and comment 0 doesn't have a lot of information about this, so I'm not quite sure what questions you're asking.
Flags: needinfo?(ehsan)
Flags: needinfo?(selin)
Blocks: TV_FxOS2.5
feature-b2g: --- → 2.5+
Priority: -- → P1
(In reply to Sean Lin [:seanlin] from comment #9)
> Actually I'm less inclined to have separate builds and still thinking to
> integrate TV simulator logic with nsITVService based on the following
seanlin,
Thank you for your comments.
Your suggestion is so great!

I implemented TV Manager API simulation using JavaScript.(comment 7)
But I think we should integrate simulator code to TVService base.
(Because we might have to maintenance two implementation.)

DOMHTMLMediaElement have a Overlay-VideoFrameContainer when set to HwMediaStream.[1]
So TV apps can get video element which playing TV's media, after setting MediaElement.src = Tuner.stream.

In simulator, I think we should play sample video(like local video file) to the MediaStream.

However, I have no idea implement to play local video file to the MediaStream.
If you have a idea to implement FakeHwMedaiStream, could you please share tips?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#3106
(In reply to Mantaroh Yoshinaga[:mantaroh](back Aug 17) from comment #11)
> In simulator, I think we should play sample video(like local video file) to
> the MediaStream.
> 
> However, I have no idea implement to play local video file to the
> MediaStream.
> If you have a idea to implement FakeHwMedaiStream, could you please share
> tips?

So far I haven't found an easy way to load a local video file and get a media stream for it. By the way, you've mentioned

"I'll inject video tag when load the Tv.js, and provide MediaStream using by mozCaptureStream."

in comment 7. So I'm curious about how you plan to inject and get a video tag (and it would be great if there's a brief example.) Maybe we can get some inspiration from it.
Flags: needinfo?(selin) → needinfo?(mantaroh)
(In reply to (Ask others for review please; out most of the week) from comment #10)
> I really have no idea what Mantaroh's goal here is, and why we're adding a
> new build type, and comment 0 doesn't have a lot of information about this,
> so I'm not quite sure what questions you're asking.

The goal is to have a simulator for TV API with some mocked data (probably loaded from a JSON file at launch.) Mantaroh had an original proposal mentioned in comment 7. But in order not to have the potential drawbacks of a new build type and duplicate WebIDL implementation, I proposed another way in comment 8. So it'd be great if you'd like to put some feedback for further improvements.
(In reply to Sean Lin [:seanlin] from comment #12)
> So far I haven't found an easy way to load a local video file and get a
> media stream for it.
Perhaps, We might create MediaStream instance from dom::File element instance.
I'll investigate in order to it is true.

> By the way, you've mentioned
> "I'll inject video tag when load the Tv.js, and provide MediaStream using by
> mozCaptureStream."
> 
> in comment 7. So I'm curious about how you plan to inject and get a video
> tag (and it would be great if there's a brief example.) Maybe we can get
> some inspiration from it.
This way is creating video element when called TV Manager API.
e.g:
 - Create video element in chrome code(tv.js) when generating TVManager(navigator.tv).
 - tv.js was injected that video element to the global window document.
 - That video element's src object is File instance(local media).
 - Set those video element's mozCaptureStream() return to TVTuner.stream.

The sample source is as follow.
----------------------------------------------------------------------------------------
Cu.importGlobalProperties(['File']);
.....
let internalFile = Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties).get("ProfD", Ci.nsIFile);
internalFile.append("dummy/test.ogg");
let localFile = new File(internalFile.path);
let internalVideoTag = gWindow.document.createElement("video");
internalVideoTag.src = gWindow.URL.createObjectURL(localFile);
.....
tvTuner[0].stream = internalVideoTag.mozCaptureStream();
----------------------------------------------------------------------------------------
Flags: needinfo?(mantaroh)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #14)
> This way is creating video element when called TV Manager API.
> e.g:
>  - Create video element in chrome code(tv.js) when generating
> TVManager(navigator.tv).
>  - tv.js was injected that video element to the global window document.
>  - That video element's src object is File instance(local media).
>  - Set those video element's mozCaptureStream() return to TVTuner.stream.
> 
> The sample source is as follow.
> -----------------------------------------------------------------------------
> -----------
> Cu.importGlobalProperties(['File']);
> .....
> let internalFile =
> Cc["@mozilla.org/file/directory_service;1"].getService(Ci.nsIProperties).
> get("ProfD", Ci.nsIFile);
> internalFile.append("dummy/test.ogg");
> let localFile = new File(internalFile.path);
> let internalVideoTag = gWindow.document.createElement("video");
> internalVideoTag.src = gWindow.URL.createObjectURL(localFile);
> .....
> tvTuner[0].stream = internalVideoTag.mozCaptureStream();
> -----------------------------------------------------------------------------
> -----------

It looks similar things could be done in |TVTuner| when creating the mocked media stream. |GetOwner()| could be used to get the window in |nsPIDOMWindow| type, and it has a method |GetExtantDoc()| [1] to get the document. Then you may use it to create a video element and get a media stream with |mozCaptureStream| [2].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsPIDOMWindow.h#164
[2] https://dxr.mozilla.org/mozilla-central/source/dom/media/webaudio/AudioContext.cpp#361
(In reply to Sean Lin [:seanlin] from comment #15)
> It looks similar things could be done in |TVTuner| when creating the mocked
> media stream. |GetOwner()| could be used to get the window in
> |nsPIDOMWindow| type, and it has a method |GetExtantDoc()| [1] to get the
> document. Then you may use it to create a video element and get a media
> stream with |mozCaptureStream| [2].
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/base/nsPIDOMWindow.h#164
> [2]
> https://dxr.mozilla.org/mozilla-central/source/dom/media/webaudio/
> AudioContext.cpp#361
Thank you for your advice!

I tried same thing in TVTuner : 
 - Create HTMLVideoElement from nsPIDOMWindow.GetExtantDoc().
 - Create File instance from local media file.  (set with other parameter. e.g. autoplay / loop )
 - Set the localmedia file to HTMLMediaElement. 
 - Get MediaStream using HTMLMediaElement.MozCaptureStream() [1]

In this way, We can get the MediaStream from the local video file.

So my next step is writing the patch of using this way.

[1] https://dxr.mozilla.org/mozilla-central/rev/90d9b7c391d38ae118865bd87b5d011feee6dded/dom/html/HTMLMediaElement.h#592
Sean,

I want to get your advice about XPCOM for TVSimulator.
It is to create TVSimulatorUtil interface.
This interface will provide loading setting from JSON file and supporting to create MediaStream.
e.g:
 - Load setting information from local JSON file.
 - Response tuner information data.
 - Create Fake MediaStream from local video file.

Interface example:
  int getTunerNum();
  string getSourceTypes();
  int getSourceNum(); ...etc

I will implement this XPCOM interface using JavaScript.
Because, Loading from JSON in native code is too complex.

If created XPCOM, I believe that our maintenance is easy.

In Init() of FakeTVService, FakeTVService will request for necessary information to TVSimulatorUtil.
FakeTVService can build the TVTunerData and other structure when receive the response.

What do you think about this idea?
Flags: needinfo?(selin)
It looks good to me to use Javascript implementation for the XPCOM interface (especially for loading JSON). Yet instead of creating a whole new one and manipulate it in |FakeTVService|, you may consider to have a |nsITVSimulatorService| extending |nsITVService| like below:

interface nsITVSimulatorService : nsITVService
{
  // Some simulator-specific methods which are not fulfilled by existent ones in |nsITVService|, such as getting the file path (or blob) which could be used to generate mocked stream.
}

Then you may implement |nsITVSimulatorService| in Javascript without relying on |FakeService|. (The simulator service looks more flexible than current FakeService, which only provides a fixed set of mocked data. In the long run, we may deprecate |FakeService| in a follow-up bug once the simulator service becomes ready.)

And please note it'd better not to let the simulator service hold reference to the document or window object. So it'd better to make the simulator service only provide document independent data and create the mocked DOM stream (comment 16) in |TVTuner| (comment 9).

In addition, you might need to update |TVServiceFactory| [1] to create the simulator service as follows. 

  nsCOMPtr<nsITVService> service = do_CreateInstance(TV_SERVICE_CONTRACTID);
  if (!service) {
    // Fallback to the fake service if preference |dom.testing.tv_enabled_for_hosted_apps| is set.
    // (Tentative use. So no need to update current TV API mochitests which rely on the fake service.)

    // Otherwise, fallback to the simulator service.
  }

In the future, when it comes to deprecate the fake service in a follow-up bug, the current TV API mochitests should switch to use the simulator service and make it covered by those tests.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/tv/TVServiceFactory.cpp#25
Flags: needinfo?(selin)
(In reply to Sean Lin [:seanlin] from comment #18)
> It looks good to me to use Javascript implementation for the XPCOM interface
> (especially for loading JSON). 
I've investigated that I can implement TVSimulatorService in Javascript.
It works fine to load setting from JSON. 
And it works as Interface API of TVService.

I've implemented as follow.
(I can call the navigator.tv.getTuners() on simulator)

[TVSimulatorService.js]
TVSimulatorService.prototype = {
    classID:           Components.ID("{f0ab9850-24b4-4f5d-83dd-0fea0c249ca1}"),
    _xpcom_factory: XPCOMUtils.generateSingletonFactory(TVSimulatorService),
    QueryInterface: XPCOMUtils.generateQI([Ci.nsITVSimulatorService, Ci.nsITVService]),

    ....    
}

[TVSimulatorService.manifest]
component {f0ab9850-24b4-4f5d-83dd-0fea0c249ca1} TVSimulatorService.js
contract @mozilla.org/tv/simulatorservice;1 {f0ab9850-24b4-4f5d-83dd-0fea0c249ca1}

[TVServiceFactory.cpp]
TVServiceFactory::AutoCreateTVService()
{
  nsresult rv;
  nsCOMPtr<nsITVService> service = do_CreateInstance(TV_SERVICE_CONTRACTID);
  if (!service) {
    // Fallback to the fake service.
    service = do_CreateInstance("@mozilla.org/tv/simulatorservice;1", &rv);
    if (NS_WARN_IF(NS_FAILED(rv))) {
      return nullptr;
    }
  }
  ......
}
Hi Sean,

I have a question about current implementation of FakeTVService.

In current implementation of FakeTVService,
Mock data is not related with other mock datas.

For example:
 According to TV Manager API Spec, TVProgramData is related with TVChannelData.
 But FakeTVService is not related each Mock data[1].

What is the point of implementing this?

I think we should implement related with each mock data.
I'll create wrapper of mock data which is holding the mock data references of relating data.
Flags: needinfo?(selin)
Indeed TVChannel and TVProgram interfaces are explicitly related in the spec; whereas TVChannelData and TVProgramData don't do the same thing in their attributes. Since TVProgramData brings program specific data through |getPrograms| or |notifyEITBroadcasted| calls where the channel has already been specified. So the implementation knows which channel those programs belong to and then can link the generated TVProgram instances to correspondent TVChannel instances.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/tv/nsITVService.idl#365
[2] https://dxr.mozilla.org/mozilla-central/source/dom/tv/nsITVService.idl#194
Flags: needinfo?(selin)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #19)
> [TVServiceFactory.cpp]
> TVServiceFactory::AutoCreateTVService()
> {
>   nsresult rv;
>   nsCOMPtr<nsITVService> service = do_CreateInstance(TV_SERVICE_CONTRACTID);
>   if (!service) {
>     // Fallback to the fake service.
>     service = do_CreateInstance("@mozilla.org/tv/simulatorservice;1", &rv);
>     if (NS_WARN_IF(NS_FAILED(rv))) {
>       return nullptr;
>     }
>   }
>   ......
> }

Please keep the way to use the fake service (only when explicitly specified with preference |dom.testing.tv_enabled_for_hosted_apps|) as below.

nsresult rv;
nsCOMPtr<nsITVService> service = do_CreateInstance(TV_SERVICE_CONTRACTID);
if (!service) {
  if (Preferences::GetBool("dom.testing.tv_enabled_for_hosted_apps", false)) {
    // Fallback to the fake service.
    service = do_CreateInstance(FAKE_TV_SERVICE_CONTRACTID, &rv);
  } else {
    // Fallback to the simulator service.
    service = do_CreateInstance("@mozilla.org/tv/simulatorservice;1", &rv);
  }

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

Otherwise you'll need to fix the mochitests under dom/tv/test/mochitest, which might be good but could be left to the follow-up bug for deprecating the fake service, before checking in the code. (FakeService has some fixed mocked data and they are currently used by some of those tests. And the tests also enable preference |dom.testing.tv_enabled_for_hosted_apps|.)
Attached patch bug1180124.patch (obsolete) — Splinter Review
(In reply to Sean Lin [:seanlin] from comment #22)
> Please keep the way to use the fake service (only when explicitly specified
> with preference |dom.testing.tv_enabled_for_hosted_apps|) as below.
> 
> nsresult rv;
> nsCOMPtr<nsITVService> service = do_CreateInstance(TV_SERVICE_CONTRACTID);
> if (!service) {
>   if (Preferences::GetBool("dom.testing.tv_enabled_for_hosted_apps", false))
> {
>     // Fallback to the fake service.
>     service = do_CreateInstance(FAKE_TV_SERVICE_CONTRACTID, &rv);
>   } else {
>     // Fallback to the simulator service.
>     service = do_CreateInstance("@mozilla.org/tv/simulatorservice;1", &rv);
>   }
> 
>   if (NS_WARN_IF(NS_FAILED(rv))) {
>     return nullptr;
>   }
> }
> ......
> 
Hi Sean,

Thank you for good advice!
I created the patch of TV Simulator Services.
This patch is around the half of all task.

I think that I would like to discuss about this patch next week in the Taipei Office.

> Otherwise you'll need to fix the mochitests under dom/tv/test/mochitest,
> which might be good but could be left to the follow-up bug for deprecating
> the fake service, before checking in the code. (FakeService has some fixed
> mocked data and they are currently used by some of those tests. And the
> tests also enable preference |dom.testing.tv_enabled_for_hosted_apps|.)
I read the mochitest.
There are setting |dom.testing.tv_enabled_for_hosted_apps| code.[1]
I think that current mochitest is work as FakeService.
Is it right?


[1] https://dxr.mozilla.org/mozilla-central/rev/87e23922be375985d0b1906ed5ba5f095f323a38/dom/tv/test/mochitest/head.js?offset=300#82
Attachment #8629830 - Attachment is obsolete: true
Attachment #8644141 - Attachment is obsolete: true
Attachment #8653994 - Flags: feedback?(selin)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #23)
> Created attachment 8653994 [details] [diff] [review]
> bug1180124.patch
> 
> (In reply to Sean Lin [:seanlin] from comment #22)
> > Otherwise you'll need to fix the mochitests under dom/tv/test/mochitest,
> > which might be good but could be left to the follow-up bug for deprecating
> > the fake service, before checking in the code. (FakeService has some fixed
> > mocked data and they are currently used by some of those tests. And the
> > tests also enable preference |dom.testing.tv_enabled_for_hosted_apps|.)
> I read the mochitest.
> There are setting |dom.testing.tv_enabled_for_hosted_apps| code.[1]
> I think that current mochitest is work as FakeService.
> Is it right?
> 
Yes. Since there's no "real" TV service in gecko yet, we always launch the fake service, where fixed mocked data are hard coded, when running mochitests. So the current tests work and might only work with the fake service. But once the simulator service is introduced, the tests might fail unless the same mocked data are provided from the simulator service, or the tests are updated to fit the new mocked data. So making fake service launched with preference |dom.testing.tv_enabled_for_hosted_apps| would ensure the tests still working even after the simulator service is introduced.
Comment on attachment 8653994 [details] [diff] [review]
bug1180124.patch

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

::: dom/tv/TVSimulatorService.js
@@ +8,5 @@
> +const Ci = Components.interfaces;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +function TVSimulatorService() { this.init(); }

nit:

function TVSimulatorService() {
  this.init();
}

@@ +13,5 @@
> +
> +TVSimulatorService.prototype = {
> +  classID:           Components.ID("{f0ab9850-24b4-4f5d-83dd-0fea0c249ca1}"),
> +  _xpcom_factory: XPCOMUtils.generateSingletonFactory(TVSimulatorService),
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsITVSimulatorService, Ci.nsITVService]),

nit: let's have the interfaces in separate lines to conform the convention of other components.

QueryInterface: XPCOMUtils.generateQI([Ci.nsITVSimulatorService,
                                       Ci.nsITVService]),

@@ +18,5 @@
> +
> +  mSourceListener : undefined,
> +  mTunersData : undefined,
> +
> +  init: function TVSim_init() {

For members supposed to be accessed internally, we usually have the naming convention starting with '_' (_sourceListener, _tunersData, _init) in JS implementation. Besides, according to the coding convention, could you move the variables into |function TVSimulatorService()| and have them like |this._sourceListener = null;|.

@@ +19,5 @@
> +  mSourceListener : undefined,
> +  mTunersData : undefined,
> +
> +  init: function TVSim_init() {
> +    //TODO load from json setting, and build mTunersData 

nit: tailing space.

@@ +29,5 @@
> +    let dsFile = Cc["@mozilla.org/file/directory_service;1"]
> +                   .getService(Ci.nsIProperties)
> +                   .get("ProfD", Ci.nsIFile);
> +    dsFile.append("dummy");
> +    dsFile.append("settings.json");

Please make a constant variable having this hard coded strings at the beginning of the file.

@@ +40,5 @@
> +    let cstream = Cc["@mozilla.org/intl/converter-input-stream;1"]
> +                    .createInstance(Ci.nsIConverterInputStream);
> +
> +    let settingStr = "";
> +    let self = this;

Can we bind this for some calls so you might not need an additional |self| variable?

@@ +47,5 @@
> +      fstream.init(file, -1, 0, 0);
> +      cstream.init(fstream, "UTF-8", 1024, Ci.nsIConverterInputStream.DEFAULT_REPLACEMENT_CHARACTER);
> +
> +      let (str = {}) {
> +        let read = 0;

Can we just use |let str = {};|? Btw, variable |read| is declared but doesn't appear used.

@@ +54,5 @@
> +        }
> +      }
> +    } catch(e) {
> +      dump("[TVSimulatorService] Error occurred : " + e );
> +      return;

Is it viable to return or throw an error code here?

@@ +61,5 @@
> +    }
> +
> +      let settingObj;
> +      try {
> +        settingObj = JSON.parse(settingStr);

Please add some comments or examples about what the parsed JSON format of the settings look like.

@@ +75,5 @@
> +      for each (let tunerData in settingObj.tuners) {
> +        let tuner = Cc["@mozilla.org/tv/tvtunerdata;1"]
> +                      .createInstance(Ci.nsITVTunerData);
> +        tuner.id = tunerData.id;
> +        tuner.streamType = tuner.TV_STREAM_TYPE_SIMULATOR;

Please consider to use |Ci.nsITVTunerData.TV_STREAM_TYPE_SIMULATOR|.

@@ +93,5 @@
> +            channel.name              = channelData.name;
> +            channel.number            = channelData.number;
> +            channel.isEmergency       = channelData.isEmergency;
> +            channel.isFree            = channelData.isFree;
> +            

nit: tailing space.

@@ +120,5 @@
> +
> +  getTuners: function TVSim_getTuners(callback) {
> +    if (!callback) {
> +        dump("[TVSimulatorService] callback is null\n");
> +      return 2147942487; // Mean NS_ERROR_INVALID_ARG

You may use |Cr.NS_ERROR_INVALID_ARG| after having |const Cr = Components.results;| at the beginning of this file. (Same thing to other places in this component.)

@@ +161,5 @@
> +  },
> +
> +  getPrograms: function TVSim_getPrograms(tunerId, sourceType, channelNumber,
> +                                          startTime, endTime, callback) {
> +   

nit: tailing space.

@@ +170,5 @@
> +
> +  set sourceListener(listener) {
> +      this.mSourceListener = listener;
> +  },
> +  

nit: tailing space.

::: dom/tv/TVTuner.cpp
@@ +222,5 @@
> +
> +  if (mStreamType == nsITVTunerData::TV_STREAM_TYPE_HW) {
> +    stream = DOMHwMediaStream::CreateHwStream(window);
> +  } else if (mStreamType == nsITVTunerData::TV_STREAM_TYPE_SIMULATOR) {
> +    // Create Simulator Stream;

Let's create a new private function like |CreateSimulatorStream| and move this specific logic over there. Then it would look 'dryer' for the whole logic of |InitMediaStream|.

Besides, in |nsISimulatorService| you may have a function like |getMockedStreamFile()| returning a nsIFile (and probably with some input parameters) to load the stream file. So TVTuner doesn't need to know the details about the loading of the simulated data. (Only simulator service does.)

Btw, could you put the name of the stream file into the settings JSON as well? Then the simulator service only has to load the JSON from a currently hard coded path (it might be ok for now).

::: dom/tv/nsITVService.idl
@@ +25,5 @@
>  {
> +  /**
> +   * Switch TVTuner.stream type.
> +   *  TV_STREAM_TYPE_SIMULATOR : Simulate the MediaStream. This MediaStream load from Profile Directory.
> +   *  TV_STREAM_TYPE_HW        : Get from real device 

nit: tailing space.

::: dom/tv/nsITVSimulatorService.idl
@@ +7,5 @@
> +
> +[scriptable, uuid(f0ab9850-24b4-4f5d-83dd-0fea0c249ca1)]
> +interface nsITVSimulatorService : nsITVService
> +{
> +  attribute nsITVSourceListener sourceListener;

|sourceListener| is already in nsITVService.
Attachment #8653994 - Flags: feedback?(selin)
Blocks: 1200133
Attached patch 1180124.patch (obsolete) — Splinter Review
(In reply to Sean Lin [:seanlin] from comment #25)
> Comment on attachment 8653994 [details] [diff] [review]
> bug1180124.patch
> 
Thank you for feedback.
And sorry for basic mistake.
I've modified this patch.

and when I go ahead next implementation,
I would like to review request for you.

>@@ +54,5 @@
>> +        }
>> +      }
>> +    } catch(e) {
>> +      dump("[TVSimulatorService] Error occurred : " + e );
>> +      return;
> 
>Is it viable to return or throw an error code here?
_init() function is XPCOM initializer.
So I think that it can't return the error code.
(If throw the Exception, It might be fail to boot.)
I think it is better to print the debug.
If TVSimulatorService doesn't initialize private member |_tunersData|, navigate.tv.getTv() don't work and return undefined.

>Btw, could you put the name of the stream file into the settings JSON as well? 
Yes. JSON have a fake movie file path.

>Then the simulator service only has to load the JSON from a currently hard coded path (it might be ok for now).
I think it should be configurable.
However, we should modify WebIDE in order to configurable.

(In reply to Sean Lin [:seanlin] from comment #24)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #23)
> So making fake service
> launched with preference |dom.testing.tv_enabled_for_hosted_apps| would
> ensure the tests still working even after the simulator service is
> introduced.
OK.I understand just now.
 TV Simulator Service has mochitest, and current mochitest is unsuited for TV Simulator Service.
So I should create the new test.
(file the bug 1200133.)
Attachment #8654739 - Attachment is obsolete: true
Attachment #8654739 - Attachment is obsolete: false
Hi sean,

I created the TV Manager API part patch.
This patch is provide basic function of TV Manager API.
(except part of creating MediaStream / creating gaia profile with dummy data.)

Almost of this function is similar to FakeTVService, and other part is ported from previous tv.js.

WIP is as follow.
 - Part of creating the MediaStream in the native code.
 - Add dummy data for simulator(Creating the dummy folder and data when build the gaia)
Attachment #8653994 - Attachment is obsolete: true
Attachment #8654739 - Attachment is obsolete: true
Attachment #8655368 - Flags: review?(selin)
Attached patch 1180124.patch (obsolete) — Splinter Review
Sorry,
Previous patch was mistake.
I reattached correct patch.
Attachment #8655368 - Attachment is obsolete: true
Attachment #8655368 - Flags: review?(selin)
Attachment #8655737 - Flags: review?(selin)
deleted some tailing space,
and renamed the patch names.
Attachment #8655737 - Attachment is obsolete: true
Attachment #8655737 - Flags: review?(selin)
Attachment #8655738 - Flags: review?(selin)
Comment on attachment 8655738 [details] [diff] [review]
PART1.Add_TVSimulatorService_for_Simulator.patch

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

::: dom/tv/TVServiceFactory.cpp
@@ +31,5 @@
> +      // Fallback to the fake service.
> +      service = do_CreateInstance(FAKE_TV_SERVICE_CONTRACTID, &rv);
> +    } else {
> +      // Fallback to the TV Simulator Service
> +      service = do_CreateInstance("@mozilla.org/tv/simulatorservice;1", &rv);

We could register TV simulator service on the same contractId (i.e. @mozilla.org/tv/tvservice) and bundle different XPCOM implementation for simulator and real device at build time.
Comment on attachment 8655738 [details] [diff] [review]
PART1.Add_TVSimulatorService_for_Simulator.patch

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

::: dom/tv/TVSimulatorService.js
@@ +19,5 @@
> +  this._init();
> +}
> +
> +TVSimulatorService.prototype = {
> +  classID:           Components.ID("{f0ab9850-24b4-4f5d-83dd-0fea0c249ca1}"),

nit: Consider to remove extra spaces.

@@ +56,5 @@
> +      while (cstream.readString(0xffffffff, str) != 0) {
> +        settingStr += str.value;
> +      }
> +    } catch(e) {
> +      dump("[TVSimulatorService] Error occurred : " + e );

You may have a debug function declared at the beginning of this file. [1] Then the log messages can be turn on/off more easily.

[1] https://dxr.mozilla.org/mozilla-central/source/b2g/components/B2GPresentationDevicePrompt.js?offset=0#8

@@ +82,5 @@
> +       *          "name":               "The channel name",
> +       *          "number" :            The LCN (Logical Channel Number) of the channel,
> +       *          "isEmergency" :       Whether this channel is emergency status,
> +       *          "isFree":             Whether this channel is free or not,
> +       *          "movieFile":          "The path of the fake movie file",

nit: movieFile => videoFilePath

@@ +106,5 @@
> +    }
> +
> +    this._internalTuners = new Array();
> +
> +    //TVTunerData

nit: In coherence with the coding convention in other files, please add a space after '//' for comments. Ex. '// TVTunerData'.
Same to other places in this file.

@@ +117,5 @@
> +                                    tunerData.supportedType);
> +
> +      let wrapTunerData = {
> +        'tuner' : tuner,
> +        'channelList' : new Array(),

Instead of an array, it'd better to have it as a hash mapping the channel number (LCN) to the wrapped channel data. So it may help access the channel data in |getPrograms| or |getSimulatedStreamFilePath| calls with specified channel number.

@@ +139,5 @@
> +          channel.isEmergency       = channelData.isEmergency;
> +          channel.isFree            = channelData.isFree;
> +
> +          let wrapChannelData = {
> +            'channel':channel,

nit: Add a space after ':'.

@@ +167,5 @@
> +      this._internalTuners.push(wrapTunerData);
> +    }
> +  },
> +
> +  getTuners: function TVSim_getTuners(callback) {

nit: Let's follow the existent convention for function names. "TVSim_getTuners" => TVSimGetTuners
Same comment to other places in this file.

@@ +200,5 @@
> +
> +    return callback.notifyError(Ci.nsITVServiceCallback.TV_ERROR_FAILURE);
> +  },
> +
> +  startScanningChannles: function TVSim_startScanningChannels(tunerId, sourceType, callback) {

Typo. s/startScanningChannles/startScanningChannels

@@ +216,5 @@
> +
> +    this._scanCompleteTimer = Cc["@mozilla.org/timer;1"]
> +                                .createInstance(Ci.nsITimer);
> +    rv = this._scanCompleteTimer.initWithCallback(this, 20,
> +                                                      Ci.nsITimer.TYPE_ONE_SHOT);

nit: Please adjust the indentation.

@@ +220,5 @@
> +                                                      Ci.nsITimer.TYPE_ONE_SHOT);
> +    return Cr.NS_OK;
> +  },
> +
> +  notify: function TVSim_EITBroadcastTimerCallback(aTimer) {

nit: Since this function is not only used for EIT broadcasting, it deserves a more general name like "TVSimTimerCallback".

::: dom/tv/TVTuner.h
@@ +67,5 @@
>    bool Init(nsITVTunerData* aData);
>  
>    nsresult InitMediaStream();
>  
> +  already_AddRefed<DOMMediaStream> CreateMediaStreamForSimulator(nsIDOMWindow* aWindow);

nit: CreateMediaStreamForSimulator => CreateSimulatedMediaStream

::: dom/tv/nsITVSimulatorService.idl
@@ +15,5 @@
> +   * @param tunerId       The ID of the tuner.
> +   * @param sourceType    The source type to be used.
> +   * @param channelNumber The LCN (Logical Channel Number) of the channel.
> +   */
> +  string getSimulatorMovieFile(in DOMString tunerId,

It looks this method is to get the file path. So it could be slightly modified as follows:

void getSimulatedVideoFilePath(in DOMString tunerId,
                                in DOMString sourceType,
                                in DOMString channelNumber,
                                [retval] out DOMString filePath);
Attachment #8655738 - Flags: review?(selin)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #32)
> Comment on attachment 8655738 [details] [diff] [review]
> PART1.Add_TVSimulatorService_for_Simulator.patch
> 
> Review of attachment 8655738 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/tv/TVServiceFactory.cpp
> @@ +31,5 @@
> > +      // Fallback to the fake service.
> > +      service = do_CreateInstance(FAKE_TV_SERVICE_CONTRACTID, &rv);
> > +    } else {
> > +      // Fallback to the TV Simulator Service
> > +      service = do_CreateInstance("@mozilla.org/tv/simulatorservice;1", &rv);
> 
> We could register TV simulator service on the same contractId (i.e.
> @mozilla.org/tv/tvservice) and bundle different XPCOM implementation for
> simulator and real device at build time.

The original mechanism tries to use the real service and falls back to the fake service if it fails at run time. It looks ok for me to let the simulator service follow the same pattern at least for now. We may re-examine and adjust the overall flow into build time in a separate bug if we decide to do so in the future. :)
Hi Sean,

I have a question about current implementation.

In the TVServiceFactory::AutoCreateService,
It created the new TVSourceListener instance.[1]

TVSourceListener have a TVSource Array using by TVSourceListenr:RegisterSource.
So, TVSourceListeners which last called have not TVSource.
And TVSourceListeners::GetSource return nullptr. because this listener have not TVSources.[2] 
(Eventually, occurred Null Pointer Exception)

Is it correct to create TVSourceListeners in the TVSErviceFactory::AutoCreateService every time?
Should we create TVSourceListener when TVServiceFactory::AutoCreateService called first time only.


[1] https://dxr.mozilla.org/mozilla-central/rev/56f5ffd44b7f13d5db5bd497018f428dcbb28fc4/dom/tv/TVServiceFactory.cpp#37
[2] https://dxr.mozilla.org/mozilla-central/rev/56f5ffd44b7f13d5db5bd497018f428dcbb28fc4/dom/tv/TVListeners.cpp?offset=700#82
Flags: needinfo?(selin)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #35)
> TVSourceListener have a TVSource Array using by
> TVSourceListenr:RegisterSource.
> So, TVSourceListeners which last called have not TVSource.
> And TVSourceListeners::GetSource return nullptr. because this listener have
> not TVSources.[2] 
> (Eventually, occurred Null Pointer Exception)
> 
> Is it correct to create TVSourceListeners in the
> TVSErviceFactory::AutoCreateService every time?
> Should we create TVSourceListener when TVServiceFactory::AutoCreateService
> called first time only.
> 
It's expected to create TVSourceListener every time when |AutoCreateService| creates a new TV service instance. (We shouldn't create a TV service without any TVSourceListener.) Yet I don't get it why it eventually results in Null Pointer Exception. (At least FakeTVService runs fine with this mechanism, doesn't it?) So could you provide more details about how you get this exception?
Flags: needinfo?(selin) → needinfo?(mantaroh)
(In reply to Sean Lin [:seanlin] from comment #36)
> It's expected to create TVSourceListener every time when |AutoCreateService|
> creates a new TV service instance. (We shouldn't create a TV service without
> any TVSourceListener.) Yet I don't get it why it eventually results in Null
> Pointer Exception. (At least FakeTVService runs fine with this mechanism,
> doesn't it?) So could you provide more details about how you get this
> exception?
Thanks.

I misunderstood that TVService instance is one always.
But TVService was created every time when called TVServiceFactory::AUtoCreateTVService(), 
and TVSourceListener was created every time too.
Do we have the same understanding?

If it is right,I should delete code from TVSimulator.js as follow.
> _xpcom_factory: XPCOMUtils.generateSingletonFactory(TVSimulatorService),
Flags: needinfo?(mantaroh) → needinfo?(selin)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #37)
> (In reply to Sean Lin [:seanlin] from comment #36)
> > It's expected to create TVSourceListener every time when |AutoCreateService|
> > creates a new TV service instance. (We shouldn't create a TV service without
> > any TVSourceListener.) Yet I don't get it why it eventually results in Null
> > Pointer Exception. (At least FakeTVService runs fine with this mechanism,
> > doesn't it?) So could you provide more details about how you get this
> > exception?
> Thanks.
> 
> I misunderstood that TVService instance is one always.
> But TVService was created every time when called
> TVServiceFactory::AUtoCreateTVService(), 
> and TVSourceListener was created every time too.
> Do we have the same understanding?
> 
> If it is right,I should delete code from TVSimulator.js as follow.
> > _xpcom_factory: XPCOMUtils.generateSingletonFactory(TVSimulatorService),
Yes, please. :)

Btw, we have bug 1091478 to refactor TV service, so in the future the service will be singleton in each process and thus become less misleading as it is today.
Flags: needinfo?(selin)
Hi Sean,

I have a problem about the creating MediaStream in native code.
The created MediaStream can't set the DOMMediaElement.

The suggestion of comment #15 is using to MozCaptureStream().
But this way was occurred JSException like 'Value being assigned to HTMLMediaElement.mozSrcObject does not implement interface MediaStream'[1].

What do you know this phenomenon?

[1] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/dom/bindings/HTMLMediaElementBinding.cpp#1565
Flags: needinfo?(selin)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #39)
> Hi Sean,
> 
> I have a problem about the creating MediaStream in native code.
> The created MediaStream can't set the DOMMediaElement.
> 
> The suggestion of comment #15 is using to MozCaptureStream().
> But this way was occurred JSException like 'Value being assigned to
> HTMLMediaElement.mozSrcObject does not implement interface MediaStream'[1].
> 
> What do you know this phenomenon?
> 
First of all, I don't get it why we need to assign the created media stream to the media element in native code. The created media stream should be accessible to the application script via |TVTuner.stream|, shouldn't it? Then it looks more straight forward to me to assign the stream to |HTMLMediaElement.srcObject| in application script (JS code). (And that's what we expect app developers to do, right?!)

Besides, according to the example provided in comment 15 [1], the returned stream of |MozCaptureStream| is in |nsRefPtr<DOMMediaStream>|. And it appears compatible with the rest of our implementation.

By the way, according to bug 1183495, mozSrcObject appears to become deprecated soon. Is there any specific reason to keep using it in this scenario?

Yet it would be helpful to upload a WIP patch when it comes to discuss about such implementation dependent problems, and make it easier to clarify what the actual issue is.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/webaudio/AudioContext.cpp#361
Flags: needinfo?(selin)
(In reply to Sean Lin [:seanlin] from comment #40)
> Then it looks more straight forward to me to assign the stream to
> |HTMLMediaElement.srcObject| in application script (JS code). (And that's
> what we expect app developers to do, right?!)
I tried the |HTMLMediaElement.srcObject|.
The result is same.

I created the few patches.
#1 Add The TV Manager 

I created the few patch.
#1 add the TV Manager API interface and implementation.
#2 add the providing MediaStream code.
#3 add the dummy data to gaia profile.

When you have a time, Could you review these patches ?
Attachment #8655738 - Attachment is obsolete: true
Comment on attachment 8658574 [details] [diff] [review]
PART1.Add TVSimulator Service for TV Simulator

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #41)
Sorry, The comment #41 is in the middle of writing.

(In reply to Sean Lin [:seanlin] from comment #40)
> Yet it would be helpful to upload a WIP patch when it comes to discuss about
> such implementation dependent problems, and make it easier to clarify what
> the actual issue is.
I created the some patches.

#1 Add TVSimulator Service for TV Simulator
#2 Add the providing MediaStream code.
#3 Add the dummy data to gaia profile.

#2 is WIP code.

When you have a time, Could you please review and feedback ?
Attachment #8658574 - Flags: review?(selin)
I created the VideoElement in the TVSimulatorService.

When change the channel, we should change the video source.
So It is easier to have video element in TVSimulatorService than to have in native.
Attachment #8658578 - Flags: feedback?(selin)
Comment on attachment 8658574 [details] [diff] [review]
PART1.Add TVSimulator Service for TV Simulator

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

Since the provided JSON data could be fully user defined in the future, we may consider to have a follow-up bug to add some validation for the data integrity during importing, such as double checking no duplicate tuner ID or channel number, or the start time is earlier than the end time for a program, etc.

::: dom/tv/TVSimulatorService.js
@@ +107,5 @@
> +      debug("File load error: " + e);
> +      return;
> +    }
> +
> +    this._internalTuners = new Array();

It'd better to have the tuners in a map with their IDs as the keys since we have many functions (setSource, setChannel, etc) passing tuner ID in nsITVService.

@@ +120,5 @@
> +                                    tunerData.supportedType);
> +
> +      let wrapTunerData = {
> +        'tuner': tuner,
> +        'channelList': new Map(),

nit: 'channelList' becomes less accurate since it's actually a map now. What about rename it to |channels| (just like the pattern of |_internalTuners|)?

@@ +143,5 @@
> +          channel.isFree            = channelData.isFree;
> +
> +          let wrapChannelData = {
> +            'channel': channel,
> +            'programList': new Array(),

nit: What about rename it to |programs| (just like the pattern of |_internalTuners|)?

@@ +170,5 @@
> +      this._internalTuners.push(wrapTunerData);
> +    }
> +  },
> +
> +  getTuners: function TVSimGetTuners(callback) {

nit: Some other JS implementations appear to prepend 'a' for argument names (ex. aCallback). Please consider to follow the convention.
Same to other places in this file.

@@ +194,5 @@
> +    }
> +
> +    for each (let wrapTunerData in this._internalTuners) {
> +      for each (let dataSourceType in wrapTunerData.tuner.getSupportedSourceTypes()) {
> +        if (dataSourceType === sourceType) {

In addition to check the source type, you should also check if the tuner ID is matched.

@@ +210,5 @@
> +      debug("[TVSimulatorService] callback is null\n");
> +      return Cr.NS_ERROR_INVALID_ARG;
> +    }
> +
> +    callback.notifySuccess(null);

Please ensure the tuner with the given ID really supports the given source type. Otherwise, it needs to invoke |notifyError| and the timer for stopping scanning shouldn't be set.

@@ +212,5 @@
> +    }
> +
> +    callback.notifySuccess(null);
> +
> +    this._eitBroadcastTimer = Cc["@mozilla.org/timer;1"]

It'd better to simulate |NotifyChannelScanned| as well.

Besides, I think you may remove |NotifyEITBroadcasted| simulation for now, and in the future we may enhance the JSON file to allow users to configure what programs to use and the timing to fire this event.

@@ +224,5 @@
> +                                                  Ci.nsITimer.TYPE_ONE_SHOT);
> +    return Cr.NS_OK;
> +  },
> +
> +  notify: function TVSimTimerCallback(aTimer) {

Please add |Ci.nsITimerCallback| to the |QueryInterface| of this class.

@@ +258,5 @@
> +      this._scanCompleteTimer.cancel();
> +      this._scanCompleteTimer = null;
> +    }
> +
> +    this._sourceListener.notifyChannelScanStopped(

Please ensure the tuner with the given ID really supports the given source type.

@@ +265,5 @@
> +
> +    return callback.notifySuccess(null);
> +  },
> +
> +  clearScanningChannels: function TVSimClearScanningChannels(tunerId, sourceType, callback) {

The correct function name is "clearScannedChannelsCache".

@@ +284,5 @@
> +      if (wrapTunerData.tuner.id === tunerId &&
> +          wrapTunerData.sourceType === sourceType) {
> +        // Bug 1180589 : We should change video source
> +
> +        channel.appendElement(wrapTunerData.channelList.get(channelNumber).channel, false);

|get(channelNumber)| might get null, so directly append |.channel| doesn't look safe. And in this case it should call |notifyError|.

@@ +294,5 @@
> +  },
> +
> +  getChannels: function TVSimGetChannels(tunerId, sourceType, callback) {
> +    if (!callback) {
> +        debug("[TVSimulatorService] callback is null\n");

nit: indentation.

@@ +309,5 @@
> +       let wrapChannelData = channelDataEntries.next();
> +       do {
> +         channelArray.appendElement(wrapChannelData.value[1].channel, false);
> +         wrapChannelData = channelDataEntries.next();
> +       } while(wrapChannelData.value);

The channel list might be empty (in the case to simulate somewhat no channel is found) and this function could still be called. So you may need to check if the iterator has next value before accessing the elements.

Besides, please ensure the channels are ordered by their channel numbers. (Otherwise, you may need to sort the channels when importing from the JSON. The JSON could be user defined in the future and doesn't guarantee the channel ordering.) https://dxr.mozilla.org/mozilla-central/source/dom/tv/nsITVService.idl#338

@@ +318,5 @@
> +  },
> +
> +  getPrograms: function TVSimGetPrograms(tunerId, sourceType, channelNumber, startTime, endTime, callback) {
> +    if (!callback) {
> +        debug("[TVSimulatorService] callback is null\n");

nit: indentation.

@@ +327,5 @@
> +
> +    for each (let wrapTunerData in this._internalTuners) {
> +      if (wrapTunerData.tuner.id === tunerId &&
> +          wrapTunerData.sourceType === sourceType) {
> +        for each (let program in wrapTunerData.channelList.get(channelNumber).programList) {

|get(channelNumber)| might get null, so directly append |.channel| doesn't look safe.

@@ +333,5 @@
> +        }
> +      }
> +    }
> +
> +    return callback.notifySuccess(programArray);

Please ensure the programs are ordered by the start time. https://dxr.mozilla.org/mozilla-central/source/dom/tv/nsITVService.idl#353

@@ +361,5 @@
> +  getSimulatorVideoFilePath: function TVSimGetSimulatorVideoFilePath(tunerId, sourceType, channelNumber) {
> +    for each (let wrapTunerData in this._internalTuners) {
> +      if (wrapTunerData.tuner.id === tunerId &&
> +          wrapTunerData.sourceType === sourceType) {
> +        return wrapTunerData.channelList.get(channelNumber).videoFilePath;

|get(channelNumber)| might get null, so directly append |.channel| doesn't look safe.

::: dom/tv/TVTuner.h
@@ +67,5 @@
>    bool Init(nsITVTunerData* aData);
>  
>    nsresult InitMediaStream();
>  
> +  already_AddRefed<DOMMediaStream> CreateSimulatedMediaStream(nsIDOMWindow* aWindow);

No need to have the parameter |nsIDOMWindow* aWindow|. You can use |GetOwner()| in this method to get the inner window in |nsPIDOMWindow| which IMO is good enough to use.

::: dom/tv/moz.build
@@ +41,5 @@
> +    'TVSimulatorService.js',
> +    'TVSimulatorService.manifest',
> +]
> +
> +LOCAL_INCLUDES += [

Just out of curiosity, why do we need to add these local includes?
Attachment #8658574 - Flags: review?(selin) → review-
Comment on attachment 8658578 [details] [diff] [review]
PART2. Add the providing MediaStream code.

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

Actually I don't think the simulator should affect the DOM structure of the document, which means any |document.body.appendChild| call introduced by the simulator should be prevented. (You may call |document.createElement| to create a video element to load the local video file, but that doesn't seem necessary to append the element to the document body.)

Besides, in order to have a clear structure between components, the simulator service is better _not_ to get involved in anything about the window (browsing context). That's why it's suggested to create the video element and use |MozCaptureStream| to get the stream in TVTuner instead of in the simulator service.

And when it comes to simulate the situation that a different video source is played when the selected channel changes, it may be better to assign the new file path to the video tag and get a new stream by using |MozCaptureStream| again. So you may probably have a new public method like |ReloadStream| in TVTuner to generate a new stream, and let it called from [1] in TVSource handling changes for the current channel.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/tv/TVSource.cpp#135

Please note under the current mechanism stated above, the application may need to get the new stream by accessing |TVTuner.stream| again once it receives |oncurrentchannelchanged| event. It looks OK at least for now. (The current spec hasn't clearly addressed the lifetime of the media stream of the tuner. We may need to let the spec clarify this part.) And ideally we may maintain only one stream instance for each tuner, so we could have a follow-up bug to have a specific implementation for simulator media streams.

::: dom/tv/TVSimulatorService.js
@@ +394,5 @@
> +      this._internalVideoElem = win.document.createElement("video");
> +      this._internalVideoElem.id = "internalTvVideo";
> +      this._internalVideoElem.autoplay = true;
> +      this._internalVideoElem.addEventListener("ended", function() { videoElem.play(); });
> +      win.document.body.appendChild(this._internalVideoElem);

We should try _not_ to affect the DOM structure of the document, and _not_ to let the service involved with the browsing context either.

::: dom/tv/TVTuner.cpp
@@ +231,5 @@
> +    return nullptr;
> +  }
> +
> +  nsCOMPtr<nsIDOMWindow> win;
> +  nsrv = mediator->GetMostRecentWindow(MOZ_UTF16("navigator:browser"), getter_AddRefs(win));

Based on comment 15 and comment 16, |GetOwner()| could be used to get the window in |nsPIDOMWindow| type, and it has a method |GetExtantDoc()| to get the document.
Attachment #8658578 - Flags: feedback?(selin)
Sean, 

Thank you for your confirmation.
Almost of code was fixed, and I'm testing now.
I can resubmit patch after test.

I have a some question about your comment #45.

(In reply to Sean Lin [:seanlin] from comment #45)
> Comment on attachment 8658574 [details] [diff] [review]
> PART1.Add TVSimulator Service for TV Simulator
> 
> Review of attachment 8658574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since the provided JSON data could be fully user defined in the future, we
> may consider to have a follow-up bug to add some validation for the data
> integrity during importing, such as double checking no duplicate tuner ID or
> channel number, or the start time is earlier than the end time for a
> program, etc.
I'll file the bug in order to validate json format.

> @@ +210,5 @@
> > +      debug("[TVSimulatorService] callback is null\n");
> > +      return Cr.NS_ERROR_INVALID_ARG;
> > +    }
> > +
> > +    callback.notifySuccess(null);
> 
> Please ensure the tuner with the given ID really supports the given source
> type. Otherwise, it needs to invoke |notifyError| and the timer for stopping
> scanning shouldn't be set.
I think that it is not necessary to invoke |notifyError|.
The parameter of this function was set by |TVSource::StartScanning|.[1]
I guess that it should return error code.(like Cr.NS_ERROR_INVALID_ARG)
What do you think about this error policy?

> ::: dom/tv/moz.build
> @@ +41,5 @@
> > +    'TVSimulatorService.js',
> > +    'TVSimulatorService.manifest',
> > +]
> > +
> > +LOCAL_INCLUDES += [
> 
> Just out of curiosity, why do we need to add these local includes?
Oh, I'm going to use the HTMLMediaElement.
So it is not necessary this patch.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/tv/TVSource.cpp#258
Flags: needinfo?(selin)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #47)
> Sean, 
> 
> Thank you for your confirmation.
> Almost of code was fixed, and I'm testing now.
> I can resubmit patch after test.
> 
> I have a some question about your comment #45.
> 
> > @@ +210,5 @@
> > > +      debug("[TVSimulatorService] callback is null\n");
> > > +      return Cr.NS_ERROR_INVALID_ARG;
> > > +    }
> > > +
> > > +    callback.notifySuccess(null);
> > 
> > Please ensure the tuner with the given ID really supports the given source
> > type. Otherwise, it needs to invoke |notifyError| and the timer for stopping
> > scanning shouldn't be set.
> I think that it is not necessary to invoke |notifyError|.
> The parameter of this function was set by |TVSource::StartScanning|.[1]
> I guess that it should return error code.(like Cr.NS_ERROR_INVALID_ARG)
> What do you think about this error policy?
> 
That also looks good to me. Thank you.
Flags: needinfo?(selin)
Attached patch bug1180589.part1.patch (obsolete) — Splinter Review
Hi sean.

Thank you for your confirmation!
I've fixed your pointing out.

In addition to this, fixed |TVSimulator.manifest|.

There are |category profile-after-change| description.
But you said that it's create nsITVService instance when call the |AutoCreateService()| in comment #36.
We don't have to create TVSimulatorService when boot the simulator.

So I deleted that description.
Attachment #8658574 - Attachment is obsolete: true
Attachment #8659697 - Flags: review?(selin)
Comment on attachment 8659697 [details] [diff] [review]
bug1180589.part1.patch

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

There are still some issues, especially for channel scanning. So I'd like to see them fixed before r+.

::: dom/tv/TVSimulatorService.js
@@ +55,5 @@
> +    try {
> +      fstream.init(file, -1, 0, 0);
> +      cstream.init(fstream, 
> +                   "UTF-8", 
> +                   1024, 

nit: tailing space at this line and the above two.

@@ +77,5 @@
> +       * Setting JSON file format:
> +       *
> +       * Note: This setting JSON is not allow empty array.
> +       *       If set the empty array, _init() will fail.
> +       *       e.g. 

nit: tailing space.

@@ +179,5 @@
> +          }
> +          wrapTunerData.channels.set(channel.number, wrapChannelData);
> +        }
> +        this._internalTuners.set(
> +                this._getTunerMapKey(tuner.id, sourceData.type), 

nit: tailing space.

@@ +229,5 @@
> +    }
> +
> +    aCallback.notifySuccess(null);
> +
> +    wrapTunerData = this._getFirstWrapTunerData();

Why do we need to get the channels belonging to the "first" tuner instead of the ones belonging to the tuner with |aTunerId| and |aSourceType|? This may cause issues. For example, tuner T1 supports source S1 and has channel C1; whereas tuner T2 supports source S2 and has channel C2. And let's assume the tuner map would put the data for T1 at the first place, so |_getFirstWrapTunerData()| always returns the data for T1. Then when it comes to call |startScanningChannels(T2, S2)|, the simulator will trigger |notifyChannelScanned| for channel C1, which actually doesn't belong to T2, instead of the expected C2. So it looks more appropriate to me to keep the result of |_getWrapTunerData(aTunerId, aSourceType)|.

@@ +236,5 @@
> +    }
> +
> +    let wrapChannelData = wrapTunerData.channels.entries().next().value[1];
> +    if (!wrapChannelData) {
> +      return Cr.NS_ERROR_INVALID_ARG;

IMO it's legitimate to have no channel for a given tuner/source, so the implementation shouldn't treat it as an error. The user might wanna simulate the case that no channel is currently available for a given tuner/source. So the channels for this tuner/source are intended to be empty in the imported JSON.

@@ +239,5 @@
> +    if (!wrapChannelData) {
> +      return Cr.NS_ERROR_INVALID_ARG;
> +    }
> +
> +    this._sourceListener.notifyChannelScanned(

It's better to invoke |notifyChannelScanned| as many times as the number of channels available for the given tuner/source, in comparison to hardcoding for only one channel.

@@ +240,5 @@
> +      return Cr.NS_ERROR_INVALID_ARG;
> +    }
> +
> +    this._sourceListener.notifyChannelScanned(
> +                                      wrapTunerData.tuner.id, 

nit: tailing space.

@@ +251,5 @@
> +    this._eitBroadcastTimer = Cc["@mozilla.org/timer;1"]
> +                                .createInstance(Ci.nsITimer);
> +    let rv = this._eitBroadcastTimer.initWithCallback(this, 10,
> +                                                      Ci.nsITimer.TYPE_ONE_SHOT);
> +    */

Let's simply remove it instead of commenting out. (In the future if we allow the user to predefine the timing of EIT broadcasting in the imported JSON, the logic to set the timer is not necessarily here.)

@@ +266,5 @@
> +    if (!wrapTunerData) {
> +      return;
> +    }
> +
> +    if (aTimer === this._eitBroadcastTimer) {

Please remove the logic for EIT broadcasting for now. (In the future if we allow the user to predefine the timing and the data of EIT broadcasting in the imported JSON, the implementation is not necessarily like this.)

@@ +320,5 @@
> +    // SimulatorService support first tuner data only.
> +    wrapTunerData = this._getFirstWrapTunerData();
> +    this._sourceListener.notifyChannelScanStopped(
> +                               wrapTunerData.tuner.id,
> +                               wrapTunerData.sourceType);

|notifyChannelScanStopped| should use |aTunerId| and |aSourceType| rather than the ones getting from |_getFirstWrapTunerData()|. Then it doesn't have to support the first tuner data only.

@@ +369,5 @@
> +      return Cr.NS_ERROR_INVALID_ARG;
> +    }
> +
> +    // sorted as Array
> +    let sortedChannelData = new Map([...wrapTunerData.channels.entries()].sort(function(a,b) {

Why don't you just take the sorted array and use it in the follow-up for loop to compose the |nsIMutableArray|? So you don't need to construct a new map from the sorted array and the loop through the map to get all elements.

nit: function(a,b) => function(a, b)
nit: please add more accurate comments like "Sort the channels according to their channel numbers."

@@ +374,5 @@
> +      // a and b is same entries().value
> +      return a[0] - b[0];
> +    }));
> +
> +    for (let[k, wrapChannelData] of sortedChannelData) {

nit: let[k, wrapChannelData] => let [key, wrapChannelData]

@@ +395,5 @@
> +      return Cr.NS_ERROR_INVALID_ARG;
> +    }
> +
> +    let wrapChannelData = wrapTunerData.channels.get(aChannelNumber);
> +    if (!wrapChannelData || !wrapChannelData.programs) {

IMO it's legitimate to have no programs. Just return an empty array in this case.

@@ +399,5 @@
> +    if (!wrapChannelData || !wrapChannelData.programs) {
> +      return Cr.NS_ERROR_INVALID_ARG;
> +    }
> +
> +    // sort as Array

nit: please add more accurate comments like "Sort the programs according to the start time."

@@ +441,5 @@
> +
> +    return wrapTunerData.channels.get(aChannelNumber).videoFilePath;
> +  },
> +
> +  _getFirstWrapTunerData: function TVSimGetFirstWrapTunerData() {

No need to have this function.

::: dom/tv/TVTuner.h
@@ +67,5 @@
>    bool Init(nsITVTunerData* aData);
>  
>    nsresult InitMediaStream();
>  
> +  already_AddRefed<DOMMediaStream> CreateSimulatedMediaStream(nsIDOMWindow* aWindow);

As mentioned in comment 45, there's no need to have the parameter |nsIDOMWindow* aWindow|. You can use |GetOwner()| in this method to get the inner window in |nsPIDOMWindow| which IMO is good enough to use.
Attachment #8659697 - Flags: review?(selin) → review-
(In reply to Sean Lin [:seanlin] from comment #50)
Thank you for reviewing so many time.

I modified the sorting position of  the channels and programs.
And removed unnecessary code.

> @@ +395,5 @@
> > +      return Cr.NS_ERROR_INVALID_ARG;
> > +    }
> > +
> > +    let wrapChannelData = wrapTunerData.channels.get(aChannelNumber);
> > +    if (!wrapChannelData || !wrapChannelData.programs) {
> 
> IMO it's legitimate to have no programs. Just return an empty array in this
> case.
I assumed this condition is invalid parameter,
because we created the array of TVProgram always.
Attachment #8659697 - Attachment is obsolete: true
Attachment #8660459 - Flags: review?(selin)
Sorry!
Previous patch is wrong Mercurial commit message.
Attachment #8660459 - Attachment is obsolete: true
Attachment #8660459 - Flags: review?(selin)
Attachment #8660461 - Flags: review?(selin)
Comment on attachment 8660461 [details] [diff] [review]
PART1. Add simulated code for TV Manager API.

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

r=me after the updates for the following comments are made.

::: dom/tv/TVSimulatorService.js
@@ +178,5 @@
> +            wrapChannelData.programs.push(program);
> +          }
> +
> +          // Sort the program according to the startTime
> +	  wrapChannelData.programs.sort(function(a,b) {

nit: function(a,b) => function(a, b)

@@ +185,5 @@
> +          wrapTunerData.channels.set(channel.number, wrapChannelData);
> +        }
> +
> +        // Sort the channel according to the channel number
> +        wrapTunerData.channels = new Map([...wrapTunerData.channels.entries()].sort(function(a,b) {

nit: function(a,b) => function(a, b)

@@ +219,5 @@
> +    }
> +
> +    let wrapTunerData = this._getWrapTunerData(aTunerId, aSourceType);
> +    if (!wrapTunerData) {
> +      return aCallback.notifyError(Ci.nsITVServiceCallback.TV_ERROR_FAILURE);

In |startScanningChannels()| it returns |Cr.NS_ERROR_INVALID_ARG| for |!wrapTunerData|, but here in |setSource| it uses the callback to notify the error. I'm fine to either way, but it'd better to adopt the same mechanism at these places no matter which one you decide to use.

@@ +239,5 @@
> +    }
> +
> +    aCallback.notifySuccess(null);
> +
> +    wrapTunerData = this._getWrapTunerData(aTunerId, aSourceType);

You don't need to make a duplicate |wrapTunerData = this._getWrapTunerData(aTunerId, aSourceType)| statement which you've already made at couple lines above.

@@ +241,5 @@
> +    aCallback.notifySuccess(null);
> +
> +    wrapTunerData = this._getWrapTunerData(aTunerId, aSourceType);
> +    if (!wrapTunerData || !wrapTunerData.channels || wrapTunerData.channels.size <= 0) {
> +      return Cr.NS_ERROR_INVALID_ARG;

It's legitimate to have no channels, so |wrapTunerData.channels.size <= 0| should be removed. And all the other checks should be moved upward right after |let wrapTunerData = this._getWrapTunerData(aTunerId, aSourceType);|.

@@ +244,5 @@
> +    if (!wrapTunerData || !wrapTunerData.channels || wrapTunerData.channels.size <= 0) {
> +      return Cr.NS_ERROR_INVALID_ARG;
> +    }
> +
> +    this._scanningWrapTunerData = wrapTunerData;

It's fine to use an internal member to track the scanning tuner, and it therefore implies the simulator only allows to scan one tuner at a time. (So it implicitly simulates a TV device with no parallel scanning.) Then it'd better to check if |this._scanningWrapTunerData| exists in |startScanningChannels()| before actually invoking |notifySuccess|. (NS_ERROR_DOM_INVALID_STATE_ERR may be used for this case.)

nit: you may consider to move this line to a place before |aCallback.notifySuccess(null)|.

@@ +246,5 @@
> +    }
> +
> +    this._scanningWrapTunerData = wrapTunerData;
> +
> +    for (let[key, wrapChannelData] of wrapTunerData.channels) {

nit: let[key, wrapChannelData] => let [key, wrapChannelData]  (Add a white space after 'let'.)

@@ +248,5 @@
> +    this._scanningWrapTunerData = wrapTunerData;
> +
> +    for (let[key, wrapChannelData] of wrapTunerData.channels) {
> +      this._sourceListener.notifyChannelScanned(
> +                                        wrapTunerData.tuner.id, 

nit: tailing space.

@@ +266,5 @@
> +      return;
> +    }
> +
> +    this._scanCompleteTimer = null;
> +    return this._sourceListener.notifyChannelScanComplete(

You probably miss |this._scanningWrapTunerData = null|.

@@ +276,5 @@
> +    if (!aCallback) {
> +      debug("aCallback is null\n");
> +      return Cr.NS_ERROR_INVALID_ARG;
> +    }
> +

It'd better to check if |this._scanningWrapTunerData| exists. (NS_ERROR_DOM_INVALID_STATE_ERR may be used when |!this._scanningWrapTunerData|.)

@@ +289,5 @@
> +    }
> +
> +    if (wrapTunerData.tuner.id === this._scanningWrapTunerData.tuner.id &&
> +        wrapTunerData.sourceType === this._scanningWrapTunerData.sourceType) {
> +      this._sourceListener.notifyChannelScanStopped(

You probably miss |this._scanningWrapTunerData = null|.

@@ +340,5 @@
> +    if (!wrapTunerData || !wrapTunerData.channels) {
> +      return Cr.NS_ERROR_INVALID_ARG;
> +    }
> +
> +    for (let[key, wrapChannelData] of wrapTunerData.channels) {

nit: let[key, wrapChannelData] => let [key, wrapChannelData] (Add a white space after 'let'.)
Attachment #8660461 - Flags: review?(selin) → review+
Chris, in this bug we need to create a simulated DOMMediaStream using a local file as a source. I'm not familiar with the details at all but, as I understand it, the current patch (attachment 8658578 [details] [diff] [review]) creates a video element in JS then uses native code to look up that element and extract its media stream. The suggestion is to create the element in native code as well.

Both arrangements seem somewhat clumsy but apparently it's not possible to create a DOMMediaStream without a corresponding element. Is that right?
Flags: needinfo?(cpearce)
Roc is the MediaStream expert, roc, can you answer birtle's question comment 55 please?
Flags: needinfo?(cpearce) → needinfo?(roc)
There are a few reasonable ways to generate a MediaStream for testing. One way is to use a media element. Another is to use getUserMedia with {video:true, fake:true} --- see dom/media/test/test_imagecapture.html for example.

Assuming you want actual video data in the stream, there's no easier way.
Flags: needinfo?(roc)
Roc, Chris
Thank you for your advice.

I'll try to create the MediaStream using MediaElement.

Sean,
I think that it is difficult to separate the TVSimulatorService and DOMWindow object.
So I would like to create function like getSimulatedVideoBlob as follow.

void getSimulatorVideoBlob(in DOMString tunerId,
                           in DOMString sourceType,
                           in DOMString channelNumber,
                           in nsIDOMWindow window,
                           [retval] out DOMString blob);

This function is creating the Blob from local video file.
TVTuner called this function and create the MediaElement using this blob.

The reason of changing this way is as follow:
 - We can't set local video path to MediaElement, However can set the blob url.
 - There aren't the way to create blob url in native.
 - TVSimulatorService can't get the Content Window itself.

I tried this way, 
confirmed that I can get the MediaStream object.

However, MozCaptureStream have bug 1177793.
I comfirmed that b2g37 is work well.
But current b2g doesn't work.
Perhaps it will be blocking-bug.

What do you think this way?
Flags: needinfo?(selin)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #58)
> Roc, Chris
> Thank you for your advice.
> 
> I'll try to create the MediaStream using MediaElement.
> 
> Sean,
> I think that it is difficult to separate the TVSimulatorService and
> DOMWindow object.
> So I would like to create function like getSimulatedVideoBlob as follow.
> 
> void getSimulatorVideoBlob(in DOMString tunerId,
>                            in DOMString sourceType,
>                            in DOMString channelNumber,
>                            in nsIDOMWindow window,
>                            [retval] out DOMString blob);
> 
> This function is creating the Blob from local video file.
> TVTuner called this function and create the MediaElement using this blob.
> 
> The reason of changing this way is as follow:
>  - We can't set local video path to MediaElement, However can set the blob
> url.
>  - There aren't the way to create blob url in native.
>  - TVSimulatorService can't get the Content Window itself.
> 
> I tried this way, 
> confirmed that I can get the MediaStream object.
> 
> However, MozCaptureStream have bug 1177793.
> I comfirmed that b2g37 is work well.
> But current b2g doesn't work.
> Perhaps it will be blocking-bug.
> 
> What do you think this way?
It looks good to me. Thank you.

And as to bug 1177793, could you set this bug (bug 1180589) depend on it and leave some comments in that bug mentioning TV simulator also gets blocked?
Flags: needinfo?(selin)
Depends on: 1177793
(In reply to Sean Lin [:seanlin] from comment #54)
> Comment on attachment 8660461 [details] [diff] [review]
> PART1. Add simulated code for TV Manager API.
> 
> Review of attachment 8660461 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me after the updates for the following comments are made.

I fixed some point of your comments.
Could you please review?
Attachment #8660461 - Attachment is obsolete: true
Attachment #8661709 - Flags: review?(selin)
Comment on attachment 8661709 [details] [diff] [review]
PART1. Add simulated code for TV Manager API.

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

Actually you don't need to request r? again after simply updating r+ comments in the previous review (assuming there's no significant logic change). In this case you may upload the updated patch and mark it as r+ on behalf of me. Yet I still appreciate it. :)
Attachment #8661709 - Flags: review?(selin) → review+
Sean,

As I mentioned comment #58,
I created the XPCOM function as getSimulatorVideoBlob.

This code is injecting video element to the App-Content when called the setChannel(),
and create MediaStream object from this video element.

Note:
I created the ReloadMediaStream function in the TVTuner.
However there are simulator code only, So we should implement the real device code.
Attachment #8658578 - Attachment is obsolete: true
Attachment #8661718 - Flags: review?(selin)
Comment on attachment 8661718 [details] [diff] [review]
PART2. Add the providing MediaStream code.

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

::: dom/tv/TVSimulatorService.js
@@ +402,1 @@
>        return null;

nit: should we return an empty string? (The native code uses a nsString to carry the returned value, but I'm not sure if it's safe enough to handle null.)

@@ +403,5 @@
>      }
>  
> +    let wrapChannelData = wrapTunerData.channels.get(aChannelNumber);
> +    if (!wrapChannelData) {
> +      return null;

Ditto.

@@ +407,5 @@
> +      return null;
> +    }
> +
> +    let movieFile = new File(this._getFilePath(wrapChannelData.videoFilePath));
> +    let movieBlobURL = aWin.URL.createObjectURL(movieFile);

nit: let's use the name 'video' like other places in this file instead of 'movie'.

::: dom/tv/TVSource.cpp
@@ +214,5 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
>    }
>  
> +  mTuner->ReloadMediaStream(ToTVSourceTypeStr(mType), aChannelNumber);

You shouldn't add the logic in already_AddRefed<Promise> SetCurrentChannel(const nsAString& aChannelNumber, ErrorResult& aRv). Instead, nsresult SetCurrentChannel(nsITVChannelData* aChannelData) [1] is the right place to do.

The latter gets called when the current channel is really about to change. And you should reload the media stream only when there's actual channel change. (Btw, please add NS_WARN_IF(NS_FAILED(rv)) check since ReloadMediaStream would become to return nsresult.)

[1] https://dxr.mozilla.org/mozilla-central/source/dom/tv/TVSource.cpp#108

::: dom/tv/TVTuner.cpp
@@ +209,5 @@
>    } else if (mStreamType == nsITVTunerData::TV_STREAM_TYPE_SIMULATOR) {
> +    stream = CreateSimulatedMediaStream();
> +    if (!stream) {
> +      return NS_ERROR_ABORT;
> +    }

The stream is legitimate to be null. [1] We should allow the simulator to do so if the user intends to simulate this scenario.

[1] http://seanyhlin.github.io/TV-Manager-API/#widl-TVTuner-stream

@@ +222,4 @@
>  {
> +  nsRefPtr<DOMMediaStream> stream = nullptr;
> +
> +  ErrorResult rv;

nit: in this file, when it comes to declare a local variable in ErrorResult, the convention is to name it as "ErrorResult error", so it would introduce less confusion with "nsresult rv".

@@ +224,5 @@
> +
> +  ErrorResult rv;
> +
> +  nsPIDOMWindow* win = GetOwner();
> +  nsIDocument* doc = win->GetExtantDoc();

nit: it doesn't seem you use variable "win" in other places. Then why don't you simply use GetOwner()->GetExtantDoc()?

@@ +227,5 @@
> +  nsPIDOMWindow* win = GetOwner();
> +  nsIDocument* doc = win->GetExtantDoc();
> +
> +  nsRefPtr<Element> elem = nullptr;
> +  elem = doc->CreateElement(VIDEO_TAG, rv);

nit: the convention in the file usually tries not to use unnecessary abbreviation. elem => element.
Same to the following "mediaElem".

nit: why not "nsRefPtr<Element> element = doc->CreateElement(VIDEO_TAG, rv)"?

@@ +228,5 @@
> +  nsIDocument* doc = win->GetExtantDoc();
> +
> +  nsRefPtr<Element> elem = nullptr;
> +  elem = doc->CreateElement(VIDEO_TAG, rv);
> +  if (rv.Failed()) {

Use NS_WARN_IF() to wrap your condition check if it's about to do some error handling. Then we would get some warning messages in debug mode. Same to other places in this patch.

@@ +237,5 @@
> +  HTMLMediaElement* mediaElem;
> +  if (domMediaElem) {
> +    nsCOMPtr<nsIContent> content(do_QueryInterface(domMediaElem));
> +    mediaElem = static_cast<HTMLMediaElement*>(content.get());
> +  }

Rather than two consecutive QueryInterface calls from Element to nsIDOMHTMLMediaElement and then to nsIContent, why don't you directly QueryInterface from Element to nsIContent?

@@ +247,5 @@
> +  mediaElem->SetAutoplay(true, rv);
> +  if (rv.Failed()) {
> +    return nullptr;
> +  }
> +  

nit: tailing space.

@@ +253,5 @@
> +  if (rv.Failed()) {
> +    return nullptr;
> +  }
> +
> +  nsCOMPtr<nsITVSimulatorService> simService(do_QueryInterface(mTVService));

nit: move this line down right before |simService->GetSimulatorVideoBlob()|. And you should ensure the result of |do_QueryInterface| is not null.

@@ +274,5 @@
> +      if (currentChannel) {
> +        currentChannel->GetNumber(currentChannelNumber);
> +      }
> +    }
> +  } else { 

nit: tailing space.

@@ +278,5 @@
> +  } else { 
> +    currentChannelNumber = aChannelNumber;
> +  }
> +
> +  if (mId.Length() > 0 && currentSourceType.Length() > 0 && currentChannelNumber.Length() > 0) {

You should make this check at the beginning of this method. And it should return null rather than a stream instance (what your current implementation does). Besides, mId is guaranteed non-empty in Init().

@@ +309,5 @@
> +TVTuner::ReloadMediaStream(const nsAString& aSourceType, const nsAString& aChannelNumber)
> +{
> +  if (aSourceType.Length() <= 0 || aChannelNumber.Length() <= 0) {
> +    return;
> +  }

if (NS_WARN_IF(aSourceType.IsEmpty() || aChannelNumber.IsEmpty())) {
  return NS_ERROR_INVALID_ARG;
}

@@ +311,5 @@
> +  if (aSourceType.Length() <= 0 || aChannelNumber.Length() <= 0) {
> +    return;
> +  }
> +
> +  nsRefPtr<DOMMediaStream> stream = nullptr;

Instead of having the same logic as that in |InitMediaStream|, why don't you just use |return InitMediaStream();|?

::: dom/tv/TVTuner.h
@@ +9,5 @@
>  
>  #include "mozilla/DOMEventTargetHelper.h"
>  // Include TVTunerBinding.h since enum TVSourceType can't be forward declared.
>  #include "mozilla/dom/TVTunerBinding.h"
> +#define VIDEO_TAG NS_LITERAL_STRING("video")

nit: please add an empty line to separate #include and #define sections.

@@ +59,5 @@
>    already_AddRefed<DOMMediaStream> GetStream() const;
>  
>    IMPL_EVENT_HANDLER(currentsourcechanged);
>  
> +  void ReloadMediaStream(const nsAString& aSourceType, const nsAString& aChannelNumber);

Instead of void, it'd better to declare the return type as nsresult. And it's not necessarily to have the two arguments. You can get the source type and its current channel from |mCurrentSource|.

@@ +71,5 @@
>  
>    nsresult InitMediaStream();
>  
> +  already_AddRefed<DOMMediaStream> CreateSimulatedMediaStream(const nsAString& aSourceType = EmptyString(),
> +                                                              const nsAString& aChannelNumber = EmptyString());

Ditto. No need to have the two arguments.

::: dom/tv/nsITVSimulatorService.idl
@@ +14,5 @@
>  [scriptable, uuid(f0ab9850-24b4-4f5d-83dd-0fea0c249ca1)]
>  interface nsITVSimulatorService : nsITVService
>  {
>    /*
> +   * Get the simulate movie blob-url.

nit: Get the URL of the simulated video blob.

@@ +20,5 @@
>     * @param tunerId       The ID of the tuner.
>     * @param sourceType    The source type to be used.
>     * @param channelNumber The LCN (Logical Channel Number) of the channel.
>     */
> +  void getSimulatorVideoBlob(in DOMString tunerId,

Since this method is supposed to get the blob URL rather than the blob object, it would be more accurate to rename to getSimulatorVideoBlobUrl. And "[retval] out DOMString blob" => "[retval] out DOMString blobUrl"
Attachment #8661718 - Flags: review?(selin) → review-
Hi Sean,

Thank you for confirm!

(In reply to Sean Lin [:seanlin] from comment #63)
> Comment on attachment 8661718 [details] [diff] [review]
> @@ +59,5 @@
> >    already_AddRefed<DOMMediaStream> GetStream() const;
> >  
> >    IMPL_EVENT_HANDLER(currentsourcechanged);
> >  
> > +  void ReloadMediaStream(const nsAString& aSourceType, const nsAString& aChannelNumber);
> 
> Instead of void, it'd better to declare the return type as nsresult. And
> it's not necessarily to have the two arguments. You can get the source type
> and its current channel from |mCurrentSource|.
I have a question about TVSource lifecycle of TV Manager API specification.

Does TV Manager API allow to access the old TVSource object after called |TVTuner.setCurrentSource| function ?
e.g.
 let oldSource, newSource;
 tuner.setCurrentSource("isdb-t").then( function(source) { oldSource = source; });
 tuner.setCurrentSource("dvb-t").then( function(source) { newSource = source; });
 oldSource.setCurrentChannel(1);

Current TVSimulatorService is not consider about this calling way.
Flags: needinfo?(selin)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #64)
> (In reply to Sean Lin [:seanlin] from comment #63)
> > Instead of void, it'd better to declare the return type as nsresult. And
> > it's not necessarily to have the two arguments. You can get the source type
> > and its current channel from |mCurrentSource|.
> I have a question about TVSource lifecycle of TV Manager API specification.
> 
> Does TV Manager API allow to access the old TVSource object after called
> |TVTuner.setCurrentSource| function ?
> e.g.
>  let oldSource, newSource;
>  tuner.setCurrentSource("isdb-t").then( function(source) { oldSource =
> source; });
>  tuner.setCurrentSource("dvb-t").then( function(source) { newSource =
> source; });
>  oldSource.setCurrentChannel(1);
> 
> Current TVSimulatorService is not consider about this calling way.
That's a good question. In this case, since the old source is no longer the current source of the tuner, it's operation shouldn't take effect (or at least the stream of the tuner shouldn't be affected). The underlying TV service could do either way:

1. Trigger an error since it conflicts to some states (ex. the current source type of the tuner) maintained in the TV module.
2. Tolerate it but won't take any real effect.

The current simulator service doesn't appear to maintain states for non-scanning operations, so option 2 looks easier to do. Then in |nsresult SetCurrentChannel(nsITVChannelData* aChannelData)| [1], you may call |ReloadMediaStream| only when this source equals the current source of |mTuner|.

Actually this case is not the suggested and expected way to use TV API. Yet we may consider to add some clarifications to the spec to address this. Thank you.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/tv/TVSource.cpp#108
Flags: needinfo?(selin)
(In reply to Sean Lin [:seanlin] from comment #65)
> Then in |nsresult
> SetCurrentChannel(nsITVChannelData* aChannelData)| [1], you may call
> |ReloadMediaStream| only when this source equals the current source of
> |mTuner|.
I created the patch of several fixed.(including above modify)
Could you please review it?
Attachment #8661718 - Attachment is obsolete: true
Attachment #8662323 - Flags: review?(selin)
Comment on attachment 8658580 [details] [review]
[gaia] mantaroh:master > mozilla-b2g:master

oh, I forgot adding the review flag.

sean,
Could you please review this PR?

In this patch, added the config json.
However the contents of config json is sample.

I'm going to modify the contents of json by bug 1182413.
Attachment #8658580 - Flags: review?(selin)
Attached patch bug1180589.part2.patch (obsolete) — Splinter Review
Sean,

I changed code which uninitialized code.
This uninitialized code will happen error on OSX.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16432ec2147e
Attachment #8662323 - Attachment is obsolete: true
Attachment #8662323 - Flags: review?(selin)
Attachment #8662729 - Flags: review?(selin)
Comment on attachment 8662729 [details] [diff] [review]
bug1180589.part2.patch

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

::: dom/tv/TVSimulatorService.js
@@ +395,5 @@
>  
> +  getSimulatorVideoBlobURL: function TVSimGetSimulatorVideoBlob(aTunerId,
> +                                                             aSourceType,
> +                                                             aChannelNumber,
> +                                                             aWin) {

nit: fix the indentation.

::: dom/tv/TVSource.cpp
@@ +133,5 @@
>    mCurrentChannel = TVChannel::Create(GetOwner(), this, aChannelData);
>    NS_ENSURE_TRUE(mCurrentChannel, NS_ERROR_DOM_ABORT_ERR);
>  
> +  nsRefPtr<TVSource> currentSource = mTuner->GetCurrentSource();
> +  if (mType == currentSource->Type()) {

|currentSource| could be null. It's null safer to do the following.

if (currentSource && mType == currentSource->Type()) {
  ...
}

@@ +135,5 @@
>  
> +  nsRefPtr<TVSource> currentSource = mTuner->GetCurrentSource();
> +  if (mType == currentSource->Type()) {
> +    rv = mTuner->ReloadMediaStream(newChannelNumber);
> +    if (NS_FAILED(rv)) {

nit: use NS_WARN_IF to wrap the conditional check.

::: dom/tv/TVTuner.cpp
@@ +229,2 @@
>  {
> +  nsRefPtr<DOMMediaStream> stream = nullptr;

nit: no need to declare it here. Move down to the line "stream = mediaElement->MozCaptureStream(error);".

@@ +234,5 @@
> +    return nullptr;
> +  }
> +
> +  nsIDocument* doc = GetOwner()->GetExtantDoc();
> +

To be safer, please add

if (NS_WARN_IF(!doc)) {
  return nullptr;
}

@@ +248,5 @@
> +  }
> +
> +  if (!mediaElement) {
> +    return nullptr;
> +  }

nit: refactor the section in a slightly more decent manner.

nsCOMPtr<nsIContent> content(do_QueryInterface(element));
if (NS_WARN_IF(!content)) {
  return nullptr;
}

HTMLMediaElement* mediaElement = static_cast<HTMLMediaElement*>(content.get());
if (NS_WARN_IF(!mediaElement)) {
  return nullptr;
}

@@ +265,5 @@
> +  nsString currentVideoBlobUrl;
> +  nsresult rv;
> +
> +  nsCOMPtr<nsITVSimulatorService> simService(do_QueryInterface(mTVService));
> +  rv = simService->GetSimulatorVideoBlobURL(mId,

nit: use "nsresult rv = ...". Then you don't need a single line for "nsresult rv;".
nit: please add some null checks.

nsCOMPtr<nsIDOMWindow> domWin(do_QueryInterface(GetOwner()));
if (NS_WARN_IF(!domWin)) {
  return nullptr;
}

nsCOMPtr<nsITVSimulatorService> simService(do_QueryInterface(mTVService));
if (NS_WARN_IF(!simService)) {
  return nullptr;
}

nsString currentVideoBlobUrl;
nsresult rv = simService->GetSimulatorVideoBlobURL(...)

::: dom/tv/TVTuner.h
@@ +60,5 @@
>    already_AddRefed<DOMMediaStream> GetStream() const;
>  
>    IMPL_EVENT_HANDLER(currentsourcechanged);
>  
> +  nsresult ReloadMediaStream(const nsAString& aChannelNumber);

It's not necessarily to have channel number as the argument. You can get the current channel from |mCurrentSource|.

@@ +69,5 @@
>    ~TVTuner();
>  
>    bool Init(nsITVTunerData* aData);
>  
> +  nsresult InitMediaStream(const nsAString& aSourceType = EmptyString());

No need to have source type argument. Use the one from |mCurrentSource| instead.

@@ +74,2 @@
>  
> +  already_AddRefed<DOMMediaStream> CreateSimulatedMediaStream(const nsAString& aChannelNumber);

Ditto.
Attachment #8662729 - Flags: review?(selin) → review-
Comment on attachment 8658580 [details] [review]
[gaia] mantaroh:master > mozilla-b2g:master

I might not be the best person to address Gaia changes. So I'd like someone more familiar with Gaia structure to review this.

Ricky, could you help with it? This patch is primarily about adding a JSON file for the configuration and mocked data of TV API simulator. If you need some background info to catch up, please feel free to contact Evelyn or me.
Attachment #8658580 - Flags: review?(selin) → review?(rchien)
Comment on attachment 8658580 [details] [review]
[gaia] mantaroh:master > mozilla-b2g:master

LGTM. I didn't see any potential impacts.
Attachment #8658580 - Flags: review?(rchien) → review+
Hi sean, 

Thank you for your confirmation.
I modified some point, and deleted |channel| parameter of adding function.
I use |channelNumber| from current channel.

Could you please confirm this patch?
Attachment #8662729 - Attachment is obsolete: true
Attachment #8663458 - Flags: review?(selin)
Comment on attachment 8663458 [details] [diff] [review]
Part2. Add creating mediastream code.

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

Overall it looks good to me.

::: dom/tv/TVTuner.cpp
@@ +239,5 @@
> +    return nullptr;
> +  }
> +
> +  HTMLMediaElement* mediaElement = static_cast<HTMLMediaElement*>(content.get());
> +  if (!mediaElement) {

nit: NS_WARN_IF(!mediaElement)

@@ +274,5 @@
> +  }
> +
> +  nsString currentChannelNumber;
> +  currentChannel->GetNumber(currentChannelNumber);
> +  if (NS_WARN_IF(!currentChannel)) {

You don't need this. (|currentChannel| has been checked couple lines above.)

@@ +296,5 @@
> +
> +  // See Media Capture from DOM Elements spec.
> +  // http://www.w3.org/TR/mediacapture-fromelement/
> +  nsRefPtr<DOMMediaStream> stream = nullptr;
> +  stream = mediaElement->MozCaptureStream(error);

nit: why don't you simply use |nsRefPtr<DOMMediaStream> stream = mediaElement->MozCaptureStream(error);|?
Attachment #8663458 - Flags: review?(selin) → review+
Modified sean's comment #74.

This update is carrying forward r+.
Attachment #8663458 - Attachment is obsolete: true
Attachment #8663632 - Flags: review+
Oops, I forgot adding the checkin-needed flag.
Keywords: checkin-needed
Hi, so the checkin-needed request is for both gecko parts and the gaia pr request or ?
Flags: needinfo?(mantaroh)
Hi Tomcat,
Thank you for confirm the checkin-needed request.

My request is all patches as follow.
 attachment #8661709 [details] [diff] [review]
 attachment #8663632 [details] [diff] [review]
 attachment #8658580 [details] [review]
Flags: needinfo?(mantaroh)
Flags: needinfo?(cbook)
Hi Mantaroh, i got when applying this patch to b2g-inbound:

remote: *************************** ERROR ***************************
remote: Push rejected because the following IDL interfaces were
remote: modified without changing the UUID:
remote:   - nsITVService in changeset 774386cdd8a7
remote:   - nsITVServiceCallback in changeset 774386cdd8a7
remote:   - nsITVSimulatorService in changeset a535db2f2ed4
remote:   - nsITVSourceListener in changeset 774386cdd8a7
remote:   - nsITVTunerData in changeset 774386cdd8a7
remote: 
remote: To update the UUID for all of the above interfaces and their
remote: descendants, run:
remote:   ./mach update-uuids nsITVService nsITVServiceCallback nsITVSimulatorService nsITVSourceListener nsITVTunerData
remote: 
remote: If you intentionally want to keep the current UUID, include
remote: 'IGNORE IDL' in the commit message.


is this expected ?
Flags: needinfo?(cbook) → needinfo?(mantaroh)
Update UUID only.
This update is carrying forward r+.
Attachment #8661709 - Attachment is obsolete: true
Attachment #8666587 - Flags: review+
Update UUID only.
This update is carrying forward r+.
Attachment #8663632 - Attachment is obsolete: true
Attachment #8666588 - Flags: review+
Sorry,Previous patch(comment #84) is same old patch.
This update is carrying forward r+.

Hi Tomcat,
(In reply to Carsten Book [:Tomcat] from comment #81)
> is this expected ?
No.
I changed UUID only.

could you land it?
Attachment #8666588 - Attachment is obsolete: true
Flags: needinfo?(cbook)
Attachment #8666590 - Flags: review+
Hi Mantaroh, please squash your Gaia patch into just one commit. (You have 5 commits in your pull request now.) Thanks!
Flags: needinfo?(mantaroh)
Attachment #8658580 - Attachment is obsolete: true
Comment on attachment 8667075 [details] [review]
[gaia] mantaroh:master > mozilla-b2g:master

Hi evelyn,
I recreated pull request.
This update is carrying forward rchien: review+.
Flags: needinfo?(mantaroh)
Attachment #8667075 - Flags: review+
I pushed the Gecko part to m-i (I asked on #developers and was told it didn't matter if I pushed to m-i or b2g-inbound).

Marking leave-open since the Gaia part has yet to land.
Flags: needinfo?(cbook)
According to bug 1207030, add the -Wshadow option to moz.build.
But, it is affect attachement #8666587.
(We will load the VideoUtils.h when using HTMLMediaElement.)

This option is the cause of build error.[1][2]
As quick solution, I want to delete this option.

[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b4185d03b6d5
[2] https://treeherder.mozilla.org/logviewer.html#?job_id=14864396&repo=mozilla-inbound
Hi Glandium,

bug 1207030 is adding the -Wshadow option to the dom/tv/moz.build.
We need to HTMLMediaElement in order to simulate SmartTV videos.
However, HTMLMediaElement needs VideoUtils.h.

In VideoUtils.h, aWork and aCondition shadow the previous variable.[1]

So I would like to disable this option in the dom/tv/moz.build.

Because I don't know impact to modify this code,
As quick solution I would like to remove this option.

[1] https://dxr.mozilla.org/mozilla-central/rev/031db40e2b558c7e4dd0b4c565db4a992c1627c8/dom/media/VideoUtils.h#287
Attachment #8667130 - Flags: review?(mh+mozilla)
Attachment #8667130 - Flags: review?(mh+mozilla)
There is no reason changing variable names in the place you point to have any impact on how the code works.
(In reply to Mike Hommey [:glandium] from comment #94)
> There is no reason changing variable names in the place you point to have
> any impact on how the code works.
Glandium,
Thank you for your advice.
I'll modify the variable name in VideoUtils.h.

bholley,
I fixed the shadow variable name in VideoUtils.h.[1]
This variable is duplicate the outer variable.
This cause the build error when using -Wshadow option.

Could you please confirm this patch?

[1] https://dxr.mozilla.org/mozilla-central/rev/031db40e2b558c7e4dd0b4c565db4a992c1627c8/dom/media/VideoUtils.h#287
Attachment #8667130 - Attachment is obsolete: true
Attachment #8667144 - Flags: review?(bobbyholley)
Keywords: leave-open
Attachment #8667144 - Flags: review?(bobbyholley) → review+
Hi evelyn, sean,

Thank you for your help.

As you guys know, Smart TV simulator have some bug.
e.g. Configurable WebIDE / Distribute from WebIDE.

Perhaps I will request feedback and review.
Thank you for your cooperation.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Depends on: 1212286
Blocks: 1214115
Blocks: 1214513
Blocks: 1214520
See Also: → 1214520
Blocks: 1215429
No longer depends on: 1177793
You need to log in before you can comment on or make changes to this bug.