Comment 1 Gene Lian [:gene] (I already quit Mozilla) 2014-04-21 05:42:25 PDT

Because this API doesn't only work for controlling the TV tuners but also work for returning some general TV information like getTuners(), we should rename this API in a more generic way. That is, TV Manager API.

Comment 2 Gene Lian [:gene] (I already quit Mozilla) 2014-04-21 05:45:36 PDT

Created attachment 8409646 [details] [diff] [review]
Part 1, DOM API & WebIDL bindings (WIP)

Comment 3 Gene Lian [:gene] (I already quit Mozilla) 2014-04-30 02:26:59 PDT

Created attachment 8415121 [details] [diff] [review]
Part 1, DOM API & WebIDL bindings (WIP)

Comment 4 Gene Lian [:gene] (I already quit Mozilla) 2014-04-30 02:27:45 PDT

Created attachment 8415123 [details] [diff] [review]
Part 2, TVManagerService (WIP)

Comment 5 Gene Lian [:gene] (I already quit Mozilla) 2014-05-08 04:49:45 PDT

Created attachment 8419333 [details] [diff] [review]
Part 1, DOM API & WebIDL bindings (WIP)

Comment 6 Gene Lian [:gene] (I already quit Mozilla) 2014-05-08 04:50:11 PDT

Created attachment 8419334 [details] [diff] [review]
Part 2, TVManagerService (WIP)

Comment 7 Gene Lian [:gene] (I already quit Mozilla) 2014-05-08 04:50:52 PDT

Created attachment 8419335 [details] [diff] [review]
Part 3, IPC for API methods (WIP)

Comment 8 Gene Lian [:gene] (I already quit Mozilla) 2014-05-09 05:16:45 PDT

Created attachment 8420081 [details] [diff] [review]
Part 1, DOM API & WebIDL bindings (WIP)

Comment 9 Gene Lian [:gene] (I already quit Mozilla) 2014-05-09 05:21:56 PDT

Created attachment 8420085 [details] [diff] [review]
Part 2, TVManagerService fo Gonk (WIP)

Comment 10 Gene Lian [:gene] (I already quit Mozilla) 2014-05-09 05:24:50 PDT

Created attachment 8420086 [details] [diff] [review]
Part 3, IPC for API methods (WIP)

Comment 11 Gene Lian [:gene] (I already quit Mozilla) 2014-05-15 02:25:54 PDT

Created attachment 8423015 [details] [diff] [review]
Part 1, DOM API & WebIDL bindings (WIP)

Comment 12 Gene Lian [:gene] (I already quit Mozilla) 2014-05-15 02:26:16 PDT

Created attachment 8423016 [details] [diff] [review]
Part 2, TVManagerService fo Gonk (WIP)

Comment 13 Gene Lian [:gene] (I already quit Mozilla) 2014-05-15 02:26:37 PDT

Created attachment 8423017 [details] [diff] [review]
Part 3, IPC for API methods (WIP)

Comment 14 Gene Lian [:gene] (I already quit Mozilla) 2014-05-15 02:27:30 PDT

Created attachment 8423018 [details] [diff] [review]
Part 4, IPC for event handlers (WIP)

Comment 15 Gene Lian [:gene] (I already quit Mozilla) 2014-05-22 04:37:03 PDT

Created attachment 8426955 [details] [diff] [review]
Part 1, DOM API & WebIDL bindings, V1

Comment 16 Gene Lian [:gene] (I already quit Mozilla) 2014-05-22 04:37:26 PDT

Created attachment 8426956 [details] [diff] [review]
Part 2, TVManagerService fo Gonk, V1

Comment 17 Gene Lian [:gene] (I already quit Mozilla) 2014-05-22 04:38:00 PDT

Created attachment 8426957 [details] [diff] [review]
Part 3, IPC for API methods/attributes, V1

Comment 18 Gene Lian [:gene] (I already quit Mozilla) 2014-05-22 04:38:23 PDT

Created attachment 8426958 [details] [diff] [review]
Part 4, IPC for event handlers, V1

Comment 19 Gene Lian [:gene] (I already quit Mozilla) 2014-05-22 04:41:54 PDT

Patches are pretty much solid, except for how to connect to the TV hardware through HAL and how to handle the MediaStream as well as its IPC.

Comment 20 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-06 03:13:43 PDT

Created attachment 8468369 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v1

Implement DOM API & WebIDL bindings. (The details of each method could be left unimplemented until follow-up patches are made. Yet the whole patch should still be able to build without error.)

The latest design draft. FYI.
http://seanyhlin.github.io/TV-Manager-API/

Comment 21 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-07 11:57:53 PDT

I'll take a look at this next week.

Comment 22 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-13 02:27:19 PDT

Created attachment 8472216 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v2

Comment 23 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-13 15:46:20 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #21)
> I'll take a look at this next week.

Either tomorrow or Friday.  Sorry for the delay, it's been an exceptionally busy week.

Comment 24 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-14 14:22:11 PDT

Comment on attachment 8472216 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v2

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

Thanks for the patch, Sean.  Please see my comments below.  It would be awesome if you could upload an interdiff alongside the patch on the next iteration to make it easier to review the parts that have changed.

Are you going to address Jonas' feedback about returnTuner() on dev-webapi in the future?

::: dom/bindings/Bindings.conf
@@ +1401,5 @@
> +},
> +
> +'TVTunerChangedEvent': {
> +    'headerFile': 'mozilla/dom/TVEvent.h',
> +},

These suck...  Can't you just put these event interfaces in their own webidl files with the right name so that you don't need these?

::: dom/tv/moz.build
@@ +3,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +PARALLEL_DIRS += ['src']

These foo/src directories are really pointless and we're trying to get rid of them gradually.  Please just put the source files in dom/tv directly and get rid of dom/tv/src completely.

::: dom/tv/src/TVChannel.h
@@ +7,5 @@
> +#ifndef mozilla_dom_TVChannel_h__
> +#define mozilla_dom_TVChannel_h__
> +
> +#include "mozilla/DOMEventTargetHelper.h"
> +#include "mozilla/dom/Promise.h"

You should be able to remove this include in most of these headers and forward declare mozilla::dom::Promise.

@@ +8,5 @@
> +#define mozilla_dom_TVChannel_h__
> +
> +#include "mozilla/DOMEventTargetHelper.h"
> +#include "mozilla/dom/Promise.h"
> +#include "mozilla/dom/TVChannelBinding.h"

Can you avoid this include in this and the other headers?

@@ +62,5 @@
> +  IMPL_EVENT_HANDLER(currentprogramchanged);
> +  IMPL_EVENT_HANDLER(protectionstatechanged);
> +
> +protected:
> +  virtual ~TVChannel();

final classes don't need virtual destructors.  Also, please make this private.

(elsewhere in this patch too)

::: dom/tv/src/TVEvent.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

I believe all of these event classes can be automatically generated if you added TVEvent.webidl to GENERATED_EVENTS_WEBIDL_FILES in dom/webidl/moz.build.  So, I didn't review this file.

::: dom/tv/src/TVEvent.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

I didn't review this either.

::: dom/tv/src/TVManager.cpp
@@ +49,5 @@
> +  nsCOMPtr<nsIScriptContext> scriptContext = sgo->GetContext();
> +  if (!scriptContext) {
> +    aRv.Throw(NS_ERROR_UNEXPECTED);
> +    return nullptr;
> +  }

What's the point of all of this?  Couldn't you just drop the Create() method and use the constructor to create the object?

::: dom/tv/src/TVProgram.h
@@ +26,5 @@
> +  explicit TVProgram(nsISupports* aOwner);
> +
> +  // WebIDL (internal functions)
> +
> +  nsISupports* GetParentObject() const;

FWIW you can easily inline this method.  Also in the future when you finish these stubs, a lot of the simpler ones (for example, simple getters and setters) can be inlined as well.

::: dom/tv/src/TVProgramList.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

This file needs to go away, so I didn't review it.

::: dom/tv/src/TVProgramList.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

This file needs to go away, so I didn't review it.

::: dom/tv/src/TVSource.cpp
@@ +95,5 @@
> +{
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetOwner());
> +  MOZ_ASSERT(global);
> +
> +  nsRefPtr<Promise> promise = Promise::Create(global, aRv);

It might be worth considering adding an overload of Promise::Create() which takes an nsPIDOMWindow* and use it everywhere here.  (Please do that in a separate bug though)

::: dom/tv/test/mochitest.ini
@@ +1,2 @@
> +[DEFAULT]
> +skip-if = e10s

Out of curiosity, do you skip these tests in e10s because they fail?

::: dom/webidl/Navigator.webidl
@@ +390,5 @@
>                       optional (ArrayBufferView or Blob or DOMString or FormData)? data = null);
>  };
> +
> +partial interface Navigator {
> +  [Throws, Pref="dom.tv.enabled", CheckPermissions="tv"]

Please use [AvailableIn=CertifiedApps], otherwise this will be exposed to non-certified apps which have the tv permission as well.

::: dom/webidl/TVChannel.webidl
@@ +17,5 @@
> +  unsigned long long duration;
> +  sequence<DOMString> eventIds;
> +};
> +
> +[NoInterfaceObject,

Why NoInterfaceObject?

@@ +19,5 @@
> +};
> +
> +[NoInterfaceObject,
> + Pref="dom.tv.enabled"]
> +interface TVChannel : EventTarget {

Please add entries for all of the interfaces that you're adding to test_interfaces.html, with the proper permission annotation.  (Note that this will only be necessary if you drop the [NoInterfaceObject]s.)

@@ +21,5 @@
> +[NoInterfaceObject,
> + Pref="dom.tv.enabled"]
> +interface TVChannel : EventTarget {
> +  [Throws]
> +  Promise getPrograms(optional TVGetProgramsOptions options);

All Promise's should now have the type to which they resolve too (such as Promise<Foo>).

::: dom/webidl/TVEvent.webidl
@@ +47,5 @@
> +
> +[Pref="dom.tv.enabled",
> + Constructor(DOMString type, optional TVEITBroadcastedEventInit eventInitDict)]
> +interface TVEITBroadcastedEvent : Event {
> +  readonly attribute TVProgramList programs;

So, we don't have proper way to express this in WebIDL yet, but in Gecko WebIDL, we can do the following:

[Pure, Cached] readonly attribute sequence<TVPorgram>? programs;

Please remove TVProgramList, use this syntax in your patch, and fix the spec to make this property an |object| and explain in the prose that the cached value for this array needs to go into an internal slot for the object, and it should be cached there until the next time that the underlying array changes when the cached value will be updated.  This will eventually get a nice syntax in the WebIDL spec.

::: dom/webidl/TVManager.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[NoInterfaceObject,

Why NoInterfaceObject?

::: dom/webidl/TVProgram.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[NoInterfaceObject, Pref="dom.tv.enabled"]

Why NoInterfaceObject?

::: dom/webidl/TVProgramList.webidl
@@ +6,5 @@
> +
> +[NoInterfaceObject,
> + ArrayClass,
> + Pref="dom.tv.enabled"]
> +interface TVProgramList {

As I said before, this interface can go away.

::: dom/webidl/TVSource.webidl
@@ +30,5 @@
> +  boolean isRescanned;
> +};
> +
> +[NoInterfaceObject,
> + Pref="dom.tv.enabled"]

Same as before, why NoInterfaceObject?

::: dom/webidl/TVTuner.webidl
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[NoInterfaceObject,
> + Pref="dom.tv.enabled"]

Same as before, why NoInterfaceObject?

Comment 25 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-14 21:07:33 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #24)
> Comment on attachment 8472216 [details] [diff] [review]
> New part 1 - DOM API & WebIDL bindings, v2
> 
> Review of attachment 8472216 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are you going to address Jonas' feedback about returnTuner() on dev-webapi
> in the future?

Actually we've replied to that email thread with more of our concerns about adding acquiring/releasing APIs on tuners, as well as the inclination of not doing it until TV Manager API becomes open to non-certified apps. And since there appears no further feedback on them, so far we don't have the plan to bring them into the current draft. Thoughts?!

> ::: dom/tv/src/TVManager.cpp
> @@ +49,5 @@
> > +  nsCOMPtr<nsIScriptContext> scriptContext = sgo->GetContext();
> > +  if (!scriptContext) {
> > +    aRv.Throw(NS_ERROR_UNEXPECTED);
> > +    return nullptr;
> > +  }
> 
> What's the point of all of this?  Couldn't you just drop the Create() method
> and use the constructor to create the object?
I found in Navagator.cpp we use static Create methods to initialize some singleton components (mozTelephony, mozPower, mozBluetooth, etc.) So I basically follow this pattern and plan to keep the constructor private.

Btw, thanks very much for the thoroughness. I'll make correspondent changes in the follow-up patch.

Comment 26 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-15 04:07:38 PDT

Created attachment 8473629 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v3

Comment 27 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-15 04:11:38 PDT

Created attachment 8473630 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, diff v2 v3

Updating based on Ehsan's comments.
Diff between v2 and v3. (Another standalone v3 patch is also attached in this bug FYI.)

Comment 28 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-15 07:18:40 PDT

(In reply to Sean Lin [:seanlin] from comment #25)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #24)
> > Comment on attachment 8472216 [details] [diff] [review]
> > New part 1 - DOM API & WebIDL bindings, v2
> > 
> > Review of attachment 8472216 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Are you going to address Jonas' feedback about returnTuner() on dev-webapi
> > in the future?
> 
> Actually we've replied to that email thread with more of our concerns about
> adding acquiring/releasing APIs on tuners, as well as the inclination of not
> doing it until TV Manager API becomes open to non-certified apps. And since
> there appears no further feedback on them, so far we don't have the plan to
> bring them into the current draft. Thoughts?!

Are we targeting a device with multiple tuners in the initial release?  If not, punting on this sounds fine...

> > ::: dom/tv/src/TVManager.cpp
> > @@ +49,5 @@
> > > +  nsCOMPtr<nsIScriptContext> scriptContext = sgo->GetContext();
> > > +  if (!scriptContext) {
> > > +    aRv.Throw(NS_ERROR_UNEXPECTED);
> > > +    return nullptr;
> > > +  }
> > 
> > What's the point of all of this?  Couldn't you just drop the Create() method
> > and use the constructor to create the object?
> I found in Navagator.cpp we use static Create methods to initialize some
> singleton components (mozTelephony, mozPower, mozBluetooth, etc.) So I
> basically follow this pattern and plan to keep the constructor private.

In C++ without exceptions (which we don't use) there is no good way to signal an error from a constructor, which is why sometimes people need to write these types of creator functions that can return a failure code if something goes wrong.  But in case there is nothing interesting to check at runtime, a simple constructor should be preferred.

Comment 29 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-15 14:13:32 PDT

Comment on attachment 8473630 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, diff v2 v3

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

This interdiff is a bit hard to use because you didn't use hg mv when moving stuff from dom/tv/src/* to dom/tv/*.  Can you please address the comments below and submit another interdiff from v2 with the files actually being moves, so that I can see the code changed and not a bunch of file removals and additions?  Thanks!

::: b2g/app/b2g.js
@@ +1010,5 @@
>  pref("dom.mapped_arraybuffer.enabled", true);
>  #endif
> +
> +// Enable TV Manager API
> +pref("dom.tv.enabled", true);

Do we really want to enable this API with a non-functional implementation in this patch?

::: dom/tests/mochitest/general/test_interfaces.html
@@ +1165,5 @@
>      {name: "TreeSelection", xbl: true},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "TreeWalker",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    {name: "TVChannel", b2g: true, pref: "dom.tv.enabled"},

You need to change these to be permission checks I think.  But please check to see how the pref and permission checks interact in this test...

::: dom/webidl/TVChannel.webidl
@@ +17,5 @@
>    unsigned long long duration;
>    sequence<DOMString> eventIds;
>  };
>  
> +[Pref="dom.tv.enabled"]

This needs [CheckPermissions="tv"].

::: dom/webidl/TVCurrentChannelChangedEvent.webidl
@@ +8,5 @@
> +  TVChannel? channel = null;
> +};
> +
> +[Pref="dom.tv.enabled",
> + Constructor(DOMString type, optional TVCurrentChannelChangedEventInit eventInitDict)]

This needs [CheckPermissions="tv"].

::: dom/webidl/TVCurrentProgramChangedEvent.webidl
@@ +8,5 @@
> +  TVProgram? program = null;
> +};
> +
> +[Pref="dom.tv.enabled",
> + Constructor(DOMString type, optional TVCurrentProgramChangedEventInit eventInitDict)]

This needs [CheckPermissions="tv"].

::: dom/webidl/TVCurrentSourceChangedEvent.webidl
@@ +8,5 @@
> +  TVSource? source = null;
> +};
> +
> +[Pref="dom.tv.enabled",
> + Constructor(DOMString type, optional TVCurrentSourceChangedEventInit eventInitDict)]

This needs [CheckPermissions="tv"].

::: dom/webidl/TVEITBroadcastedEvent.webidl
@@ +8,5 @@
> +  sequence<TVProgram> programs = [];
> +};
> +
> +[Pref="dom.tv.enabled",
> + Constructor(DOMString type, optional TVEITBroadcastedEventInit eventInitDict)]

This needs [CheckPermissions="tv"].

::: dom/webidl/TVManager.webidl
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/.
>   */
>  
> +[Pref="dom.tv.enabled"]

This needs [CheckPermissions="tv"].

::: dom/webidl/TVParentalControlChangedEvent.webidl
@@ +8,5 @@
> +  boolean isParentalControlLocked = false;
> +};
> +
> +[Pref="dom.tv.enabled",
> + Constructor(DOMString type, optional TVParentalControlChangedEventInit eventInitDict)]

This needs [CheckPermissions="tv"].

::: dom/webidl/TVProgram.webidl
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/.
>   */
>  
> +[Pref="dom.tv.enabled"]

This needs [CheckPermissions="tv"].

::: dom/webidl/TVProtectionStateChangedEvent.webidl
@@ +8,5 @@
> +  boolean isProtected = false;
> +};
> +
> +[Pref="dom.tv.enabled",
> + Constructor(DOMString type, optional TVProtectionStateChangedEventInit eventInitDict)]

This needs [CheckPermissions="tv"].

::: dom/webidl/TVScanningStateChangedEvent.webidl
@@ +16,5 @@
> +  TVChannel? channel = null;
> +};
> +
> +[Pref="dom.tv.enabled",
> + Constructor(DOMString type, optional TVScanningStateChangedEventInit eventInitDict)]

This needs [CheckPermissions="tv"].

::: dom/webidl/TVSource.webidl
@@ +29,5 @@
>  dictionary TVStartScanningOptions {
>    boolean isRescanned;
>  };
>  
> +[Pref="dom.tv.enabled"]

This needs [CheckPermissions="tv"].

::: dom/webidl/TVTuner.webidl
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/.
>   */
>  
> +[Pref="dom.tv.enabled"]

This needs [CheckPermissions="tv"].

::: dom/webidl/TVTunerChangedEvent.webidl
@@ +14,5 @@
> +  TVTuner? tuner = null;
> +};
> +
> +[Pref="dom.tv.enabled",
> + Constructor(DOMString type, optional TVTunerChangedEventInit eventInitDict)]

This needs [CheckPermissions="tv"].

Comment 30 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-17 20:34:56 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #28)
> (In reply to Sean Lin [:seanlin] from comment #25)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #24)
> > > Comment on attachment 8472216 [details] [diff] [review]
> > > New part 1 - DOM API & WebIDL bindings, v2
> > > 
> > > Review of attachment 8472216 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Are you going to address Jonas' feedback about returnTuner() on dev-webapi
> > > in the future?
> > 
> > Actually we've replied to that email thread with more of our concerns about
> > adding acquiring/releasing APIs on tuners, as well as the inclination of not
> > doing it until TV Manager API becomes open to non-certified apps. And since
> > there appears no further feedback on them, so far we don't have the plan to
> > bring them into the current draft. Thoughts?!
> 
> Are we targeting a device with multiple tuners in the initial release?  If
> not, punting on this sounds fine...
> 
Actually we're. But we may conitnue the discussion back to the dev-webapi thread. (Here's the link to the latest discussion about the two APIs. https://groups.google.com/forum/#!msg/mozilla.dev.webapi/IXG2NASKH3Q/ZJ1kk5rcFQ4J) If at last we still feel they're necessary for the initial release, we could add the implementation to a follow-up patch in this bug.

Comment 31 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-17 20:40:24 PDT

Created attachment 8474337 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, diff v2 v4

Add permission checks to most newly-added webidls.
Don't enable TV Manager APIs in B2G until they're fully implemented.
Simply use the constructor instead of a static Create() method to instantiate TVManager.

Comment 32 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-17 20:52:50 PDT

Created attachment 8474341 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, diff v2 v4 (new)

(Add a file missing in the previous diff.)

Add permission checks to most newly-added webidls.
Don't enable TV Manager APIs in B2G until they're fully implemented.
Simply use the constructor instead of a static Create() method to instantiate TVManager.

Comment 33 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-18 08:10:39 PDT

(In reply to Sean Lin [:seanlin] from comment #30)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #28)
> > (In reply to Sean Lin [:seanlin] from comment #25)
> > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > comment #24)
> > > > Comment on attachment 8472216 [details] [diff] [review]
> > > > New part 1 - DOM API & WebIDL bindings, v2
> > > > 
> > > > Review of attachment 8472216 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > Are you going to address Jonas' feedback about returnTuner() on dev-webapi
> > > > in the future?
> > > 
> > > Actually we've replied to that email thread with more of our concerns about
> > > adding acquiring/releasing APIs on tuners, as well as the inclination of not
> > > doing it until TV Manager API becomes open to non-certified apps. And since
> > > there appears no further feedback on them, so far we don't have the plan to
> > > bring them into the current draft. Thoughts?!
> > 
> > Are we targeting a device with multiple tuners in the initial release?  If
> > not, punting on this sounds fine...
> > 
> Actually we're. But we may conitnue the discussion back to the dev-webapi
> thread. (Here's the link to the latest discussion about the two APIs.
> https://groups.google.com/forum/#!msg/mozilla.dev.webapi/IXG2NASKH3Q/
> ZJ1kk5rcFQ4J) If at last we still feel they're necessary for the initial
> release, we could add the implementation to a follow-up patch in this bug.

Is there any reason not to support them in the initial release?

Comment 34 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-18 08:14:23 PDT

Comment on attachment 8474341 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, diff v2 v4 (new)

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

::: dom/webidl/Navigator.webidl
@@ +390,5 @@
>                       optional (ArrayBufferView or Blob or DOMString or FormData)? data = null);
>  };
>  
>  partial interface Navigator {
> +  [Throws, Pref="dom.tv.enabled", CheckPermissions="tv", AvailableIn=CertifiedApps]

Ah great, I was not sure that adding AvailableIn=CertifiedApps would make it harder to test this...  Now that you've successfully done that, please add AvailableIn=CertifiedApps to everywhere else where you have CheckPermisions="tv" on the interfaces too.

Comment 35 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-19 02:03:13 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #33)
> (In reply to Sean Lin [:seanlin] from comment #30)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #28)
> > > (In reply to Sean Lin [:seanlin] from comment #25)
> > > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > > comment #24)
> > > > > Comment on attachment 8472216 [details] [diff] [review]
> > > > > New part 1 - DOM API & WebIDL bindings, v2
> > > > > 
> > > > > Review of attachment 8472216 [details] [diff] [review]:
> > > > > -----------------------------------------------------------------
> > > > > 
> > > > > Are you going to address Jonas' feedback about returnTuner() on dev-webapi
> > > > > in the future?
> > > > 
> > > > Actually we've replied to that email thread with more of our concerns about
> > > > adding acquiring/releasing APIs on tuners, as well as the inclination of not
> > > > doing it until TV Manager API becomes open to non-certified apps. And since
> > > > there appears no further feedback on them, so far we don't have the plan to
> > > > bring them into the current draft. Thoughts?!
> > > 
> > > Are we targeting a device with multiple tuners in the initial release?  If
> > > not, punting on this sounds fine...
> > > 
> > Actually we're. But we may conitnue the discussion back to the dev-webapi
> > thread. (Here's the link to the latest discussion about the two APIs.
> > https://groups.google.com/forum/#!msg/mozilla.dev.webapi/IXG2NASKH3Q/
> > ZJ1kk5rcFQ4J) If at last we still feel they're necessary for the initial
> > release, we could add the implementation to a follow-up patch in this bug.
> 
> Is there any reason not to support them in the initial release?

One of the concerns we pointed out in the email thread is the following use case:

"While recording a channel, the user may still want to watch that channel at the same time. And it could be done by two apps (recording app and TV app).
(And if the user tries to switch channels, a pop-out will remind him/her of the fact that the recording would stop.)"

If app-based acquiring/releasing APIs are applied on tuners, it needs two dedicated tuners on the device to suffice that use case. However, we can barely keep the same experience on some current TVs which may not have sufficient numbers of tuners. And it might not be coherent with some of status quo under this situation. (Maybe someday in the future we can say for sure the limited number of tuners becomes no longer a concern for almost all TVs.)

On the other hand, so far the TV Manager API is only available for certified apps. So the downsides of not dedicating a tuner to an app appears comparatively limited. And they could also be well handled to some extent by those certified apps collaborated with Datastore APIs. (It could be sort of similar to the mechanism of using Datastore to handle contention-based time slots and tuners for recording scheduling.)

Yet we all agree the APIs of acuqiring/releasing tuners may become more important when it comes to bring more apps as the clients of TV Manager API. But in the initial release, we plan to provide a set of APIs with higher flexibility. Thus we're not inclined to bring them in until TV Manager API is ready for privileged apps someday. And we may also collect some more feedback after the initial release to see the trend of new API candidates.

Comment 36 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-19 02:09:48 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #34)
> Comment on attachment 8474341 [details] [diff] [review]
> New part 1 - DOM API & WebIDL bindings, diff v2 v4 (new)
> 
> Review of attachment 8474341 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/Navigator.webidl
> @@ +390,5 @@
> >                       optional (ArrayBufferView or Blob or DOMString or FormData)? data = null);
> >  };
> >  
> >  partial interface Navigator {
> > +  [Throws, Pref="dom.tv.enabled", CheckPermissions="tv", AvailableIn=CertifiedApps]
> 
> Ah great, I was not sure that adding AvailableIn=CertifiedApps would make it
> harder to test this...  Now that you've successfully done that, please add
> AvailableIn=CertifiedApps to everywhere else where you have
> CheckPermisions="tv" on the interfaces too.

Thank you Ehsan. I'll post an aggregated patch for this part as a reference base for the follow-up development. Btw, do I still need to request a super review with a DOM peer? Or you've already covered the role for this?!

Comment 37 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-19 12:23:18 PDT

(In reply to Sean Lin [:seanlin] from comment #36)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #34)
> > Comment on attachment 8474341 [details] [diff] [review]
> > New part 1 - DOM API & WebIDL bindings, diff v2 v4 (new)
> > 
> > Review of attachment 8474341 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/webidl/Navigator.webidl
> > @@ +390,5 @@
> > >                       optional (ArrayBufferView or Blob or DOMString or FormData)? data = null);
> > >  };
> > >  
> > >  partial interface Navigator {
> > > +  [Throws, Pref="dom.tv.enabled", CheckPermissions="tv", AvailableIn=CertifiedApps]
> > 
> > Ah great, I was not sure that adding AvailableIn=CertifiedApps would make it
> > harder to test this...  Now that you've successfully done that, please add
> > AvailableIn=CertifiedApps to everywhere else where you have
> > CheckPermisions="tv" on the interfaces too.
> 
> Thank you Ehsan. I'll post an aggregated patch for this part as a reference
> base for the follow-up development. Btw, do I still need to request a super
> review with a DOM peer? Or you've already covered the role for this?!

Yes, please do.  I'm not a DOM peer.  You can find the list here: https://wiki.mozilla.org/Modules/Core#Document_Object_Model

Comment 38 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-19 12:26:57 PDT

(In reply to Sean Lin [:seanlin] from comment #35)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #33)
> > (In reply to Sean Lin [:seanlin] from comment #30)
> > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > comment #28)
> > > > (In reply to Sean Lin [:seanlin] from comment #25)
> > > > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > > > comment #24)
> > > > > > Comment on attachment 8472216 [details] [diff] [review]
> > > > > > New part 1 - DOM API & WebIDL bindings, v2
> > > > > > 
> > > > > > Review of attachment 8472216 [details] [diff] [review]:
> > > > > > -----------------------------------------------------------------
> > > > > > 
> > > > > > Are you going to address Jonas' feedback about returnTuner() on dev-webapi
> > > > > > in the future?
> > > > > 
> > > > > Actually we've replied to that email thread with more of our concerns about
> > > > > adding acquiring/releasing APIs on tuners, as well as the inclination of not
> > > > > doing it until TV Manager API becomes open to non-certified apps. And since
> > > > > there appears no further feedback on them, so far we don't have the plan to
> > > > > bring them into the current draft. Thoughts?!
> > > > 
> > > > Are we targeting a device with multiple tuners in the initial release?  If
> > > > not, punting on this sounds fine...
> > > > 
> > > Actually we're. But we may conitnue the discussion back to the dev-webapi
> > > thread. (Here's the link to the latest discussion about the two APIs.
> > > https://groups.google.com/forum/#!msg/mozilla.dev.webapi/IXG2NASKH3Q/
> > > ZJ1kk5rcFQ4J) If at last we still feel they're necessary for the initial
> > > release, we could add the implementation to a follow-up patch in this bug.
> > 
> > Is there any reason not to support them in the initial release?
> 
> One of the concerns we pointed out in the email thread is the following use
> case:
> 
> "While recording a channel, the user may still want to watch that channel at
> the same time. And it could be done by two apps (recording app and TV app).
> (And if the user tries to switch channels, a pop-out will remind him/her of
> the fact that the recording would stop.)"
> 
> If app-based acquiring/releasing APIs are applied on tuners, it needs two
> dedicated tuners on the device to suffice that use case. However, we can
> barely keep the same experience on some current TVs which may not have
> sufficient numbers of tuners. And it might not be coherent with some of
> status quo under this situation. (Maybe someday in the future we can say for
> sure the limited number of tuners becomes no longer a concern for almost all
> TVs.)

I'm now completely confused!  I first asked whether we are targeting a device with multiple tuners precisely because I was thinking of this exact discussion in the email thread.  :-)  And you said yes we do, but it seems from this paragraph that the device that we're targeting has one tuner.  So, which one is it?

> On the other hand, so far the TV Manager API is only available for certified
> apps. So the downsides of not dedicating a tuner to an app appears
> comparatively limited. And they could also be well handled to some extent by
> those certified apps collaborated with Datastore APIs. (It could be sort of
> similar to the mechanism of using Datastore to handle contention-based time
> slots and tuners for recording scheduling.)
> 
> Yet we all agree the APIs of acuqiring/releasing tuners may become more
> important when it comes to bring more apps as the clients of TV Manager API.
> But in the initial release, we plan to provide a set of APIs with higher
> flexibility. Thus we're not inclined to bring them in until TV Manager API
> is ready for privileged apps someday. And we may also collect some more
> feedback after the initial release to see the trend of new API candidates.

To be clear, I'm OK with not doing this right now.  But I'd like to understand the actual reason better (whether it's the hardware of the targeted device, or it's a schedule pressure, etc., all of which are fine answers!).

Thanks!

Comment 39 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-19 18:54:55 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #38)
> (In reply to Sean Lin [:seanlin] from comment #35)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #33)
> > > (In reply to Sean Lin [:seanlin] from comment #30)
> > > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > > comment #28)
> > > > > (In reply to Sean Lin [:seanlin] from comment #25)
> > > > > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > > > > comment #24)
> > > > > > > Comment on attachment 8472216 [details] [diff] [review]
> > > > > > > New part 1 - DOM API & WebIDL bindings, v2
> > > > > > > 
> > > > > > > Review of attachment 8472216 [details] [diff] [review]:
> > > > > > > -----------------------------------------------------------------
> > > > > > > 
> > > > > > > Are you going to address Jonas' feedback about returnTuner() on dev-webapi
> > > > > > > in the future?
> > > > > > 
> > > > > > Actually we've replied to that email thread with more of our concerns about
> > > > > > adding acquiring/releasing APIs on tuners, as well as the inclination of not
> > > > > > doing it until TV Manager API becomes open to non-certified apps. And since
> > > > > > there appears no further feedback on them, so far we don't have the plan to
> > > > > > bring them into the current draft. Thoughts?!
> > > > > 
> > > > > Are we targeting a device with multiple tuners in the initial release?  If
> > > > > not, punting on this sounds fine...
> > > > > 
> > > > Actually we're. But we may conitnue the discussion back to the dev-webapi
> > > > thread. (Here's the link to the latest discussion about the two APIs.
> > > > https://groups.google.com/forum/#!msg/mozilla.dev.webapi/IXG2NASKH3Q/
> > > > ZJ1kk5rcFQ4J) If at last we still feel they're necessary for the initial
> > > > release, we could add the implementation to a follow-up patch in this bug.
> > > 
> > > Is there any reason not to support them in the initial release?
> > 
> > One of the concerns we pointed out in the email thread is the following use
> > case:
> > 
> > "While recording a channel, the user may still want to watch that channel at
> > the same time. And it could be done by two apps (recording app and TV app).
> > (And if the user tries to switch channels, a pop-out will remind him/her of
> > the fact that the recording would stop.)"
> > 
> > If app-based acquiring/releasing APIs are applied on tuners, it needs two
> > dedicated tuners on the device to suffice that use case. However, we can
> > barely keep the same experience on some current TVs which may not have
> > sufficient numbers of tuners. And it might not be coherent with some of
> > status quo under this situation. (Maybe someday in the future we can say for
> > sure the limited number of tuners becomes no longer a concern for almost all
> > TVs.)
> 
> I'm now completely confused!  I first asked whether we are targeting a
> device with multiple tuners precisely because I was thinking of this exact
> discussion in the email thread.  :-)  And you said yes we do, but it seems
> from this paragraph that the device that we're targeting has one tuner.  So,
> which one is it?
> 
Sorry for the confusion and I didn't explain it well. I was trying to say that devices with single or multiple tuners were covered in the scope.

And on some devices, even though they are equipped with multiple tuners, those tuners may be designed for different signal types. For example, one tuner is for DVB-S and another one is for DVB-C. So they could also be relevant to this concern since there's no spare tuner for a given signal.

Comment 40 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-19 19:54:26 PDT

Created attachment 8475645 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v5

Hi Andrea,

Could you super review this patch since the change requires a DOM peer review?

Comment 41 Andrea Marchesini (:baku) 2014-08-20 02:08:55 PDT

Comment on attachment 8475645 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v5

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

::: dom/tests/mochitest/general/test_interfaces.html
@@ +1165,5 @@
>      {name: "TreeSelection", xbl: true},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "TreeWalker",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    {name: "TVChannel", b2g: true, permission: "tv"},

pref="dom.tv.enabled" everywhere

::: dom/tv/test/file_app.sjs
@@ +1,2 @@
> +var gBasePath = "tests/dom/tv/test/";
> +

maybe, hg copy from where you took this file (I suspect dom/datastore/tests/file_apps.sjs :)

::: dom/webidl/TVChannel.webidl
@@ +1,5 @@
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */

Do we have a spec? can you write the URL of the spec in any webidl file?

Comment 42 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-20 20:44:55 PDT

Created attachment 8476450 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v6

Polishing based on the comments in r+ reviews.
(I'll add |pref: "dom.tv.enabled"| to test_interfaces.html for the TV ones when we're ready to enable |dom.tv.enabled| in b2g.js.)

Comment 43 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-21 02:17:36 PDT

Created attachment 8476539 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v7

Minor fix for typo. Replace |oncurrentsourcechanged| with |oncurrentchannelchanged| in TVSource.webidl.

Comment 44 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-21 03:35:27 PDT

Created attachment 8476570 [details] [diff] [review]
New part 2 - Basic Gecko implementation & Gonk TV service, v1

New part 2 - Basic Gecko implementation & Gonk TV service

Add XPCOM interface of TV service and implement most part of TV interfaces except for handling channel/program data, which can be stored in IndexedDB. (The interaction with IndexedDB will be implemented in follow-up patches. Some unfinished implementation in Gonk TV service should be done in follow-up patches as well.)

Comment 45 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-21 10:39:33 PDT

Comment on attachment 8476570 [details] [diff] [review]
New part 2 - Basic Gecko implementation & Gonk TV service, v1

Can you please split this patch up into several small chunks?  Ideally what I would like to see is the implementation of each one of the WebIDL interfaces go into its own small patch.  Also, any infrastructure that you need for this stuff (such as any IPDL machinery, or the XPCOM tv service etc) should go into their own small patches.  Also, it would be great if you could make sure that the patches are split across logical boundaries (e.g. they all make sense as individual units built on top of each other.)  That would make it much easier to review these patches.

Thanks!

Comment 46 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-21 22:56:48 PDT

Created attachment 8477236 [details] [diff] [review]
New Part 2 - TV service, v1

Adding XPCOM interface of TV service and updating relevant components.

Comment 47 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-21 23:00:06 PDT

Created attachment 8477238 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, v1

Implementing TVManager, TVTuner, and TVSource.
(The interaction with IndexedDB will be implemented in follow-up patches.)

Comment 48 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-21 23:02:50 PDT

Created attachment 8477239 [details] [diff] [review]
New Part 4 - TV channel & TV program, v1

Implementing TVChannel & TVProgram.
(The interaction with IndexedDB will be implemented in follow-up patches.)

Comment 49 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-22 13:08:19 PDT

Thanks, Sean.  I'll look at the patches next week, but I'm wondering, what is the purpose of the XPCOM services created in part 2?  Wouldn't it be simpler to eliminate this middle XPCOM layer and implement the necessary logic inside the implementation of the WebIDL objects directly?

Comment 50 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-25 03:09:27 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #49)
> Thanks, Sean.  I'll look at the patches next week, but I'm wondering, what
> is the purpose of the XPCOM services created in part 2?  Wouldn't it be
> simpler to eliminate this middle XPCOM layer and implement the necessary
> logic inside the implementation of the WebIDL objects directly?

The XPCOM interface is designated for the portability across different platforms/devices. Different partners may have their own implementations of TV service based on the underlying specs. But for easier validation, we may need to have our own "fake" TV service providing simulated data and behaviors, just like FakeSpeechRecognitionService.

Btw, I was trying to follow the structure under dom/telephony. But it seems to me that moving our TV service from |gonk| subfolder to |test| might be another option. Thoughts?!

Comment 51 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-25 15:30:09 PDT

(In reply to Sean Lin [:seanlin] from comment #50)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #49)
> > Thanks, Sean.  I'll look at the patches next week, but I'm wondering, what
> > is the purpose of the XPCOM services created in part 2?  Wouldn't it be
> > simpler to eliminate this middle XPCOM layer and implement the necessary
> > logic inside the implementation of the WebIDL objects directly?
> 
> The XPCOM interface is designated for the portability across different
> platforms/devices. Different partners may have their own implementations of
> TV service based on the underlying specs. But for easier validation, we may
> need to have our own "fake" TV service providing simulated data and
> behaviors, just like FakeSpeechRecognitionService.

In that case they should have clear and well defined extension points.  They wouldn't want to replace *all* of our implementation with theirs.

You should probably define a small API which is hardware specific, split off those parts in a tiny XPCOM service, have a fake implementation of only that service, and expect partners to provide their own implementation for that.  We don't need to abstract away the entire code into giant XPCOM services.

> Btw, I was trying to follow the structure under dom/telephony. But it seems
> to me that moving our TV service from |gonk| subfolder to |test| might be
> another option. Thoughts?!

I think doing what I see in the patches here is definitely over engineering, so I'd prefer to not do that, and instead focus on having a small API for the hardware specific stuff.  I am not familiar with the tech involved here so I don't have a more concrete proposal.  Please feel free to make one.

With that being said, is there a point in me reviewing these patches at the current stage?  It seems like they will all need to change based on my feedback here.

Comment 52 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-26 04:04:28 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #51)
> (In reply to Sean Lin [:seanlin] from comment #50)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #49)
> > > Thanks, Sean.  I'll look at the patches next week, but I'm wondering, what
> > > is the purpose of the XPCOM services created in part 2?  Wouldn't it be
> > > simpler to eliminate this middle XPCOM layer and implement the necessary
> > > logic inside the implementation of the WebIDL objects directly?
> > 
> > The XPCOM interface is designated for the portability across different
> > platforms/devices. Different partners may have their own implementations of
> > TV service based on the underlying specs. But for easier validation, we may
> > need to have our own "fake" TV service providing simulated data and
> > behaviors, just like FakeSpeechRecognitionService.
> 
> In that case they should have clear and well defined extension points.  They
> wouldn't want to replace *all* of our implementation with theirs.
> 
> You should probably define a small API which is hardware specific, split off
> those parts in a tiny XPCOM service, have a fake implementation of only that
> service, and expect partners to provide their own implementation for that. 
> We don't need to abstract away the entire code into giant XPCOM services.
I could move |isParentalControlLocked| out of |nsITVService| interface. However, almost all the other 6 functions in |nsITVService| are about tuner/channel management and appears hardware specific since they are all involved with tuners. Besides, there are couple listener registration functions in |nsITVService| since we may need to know some unsolocited changes from underlying layer, such as newly scanned channels or new EITs. So far how the device detects those changes seems sort of hardware specific too.

Actually |nsITVServiceCallback| and all of the |nsITV[XXX]Listener| interfaces are already implemented by us in each TV gecko component. Only |nsITVService| could become the implementation burden for our partners. But our partners may need to know those interfaces since they may get called when |nsITVService| gets implemented.

From my point of view, the remaining stuff in |nsITVService| is already hardware specifc to some extent, though the size might not be perfectly small (6 operations and 8 listener registration functions).

> > Btw, I was trying to follow the structure under dom/telephony. But it seems
> > to me that moving our TV service from |gonk| subfolder to |test| might be
> > another option. Thoughts?!
> 
> I think doing what I see in the patches here is definitely over engineering,
> so I'd prefer to not do that, and instead focus on having a small API for
> the hardware specific stuff.  I am not familiar with the tech involved here
> so I don't have a more concrete proposal.  Please feel free to make one.
> 
> With that being said, is there a point in me reviewing these patches at the
> current stage?  It seems like they will all need to change based on my
> feedback here.
How about starting with the TV service interface (part 2)? We may try to approach a smaller set of hardware specific APIs if some of them are still not in good shape enough.

Comment 53 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-26 10:59:40 PDT

Comment on attachment 8477236 [details] [diff] [review]
New Part 2 - TV service, v1

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

I have some specific high level comments below.  Please address them and let's first iterate over the partner facing API before writing more code, because the implementation will need to change based on what we agree on here.  (Note that I have not yet looked at the rest of the code.)

(In reply to Sean Lin [:seanlin] from comment #52)
> I could move |isParentalControlLocked| out of |nsITVService| interface.

Please do.

> However, almost all the other 6 functions in |nsITVService| are about
> tuner/channel management and appears hardware specific since they are all
> involved with tuners. Besides, there are couple listener registration
> functions in |nsITVService| since we may need to know some unsolocited
> changes from underlying layer, such as newly scanned channels or new EITs.
> So far how the device detects those changes seems sort of hardware specific
> too.

Here is what I'm looking for: I'd like to have clear hardware specific use cases for every single API that we expect the partner to implement.  And we should do everything that we can to keep that API as small as possible.  For example, see my comment below about nsITVManagerListener.  Unless tuners can be hot-plugged, I don't think that it makes sense to expect that the hardware can control whether new tuners appear or disappear, so we should probably remove those parts of the API.

Note that as I said before, I am guessing many things here as I'm not familiar with the underlying tech, but please make sure to include extensive comments and hardware specific use cases in the next iteration of the patch.  Links to references etc are immensely helpful as well.

> Actually |nsITVServiceCallback| and all of the |nsITV[XXX]Listener|
> interfaces are already implemented by us in each TV gecko component. Only
> |nsITVService| could become the implementation burden for our partners. But
> our partners may need to know those interfaces since they may get called
> when |nsITVService| gets implemented.

Even those callbacks are a bit over engineered.  :)  See my comments below please.

Also, I do not want to expose anything to the partner implemented binary unless if we have to, so, just because something is "nice" doesn't mean we should expose it.  Note that these will effectively become APIs that the partner will rely on, and we may not be able to change it much in the future, so the bigger the API is the worse the compat story becomes.  Also, keeping the API as small as possible will make it easier for future partners to adopt it as well.

> From my point of view, the remaining stuff in |nsITVService| is already
> hardware specifc to some extent, though the size might not be perfectly
> small (6 operations and 8 listener registration functions).

I'd like to understand more, please see my comments below.  Thanks!

::: dom/tv/interface/nsITVService.idl
@@ +16,5 @@
> +[ref] native constTVChannelData(const mozilla::dom::TVChannelData);
> +[ref] native constTVProgramData(const mozilla::dom::TVProgramData);
> +[ref] native constTVProgramDataArray(const nsTArray<mozilla::dom::TVProgramData>);
> +[ref] native constTVChannelProgramsHash(const nsClassHashtable<nsStringHashKey, nsTArray<mozilla::dom::TVProgramData>>);
> +[ref] native TVTunerDataArray(nsTArray<mozilla::dom::TVTunerData>);

The API for the hardware specific access needs to be binary compatible.  Using our internal types like this is a terrible idea, because it means that changes to the definitions of these types can break the component's binary component, resulting in a crash etc.

Please stick to passing the IDL built-in types.

@@ +125,5 @@
> +interface nsITVChannelListener : nsISupports
> +{
> +  /**
> +   * Called when a scheduled program is interrupted by another program, which
> +   * was not originally designated based on the current time table.

Is this interruption hardware generated?

@@ +161,5 @@
> +
> +  /**
> +   * Called when |nsITVService::setChannel()| succeeds.
> +   */
> +  void notifySetChannelSuccess();

Instead of all of these methods, you can just have:

void notifySuccess();

@@ +168,5 @@
> +   * Called when |nsITVService::getPrograms()| succeeds.
> +   *
> +   * @param programDataList A list of program data.
> +   */
> +  void notifyGetProgramsSuccess(in constTVProgramDataArray programDataList);

I think you want to use a different interface here, which has a notifySuccess function which takes an XPCOM array (not an nsTArray), and a notifyError.

@@ +175,5 @@
> +   * Called when something wrong happens.
> +   *
> +   * @param error Error from the underlying layer.
> +   */
> +  void notifyError(in DOMString error);

What is the error string?  Instead of allowing them to provide arbitrary error strings which will be non-localized, we should allow them only to use a limited number of error codes with very specific meanings.

@@ +188,5 @@
> +
> +[builtinclass, uuid(1b17e3cc-1c84-11e4-a4d4-74d02b97e723)]
> +interface nsITVService : nsISupports
> +{
> +  attribute bool isParentalControlLocked;

This should not be hardware specific, please remove it.

@@ +197,5 @@
> +   *
> +   * @param listener The event listener.
> +   */
> +  void registerManagerListener(in nsITVManagerListener listener);
> +  void unregisterManagerListener(in nsITVManagerListener listener);

Who controls the tuners being added or removed?  Is it the hardware?  If so, why?  Do these devices support hot-plugging tuners?

@@ +206,5 @@
> +   *
> +   * @param listener The event listener.
> +   */
> +  void registerTunerListener(in nsITVTunerListener listener);
> +  void unregisterTunerListener(in nsITVTunerListener listener);

If this can only happen after setSource as documented, these two plus nsITVTunerListener can be removed completely.

@@ +215,5 @@
> +   *
> +   * @param listener The event listener.
> +   */
> +  void registerSourceListener(in nsITVSourceListener listener);
> +  void unregisterSourceListener(in nsITVSourceListener listener);

The callback argument to setSource should be enough here, please remove these two methods and nsITVSourceListener.

@@ +224,5 @@
> +   *
> +   * @param listener The event listener.
> +   */
> +  void registerChannelListener(in nsITVChannelListener listener);
> +  void unregisterChannelListener(in nsITVChannelListener listener);

This model is needlessly complicated, because it requires the implementation to maintain an array of these listeners.  Instead, just use:

attribute nsITVChannelListener channelListener;

@@ +229,5 @@
> +
> +  /**
> +   * Get all tuners.
> +   */
> +  TVTunerDataArray getTuners();

Why is this synchronous?

Comment 54 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-26 11:26:30 PDT

Comment on attachment 8477238 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, v1

I don't think it makes sense to review these parts until we settle on the details of the partner facing API.

Comment 55 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-26 20:00:45 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #53)
> ::: dom/tv/interface/nsITVService.idl
> @@ +125,5 @@
> > +interface nsITVChannelListener : nsISupports
> > +{
> > +  /**
> > +   * Called when a scheduled program is interrupted by another program, which
> > +   * was not originally designated based on the current time table.
> 
> Is this interruption hardware generated?

It could be. In some areas, the broadcaster or TV service provider might provide some info along with the interrupting events, such as emergent news or speeches. It might or might not go through general EIT broadcasting.

> @@ +197,5 @@
> > +   *
> > +   * @param listener The event listener.
> > +   */
> > +  void registerManagerListener(in nsITVManagerListener listener);
> > +  void unregisterManagerListener(in nsITVManagerListener listener);
> 
> Who controls the tuners being added or removed?  Is it the hardware?  If so,
> why?  Do these devices support hot-plugging tuners?

Some devices could support hot-plugging tuners. Though not all typical TVs support this, some devices with TV capability may also have extensions for this.

> @@ +215,5 @@
> > +   *
> > +   * @param listener The event listener.
> > +   */
> > +  void registerSourceListener(in nsITVSourceListener listener);
> > +  void unregisterSourceListener(in nsITVSourceListener listener);
> 
> The callback argument to setSource should be enough here, please remove
> these two methods and nsITVSourceListener.

There are still some channel scanning or EIT notify functions in source listener. So I'll keep nsITVSourceListener but make it as an attribute instead.

Btw, I'll make correspondent changes according to other comments.

Comment 56 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-27 06:41:59 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #53)
> ::: dom/tv/interface/nsITVService.idl
> @@ +224,5 @@
> > +   *
> > +   * @param listener The event listener.
> > +   */
> > +  void registerChannelListener(in nsITVChannelListener listener);
> > +  void unregisterChannelListener(in nsITVChannelListener listener);
> 
> This model is needlessly complicated, because it requires the implementation
> to maintain an array of these listeners.  Instead, just use:
> 
> attribute nsITVChannelListener channelListener;

I just realized that having only one sourceListener could introduce some trouble for multiple TVSource instances, when we plan to use singleton on TV service and let TVSource class implement nsITVSourceListener as well. For example, there may be multiple source instances since a device could have multiple tuners at the same time. But only one of these source instances could be set as the sourceListener of the singleton TV service at a specific moment. When the underlying hardware detects a newly broadcasted EIT, the right source instance could miss the correspondent notification if it's not set as the listener. We're unsure when an new EIT gets broadcasted, so we need all source instances to listen to notifications. But it appears kinda resource consuming if we don't make TV service singleton.

Comment 57 Marco Chen [:mchen] 2014-08-27 08:00:06 PDT

Hi Ehsan,

It seems that you asked to have a HAL interface that is something like galloc HAL [1].
As I know the functionality inside HAL should be straightforward to access HW capability without too more logic inside it. And the complex logic should be kept in some kind of service component which will leverage HAL.

Refer to our camera implementation [2], we didn't use Camera HAL directly instead of we use ICamera interface to communicate with Camera Service inside MediaServer process from AOSP. That said there are a complex camera service on top of HAL and we don't want to write it by ourself. The result is we leveraged camera service not camera HAL.

Refer to WebSpeech implementation [3], it only defined high level IDL for third-party vendor to implement it's own service which control the logic and access to it's own library. In this way we don't need to deal with these logic effort and vendor will be more flexible to adapt their library.

In the reality, each TV vendors have their own TV stack (as a daemon/process) which access into HW capability. This case will be something like the case of camera. We would like to leverage existed TV stack but not to write another Mozilla TV stack then exposing just HAL interface to vendor. To be honest, partner's TV stack is robust enough and present in real product already.

Thus refer to WebSpeech we don't want to define HAL interface instead of that we expose a IDL for each TV vendor to build their own nsITVService which connects to it's own TV stack/daemon via any kind of IPC.

That is why Sean proposed to nsITVService level only not deep into HAL interface. 

[1] https://github.com/android/platform_hardware_libhardware/blob/master/include/hardware/gralloc.h#L160
[2] http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraHwMgr.cpp#216
[3] http://dxr.mozilla.org/mozilla-central/source/content/media/webspeech/recognition/nsISpeechRecognitionService.idl

Comment 58 Marco Chen [:mchen] 2014-08-27 08:05:22 PDT

s/instead of/but/
s/instead of that/but/

Comment 59 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-27 20:30:16 PDT

Created attachment 8480320 [details] [diff] [review]
New Part 2 - TV service, diff v1 v2 nsITVService.idl

Comment 60 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-08-27 20:42:21 PDT

Created attachment 8480322 [details] [diff] [review]
New Part 2 - TV service, v2

Updating based on previous review comments. But keeping the way of maintaining an array of listeners due to the concern stated in comment 56.

The diff between v1 and v2 on nsITVService.idl is available at
https://bugzilla.mozilla.org/attachment.cgi?id=8480320&action=diff

Comment 61 :Ehsan Akhgari (busy, don't ask for review please) 2014-08-29 12:30:57 PDT

OK, I think in order to be able to provide better technical feedback here, I need to learn more about the hardware components involved, and the APIs that are typically exposed through such hardware.  Sean, Marco, can one of you please provide me with some documentation that can help me understand more about the hardware involved, the capabilities and limitations that it has, and the typical APIs that people have built on top of that hardware in other contexts?

Thanks!

Comment 62 Marco Chen [:mchen] 2014-09-01 01:15:42 PDT

Hi Ehsan,

Provide you some material first.

The [1] & [2] are the open standard APIs for analog (V4L) and digital (DVB API) from linuxtv.org.
And [3] is the more popular open API for TV vendors currently. You can refer to [4] for more API descriptions.

[1] http://www.linuxtv.org/wiki/index.php/Development:_Video4Linux_APIs
[2] http://www.linuxtv.org/docs/dvbapi/dvbapi.html
[3] http://www.oipf.tv/specifications
[4] Volume 5 - Declarative Application Environment R2 v2.3

Comment 63 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-09-05 03:39:37 PDT

Created attachment 8484895 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, v2

Updating based on current TV service interface (pending review).

Comment 64 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-09-05 03:40:30 PDT

Created attachment 8484896 [details] [diff] [review]
New Part 4 - TV channel & TV program, v2

Updating based on current TV service interface (pending review).

Comment 65 :Ehsan Akhgari (busy, don't ask for review please) 2014-09-05 12:06:54 PDT

Sorry for the delay here guys, I am struggling to find the time to read the docs and then get back to you.  I hope to do that next week.

Comment 66 :Ehsan Akhgari (busy, don't ask for review please) 2014-09-25 12:32:46 PDT

Comment on attachment 8480322 [details] [diff] [review]
New Part 2 - TV service, v2

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

Apologies for the delay.  Again, I only focused on reviewing the IDL here.  Please see my comments below.

::: dom/tv/interface/nsITVService.idl
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"
> +
> +interface nsIArray;

Please clarify anywhere that nsIArray is used whether nsIMutableArray should be supported on the object.  (I don't think there is a good reason to support that, but this needs to be documented.)

@@ +9,5 @@
> +[builtinclass, uuid(ed65422e-2d97-11e4-89d6-74d02b97e723)]
> +interface nsITVTunerData : nsISupports
> +{
> +  attribute DOMString id;
> +  attribute nsIArray supportedSourceTypes;

These should probably be readonly.

@@ +23,5 @@
> +  attribute DOMString number;
> +  attribute DOMString name;
> +  attribute bool isEmergency;
> +  attribute bool isFree;
> +  attribute bool isProtected;

These should probably be readonly.

@@ +45,5 @@
> +  /**
> +   * An array of nsISupportsString. Please refer to
> +   * http://seanyhlin.github.io/TV-Manager-API/ for available values.
> +   */
> +  attribute nsIArray subtitleLanguages;

Readonly.

Also, do we need a full-blown array for these?  Having a method that returns an [array] seems much simpler.

@@ +56,5 @@
> +   * Called when a new tuner is detected.
> +   *
> +   * @param tunerData The associated data of the added tuner.
> +   */
> +  void notifyTunerAdded(in nsITVTunerData tunerData);

I was unaware of finding a mention of something similar to this in the OIPF document.  I'm not really sure how we want to handle dynamic tuner removals.  Thinking about this more, what happens to the objects that we expose to the web page through the web facing API, for example?  Will the content still be able to manipulate these objects, etc.?

Can we leave this part out until we discuss this in person in October?

@@ +122,5 @@
> +   */
> +  void notifyEITBroadcasted(in DOMString tunerId,
> +                            in DOMString sourceType,
> +                            in DOMString channelNumber,
> +                            in nsIArray programDataList);

Can't you make programDataList an [array] instead?

@@ +130,5 @@
> +interface nsITVChannelListener : nsISupports
> +{
> +  /**
> +   * Called when a scheduled program is interrupted by another program, which
> +   * was not originally designated based on the current time table.

I looked for anything resembling this in the OIPF document, and couldn't find any.  The only mentions of interruption were about things such as interruption because of the underlying network socket getting interrupted, or something similar.  Please confirm that this is indeed a hardware specific functionality, and remove it otherwise.  Thanks!

@@ +180,5 @@
> +   * Called when the operation succeeds.
> +   *
> +   * @param dataList A list of data.
> +   */
> +  void notifySuccess(in nsIArray dataList);

This looks much simpler than before, but I think we can still make it simpler.  You can make dataList [optional] and pass in null if the callback interface is passed to a setter.  This will allow you to merge nsITVServiceGetterCallback and nsITVServiceSetterCallback.  And once you do that, we wouldn't need to separate out notifySuccess into its own interface, so you can just put that method into nsITVServiceCallback.  (Sorry for not suggesting this before!)

@@ +185,5 @@
> +};
> +
> +%{C++
> +#define TV_SERVICE_CID \
> +  { 0x60fb3c53, 0x017f, 0x4340, { 0x91, 0x1b, 0xd5, 0x5c, 0x31, 0x28, 0x88, 0xb6 } }

This should be provided by the partner, right?  We will use the contractid for a call to do_GetService().

@@ +190,5 @@
> +#define TV_SERVICE_CONTRACTID \
> +  "@mozilla.org/tv/tvservice;1"
> +%}
> +
> +[builtinclass, uuid(1b17e3cc-1c84-11e4-a4d4-74d02b97e723)]

Why is this builtinclass?  Do we have a good reason to prevent partners from implementing this interface in JS if they want to?

@@ +191,5 @@
> +  "@mozilla.org/tv/tvservice;1"
> +%}
> +
> +[builtinclass, uuid(1b17e3cc-1c84-11e4-a4d4-74d02b97e723)]
> +interface nsITVService : nsISupports

You should clearly document which interfaces should be expected to be implemented by the TV vendor's component.  For example, it seems nsITVTunerData, nsITVChannelData and friends will be implemented by the partner too?

Also, the threading requirements should be clearly documented as well.

@@ +218,5 @@
> +   *
> +   * @param listener The event listener.
> +   */
> +  void registerChannelListener(in nsITVChannelListener listener);
> +  void unregisterChannelListener(in nsITVChannelListener listener);

I don't think comment 56 is a good reason to require the component provided code to deal with this complexity.  Here is an alternative way of getting that scenario to work without requiring this component from maintaining an array of listeners: Instead of requiring the TVSource class to implement nsITVChannelListener, for example, make that implement a similar interface (let's call it nsITVChannelListenerInternal) which is not exposed to the partner provided TV component.  Then, we can write a standalone implementation of nsITVChannelListener inside Gecko, and make that maintain an array of nsITVChannelListenerInternal objects.  That way the partner provided service will need to maintain only one nsITVChannelListener pointer, and notifies that of the respective changes.  The implementation of that component inside Gecko calls the corresponding nsITVChannelListenerInternal method on its internal array, that way, TVSource objects can register themselves with the implementation of nsITVChannelListener without directly dealing with the partner provided component.

We can use a similar idea for the other listener arrays here.

Comment 67 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-09-26 03:33:09 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #66)
> Comment on attachment 8480322 [details] [diff] [review]
> New Part 2 - TV service, v2
> 
> Review of attachment 8480322 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +56,5 @@
> > +   * Called when a new tuner is detected.
> > +   *
> > +   * @param tunerData The associated data of the added tuner.
> > +   */
> > +  void notifyTunerAdded(in nsITVTunerData tunerData);
> 
> I was unaware of finding a mention of something similar to this in the OIPF
> document.  I'm not really sure how we want to handle dynamic tuner removals.
> Thinking about this more, what happens to the objects that we expose to the
> web page through the web facing API, for example?  Will the content still be
> able to manipulate these objects, etc.?
> 
> Can we leave this part out until we discuss this in person in October?

Sure. I'll move it out for now and see how it goes in the discussion.

> @@ +130,5 @@
> > +interface nsITVChannelListener : nsISupports
> > +{
> > +  /**
> > +   * Called when a scheduled program is interrupted by another program, which
> > +   * was not originally designated based on the current time table.
> 
> I looked for anything resembling this in the OIPF document, and couldn't
> find any.  The only mentions of interruption were about things such as
> interruption because of the underlying network socket getting interrupted,
> or something similar.  Please confirm that this is indeed a hardware
> specific functionality, and remove it otherwise.  Thanks!

Now that OIPF doesn't seem to put much detail on this. It appears an acceptable way to remove it for now since it doesn't seem widely used. (An EIT broadcasted event could also be used for those scenarios instead.) We may consider to add it if it looks necessary in the future.

> @@ +185,5 @@
> > +};
> > +
> > +%{C++
> > +#define TV_SERVICE_CID \
> > +  { 0x60fb3c53, 0x017f, 0x4340, { 0x91, 0x1b, 0xd5, 0x5c, 0x31, 0x28, 0x88, 0xb6 } }
> 
> This should be provided by the partner, right?  We will use the contractid
> for a call to do_GetService().

It's used when loading the TV service (through TV service factory, in a singleton way) in layout/build/nsLayoutModule.cpp. CID and Contract ID both look necessary in that file.

Thoughts?!

> 
> @@ +190,5 @@
> > +#define TV_SERVICE_CONTRACTID \
> > +  "@mozilla.org/tv/tvservice;1"
> > +%}
> > +
> > +[builtinclass, uuid(1b17e3cc-1c84-11e4-a4d4-74d02b97e723)]
> 
> Why is this builtinclass?  Do we have a good reason to prevent partners from
> implementing this interface in JS if they want to?

Will eliminate the restriction. Thanks.

> @@ +191,5 @@
> > +  "@mozilla.org/tv/tvservice;1"
> > +%}
> > +
> > +[builtinclass, uuid(1b17e3cc-1c84-11e4-a4d4-74d02b97e723)]
> > +interface nsITVService : nsISupports
> 
> You should clearly document which interfaces should be expected to be
> implemented by the TV vendor's component.  For example, it seems
> nsITVTunerData, nsITVChannelData and friends will be implemented by the
> partner too?

I'm thinking to implement nsITVTunerData, nsITVChannelData, nsITVProgramData by ourselves. They don't seem vendor dependent and may also act as data containers for IPC or IndexedDB operations. Since all the fields become read-only now, I plan to add an |init| method to each of the containers. So we may initialize the fields once creating an instance.

Thus only nsITVService is expected to be implemented by vendors.

Comment 68 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-09-26 03:37:54 PDT

Created attachment 8495868 [details] [diff] [review]
New Part 2 - TV service, v3

Updating the interface of TV service and correspondent implementation.

Comment 69 :Ehsan Akhgari (busy, don't ask for review please) 2014-09-26 12:48:49 PDT

(In reply to Sean Lin [:seanlin] from comment #67)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #66)
> > Comment on attachment 8480322 [details] [diff] [review]
> > New Part 2 - TV service, v2
> > 
> > Review of attachment 8480322 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > @@ +56,5 @@
> > > +   * Called when a new tuner is detected.
> > > +   *
> > > +   * @param tunerData The associated data of the added tuner.
> > > +   */
> > > +  void notifyTunerAdded(in nsITVTunerData tunerData);
> > 
> > I was unaware of finding a mention of something similar to this in the OIPF
> > document.  I'm not really sure how we want to handle dynamic tuner removals.
> > Thinking about this more, what happens to the objects that we expose to the
> > web page through the web facing API, for example?  Will the content still be
> > able to manipulate these objects, etc.?
> > 
> > Can we leave this part out until we discuss this in person in October?
> 
> Sure. I'll move it out for now and see how it goes in the discussion.

Cool.

> > @@ +130,5 @@
> > > +interface nsITVChannelListener : nsISupports
> > > +{
> > > +  /**
> > > +   * Called when a scheduled program is interrupted by another program, which
> > > +   * was not originally designated based on the current time table.
> > 
> > I looked for anything resembling this in the OIPF document, and couldn't
> > find any.  The only mentions of interruption were about things such as
> > interruption because of the underlying network socket getting interrupted,
> > or something similar.  Please confirm that this is indeed a hardware
> > specific functionality, and remove it otherwise.  Thanks!
> 
> Now that OIPF doesn't seem to put much detail on this. It appears an
> acceptable way to remove it for now since it doesn't seem widely used. (An
> EIT broadcasted event could also be used for those scenarios instead.) We
> may consider to add it if it looks necessary in the future.

Great!

> > @@ +185,5 @@
> > > +};
> > > +
> > > +%{C++
> > > +#define TV_SERVICE_CID \
> > > +  { 0x60fb3c53, 0x017f, 0x4340, { 0x91, 0x1b, 0xd5, 0x5c, 0x31, 0x28, 0x88, 0xb6 } }
> > 
> > This should be provided by the partner, right?  We will use the contractid
> > for a call to do_GetService().
> 
> It's used when loading the TV service (through TV service factory, in a
> singleton way) in layout/build/nsLayoutModule.cpp. CID and Contract ID both
> look necessary in that file.

I thought this interface must be implemented by the third-party component?!  We should not have an implementation for "@mozilla.org/tv/tvservice;1" in our tree.

> > @@ +191,5 @@
> > > +  "@mozilla.org/tv/tvservice;1"
> > > +%}
> > > +
> > > +[builtinclass, uuid(1b17e3cc-1c84-11e4-a4d4-74d02b97e723)]
> > > +interface nsITVService : nsISupports
> > 
> > You should clearly document which interfaces should be expected to be
> > implemented by the TV vendor's component.  For example, it seems
> > nsITVTunerData, nsITVChannelData and friends will be implemented by the
> > partner too?
> 
> I'm thinking to implement nsITVTunerData, nsITVChannelData, nsITVProgramData
> by ourselves. They don't seem vendor dependent and may also act as data
> containers for IPC or IndexedDB operations. Since all the fields become
> read-only now, I plan to add an |init| method to each of the containers. So
> we may initialize the fields once creating an instance.
> 
> Thus only nsITVService is expected to be implemented by vendors.

I'm not sure how that would work.  For example, nsITVService::GetTuners will accept a callback argument, which the partner provided component can use in order to notify us about the tuners.  When that code calls NotifySuccess on the interface callback, it needs to pass it an array of XPCOM objects implementing nsITVTunerData.  How would that code use our implementation?  Or is your idea that it would use do_CreateInstance() to create the Gecko provided implementation, and just uses the setter functions to adjust the properties of the object before passing it to NotifySuccess?  If that is what you're thinking, you should ignore my comments about removing the readonly.  And that is why we need to document this better, because if it's not obvious to me as a patch reviewer, it would be even less obvious to the folks who need to implement this.  :-)

I'll hold off the review until you get back to me on this issue, and then I think I will be ready to review the implementation as well!

Comment 70 :Ehsan Akhgari (busy, don't ask for review please) 2014-09-26 12:50:07 PDT

FWIW I do support the second option in comment 69, if that is what you had in mind.  Thinking about it more, that definitely sounds better because the partner would need to write even less code, which is nice.

Comment 71 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-09-28 19:40:17 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #69)
> (In reply to Sean Lin [:seanlin] from comment #67)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #66)
> > > @@ +185,5 @@
> > > > +};
> > > > +
> > > > +%{C++
> > > > +#define TV_SERVICE_CID \
> > > > +  { 0x60fb3c53, 0x017f, 0x4340, { 0x91, 0x1b, 0xd5, 0x5c, 0x31, 0x28, 0x88, 0xb6 } }
> > > 
> > > This should be provided by the partner, right?  We will use the contractid
> > > for a call to do_GetService().
> > 
> > It's used when loading the TV service (through TV service factory, in a
> > singleton way) in layout/build/nsLayoutModule.cpp. CID and Contract ID both
> > look necessary in that file.
> 
> I thought this interface must be implemented by the third-party component?! 
> We should not have an implementation for "@mozilla.org/tv/tvservice;1" in
> our tree.

Though the interface must be implemented by the third party, we may have our own fake service implementation for test uses. Yet maybe it'd be better to use a specific CID for the fake service only, and remove the current CID which appears for a general implementation to reduce the confusion, just like FakeSpeechRecognitionService [1].

Thoughts?!

[1] http://dxr.mozilla.org/mozilla-central/source/content/media/webspeech/recognition/test/FakeSpeechRecognitionService.h#25

> 
> > > @@ +191,5 @@
> > > > +  "@mozilla.org/tv/tvservice;1"
> > > > +%}
> > > > +
> > > > +[builtinclass, uuid(1b17e3cc-1c84-11e4-a4d4-74d02b97e723)]
> > > > +interface nsITVService : nsISupports
> > > 
> > > You should clearly document which interfaces should be expected to be
> > > implemented by the TV vendor's component.  For example, it seems
> > > nsITVTunerData, nsITVChannelData and friends will be implemented by the
> > > partner too?
> > 
> > I'm thinking to implement nsITVTunerData, nsITVChannelData, nsITVProgramData
> > by ourselves. They don't seem vendor dependent and may also act as data
> > containers for IPC or IndexedDB operations. Since all the fields become
> > read-only now, I plan to add an |init| method to each of the containers. So
> > we may initialize the fields once creating an instance.
> > 
> > Thus only nsITVService is expected to be implemented by vendors.
> 
> I'm not sure how that would work.  For example, nsITVService::GetTuners will
> accept a callback argument, which the partner provided component can use in
> order to notify us about the tuners.  When that code calls NotifySuccess on
> the interface callback, it needs to pass it an array of XPCOM objects
> implementing nsITVTunerData.  How would that code use our implementation? 
> Or is your idea that it would use do_CreateInstance() to create the Gecko
> provided implementation, and just uses the setter functions to adjust the
> properties of the object before passing it to NotifySuccess?  If that is
> what you're thinking, you should ignore my comments about removing the
> readonly.  And that is why we need to document this better, because if it's
> not obvious to me as a patch reviewer, it would be even less obvious to the
> folks who need to implement this.  :-)

Yeah, what I was trying to convey was the one with do_CreateInstance(). So I'll remove the readonly and add more descriptions to make things clear.

Comment 72 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-09-29 01:35:00 PDT

Created attachment 8496730 [details] [diff] [review]
New Part 2 - TV service, v4

Comment 73 :Ehsan Akhgari (busy, don't ask for review please) 2014-09-29 12:56:43 PDT

(In reply to Sean Lin [:seanlin] from comment #71)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #69)
> > (In reply to Sean Lin [:seanlin] from comment #67)
> > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > comment #66)
> > > > @@ +185,5 @@
> > > > > +};
> > > > > +
> > > > > +%{C++
> > > > > +#define TV_SERVICE_CID \
> > > > > +  { 0x60fb3c53, 0x017f, 0x4340, { 0x91, 0x1b, 0xd5, 0x5c, 0x31, 0x28, 0x88, 0xb6 } }
> > > > 
> > > > This should be provided by the partner, right?  We will use the contractid
> > > > for a call to do_GetService().
> > > 
> > > It's used when loading the TV service (through TV service factory, in a
> > > singleton way) in layout/build/nsLayoutModule.cpp. CID and Contract ID both
> > > look necessary in that file.
> > 
> > I thought this interface must be implemented by the third-party component?! 
> > We should not have an implementation for "@mozilla.org/tv/tvservice;1" in
> > our tree.
> 
> Though the interface must be implemented by the third party, we may have our
> own fake service implementation for test uses. Yet maybe it'd be better to
> use a specific CID for the fake service only, and remove the current CID
> which appears for a general implementation to reduce the confusion, just
> like FakeSpeechRecognitionService [1].
> 
> Thoughts?!
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/content/media/webspeech/
> recognition/test/FakeSpeechRecognitionService.h#25

Yes, please do that!

> > > > @@ +191,5 @@
> > > > > +  "@mozilla.org/tv/tvservice;1"
> > > > > +%}
> > > > > +
> > > > > +[builtinclass, uuid(1b17e3cc-1c84-11e4-a4d4-74d02b97e723)]
> > > > > +interface nsITVService : nsISupports
> > > > 
> > > > You should clearly document which interfaces should be expected to be
> > > > implemented by the TV vendor's component.  For example, it seems
> > > > nsITVTunerData, nsITVChannelData and friends will be implemented by the
> > > > partner too?
> > > 
> > > I'm thinking to implement nsITVTunerData, nsITVChannelData, nsITVProgramData
> > > by ourselves. They don't seem vendor dependent and may also act as data
> > > containers for IPC or IndexedDB operations. Since all the fields become
> > > read-only now, I plan to add an |init| method to each of the containers. So
> > > we may initialize the fields once creating an instance.
> > > 
> > > Thus only nsITVService is expected to be implemented by vendors.
> > 
> > I'm not sure how that would work.  For example, nsITVService::GetTuners will
> > accept a callback argument, which the partner provided component can use in
> > order to notify us about the tuners.  When that code calls NotifySuccess on
> > the interface callback, it needs to pass it an array of XPCOM objects
> > implementing nsITVTunerData.  How would that code use our implementation? 
> > Or is your idea that it would use do_CreateInstance() to create the Gecko
> > provided implementation, and just uses the setter functions to adjust the
> > properties of the object before passing it to NotifySuccess?  If that is
> > what you're thinking, you should ignore my comments about removing the
> > readonly.  And that is why we need to document this better, because if it's
> > not obvious to me as a patch reviewer, it would be even less obvious to the
> > folks who need to implement this.  :-)
> 
> Yeah, what I was trying to convey was the one with do_CreateInstance(). So
> I'll remove the readonly and add more descriptions to make things clear.

OK, sounds good!

Comment 74 :Ehsan Akhgari (busy, don't ask for review please) 2014-09-30 13:40:43 PDT

Comment on attachment 8496730 [details] [diff] [review]
New Part 2 - TV service, v4

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

::: b2g/installer/package-manifest.in
@@ +198,5 @@
>  @BINPATH@/components/dom_stylesheets.xpt
>  @BINPATH@/components/dom_telephony.xpt
>  @BINPATH@/components/dom_threads.xpt
>  @BINPATH@/components/dom_traversal.xpt
> +@BINPATH@/components/dom_tv.xpt

You should do this in mobile/android/installer/package-manifest.in too.

::: dom/tv/TVServiceCallbacks.cpp
@@ +23,5 @@
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(TVServiceSourceSetterCallback)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(TVServiceSourceSetterCallback)
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(TVServiceSourceSetterCallback)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsITVServiceCallback)

Where does the ambiguity come from here?  All of these callback types should only have one nsISupports base, right?

@@ +35,5 @@
> +  , mPromise(aPromise)
> +  , mSourceType(aSourceType)
> +{
> +  MOZ_ASSERT(mTuner);
> +  MOZ_ASSERT(mPromise);

I assume that there is no way for the partner code to directly invoke these constructors, so the assertions are fine...  (Please correct me if I'm wrong.)

@@ +36,5 @@
> +  , mSourceType(aSourceType)
> +{
> +  MOZ_ASSERT(mTuner);
> +  MOZ_ASSERT(mPromise);
> +  MOZ_ASSERT(mSourceType);

What does this last assertion mean exactly?

@@ +42,5 @@
> +
> +/* virtual */ NS_IMETHODIMP
> +TVServiceSourceSetterCallback::NotifySuccess(nsIArray* aDataList)
> +{
> +  MOZ_ASSERT(!aDataList);

This is called by partner code, so we can't just assert things, we need to do proper error checking.

@@ +71,5 @@
> +  default:
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  }
> +
> +  mPromise->MaybeRejectBrokenly(error);

Can we reject to an nsresult instead?  We don't need to use DOMError here, do we?

More importantly, the spec seems to be silent on what we need to reject these promises to.  We should first figure that out and fix it in the spec.

@@ +86,5 @@
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(TVServiceChannelScanCallback)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(TVServiceChannelScanCallback)
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(TVServiceChannelScanCallback)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsITVServiceCallback)

And this.

@@ +104,5 @@
> +
> +/* virtual */ NS_IMETHODIMP
> +TVServiceChannelScanCallback::NotifySuccess(nsIArray* aDataList)
> +{
> +  MOZ_ASSERT(!aDataList);

Ditto.

@@ +133,5 @@
> +  default:
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  }
> +
> +  mPromise->MaybeRejectBrokenly(error);

This too.

@@ +148,5 @@
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(TVServiceChannelSetterCallback)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(TVServiceChannelSetterCallback)
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(TVServiceChannelSetterCallback)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsITVServiceCallback)

And this.

@@ +166,5 @@
> +
> +/* virtual */ NS_IMETHODIMP
> +TVServiceChannelSetterCallback::NotifySuccess(nsIArray* aDataList)
> +{
> +  MOZ_ASSERT(!aDataList);

And this.

@@ +195,5 @@
> +  default:
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  }
> +
> +  mPromise->MaybeRejectBrokenly(error);

And this.

@@ +210,5 @@
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(TVServiceTunerGetterCallback)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(TVServiceTunerGetterCallback)
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(TVServiceTunerGetterCallback)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsITVServiceCallback)

And this.

@@ +226,5 @@
> +
> +/* virtual */ NS_IMETHODIMP
> +TVServiceTunerGetterCallback::NotifySuccess(nsIArray* aDataList)
> +{
> +  MOZ_ASSERT(aDataList);

And this.

@@ +254,5 @@
> +  default:
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  }
> +
> +  mPromise->MaybeRejectBrokenly(error);

And this.

@@ +269,5 @@
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(TVServiceProgramGetterCallback)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(TVServiceProgramGetterCallback)
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(TVServiceProgramGetterCallback)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsITVServiceCallback)

And this.

@@ +276,5 @@
> +
> +/* virtual */ NS_IMETHODIMP
> +TVServiceProgramGetterCallback::NotifySuccess(nsIArray* aDataList)
> +{
> +  MOZ_ASSERT(aDataList);

And this.

::: dom/tv/TVServiceCallbacks.h
@@ +25,5 @@
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_NSITVSERVICECALLBACK
> +  NS_DECL_CYCLE_COLLECTION_CLASS(TVServiceSourceSetterCallback)
> +
> +  explicit TVServiceSourceSetterCallback(TVTuner* aTuner,

explicit is only useful on constructors that can be invoked only with one argument.  Please drop it from here and below.

@@ +27,5 @@
> +  NS_DECL_CYCLE_COLLECTION_CLASS(TVServiceSourceSetterCallback)
> +
> +  explicit TVServiceSourceSetterCallback(TVTuner* aTuner,
> +                                         Promise* aPromise,
> +                                         const TVSourceType aSourceType);

const value arguments are not that useful...

::: dom/tv/TVServiceFactory.cpp
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#if defined(MOZ_WIDGET_GONK)

Again, I don't think there should be anything gonk specific here.

@@ +18,5 @@
> +{
> +  nsCOMPtr<nsITVService> service;
> +
> +#if defined(MOZ_WIDGET_GONK)
> +  service = do_CreateInstance(FAKE_TV_SERVICE_CONTRACTID);

Hmm, this is not correct.  Where is the code that looks for the real service?

::: dom/tv/TVServiceFactory.h
@@ +7,5 @@
> +#ifndef mozilla_dom_TVServiceFactory_h__
> +#define mozilla_dom_TVServiceFactory_h__
> +
> +#include "nsCOMPtr.h"
> +#include "nsITVService.h"

Nit: please forward declare nsITVService.

::: dom/tv/TVTypes.cpp
@@ +37,5 @@
> +  return NS_OK;
> +}
> +
> +/* virtual */ NS_IMETHODIMP
> +TVTunerData::GetSupportedSourceTypes(uint32_t* aCount, char*** aSourceTypes)

Since this code is called by partner code which can do anything, please do proper validation of input arguments everywhere.  i.e., don't assume that things are null, or point to sane values, etc.  For example, does it make sense here to have an object with an empty string as an ID?  If not, you should check that here, and in SetId too.  And so on...  this patch is currently extremely light on error checking. ;-)

@@ +42,5 @@
> +{
> +  *aCount = mCount;
> +
> +  char** sourceTypes = static_cast<char **>(NS_Alloc(mCount * sizeof(char*)));
> +  if (!sourceTypes) {

NS_Alloc is infallible.

@@ +48,5 @@
> +  }
> +
> +  for (uint32_t i = 0; i < mCount; i++) {
> +    sourceTypes[i] = NS_strdup(mSupportedSourceTypes[i]);
> +    if (!sourceTypes[i]) {

NS_strdup is also infallible.

@@ +49,5 @@
> +
> +  for (uint32_t i = 0; i < mCount; i++) {
> +    sourceTypes[i] = NS_strdup(mSupportedSourceTypes[i]);
> +    if (!sourceTypes[i]) {
> +      NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(i, sourceTypes);

But if it wasn't, this would be insufficient, FYI.  You'd need to loop from 0 to i and free all of the previously duped strings...

@@ +69,5 @@
> +
> +  mCount = aCount;
> +
> +  mSupportedSourceTypes = static_cast<char **>(NS_Alloc(mCount * sizeof(char*)));
> +  if (!mSupportedSourceTypes) {

Ditto.

@@ +75,5 @@
> +  }
> +
> +  for (uint32_t i = 0; i < mCount; i++) {
> +    mSupportedSourceTypes[i] = NS_strdup(aSourceTypes[i]);
> +    if (!mSupportedSourceTypes[i]) {

Ditto.

@@ +101,5 @@
> +
> +/* virtual */ NS_IMETHODIMP
> +TVChannelData::SetNetworkId(const nsAString& aNetworkId)
> +{
> +  mNetworkId = aNetworkId;

Again, do we need to do any error checking for these setters?

@@ +338,5 @@
> +{
> +  *aCount = mAudioLanguageCount;
> +
> +  char** languages = static_cast<char **>(NS_Alloc(mAudioLanguageCount * sizeof(char*)));
> +  if (!languages) {

Same comments as above here and below.

::: dom/tv/TVTypes.h
@@ +22,5 @@
> +  ~TVTunerData();
> +
> +  nsString mId;
> +  char** mSupportedSourceTypes;
> +  unsigned long mCount;

The sizes here should be of type size_t.

::: dom/tv/gonk/FakeTVService.cpp
@@ +23,5 @@
> +
> +/* virtual */ NS_IMETHODIMP
> +FakeTVService::SetSourceListener(nsITVSourceListener* aSourceListener)
> +{
> +  mSourceListener = aSourceListener;

Not sure where nsITVSourceListener is implemented, but we need to make sure that it doesn't hold a string reference to this object, otherwise this will form a cycle, in which case we would need to make sure these objects participate in cycle collection.

Can you please clarify the object ownerships here?

@@ +30,5 @@
> +
> +/* virtual */ NS_IMETHODIMP
> +FakeTVService::GetTuners(nsITVServiceCallback* aCallback)
> +{
> +  nsCOMPtr<nsIMutableArray> tunerDataList = do_CreateInstance(NS_ARRAY_CONTRACTID);

Hopefully, you wouldn't need to do this dance...

@@ +37,5 @@
> +  }
> +
> +  // TODO Implement in follow-up patches.
> +
> +  nsresult rv = aCallback->NotifySuccess(tunerDataList);

I think we should document that these callbacks must be called asynchronously, and even try to detect that and return an error code.  Otherwise, if the callback calls back into nsITVService, we'd need to deal with re-entrancy issues, which can be a huge pain.  So, here and elsewhere in this file you'd need to create actual runnable objects and call the callback off of those runnables, after the function returns.

::: dom/tv/gonk/FakeTVService.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_FakeTVService_h__

Nit: please remove the trailing underscores here and elsewhere.

::: dom/tv/interface/moz.build
@@ +1,1 @@
> +# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-

This file will need to go away.

::: dom/tv/interface/nsITVService.idl
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

A general comment that I forgot to say before, can you please go through all of the Gecko implemented stuff in this IDL file, and everywhere that the only possible error code is NS_OK, make the corresponding IDL declaration [infallible]?  Thanks!

@@ +32,5 @@
> +   * @param count       The number of supported source types.
> +   * @param sourceTypes An array of supported source types.
> +   */
> +  void getSupportedSourceTypes([optional] out unsigned long count,
> +                               [retval, array, size_is(count)] out string sourceTypes);

Do you really want "string" for these?

@@ +188,5 @@
> +   * @param tunerId         The ID of the tuner which the source belongs to.
> +   * @param sourceType      The type of the source which the channel belongs to.
> +   *                        Please refer to http://seanyhlin.github.io/TV-Manager-API/
> +   *                        for available values.
> +   * @param channelNumber   The LCN of the channel which the programs belong to.

Please document what "LCN" means.

@@ +219,5 @@
> +   * Called when the operation succeeds.
> +   *
> +   * @param dataList A list of data.
> +   */
> +  void notifySuccess([optional] in nsIArray dataList);

You didn't address this comment: "Please clarify anywhere that nsIArray is used whether nsIMutableArray should be supported on the object. (I don't think there is a good reason to support that, but this needs to be documented.)".  That being said, we can just use an IDL here too, I think, and not bother with nsIArray at all.

@@ +270,5 @@
> +   *
> +   * @param tunerId     The ID of the tuner.
> +   * @param sourceType  The source type to be used.
> +   * @param isRescanned This flag indicates whether the underlying layer should
> +   *                    clear all cached channels, if any, before this scanning.

Instead of doing this, why not have a separate method called clearScannedChannelsCache() or some such?

::: dom/tv/moz.build
@@ +3,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +DIRS += ['interface']

Please get rid of the interface directory and just put the IDL in the same directory as the source files.

@@ +31,3 @@
>  ]
>  
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':

Is there any good reason to build the fake TV service only on gonk?  Why not build it (and run its tests) everywhere?

Comment 75 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-01 03:12:46 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #74)
> Comment on attachment 8496730 [details] [diff] [review]
> New Part 2 - TV service, v4
> 
> Review of attachment 8496730 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/tv/TVTypes.cpp
> @@ +37,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +/* virtual */ NS_IMETHODIMP
> > +TVTunerData::GetSupportedSourceTypes(uint32_t* aCount, char*** aSourceTypes)
> 
> Since this code is called by partner code which can do anything, please do
> proper validation of input arguments everywhere.  i.e., don't assume that
> things are null, or point to sane values, etc.  For example, does it make
> sense here to have an object with an empty string as an ID?  If not, you
> should check that here, and in SetId too.  And so on...  this patch is
> currently extremely light on error checking. ;-)

Will add more sanity checks.

> ::: dom/tv/gonk/FakeTVService.cpp
> @@ +23,5 @@
> > +
> > +/* virtual */ NS_IMETHODIMP
> > +FakeTVService::SetSourceListener(nsITVSourceListener* aSourceListener)
> > +{
> > +  mSourceListener = aSourceListener;
> 
> Not sure where nsITVSourceListener is implemented, but we need to make sure
> that it doesn't hold a string reference to this object, otherwise this will
> form a cycle, in which case we would need to make sure these objects
> participate in cycle collection.
> 
> Can you please clarify the object ownerships here?

Will add the implementation of nsITVSourceListener to this patch. So it'd be easier to ensure no unexpected reference is hold.

> @@ +32,5 @@
> > +   * @param count       The number of supported source types.
> > +   * @param sourceTypes An array of supported source types.
> > +   */
> > +  void getSupportedSourceTypes([optional] out unsigned long count,
> > +                               [retval, array, size_is(count)] out string sourceTypes);
> 
> Do you really want "string" for these?

I was thinking source types were in ascii strings like "dvb-t" so "string" seemed enough. Would it be better to use "wstring" or some other types for it?

> @@ +219,5 @@
> > +   * Called when the operation succeeds.
> > +   *
> > +   * @param dataList A list of data.
> > +   */
> > +  void notifySuccess([optional] in nsIArray dataList);
> 
> You didn't address this comment: "Please clarify anywhere that nsIArray is
> used whether nsIMutableArray should be supported on the object. (I don't
> think there is a good reason to support that, but this needs to be
> documented.)".  That being said, we can just use an IDL here too, I think,
> and not bother with nsIArray at all.
|dataList| would be an array of |nsITVTunerData| when it's used for |getTuners()|; whereas it'd be an array of |nsITVProgramData| for |getPrograms()|. Keeping using nsIArray may prevent |nsITVServiceCallback| from being split into multiple interfaces with different |notifySuccess|. Though the implementation of TV service may need nsIMutableArray to fill in the array, it doesn't seem necessary for other places to use nsIMutableArray. Thoughts?!

Comment 76 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-02 00:16:34 PDT

Created attachment 8498687 [details] [diff] [review]
New Part 2 - TV service, v5

Updating based on the comments for the previous revision.

Besides, I brought |TV_SERVICE_CID| back to nsITVService.idl and removed the specific CID for the fake service implementation, since a preference value is used in |TVServiceFactory| to decide to load either the real service or the fake one. And it doesn't appear we could leave |TV_SERVICE_CID| undefined since it's necessary in nsLayoutModule.cpp for XPCOM mapping. So when implementing TVManager, TVSource and similar classes, we may simply use |do_getService(TV_SERVICE_CONTRACTID)|.

Comment 77 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-02 03:31:37 PDT

Created attachment 8498791 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, v3

Updating based on current TV service interface.

Comment 78 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-02 03:32:59 PDT

Created attachment 8498792 [details] [diff] [review]
New Part 4 - TV channel & TV program, v3

Updating based on current TV service interface.

Comment 79 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-02 12:01:47 PDT

(In reply to Sean Lin [:seanlin] from comment #75)
> > @@ +32,5 @@
> > > +   * @param count       The number of supported source types.
> > > +   * @param sourceTypes An array of supported source types.
> > > +   */
> > > +  void getSupportedSourceTypes([optional] out unsigned long count,
> > > +                               [retval, array, size_is(count)] out string sourceTypes);
> > 
> > Do you really want "string" for these?
> 
> I was thinking source types were in ascii strings like "dvb-t" so "string"
> seemed enough. Would it be better to use "wstring" or some other types for
> it?

I was thinking DOMString, but I guess I don't care very strongly.

> > @@ +219,5 @@
> > > +   * Called when the operation succeeds.
> > > +   *
> > > +   * @param dataList A list of data.
> > > +   */
> > > +  void notifySuccess([optional] in nsIArray dataList);
> > 
> > You didn't address this comment: "Please clarify anywhere that nsIArray is
> > used whether nsIMutableArray should be supported on the object. (I don't
> > think there is a good reason to support that, but this needs to be
> > documented.)".  That being said, we can just use an IDL here too, I think,
> > and not bother with nsIArray at all.
> |dataList| would be an array of |nsITVTunerData| when it's used for
> |getTuners()|; whereas it'd be an array of |nsITVProgramData| for
> |getPrograms()|. Keeping using nsIArray may prevent |nsITVServiceCallback|
> from being split into multiple interfaces with different |notifySuccess|.
> Though the implementation of TV service may need nsIMutableArray to fill in
> the array, it doesn't seem necessary for other places to use
> nsIMutableArray. Thoughts?!

All great points!  Please document them in the IDL.

Comment 80 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-02 12:03:40 PDT

Comment on attachment 8498687 [details] [diff] [review]
New Part 2 - TV service, v5

Please submit an interdiff.  (It's a pain to review the whole patch every time because I need to reconstruct the changed parts in my head manually, and I don't trust Bugzilla's interdiff at all.)

Holding off reviewing the other parts because I want to get a good understanding of how things stack on top of each other...

Comment 81 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-02 20:55:44 PDT

Created attachment 8499320 [details] [diff] [review]
New Part 2 - TV service, diff v4 v5

The interdiff between v4 and v5.

Comment 82 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-02 21:10:00 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #79)
> (In reply to Sean Lin [:seanlin] from comment #75)
> > > @@ +32,5 @@
> > > > +   * @param count       The number of supported source types.
> > > > +   * @param sourceTypes An array of supported source types.
> > > > +   */
> > > > +  void getSupportedSourceTypes([optional] out unsigned long count,
> > > > +                               [retval, array, size_is(count)] out string sourceTypes);
> > > 
> > > Do you really want "string" for these?
> > 
> > I was thinking source types were in ascii strings like "dvb-t" so "string"
> > seemed enough. Would it be better to use "wstring" or some other types for
> > it?
> 
> I was thinking DOMString, but I guess I don't care very strongly.

Actually DOMString was my first shot. But it ended up generating "nsAString&*" or "const nsAString&*", and caused build errors like "cannot declare pointer to ‘class nsAString_internal&’". And there appears no current example to use "array, size_is" with DOMString in our code base. So I guess DOMString might not be well supported for this case. Just FYI. :)

Comment 83 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-03 10:34:11 PDT

(In reply to Sean Lin [:seanlin] from comment #82)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #79)
> > (In reply to Sean Lin [:seanlin] from comment #75)
> > > > @@ +32,5 @@
> > > > > +   * @param count       The number of supported source types.
> > > > > +   * @param sourceTypes An array of supported source types.
> > > > > +   */
> > > > > +  void getSupportedSourceTypes([optional] out unsigned long count,
> > > > > +                               [retval, array, size_is(count)] out string sourceTypes);
> > > > 
> > > > Do you really want "string" for these?
> > > 
> > > I was thinking source types were in ascii strings like "dvb-t" so "string"
> > > seemed enough. Would it be better to use "wstring" or some other types for
> > > it?
> > 
> > I was thinking DOMString, but I guess I don't care very strongly.
> 
> Actually DOMString was my first shot. But it ended up generating
> "nsAString&*" or "const nsAString&*", and caused build errors like "cannot
> declare pointer to ‘class nsAString_internal&’". And there appears no
> current example to use "array, size_is" with DOMString in our code base. So
> I guess DOMString might not be well supported for this case. Just FYI. :)

OK no problem!

Comment 84 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-03 12:09:56 PDT

Comment on attachment 8499320 [details] [diff] [review]
New Part 2 - TV service, diff v4 v5

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

Looks good overall, this is a lot closer!

::: dom/tv/TVListeners.cpp
@@ +52,5 @@
> +
> +  nsRefPtr<TVSource> source;
> +  if (!tunerSources->Get(sourceType, getter_AddRefs(source))) {
> +    return NS_OK;
> +  }

It looks like you can move this boilerplate into a helper member function such as GetTVSourceForTunerAndSource.

::: dom/tv/TVListeners.h
@@ +23,5 @@
> +  NS_DECL_NSITVSOURCELISTENER
> +
> +  nsresult RegisterSource(const nsRefPtr<TVSource>& aSource);
> +
> +  nsresult UnregisterSource(const nsRefPtr<TVSource>& aSource);

Nit: Instead of passing a ref pointer, pass a TVSource* directly.

@@ +32,5 @@
> +  static nsresult GetTunerIdAndSourceType(const nsRefPtr<TVSource>& aSource,
> +                                          nsAString& aTunerId,
> +                                          nsAString& aSourceType);
> +
> +  nsClassHashtable<nsStringHashKey, nsRefPtrHashtable<nsStringHashKey, TVSource>> mSources;

Please document what the keys to these two hashtables are.

::: dom/tv/TVServiceCallbacks.cpp
@@ -42,5 @@
>  
>  /* virtual */ NS_IMETHODIMP
>  TVServiceSourceSetterCallback::NotifySuccess(nsIArray* aDataList)
>  {
> -  MOZ_ASSERT(!aDataList);

Sorry if I wasn't clear before.  You do need to check all of the arguments to the functions that can be called by the partner code.  For example, if one of these functions (like this one) accepts a pointer, you cannot assume that the pointer is null, so you should do something like:

NS_ENSURE_ARG_POINTER(aDataList);

to make sure that the function returns an error code if someone passes in null, and doesn't crash.

Please do this for this function and all of the others below where the arguments are coming from the partner provided code.

Thanks!

@@ +65,3 @@
>      break;
>    default:
> +    mPromise->MaybeReject(NS_ERROR_ILLEGAL_VALUE);

You're handling ERROR_NOT_SUPPORTED properly, but for others, you're rejecting the promises with our internal error code values.  Can you please clarify in the spec what error codes can be used for rejecting the promise here, and use that?  You can find a list of all of the defined DOMException codes here <https://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#exception-domexception>.  Typically, in our code you can use a NS_ERROR_DOM_ prefix to get those (like the above for NS_ERROR_DOM_NOT_SUPPORTED_ERR.)

@@ +71,1 @@
>    return NS_OK;

What if the error code is not something that we expect?  I don't think it's fine to let the promise linger without either resolving it or rejecting it.

@@ -104,5 @@
>  
>  /* virtual */ NS_IMETHODIMP
>  TVServiceChannelScanCallback::NotifySuccess(nsIArray* aDataList)
>  {
> -  MOZ_ASSERT(!aDataList);

Ditto.

@@ +130,1 @@
>    return NS_OK;

All of the same comments as above apply here too.

@@ +152,4 @@
>  {
>    MOZ_ASSERT(mSource);
>    MOZ_ASSERT(mPromise);
> +  MOZ_ASSERT(!mChannelNumber.IsEmpty());

Why is it OK to just assert this?

@@ -166,5 @@
>  
>  /* virtual */ NS_IMETHODIMP
>  TVServiceChannelSetterCallback::NotifySuccess(nsIArray* aDataList)
>  {
> -  MOZ_ASSERT(!aDataList);

This too.

@@ +185,3 @@
>      return NS_ERROR_ILLEGAL_VALUE;
>    }
>  

Here too.

@@ +216,5 @@
>  TVServiceTunerGetterCallback::NotifySuccess(nsIArray* aDataList)
>  {
> +  if (!aDataList) {
> +    return NS_ERROR_INVALID_ARG;
> +  }

Nice!

@@ +245,3 @@
>      return NS_ERROR_ILLEGAL_VALUE;
>    }
>  

Here too.

::: dom/tv/TVTypes.cpp
@@ +57,5 @@
>    return NS_OK;
>  }
>  
>  /* virtual */ NS_IMETHODIMP
> +TVTunerData::SetSupportedSourceTypes(uint32_t aCount,

You need to handle the case where aCount is zero too.

@@ +167,1 @@
>    mType = aType;

I'm not sure how you're going to use mType later on, but please consider storing the enum value internally if you're going to need to do a lot of string comparisons on it.

@@ +375,2 @@
>  {
>    *aCount = mAudioLanguageCount;

What if mAudioLanguageCount is zero?

@@ +395,5 @@
>    if (mAudioLanguages) {
>      NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(mAudioLanguageCount, mAudioLanguages);
>    }
>  
>    mAudioLanguageCount = aCount;

Ditto.

@@ +412,2 @@
>  {
>    *aCount = mSubtitleLanguageCount;

Same here.

::: dom/tv/TVUtils.h
@@ +18,5 @@
> +nsString
> +ToTVSourceTypeStr(const TVSourceType aSourceType)
> +{
> +  nsString str;
> +  str.AssignASCII(TVSourceTypeValues::strings[uint32_t(aSourceType)].value);

Please MOZ_ASSERT that aSourceType doesn't point to past the end of the buffer.

@@ +109,5 @@
> +ToTVSourceType(const char* aStr)
> +{
> +  nsString str;
> +  str.AssignASCII(aStr);
> +  return ToTVSourceType(str);

You should be able to just say:

return ToTVSourceType(aStr);

::: dom/tv/gonk/FakeTVService.cpp
@@ +10,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +class TVServiceRunnable : public nsRunnable

Nit: can you please give this a less generic name?  Perhaps TVServiceNotifyResultRunnable?

@@ +15,5 @@
> +{
> +public:
> +  TVServiceRunnable(nsITVServiceCallback* aCallback,
> +                    uint16_t aErrorCode,
> +                    nsIArray* aDataList)

I suggest moving aErrorCode to the third argument, and giving it a NO_ERROR default value, so that you wouldn't need to repeat NO_ERROR everywhere below.

@@ +34,5 @@
> +private:
> +  nsCOMPtr<nsITVServiceCallback> mCallback;
> +  uint16_t mErrorCode;
> +  nsCOMPtr<nsIArray> mDataList;
> +};

It looks like the partner can use this class as a utility for calling the callback.  Please move this into its own header to facilitate that, and please document how it should be used, etc.

@@ +64,5 @@
>  
>    // TODO Implement in follow-up patches.
>  
> +  nsCOMPtr<nsIRunnable> runnable =
> +    new TVServiceRunnable(aCallback, nsITVServiceCallback::NO_ERROR, tunerDataList);

You need to null chck aCallback (mostly for code hygine) here and below.

::: dom/tv/moz.build
@@ +7,4 @@
>  TEST_DIRS += ['test']
>  
>  EXPORTS.mozilla.dom += [
> +    'gonk/FakeTVService.h',

Please move these two files into the dom/tv directory too.

::: dom/tv/nsITVService.idl
@@ +235,5 @@
>  %{C++
>  #define TV_SERVICE_CONTRACTID \
>    "@mozilla.org/tv/tvservice;1"
> +#define TV_SERVICE_CID \
> +  { 0x60fb3c53, 0x017f, 0x4340, { 0x91, 0x1b, 0xd5, 0x5c, 0x31, 0x28, 0x88, 0xb6 } }

Why did you add this back again?  We cannot know the CID for the TV Service because it will be provided by the partner component.

::: layout/build/nsLayoutModule.cpp
@@ +802,5 @@
>  #ifdef ACCESSIBILITY
>  NS_DEFINE_NAMED_CID(NS_ACCESSIBILITY_SERVICE_CID);
>  #endif
>  NS_DEFINE_NAMED_CID(TELEPHONY_SERVICE_CID);
> +NS_DEFINE_NAMED_CID(TV_SERVICE_CID);

Please call this FAKE_TV_SERVICE_ID again.

Comment 85 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-03 14:00:59 PDT

Comment on attachment 8498791 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, v3

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

::: dom/tv/TVManager.cpp
@@ +14,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +NS_IMPL_CYCLE_COLLECTION_INHERITED(TVManager, DOMEventTargetHelper, mTVService)

You forgot mTuners and mPendingGetTunersPromises.

@@ +33,5 @@
> +
> +  nsCOMPtr<nsITVServiceCallback> callback =
> +    new TVServiceTunerGetterCallback(this);
> +  nsresult rv = mTVService->GetTuners(callback);
> +  NS_WARN_IF(NS_FAILED(rv));

Same comments as before.

@@ +48,5 @@
>  
> +void
> +TVManager::SetTuners(const nsTArray<nsRefPtr<TVTuner>>& aTuners)
> +{
> +  MOZ_ASSERT(!mIsReady);

Proper error checking, please.

@@ +58,5 @@
> +  uint32_t length = mPendingGetTunersPromises.Length();
> +  for(uint32_t i = 0; i < length; i++) {
> +    mPendingGetTunersPromises[i]->MaybeResolve(mTuners);
> +  }
> +  mPendingGetTunersPromises.RemoveElementsAt(0, length);

Why not just Clear() the array?

@@ +64,5 @@
> +
> +void
> +TVManager::RejectPendingGetTunersPromises(nsresult aRv)
> +{
> +  MOZ_ASSERT(!mIsReady);

Ditto.

@@ +71,5 @@
> +  uint32_t length = mPendingGetTunersPromises.Length();
> +  for(uint32_t i = 0; i < length; i++) {
> +    mPendingGetTunersPromises[i]->MaybeReject(aRv);
> +  }
> +  mPendingGetTunersPromises.RemoveElementsAt(0, length);

Here too.

@@ +133,5 @@
> +  nsRefPtr<TVParentalControlChangedEvent> event =
> +    TVParentalControlChangedEvent::Constructor(this,
> +                                               NS_LITERAL_STRING("parentalcontrolchanged"),
> +                                               init);
> +  return DispatchTrustedEvent(event);

Please dispatch asynchronously.

::: dom/tv/TVServiceCallbacks.cpp
@@ +234,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +
> +  nsTArray<nsRefPtr<TVTuner>> tuners;

Please set the capacity of the array to |length| to avoid growing the array needlessly.

@@ +268,3 @@
>      break;
>    default:
> +    mManager->RejectPendingGetTunersPromises(NS_ERROR_ILLEGAL_VALUE);

Same comment as before.

::: dom/tv/TVServiceCallbacks.h
@@ +82,5 @@
>    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>    NS_DECL_NSITVSERVICECALLBACK
>    NS_DECL_CYCLE_COLLECTION_CLASS(TVServiceTunerGetterCallback)
>  
> +  TVServiceTunerGetterCallback(TVManager* aManager);

Nit: explicit.

::: dom/tv/TVSource.cpp
@@ +19,5 @@
>  namespace mozilla {
>  namespace dom {
>  
> +NS_IMPL_CYCLE_COLLECTION_INHERITED(TVSource, DOMEventTargetHelper,
> +                                   mTVService, mTuner, mCurrentChannel)

Upon further thought, it seems like making an XPCOM service participate in cycle collection is a mistake, since XPCOM services basically live for the lifetime of the process, so if they ever hold a reference to for example a DOM object, we'd be risking holding alive that entire document until shutdown, which is terrible.

I think we need to just use a normal XPCOM object here, perhaps by renaming nsITVService to nsITVBackend and ensuring that we create that object using do_CreateInstance as opposed to do_GetService, etc.  Then, we need to make sure that the TV backend object does participate in cycle collection.  In order to do that, we should QI the object to nsCycleCollectionISupports and report an error if that QI fails.  (Since that would be a large change which would be mostly renaming, it'd be great if you could do that in a separate patch.)

@@ +34,3 @@
>    : DOMEventTargetHelper(aWindow)
> +  , mTuner(aTuner)
> +  , mCurrentChannel(nullptr)

This is not needed.

@@ +39,4 @@
>  {
> +  MOZ_ASSERT(mTuner);
> +  mTVService = do_GetService(TV_SERVICE_CONTRACTID);
> +  MOZ_ASSERT(mTVService);

You can't assert success here, you need to have a fallible Init method and do proper error checking there.

@@ +40,5 @@
> +  MOZ_ASSERT(mTuner);
> +  mTVService = do_GetService(TV_SERVICE_CONTRACTID);
> +  MOZ_ASSERT(mTVService);
> +
> +  nsITVSourceListener* sourceListener;

nsCOMPtr, please.

(FWIW this code should leak, please take a moment to investigate why you haven't caught the leak locally.)

@@ +42,5 @@
> +  MOZ_ASSERT(mTVService);
> +
> +  nsITVSourceListener* sourceListener;
> +  mTVService->GetSourceListener(&sourceListener);
> +  MOZ_ASSERT(sourceListener);

Again, you cannot assert success.

@@ +48,5 @@
>  }
>  
>  TVSource::~TVSource()
>  {
> +  MOZ_ASSERT(mTVService);

Why is this needed?

@@ +52,5 @@
> +  MOZ_ASSERT(mTVService);
> +
> +  nsITVSourceListener* sourceListener;
> +  mTVService->GetSourceListener(&sourceListener);
> +  MOZ_ASSERT(sourceListener);

Same comments as above.

@@ +125,5 @@
>    }
>  
> +  // The operation is prohibited when the source is scanning channels.
> +  if (mIsScanning) {
> +    promise->MaybeReject(NS_ERROR_DOM_SECURITY_ERR);

InvalidStateErr looks like a better error code (please adjust the spec too.)

@@ +148,5 @@
>    }
>  
> +  // The operation is prohibited when the source is scanning channels.
> +  if (mIsScanning) {
> +    promise->MaybeReject(NS_ERROR_DOM_SECURITY_ERR);

Ditto.

@@ +155,5 @@
> +
> +  nsString tunerId;
> +  mTuner->GetId(tunerId, aRv);
> +  if (aRv.Failed()) {
> +    promise->MaybeReject(aRv.ErrorCode());

In general it's better to not reject promises with our internal error codes (I won't repear this everywhere)

@@ +193,5 @@
> +  bool isRescanned = aOptions.mIsRescanned.WasPassed() &&
> +                     aOptions.mIsRescanned.Value();
> +
> +  if (isRescanned) {
> +    // TODO Clear the stored channel data from IndexedDB in follow-up patches.

FWIW using IndexedDB from C++ is a bit painful.  Any specific reason why you want to do that?

@@ +268,5 @@
>  
>  bool
>  TVSource::GetIsScanning(ErrorResult& aRv) const
>  {
> +  return mIsScanning;

Nit: these two methods can be inline.  I'm fine either way.

@@ +290,5 @@
> +
> +nsresult
> +TVSource::NotifyChannelScanComplete()
> +{
> +  this->SetIsScanning(false);

Drop |this->| here and elsewhere.

@@ +314,5 @@
> +  return DispatchEITBroadcastedEvent(programs);
> +}
> +
> +nsresult
> +TVSource::DispatchCurrentChannelChangedEvent(const nsRefPtr<TVChannel>& aChannel)

I think it's better to dispatch all of these events asynchronously.  Please do that, and update the spec accordingly.

::: dom/tv/TVSource.h
@@ +33,5 @@
>    NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(TVSource, DOMEventTargetHelper)
>  
> +  explicit TVSource(nsPIDOMWindow* aWindow,
> +                    TVSourceType aType,
> +                    TVTuner* aTuner);

Please drop explicit.

::: dom/tv/TVTuner.cpp
@@ +17,5 @@
>  namespace mozilla {
>  namespace dom {
>  
> +NS_IMPL_CYCLE_COLLECTION_INHERITED(TVTuner, DOMEventTargetHelper,
> +                                   mTVService, mStream, mCurrentSource)

You forgot mSources.

@@ +31,2 @@
>    : DOMEventTargetHelper(aWindow)
> +  , mCurrentSource(nullptr)

This is not needed.

@@ +50,5 @@
> +  }
> +  NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(count, supportedSourceTypes);
> +
> +  mTVService = do_GetService(TV_SERVICE_CONTRACTID);
> +  MOZ_ASSERT(mTVService);

Please avoid doing non-trivial construction work which is fallible in the constructor.  Instead add a fallible Init method and move this work there.  Also, again, you can't assert success.

@@ +105,3 @@
>  void
>  TVTuner::GetSupportedSourceTypes(nsTArray<TVSourceType>& aSourceTypes,
>                                   ErrorResult& aRv) const

Why is this method fallible then?

@@ +148,5 @@
>    return promise.forget();
>  }
>  
>  void
>  TVTuner::GetId(nsAString& aId, ErrorResult& aRv) const

Ditto.

@@ +155,4 @@
>  }
>  
>  already_AddRefed<TVSource>
>  TVTuner::GetCurrentSource(ErrorResult& aRv) const

This too.

@@ +162,4 @@
>  }
>  
>  already_AddRefed<DOMMediaStream>
>  TVTuner::GetStream(ErrorResult& aRv) const

And so on... please check all of these methods for this issue in all of the patchs.

::: dom/tv/TVTuner.h
@@ +29,5 @@
>    NS_DECL_ISUPPORTS_INHERITED
>    NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(TVTuner, DOMEventTargetHelper)
>  
> +  explicit TVTuner(nsPIDOMWindow* aWindow,
> +                   nsITVTunerData* aData);

Drop explicit.

@@ +35,5 @@
>    // WebIDL (internal functions)
>  
>    virtual JSObject* WrapObject(JSContext *aCx) MOZ_OVERRIDE;
>  
> +  nsresult SetCurrentSource(const TVSourceType aSourceType);

const arguments are mostly useless. :)

@@ +60,5 @@
>    ~TVTuner();
>  
> +  nsresult InitMediaStream();
> +
> +  nsresult DispatchCurrentSourceChangedEvent(const nsRefPtr<TVSource>& aSource);

Nit: TVSource*.

Comment 86 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-03 14:20:03 PDT

Comment on attachment 8498792 [details] [diff] [review]
New Part 4 - TV channel & TV program, v3

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

::: dom/tv/TVChannel.cpp
@@ +50,5 @@
> +
> +  mTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
> +  MOZ_ASSERT(mTimer);
> +
> +  NS_WARN_IF(NS_FAILED(LoadCurrentProgram()));

Same comment as before.

@@ +55,5 @@
>  }
>  
>  TVChannel::~TVChannel()
>  {
> +  MOZ_ASSERT(mTimer);

Why is this needed?

@@ +56,5 @@
>  
>  TVChannel::~TVChannel()
>  {
> +  MOZ_ASSERT(mTimer);
> +  mTimer->Cancel();

Cycle collected objects can be destroyed a long time after they're no longer useful, if they end up in a cycle and the CC kills them.  Therefore, we usually do useful uninitialization work like this in a separate helper method, and we call that method from both the destructor and the Unlink() method.  For that, you of course need to expand the CC macros above again.

@@ +69,5 @@
> +nsresult
> +TVChannel::LoadCurrentProgram()
> +{
> +  // TODO Load current program data from IndexedDB in follow-up patches.
> +  nsRefPtr<nsITVProgramData> programData = nullptr;

No need to assign to nullptr.  Also, use nsCOMPtr for XPCOM interfaces please.

@@ +70,5 @@
> +TVChannel::LoadCurrentProgram()
> +{
> +  // TODO Load current program data from IndexedDB in follow-up patches.
> +  nsRefPtr<nsITVProgramData> programData = nullptr;
> +  if (!programData) {

???  How can programData *not* be null here? ;-)

@@ +78,5 @@
> +  mCurrentProgram = new TVProgram(GetOwner(), this, programData);
> +
> +  // Set the timer for the next program.
> +  mTimer->Cancel();
> +  return SetTimer(mCurrentProgram);

Please cancel the timer in SetTimer if needed.

@@ +87,5 @@
>  {
>    nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetOwner());
>    MOZ_ASSERT(global);
>  
> +  nsCOMPtr<Promise> promise = Promise::Create(global, aRv);

nsRePtr.

@@ +124,5 @@
>  }
>  
>  void
> +TVChannel::GetNetworkId(nsAString& aNetworkId,
> +                        ErrorResult& aRv) const

Same comment about infallible functions everywhere in this patch.

@@ +211,5 @@
> +nsresult
> +TVChannel::SetTimer(const nsRefPtr<TVProgram>& aProgram)
> +{
> +  ErrorResult error;
> +  uint64_t startTime = aProgram->GetStartTime(error);

Absolute times like this are very dangerous.  What if the system time is changed between the point where the start time of the TV program is set and the time we read it here?

@@ +221,5 @@
> +    return error.ErrorCode();
> +  }
> +  mTimer->InitWithCallback(this,
> +                           startTime + duration - PR_Now(),
> +                           nsITimer::TYPE_ONE_SHOT);

Isn't this assuming that the program will run until it's complete?  That seems wrong.

@@ +235,5 @@
> +  nsRefPtr<TVCurrentProgramChangedEvent> event =
> +    TVCurrentProgramChangedEvent::Constructor(this,
> +                                              NS_LITERAL_STRING("currentprogramchanged"),
> +                                              init);
> +  return DispatchTrustedEvent(event);

Dispatch events asynchronously please.

::: dom/tv/TVChannel.h
@@ +75,5 @@
>    ~TVChannel();
>  
> +  NS_DECL_NSITIMERCALLBACK
> +
> +  nsresult SetTimer(const nsRefPtr<TVProgram>& aProgram);

TVProgram*.

@@ +77,5 @@
> +  NS_DECL_NSITIMERCALLBACK
> +
> +  nsresult SetTimer(const nsRefPtr<TVProgram>& aProgram);
> +
> +  nsresult DispatchCurrentProgramChangedEvent(const nsRefPtr<TVProgram>& aProgram);

Same.

@@ +79,5 @@
> +  nsresult SetTimer(const nsRefPtr<TVProgram>& aProgram);
> +
> +  nsresult DispatchCurrentProgramChangedEvent(const nsRefPtr<TVProgram>& aProgram);
> +
> +  nsresult DispatchProtectionStateChangedEvent(const bool aIsProtected);

s/const// everywhere for value arguments.

::: dom/tv/TVProgram.cpp
@@ +48,5 @@
> +    mAudioLanguages.AppendElement(language);
> +  }
> +  NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(count, languages);
> +
> +  aData->GetAudioLanguages(&count, &languages);

You mean subtitle languages here.

@@ +54,5 @@
> +    nsString language;
> +    language.AssignASCII(languages[i]);
> +    mSubtitleLanguages.AppendElement(language);
> +  }
> +  NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(count, languages);

You can add a helper for this that takes a nsString& and an array and a count and you can then call it once for audio languages and once for subtitle languages.

::: dom/tv/TVSource.cpp
@@ +331,1 @@
>    return DispatchEITBroadcastedEvent(programs);

Should you dispatch the event regardless of whether you actually did something here?

Comment 87 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-03 14:20:48 PDT

Sean, please send an intent email to dev-platform according to https://wiki.mozilla.org/WebAPI/ExposureGuidelines.  Thanks!

Comment 88 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-06 02:58:33 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #84)
> Comment on attachment 8499320 [details] [diff] [review]
> New Part 2 - TV service, diff v4 v5
> 
> Review of attachment 8499320 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/tv/TVServiceCallbacks.cpp
> @@ -42,5 @@
> >  
> >  /* virtual */ NS_IMETHODIMP
> >  TVServiceSourceSetterCallback::NotifySuccess(nsIArray* aDataList)
> >  {
> > -  MOZ_ASSERT(!aDataList);
> 
> Sorry if I wasn't clear before.  You do need to check all of the arguments
> to the functions that can be called by the partner code.  For example, if
> one of these functions (like this one) accepts a pointer, you cannot assume
> that the pointer is null, so you should do something like:
> 
> NS_ENSURE_ARG_POINTER(aDataList);
> 
> to make sure that the function returns an error code if someone passes in
> null, and doesn't crash.
> 
> Please do this for this function and all of the others below where the
> arguments are coming from the partner provided code.
> 
> Thanks!

Actually |aDataList| of |notifySuccess| is expected null for setters. (And non-null for getters.) The generated code from IDL is as follows:

  /* void notifySuccess ([optional] in nsIArray dataList); */
  NS_IMETHOD NotifySuccess(nsIArray *dataList) = 0;

So it appears we need to keep the prototype but expect nothing to be passed in. Thoughts?!

> @@ +71,1 @@
> >    return NS_OK;
> 
> What if the error code is not something that we expect?  I don't think it's
> fine to let the promise linger without either resolving it or rejecting it.

Unexpected error code should be already handled at the default case of the switch statement above.

> @@ +152,4 @@
> >  {
> >    MOZ_ASSERT(mSource);
> >    MOZ_ASSERT(mPromise);
> > +  MOZ_ASSERT(!mChannelNumber.IsEmpty());
> 
> Why is it OK to just assert this?

Only non-empty channel numbers, which could be strings like 12-1, are acceptable. But we're not very sure about the exact patterns of them and afraid they might vary over different regions. So I just add a loose check.

> @@ +375,2 @@
> >  {
> >    *aCount = mAudioLanguageCount;
> 
> What if mAudioLanguageCount is zero?

Thought it's rare, there might be no audio language for a program, like silent drama. So be subtitle languages since a program might have no subtitle. When it's 0, the current implementation would set |mCount| to 0 without going through any loop, and free previous language data if there's any. So far it looks good to me.

> ::: dom/tv/nsITVService.idl
> @@ +235,5 @@
> >  %{C++
> >  #define TV_SERVICE_CONTRACTID \
> >    "@mozilla.org/tv/tvservice;1"
> > +#define TV_SERVICE_CID \
> > +  { 0x60fb3c53, 0x017f, 0x4340, { 0x91, 0x1b, 0xd5, 0x5c, 0x31, 0x28, 0x88, 0xb6 } }
> 
> Why did you add this back again?  We cannot know the CID for the TV Service
> because it will be provided by the partner component.
I explained the reason in comment 76, and copy it below:

I brought |TV_SERVICE_CID| back to nsITVService.idl and removed the specific CID for the fake service implementation, since a preference value is used in |TVServiceFactory| to decide to load either the real service or the fake one. And it doesn't appear we could leave |TV_SERVICE_CID| undefined since it's necessary in nsLayoutModule.cpp for XPCOM mapping. So when implementing TVManager, TVSource and similar classes, we may simply use |do_getService(TV_SERVICE_CONTRACTID)|

But I'm not sure if it's the best way to do, so still looking for some better ideas. Thoughts?!

Comment 89 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-06 03:34:26 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #85)
> Comment on attachment 8498791 [details] [diff] [review]
> New Part 3 - TV manager, TV tuner, TV source, v3
> 
> Review of attachment 8498791 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/tv/TVServiceCallbacks.cpp
> ::: dom/tv/TVSource.cpp
> @@ +19,5 @@
> >  namespace mozilla {
> >  namespace dom {
> >  
> > +NS_IMPL_CYCLE_COLLECTION_INHERITED(TVSource, DOMEventTargetHelper,
> > +                                   mTVService, mTuner, mCurrentChannel)
> 
> Upon further thought, it seems like making an XPCOM service participate in
> cycle collection is a mistake, since XPCOM services basically live for the
> lifetime of the process, so if they ever hold a reference to for example a
> DOM object, we'd be risking holding alive that entire document until
> shutdown, which is terrible.
> 
> I think we need to just use a normal XPCOM object here, perhaps by renaming
> nsITVService to nsITVBackend and ensuring that we create that object using
> do_CreateInstance as opposed to do_GetService, etc.  Then, we need to make
> sure that the TV backend object does participate in cycle collection.  In
> order to do that, we should QI the object to nsCycleCollectionISupports and
> report an error if that QI fails.  (Since that would be a large change which
> would be mostly renaming, it'd be great if you could do that in a separate
> patch.)

Will do it in a separate patch once the current implementation becomes stable.

> @@ +193,5 @@
> > +  bool isRescanned = aOptions.mIsRescanned.WasPassed() &&
> > +                     aOptions.mIsRescanned.Value();
> > +
> > +  if (isRescanned) {
> > +    // TODO Clear the stored channel data from IndexedDB in follow-up patches.
> 
> FWIW using IndexedDB from C++ is a bit painful.  Any specific reason why you
> want to do that?

If we may store those data, then we don't need to rely the underlying service to fetch channel and program data everytime. Besides, there's a |isProtected| attribute for channels which can be turned on/off by apps. Unless the underlying service is expected to support this, we have to find a place to store the protection state.

I'm thinking to create another XPCOM component, such as TVDBService, which could be implemented in JS. Then we could access IndexedDB through JS, and then use callbacks to interact with that component over XPCOM interface.

Thoughts?!

> @@ +105,3 @@
> >  void
> >  TVTuner::GetSupportedSourceTypes(nsTArray<TVSourceType>& aSourceTypes,
> >                                   ErrorResult& aRv) const
> 
> Why is this method fallible then?

The generated WebIDL implementation (XXXBinding.cpp) appears to access the members in a fallible way.

Comment 90 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-06 03:53:33 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #86)
> Comment on attachment 8498792 [details] [diff] [review]
> New Part 4 - TV channel & TV program, v3
> 
> Review of attachment 8498792 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +55,5 @@
> >  }
> >  
> >  TVChannel::~TVChannel()
> >  {
> > +  MOZ_ASSERT(mTimer);
> 
> Why is this needed?

If we store channel and program data (EPG) in IndexedDB and don't rely on the underlying service to detect program changes in a channel. Then we need a way to know when it's time to dispatch the "programChanged" event defined in WebIDL.

> @@ +211,5 @@
> > +nsresult
> > +TVChannel::SetTimer(const nsRefPtr<TVProgram>& aProgram)
> > +{
> > +  ErrorResult error;
> > +  uint64_t startTime = aProgram->GetStartTime(error);
> 
> Absolute times like this are very dangerous.  What if the system time is
> changed between the point where the start time of the TV program is set and
> the time we read it here?

Instead of decided by the device, the start time of a program should be stated in EPG and provided by the service provider. So it shouldn't be device dependent.

> 
> @@ +221,5 @@
> > +    return error.ErrorCode();
> > +  }
> > +  mTimer->InitWithCallback(this,
> > +                           startTime + duration - PR_Now(),
> > +                           nsITimer::TYPE_ONE_SHOT);
> 
> Isn't this assuming that the program will run until it's complete?  That
> seems wrong.

The logic is to follow what we have in EPG. For a given channel, the current program will probably run until its scheduled end time, unless we receive a newly broadcasted EIT event to override the program info and reset the timer.

Comment 91 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-06 13:48:25 PDT

(In reply to Sean Lin [:seanlin] from comment #88)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #84)
> > Comment on attachment 8499320 [details] [diff] [review]
> > New Part 2 - TV service, diff v4 v5
> > 
> > Review of attachment 8499320 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: dom/tv/TVServiceCallbacks.cpp
> > @@ -42,5 @@
> > >  
> > >  /* virtual */ NS_IMETHODIMP
> > >  TVServiceSourceSetterCallback::NotifySuccess(nsIArray* aDataList)
> > >  {
> > > -  MOZ_ASSERT(!aDataList);
> > 
> > Sorry if I wasn't clear before.  You do need to check all of the arguments
> > to the functions that can be called by the partner code.  For example, if
> > one of these functions (like this one) accepts a pointer, you cannot assume
> > that the pointer is null, so you should do something like:
> > 
> > NS_ENSURE_ARG_POINTER(aDataList);
> > 
> > to make sure that the function returns an error code if someone passes in
> > null, and doesn't crash.
> > 
> > Please do this for this function and all of the others below where the
> > arguments are coming from the partner provided code.
> > 
> > Thanks!
> 
> Actually |aDataList| of |notifySuccess| is expected null for setters. (And
> non-null for getters.) The generated code from IDL is as follows:
> 
>   /* void notifySuccess ([optional] in nsIArray dataList); */
>   NS_IMETHOD NotifySuccess(nsIArray *dataList) = 0;
> 
> So it appears we need to keep the prototype but expect nothing to be passed
> in. Thoughts?!

That was just a dumb example.  :-)  Let me rephrase: if logically it doesn't make sense for something to be null/invalid, you need to have proper error checking for that, not just an assertion.

> > @@ +71,1 @@
> > >    return NS_OK;
> > 
> > What if the error code is not something that we expect?  I don't think it's
> > fine to let the promise linger without either resolving it or rejecting it.
> 
> Unexpected error code should be already handled at the default case of the
> switch statement above.

Oh right.  Then please remove the default case to make this less confusing.

> > @@ +152,4 @@
> > >  {
> > >    MOZ_ASSERT(mSource);
> > >    MOZ_ASSERT(mPromise);
> > > +  MOZ_ASSERT(!mChannelNumber.IsEmpty());
> > 
> > Why is it OK to just assert this?
> 
> Only non-empty channel numbers, which could be strings like 12-1, are
> acceptable. But we're not very sure about the exact patterns of them and
> afraid they might vary over different regions. So I just add a loose check.

I meant, why an assertion and not error checking?

> > @@ +375,2 @@
> > >  {
> > >    *aCount = mAudioLanguageCount;
> > 
> > What if mAudioLanguageCount is zero?
> 
> Thought it's rare, there might be no audio language for a program, like
> silent drama. So be subtitle languages since a program might have no
> subtitle. When it's 0, the current implementation would set |mCount| to 0
> without going through any loop, and free previous language data if there's
> any. So far it looks good to me.

Can you avoid allocating an array in that case?  (My memory of the required XPCOM semantics there is fuzzy, please double check with froydnj if needed.)

> > ::: dom/tv/nsITVService.idl
> > @@ +235,5 @@
> > >  %{C++
> > >  #define TV_SERVICE_CONTRACTID \
> > >    "@mozilla.org/tv/tvservice;1"
> > > +#define TV_SERVICE_CID \
> > > +  { 0x60fb3c53, 0x017f, 0x4340, { 0x91, 0x1b, 0xd5, 0x5c, 0x31, 0x28, 0x88, 0xb6 } }
> > 
> > Why did you add this back again?  We cannot know the CID for the TV Service
> > because it will be provided by the partner component.
> I explained the reason in comment 76, and copy it below:
> 
> I brought |TV_SERVICE_CID| back to nsITVService.idl and removed the specific
> CID for the fake service implementation, since a preference value is used in
> |TVServiceFactory| to decide to load either the real service or the fake
> one. And it doesn't appear we could leave |TV_SERVICE_CID| undefined since
> it's necessary in nsLayoutModule.cpp for XPCOM mapping. So when implementing
> TVManager, TVSource and similar classes, we may simply use
> |do_getService(TV_SERVICE_CONTRACTID)|
> 
> But I'm not sure if it's the best way to do, so still looking for some
> better ideas. Thoughts?!

Usually the way these things work is someone re-implements a contact id with their own factory method, etc, and when we want to ask the XPCOM component manager to get us an instance of that class, it will use the overridden implementation when it wants to look up a contract ID.  The CID should not be relied on by anyone, the contract ID is what matters when we want to create one of these objects.

Comment 92 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-06 13:58:03 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #91)
> > > ::: dom/tv/nsITVService.idl
> > > @@ +235,5 @@
> > > >  %{C++
> > > >  #define TV_SERVICE_CONTRACTID \
> > > >    "@mozilla.org/tv/tvservice;1"
> > > > +#define TV_SERVICE_CID \
> > > > +  { 0x60fb3c53, 0x017f, 0x4340, { 0x91, 0x1b, 0xd5, 0x5c, 0x31, 0x28, 0x88, 0xb6 } }
> > > 
> > > Why did you add this back again?  We cannot know the CID for the TV Service
> > > because it will be provided by the partner component.
> > I explained the reason in comment 76, and copy it below:
> > 
> > I brought |TV_SERVICE_CID| back to nsITVService.idl and removed the specific
> > CID for the fake service implementation, since a preference value is used in
> > |TVServiceFactory| to decide to load either the real service or the fake
> > one. And it doesn't appear we could leave |TV_SERVICE_CID| undefined since
> > it's necessary in nsLayoutModule.cpp for XPCOM mapping. So when implementing
> > TVManager, TVSource and similar classes, we may simply use
> > |do_getService(TV_SERVICE_CONTRACTID)|
> > 
> > But I'm not sure if it's the best way to do, so still looking for some
> > better ideas. Thoughts?!
> 
> Usually the way these things work is someone re-implements a contact id with
> their own factory method, etc, and when we want to ask the XPCOM component
> manager to get us an instance of that class, it will use the overridden
> implementation when it wants to look up a contract ID.  The CID should not
> be relied on by anyone, the contract ID is what matters when we want to
> create one of these objects.

Actually we may be talking about different things.  Can you please provide a very specific overview of how loading this component and falling back to the fake service works with what you have in your mind so that we're both on the same page?

Comment 93 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-06 14:02:23 PDT

(In reply to Sean Lin [:seanlin] from comment #89)
> > @@ +193,5 @@
> > > +  bool isRescanned = aOptions.mIsRescanned.WasPassed() &&
> > > +                     aOptions.mIsRescanned.Value();
> > > +
> > > +  if (isRescanned) {
> > > +    // TODO Clear the stored channel data from IndexedDB in follow-up patches.
> > 
> > FWIW using IndexedDB from C++ is a bit painful.  Any specific reason why you
> > want to do that?
> 
> If we may store those data, then we don't need to rely the underlying
> service to fetch channel and program data everytime. Besides, there's a
> |isProtected| attribute for channels which can be turned on/off by apps.
> Unless the underlying service is expected to support this, we have to find a
> place to store the protection state.

I'm not objecting to storing things.  ;-)

> I'm thinking to create another XPCOM component, such as TVDBService, which
> could be implemented in JS. Then we could access IndexedDB through JS, and
> then use callbacks to interact with that component over XPCOM interface.
> 
> Thoughts?!

Wouldn't that be overkill?  Why not just use mozStorage?  Or alternatively if you want to do more things in JS, why not move for example the implementation of all of these data container objects to JS?

> > @@ +105,3 @@
> > >  void
> > >  TVTuner::GetSupportedSourceTypes(nsTArray<TVSourceType>& aSourceTypes,
> > >                                   ErrorResult& aRv) const
> > 
> > Why is this method fallible then?
> 
> The generated WebIDL implementation (XXXBinding.cpp) appears to access the
> members in a fallible way.

That's because you have [Throws] on them. :-)  If for example you only want the setter to throw, use [SetterThrows].  We have [GetterThrows] for the opposite case too.

Comment 94 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-06 14:08:07 PDT

(In reply to Sean Lin [:seanlin] from comment #90)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #86)
> > Comment on attachment 8498792 [details] [diff] [review]
> > New Part 4 - TV channel & TV program, v3
> > 
> > Review of attachment 8498792 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > @@ +55,5 @@
> > >  }
> > >  
> > >  TVChannel::~TVChannel()
> > >  {
> > > +  MOZ_ASSERT(mTimer);
> > 
> > Why is this needed?
> 
> If we store channel and program data (EPG) in IndexedDB and don't rely on
> the underlying service to detect program changes in a channel. Then we need
> a way to know when it's time to dispatch the "programChanged" event defined
> in WebIDL.

You lost me here.  :-)  I think part of the difficulty here is that I am not aware of the full implementation plan you have in mind, so in these cases it would be *much* better to see the whole implementation than looking at the individual bits and pieces and try to imagine how the rest will be incorporated with everything else.

As to the specific topic at hand, I don't understand how your answer relates to my question...

> > @@ +211,5 @@
> > > +nsresult
> > > +TVChannel::SetTimer(const nsRefPtr<TVProgram>& aProgram)
> > > +{
> > > +  ErrorResult error;
> > > +  uint64_t startTime = aProgram->GetStartTime(error);
> > 
> > Absolute times like this are very dangerous.  What if the system time is
> > changed between the point where the start time of the TV program is set and
> > the time we read it here?
> 
> Instead of decided by the device, the start time of a program should be
> stated in EPG and provided by the service provider. So it shouldn't be
> device dependent.

The same problem still exists.  If for example the start date of a program is Jan 1, 2015, at 00:02, and at Jan 1, 2015, 00:03 the user changes the time of their device to 00:01, then the start date of the program will "occur twice".  Do you see the issue now?

> > @@ +221,5 @@
> > > +    return error.ErrorCode();
> > > +  }
> > > +  mTimer->InitWithCallback(this,
> > > +                           startTime + duration - PR_Now(),
> > > +                           nsITimer::TYPE_ONE_SHOT);
> > 
> > Isn't this assuming that the program will run until it's complete?  That
> > seems wrong.
> 
> The logic is to follow what we have in EPG. For a given channel, the current
> program will probably run until its scheduled end time, unless we receive a
> newly broadcasted EIT event to override the program info and reset the timer.

There's a ton of cases that this model won't handle though.  What if I turn my TV off in the middle of that program and turn it back on immediately?  What if I mock with the system time as stated before?  What if the gecko process crashes and is resurrected again, without a timer?  This is way too fragile, it seems.

Comment 95 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-06 20:47:07 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #91)
> (In reply to Sean Lin [:seanlin] from comment #88)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #84)
> > > Comment on attachment 8499320 [details] [diff] [review]
> > > New Part 2 - TV service, diff v4 v5
> > > @@ +152,4 @@
> > > >  {
> > > >    MOZ_ASSERT(mSource);
> > > >    MOZ_ASSERT(mPromise);
> > > > +  MOZ_ASSERT(!mChannelNumber.IsEmpty());
> > > 
> > > Why is it OK to just assert this?
> > 
> > Only non-empty channel numbers, which could be strings like 12-1, are
> > acceptable. But we're not very sure about the exact patterns of them and
> > afraid they might vary over different regions. So I just add a loose check.
> 
> I meant, why an assertion and not error checking?

I assume the callback constructors are all invoked by our code in TVSource. And there will be some sanity checks for the input of SetCurrentChannel. Besides, the partner code shouldn't directly invoke the constructors, so I use the assertions.

> > > @@ +375,2 @@
> > > >  {
> > > >    *aCount = mAudioLanguageCount;
> > > 
> > > What if mAudioLanguageCount is zero?
> > 
> > Thought it's rare, there might be no audio language for a program, like
> > silent drama. So be subtitle languages since a program might have no
> > subtitle. When it's 0, the current implementation would set |mCount| to 0
> > without going through any loop, and free previous language data if there's
> > any. So far it looks good to me.
> 
> Can you avoid allocating an array in that case?  (My memory of the required
> XPCOM semantics there is fuzzy, please double check with froydnj if needed.)

Sure. Will avoid it.

Comment 96 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-06 21:06:17 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #92)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #91)
> > > > ::: dom/tv/nsITVService.idl
> > > > @@ +235,5 @@
> > > > >  %{C++
> > > > >  #define TV_SERVICE_CONTRACTID \
> > > > >    "@mozilla.org/tv/tvservice;1"
> > > > > +#define TV_SERVICE_CID \
> > > > > +  { 0x60fb3c53, 0x017f, 0x4340, { 0x91, 0x1b, 0xd5, 0x5c, 0x31, 0x28, 0x88, 0xb6 } }
> > > > 
> > > > Why did you add this back again?  We cannot know the CID for the TV Service
> > > > because it will be provided by the partner component.
> > > I explained the reason in comment 76, and copy it below:
> > > 
> > > I brought |TV_SERVICE_CID| back to nsITVService.idl and removed the specific
> > > CID for the fake service implementation, since a preference value is used in
> > > |TVServiceFactory| to decide to load either the real service or the fake
> > > one. And it doesn't appear we could leave |TV_SERVICE_CID| undefined since
> > > it's necessary in nsLayoutModule.cpp for XPCOM mapping. So when implementing
> > > TVManager, TVSource and similar classes, we may simply use
> > > |do_getService(TV_SERVICE_CONTRACTID)|
> > > 
> > > But I'm not sure if it's the best way to do, so still looking for some
> > > better ideas. Thoughts?!
> > 
> > Usually the way these things work is someone re-implements a contact id with
> > their own factory method, etc, and when we want to ask the XPCOM component
> > manager to get us an instance of that class, it will use the overridden
> > implementation when it wants to look up a contract ID.  The CID should not
> > be relied on by anyone, the contract ID is what matters when we want to
> > create one of these objects.
> 
> Actually we may be talking about different things.  Can you please provide a
> very specific overview of how loading this component and falling back to the
> fake service works with what you have in your mind so that we're both on the
> same page?

nsLayoutModule.cpp uses contact ID, CID, and TVServiceFactory as the singleton constructor. And in TVServiceFactory we instantiate the fake or real service based on preference |dom.tv.fake.service.enabled|. So the CID here looks slightly more TVServiceFactory bound rather than specific to either the fake or real service. And if they share the same contract ID and CID, TVManager and TVSource don't need to be aware of the preference or what kind of TV service they get by simply using TV_SERVICE_CONTRACT_ID. Otherwise, when TVManager and TVSource loads TV service, they may need to do something similar to the logic of TVServiceFactory, and decide to use TV_SERVICE_CONTRACT_ID or FAKE_TV_SERVICE_CONTRACT_ID on their own.

Thoughts?!

Comment 97 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-06 22:38:25 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #93)
> (In reply to Sean Lin [:seanlin] from comment #89)
> > I'm thinking to create another XPCOM component, such as TVDBService, which
> > could be implemented in JS. Then we could access IndexedDB through JS, and
> > then use callbacks to interact with that component over XPCOM interface.
> > 
> > Thoughts?!
> 
> Wouldn't that be overkill?  Why not just use mozStorage?  Or alternatively
> if you want to do more things in JS, why not move for example the
> implementation of all of these data container objects to JS?

Ah! I wasn't aware of mozStorage. It looks a better fit for this. :)

Comment 98 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-06 23:31:42 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #94)
> (In reply to Sean Lin [:seanlin] from comment #90)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #86)
> > > Comment on attachment 8498792 [details] [diff] [review]
> > > New Part 4 - TV channel & TV program, v3
> > > 
> > > Review of attachment 8498792 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > @@ +55,5 @@
> > > >  }
> > > >  
> > > >  TVChannel::~TVChannel()
> > > >  {
> > > > +  MOZ_ASSERT(mTimer);
> > > 
> > > Why is this needed?
> > 
> > If we store channel and program data (EPG) in IndexedDB and don't rely on
> > the underlying service to detect program changes in a channel. Then we need
> > a way to know when it's time to dispatch the "programChanged" event defined
> > in WebIDL.
> 
> You lost me here.  :-)  I think part of the difficulty here is that I am not
> aware of the full implementation plan you have in mind, so in these cases it
> would be *much* better to see the whole implementation than looking at the
> individual bits and pieces and try to imagine how the rest will be
> incorporated with everything else.
> 
> As to the specific topic at hand, I don't understand how your answer relates
> to my question...

Sorry I misunderstood and answered the wrong thing. Will remove the unnecessary assertion.

> > > @@ +211,5 @@
> > > > +nsresult
> > > > +TVChannel::SetTimer(const nsRefPtr<TVProgram>& aProgram)
> > > > +{
> > > > +  ErrorResult error;
> > > > +  uint64_t startTime = aProgram->GetStartTime(error);
> > > 
> > > Absolute times like this are very dangerous.  What if the system time is
> > > changed between the point where the start time of the TV program is set and
> > > the time we read it here?
> > 
> > Instead of decided by the device, the start time of a program should be
> > stated in EPG and provided by the service provider. So it shouldn't be
> > device dependent.
> 
> The same problem still exists.  If for example the start date of a program
> is Jan 1, 2015, at 00:02, and at Jan 1, 2015, 00:03 the user changes the
> time of their device to 00:01, then the start date of the program will
> "occur twice".  Do you see the issue now?

Now I get it. Thanks.

> > > @@ +221,5 @@
> > > > +    return error.ErrorCode();
> > > > +  }
> > > > +  mTimer->InitWithCallback(this,
> > > > +                           startTime + duration - PR_Now(),
> > > > +                           nsITimer::TYPE_ONE_SHOT);
> > > 
> > > Isn't this assuming that the program will run until it's complete?  That
> > > seems wrong.
> > 
> > The logic is to follow what we have in EPG. For a given channel, the current
> > program will probably run until its scheduled end time, unless we receive a
> > newly broadcasted EIT event to override the program info and reset the timer.
> 
> There's a ton of cases that this model won't handle though.  What if I turn
> my TV off in the middle of that program and turn it back on immediately? 
> What if I mock with the system time as stated before?  What if the gecko
> process crashes and is resurrected again, without a timer?  This is way too
> fragile, it seems.

I agree. It seems using a timer might not be a reliable enough way. But since the video stream doesn't seem to have enough program data, it appears we may only rely on the stored EPG, or the underlying service to support program change notification. The mechanism with timers tries to utilize the stored EPG, because we're not quite sure how well it could be supported across different TV partners.

On the other hand, I'm getting in doubt about whether the program change event in WebIDL is really necessary, especially when we might not have an accurate way to detect it. Apps still seem to have some workarounds without this event.

Comment 99 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-07 02:42:34 PDT

Created attachment 8500929 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v8

Removing [GetterThrow] from some infallible members of TV Web interfaces.
Merging with the latest code base.

Comment 100 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-07 02:46:43 PDT

Created attachment 8500932 [details] [diff] [review]
New Part 2 - TV service, diff v5 v6

Updating based on the review comments.
Besides, using TV_SERVICE_FACTORY_CID instead of TV_SERVICE_CID to try to prevent some potential confusion.

Comment 101 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-07 13:42:27 PDT

(In reply to Sean Lin [:seanlin] from comment #95)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #91)
> > (In reply to Sean Lin [:seanlin] from comment #88)
> > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > > comment #84)
> > > > Comment on attachment 8499320 [details] [diff] [review]
> > > > New Part 2 - TV service, diff v4 v5
> > > > @@ +152,4 @@
> > > > >  {
> > > > >    MOZ_ASSERT(mSource);
> > > > >    MOZ_ASSERT(mPromise);
> > > > > +  MOZ_ASSERT(!mChannelNumber.IsEmpty());
> > > > 
> > > > Why is it OK to just assert this?
> > > 
> > > Only non-empty channel numbers, which could be strings like 12-1, are
> > > acceptable. But we're not very sure about the exact patterns of them and
> > > afraid they might vary over different regions. So I just add a loose check.
> > 
> > I meant, why an assertion and not error checking?
> 
> I assume the callback constructors are all invoked by our code in TVSource.
> And there will be some sanity checks for the input of SetCurrentChannel.
> Besides, the partner code shouldn't directly invoke the constructors, so I
> use the assertions.

OK.

(In reply to Sean Lin [:seanlin] from comment #96)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #92)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #91)
> > > > > ::: dom/tv/nsITVService.idl
> > > > > @@ +235,5 @@
> > > > > >  %{C++
> > > > > >  #define TV_SERVICE_CONTRACTID \
> > > > > >    "@mozilla.org/tv/tvservice;1"
> > > > > > +#define TV_SERVICE_CID \
> > > > > > +  { 0x60fb3c53, 0x017f, 0x4340, { 0x91, 0x1b, 0xd5, 0x5c, 0x31, 0x28, 0x88, 0xb6 } }
> > > > > 
> > > > > Why did you add this back again?  We cannot know the CID for the TV Service
> > > > > because it will be provided by the partner component.
> > > > I explained the reason in comment 76, and copy it below:
> > > > 
> > > > I brought |TV_SERVICE_CID| back to nsITVService.idl and removed the specific
> > > > CID for the fake service implementation, since a preference value is used in
> > > > |TVServiceFactory| to decide to load either the real service or the fake
> > > > one. And it doesn't appear we could leave |TV_SERVICE_CID| undefined since
> > > > it's necessary in nsLayoutModule.cpp for XPCOM mapping. So when implementing
> > > > TVManager, TVSource and similar classes, we may simply use
> > > > |do_getService(TV_SERVICE_CONTRACTID)|
> > > > 
> > > > But I'm not sure if it's the best way to do, so still looking for some
> > > > better ideas. Thoughts?!
> > > 
> > > Usually the way these things work is someone re-implements a contact id with
> > > their own factory method, etc, and when we want to ask the XPCOM component
> > > manager to get us an instance of that class, it will use the overridden
> > > implementation when it wants to look up a contract ID.  The CID should not
> > > be relied on by anyone, the contract ID is what matters when we want to
> > > create one of these objects.
> > 
> > Actually we may be talking about different things.  Can you please provide a
> > very specific overview of how loading this component and falling back to the
> > fake service works with what you have in your mind so that we're both on the
> > same page?
> 
> nsLayoutModule.cpp uses contact ID, CID, and TVServiceFactory as the
> singleton constructor. And in TVServiceFactory we instantiate the fake or
> real service based on preference |dom.tv.fake.service.enabled|. So the CID
> here looks slightly more TVServiceFactory bound rather than specific to
> either the fake or real service. And if they share the same contract ID and
> CID, TVManager and TVSource don't need to be aware of the preference or what
> kind of TV service they get by simply using TV_SERVICE_CONTRACT_ID.
> Otherwise, when TVManager and TVSource loads TV service, they may need to do
> something similar to the logic of TVServiceFactory, and decide to use
> TV_SERVICE_CONTRACT_ID or FAKE_TV_SERVICE_CONTRACT_ID on their own.

I still don't understand how you think this setup is going to work.  The code that constructs the real service does not live in our tree.  So how can TVServiceFactory possibly create a real service object?!  The answer is that it cannot, and therefore, it makes no sense to define a CID for an object that we don't know how to create.  Also, I'm not sure what the pref that you're talking about is supposed to do.

Here is how I think this should work.  At the time that we want to create the service (and notice that everywhere I say "service" here, it will eventually be "component" as per previous comments), we should:

1. Attempt to create an instance of "@mozilla.org/tvservice;1" object (the contract IDs are made up, the exact names don't matter.)  No code in our tree should attempt to create an object with this contract ID.  This object will be provided by the partner.  If this succeeds, go to (3).
2. If (1) fails, attempt to create an instance of "@mozilla.org/faketvservice;1" object.  This contract ID designates our fake service.  If this step fails, abort the operations.
3. Call QueryInterface for nsITVService on the object obtained through (1) or (2) above, and use the result if successful.

IOW, we should always try to create the real object, and if that fails, always fall back to the fake object.  This way, for example, the partner can plug in their real service implementation, and run our test suite which is oblivious to the underlying implementation, and see if the tests pass etc.

I you disagree with this plan, please explain why and propose an alternative.

(In reply to Sean Lin [:seanlin] from comment #98)
> > You lost me here.  :-)  I think part of the difficulty here is that I am not
> > aware of the full implementation plan you have in mind, so in these cases it
> > would be *much* better to see the whole implementation than looking at the
> > individual bits and pieces and try to imagine how the rest will be
> > incorporated with everything else.
> > 
> > As to the specific topic at hand, I don't understand how your answer relates
> > to my question...
> 
> Sorry I misunderstood and answered the wrong thing. Will remove the
> unnecessary assertion.

OK, np.  :-)

> > > > @@ +221,5 @@
> > > > > +    return error.ErrorCode();
> > > > > +  }
> > > > > +  mTimer->InitWithCallback(this,
> > > > > +                           startTime + duration - PR_Now(),
> > > > > +                           nsITimer::TYPE_ONE_SHOT);
> > > > 
> > > > Isn't this assuming that the program will run until it's complete?  That
> > > > seems wrong.
> > > 
> > > The logic is to follow what we have in EPG. For a given channel, the current
> > > program will probably run until its scheduled end time, unless we receive a
> > > newly broadcasted EIT event to override the program info and reset the timer.
> > 
> > There's a ton of cases that this model won't handle though.  What if I turn
> > my TV off in the middle of that program and turn it back on immediately? 
> > What if I mock with the system time as stated before?  What if the gecko
> > process crashes and is resurrected again, without a timer?  This is way too
> > fragile, it seems.
> 
> I agree. It seems using a timer might not be a reliable enough way. But
> since the video stream doesn't seem to have enough program data, it appears
> we may only rely on the stored EPG, or the underlying service to support
> program change notification. The mechanism with timers tries to utilize the
> stored EPG, because we're not quite sure how well it could be supported
> across different TV partners.
> 
> On the other hand, I'm getting in doubt about whether the program change
> event in WebIDL is really necessary, especially when we might not have an
> accurate way to detect it. Apps still seem to have some workarounds without
> this event.

OK, I'm still not sure what you're planning to do instead though...

Comment 102 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-07 13:50:33 PDT

Comment on attachment 8500932 [details] [diff] [review]
New Part 2 - TV service, diff v5 v6

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

r- mostly for the service creation bits in comment 101, but otherwise looks mostly good.

::: dom/tv/FakeTVService.cpp
@@ +40,5 @@
>  
>    // TODO Implement in follow-up patches.
>  
> +  if (!aCallback) {
> +    return NS_OK;

Why is it OK to pass in a null callback?

::: dom/tv/TVServiceCallbacks.cpp
@@ +40,5 @@
>  
>  /* virtual */ NS_IMETHODIMP
>  TVServiceSourceSetterCallback::NotifySuccess(nsIArray* aDataList)
>  {
> +  // |aDataList| is expected null for setter callbacks.

Nit: is expected to be

::: dom/tv/TVServiceFactory.h
@@ +9,5 @@
>  
>  #include "nsCOMPtr.h"
>  
> +#define TV_SERVICE_FACTORY_CID \
> +  { 0x60fb3c53, 0x017f, 0x4340, { 0x91, 0x1b, 0xd5, 0x5c, 0x31, 0x28, 0x88, 0xb6 } }

See the previous comment. :)

::: dom/tv/TVServiceRunnables.h
@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class TVServiceNotifyRunnable : public nsRunnable

Nit: Make this final, please.

Also, please add some documentation to help the partner understand how to use this object and why it's useful.

::: dom/tv/TVTypes.cpp
@@ +49,5 @@
>    *aCount = mCount;
>  
> +  char** sourceTypes = (mCount > 0) ?
> +                       static_cast<char **>(NS_Alloc(mCount * sizeof(char*))) :
> +                       nullptr;

Please ask froydnj to review these bits..

Comment 103 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-07 20:03:17 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #101)
> (In reply to Sean Lin [:seanlin] from comment #96)
> > nsLayoutModule.cpp uses contact ID, CID, and TVServiceFactory as the
> > singleton constructor. And in TVServiceFactory we instantiate the fake or
> > real service based on preference |dom.tv.fake.service.enabled|. So the CID
> > here looks slightly more TVServiceFactory bound rather than specific to
> > either the fake or real service. And if they share the same contract ID and
> > CID, TVManager and TVSource don't need to be aware of the preference or what
> > kind of TV service they get by simply using TV_SERVICE_CONTRACT_ID.
> > Otherwise, when TVManager and TVSource loads TV service, they may need to do
> > something similar to the logic of TVServiceFactory, and decide to use
> > TV_SERVICE_CONTRACT_ID or FAKE_TV_SERVICE_CONTRACT_ID on their own.
> 
> I still don't understand how you think this setup is going to work.  The
> code that constructs the real service does not live in our tree.  So how can
> TVServiceFactory possibly create a real service object?!  The answer is that
> it cannot, and therefore, it makes no sense to define a CID for an object
> that we don't know how to create.  Also, I'm not sure what the pref that
> you're talking about is supposed to do.
> 
> Here is how I think this should work.  At the time that we want to create
> the service (and notice that everywhere I say "service" here, it will
> eventually be "component" as per previous comments), we should:
> 
> 1. Attempt to create an instance of "@mozilla.org/tvservice;1" object (the
> contract IDs are made up, the exact names don't matter.)  No code in our
> tree should attempt to create an object with this contract ID.  This object
> will be provided by the partner.  If this succeeds, go to (3).
> 2. If (1) fails, attempt to create an instance of
> "@mozilla.org/faketvservice;1" object.  This contract ID designates our fake
> service.  If this step fails, abort the operations.
> 3. Call QueryInterface for nsITVService on the object obtained through (1)
> or (2) above, and use the result if successful.
> 
> IOW, we should always try to create the real object, and if that fails,
> always fall back to the fake object.  This way, for example, the partner can
> plug in their real service implementation, and run our test suite which is
> oblivious to the underlying implementation, and see if the tests pass etc.
> 
> I you disagree with this plan, please explain why and propose an alternative.

Ok. The mechanism looks good to me. Will do it. :)

> > > > > @@ +221,5 @@
> > > > > > +    return error.ErrorCode();
> > > > > > +  }
> > > > > > +  mTimer->InitWithCallback(this,
> > > > > > +                           startTime + duration - PR_Now(),
> > > > > > +                           nsITimer::TYPE_ONE_SHOT);
> > > > > 
> > > > > Isn't this assuming that the program will run until it's complete?  That
> > > > > seems wrong.
> > > > 
> > > > The logic is to follow what we have in EPG. For a given channel, the current
> > > > program will probably run until its scheduled end time, unless we receive a
> > > > newly broadcasted EIT event to override the program info and reset the timer.
> > > 
> > > There's a ton of cases that this model won't handle though.  What if I turn
> > > my TV off in the middle of that program and turn it back on immediately? 
> > > What if I mock with the system time as stated before?  What if the gecko
> > > process crashes and is resurrected again, without a timer?  This is way too
> > > fragile, it seems.
> > 
> > I agree. It seems using a timer might not be a reliable enough way. But
> > since the video stream doesn't seem to have enough program data, it appears
> > we may only rely on the stored EPG, or the underlying service to support
> > program change notification. The mechanism with timers tries to utilize the
> > stored EPG, because we're not quite sure how well it could be supported
> > across different TV partners.
> > 
> > On the other hand, I'm getting in doubt about whether the program change
> > event in WebIDL is really necessary, especially when we might not have an
> > accurate way to detect it. Apps still seem to have some workarounds without
> > this event.
> 
> OK, I'm still not sure what you're planning to do instead though...

I'm thinking if we need to remove the program change event from the spec for now. Otherwise, we may need to reset the timer of current program for the selected channel once it's initialized even after power on, the resurrection due to process crash, or system time resetting. Or the TV service has to add an new notification method for program change.

Comment 104 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-07 22:43:20 PDT

> Comment on attachment 8500932 [details] [diff] [review]
> New Part 2 - TV service, diff v5 v6
> 
> Review of attachment 8500932 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/tv/TVTypes.cpp
> @@ +49,5 @@
> >    *aCount = mCount;
> >  
> > +  char** sourceTypes = (mCount > 0) ?
> > +                       static_cast<char **>(NS_Alloc(mCount * sizeof(char*))) :
> > +                       nullptr;
> 
> Please ask froydnj to review these bits..

Hi Nathan,

Could you help us double check this to see if the implementation above (dom/tv/TVTypes.cpp) is in good shape?

Comment 105 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-08 03:49:11 PDT

Created attachment 8501657 [details] [diff] [review]
New Part 2 - TV service, diff v6 v7

Updating based on latest review comments.

Comment 106 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-08 03:58:08 PDT

(In reply to Sean Lin [:seanlin] from comment #89)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #85)
> > Comment on attachment 8498791 [details] [diff] [review]
> > New Part 3 - TV manager, TV tuner, TV source, v3
> > 
> > Review of attachment 8498791 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: dom/tv/TVServiceCallbacks.cpp
> > ::: dom/tv/TVSource.cpp
> > @@ +19,5 @@
> > >  namespace mozilla {
> > >  namespace dom {
> > >  
> > > +NS_IMPL_CYCLE_COLLECTION_INHERITED(TVSource, DOMEventTargetHelper,
> > > +                                   mTVService, mTuner, mCurrentChannel)
> > 
> > Upon further thought, it seems like making an XPCOM service participate in
> > cycle collection is a mistake, since XPCOM services basically live for the
> > lifetime of the process, so if they ever hold a reference to for example a
> > DOM object, we'd be risking holding alive that entire document until
> > shutdown, which is terrible.
> > 
> > I think we need to just use a normal XPCOM object here, perhaps by renaming
> > nsITVService to nsITVBackend and ensuring that we create that object using
> > do_CreateInstance as opposed to do_GetService, etc.  Then, we need to make
> > sure that the TV backend object does participate in cycle collection.  In
> > order to do that, we should QI the object to nsCycleCollectionISupports and
> > report an error if that QI fails.  (Since that would be a large change which
> > would be mostly renaming, it'd be great if you could do that in a separate
> > patch.)
> 
> Will do it in a separate patch once the current implementation becomes
> stable.

My second thought: Would it be OK if we just don't hook TV service as a instance member of the class? We simply call do_GetService in each method which needs it and use a local variable for it. Therefore TV service won't get involved with CC. Thoughts?!

Comment 107 Nathan Froyd [:froydnj] 2014-10-08 07:14:42 PDT

(In reply to Sean Lin [:seanlin] from comment #104)
> > Comment on attachment 8500932 [details] [diff] [review]
> > New Part 2 - TV service, diff v5 v6
> > 
> > Review of attachment 8500932 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: dom/tv/TVTypes.cpp
> > @@ +49,5 @@
> > >    *aCount = mCount;
> > >  
> > > +  char** sourceTypes = (mCount > 0) ?
> > > +                       static_cast<char **>(NS_Alloc(mCount * sizeof(char*))) :
> > > +                       nullptr;
> > 
> > Please ask froydnj to review these bits..
> 
> Hi Nathan,
> 
> Could you help us double check this to see if the implementation above
> (dom/tv/TVTypes.cpp) is in good shape?

Sure, looks fine to me, though I am unclear why the changes are necessary for the work in this bug.

Comment 108 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-08 12:15:59 PDT

(In reply to Sean Lin [:seanlin] from comment #103)
> > > > > > @@ +221,5 @@
> > > > > > > +    return error.ErrorCode();
> > > > > > > +  }
> > > > > > > +  mTimer->InitWithCallback(this,
> > > > > > > +                           startTime + duration - PR_Now(),
> > > > > > > +                           nsITimer::TYPE_ONE_SHOT);
> > > > > > 
> > > > > > Isn't this assuming that the program will run until it's complete?  That
> > > > > > seems wrong.
> > > > > 
> > > > > The logic is to follow what we have in EPG. For a given channel, the current
> > > > > program will probably run until its scheduled end time, unless we receive a
> > > > > newly broadcasted EIT event to override the program info and reset the timer.
> > > > 
> > > > There's a ton of cases that this model won't handle though.  What if I turn
> > > > my TV off in the middle of that program and turn it back on immediately? 
> > > > What if I mock with the system time as stated before?  What if the gecko
> > > > process crashes and is resurrected again, without a timer?  This is way too
> > > > fragile, it seems.
> > > 
> > > I agree. It seems using a timer might not be a reliable enough way. But
> > > since the video stream doesn't seem to have enough program data, it appears
> > > we may only rely on the stored EPG, or the underlying service to support
> > > program change notification. The mechanism with timers tries to utilize the
> > > stored EPG, because we're not quite sure how well it could be supported
> > > across different TV partners.
> > > 
> > > On the other hand, I'm getting in doubt about whether the program change
> > > event in WebIDL is really necessary, especially when we might not have an
> > > accurate way to detect it. Apps still seem to have some workarounds without
> > > this event.
> > 
> > OK, I'm still not sure what you're planning to do instead though...
> 
> I'm thinking if we need to remove the program change event from the spec for
> now. Otherwise, we may need to reset the timer of current program for the
> selected channel once it's initialized even after power on, the resurrection
> due to process crash, or system time resetting. Or the TV service has to add
> an new notification method for program change.

OK removing this for now sounds good.  Please do that in a separate patch.

(In reply to Sean Lin [:seanlin] from comment #106)
> (In reply to Sean Lin [:seanlin] from comment #89)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #85)
> > > Comment on attachment 8498791 [details] [diff] [review]
> > > New Part 3 - TV manager, TV tuner, TV source, v3
> > > 
> > > Review of attachment 8498791 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > ::: dom/tv/TVServiceCallbacks.cpp
> > > ::: dom/tv/TVSource.cpp
> > > @@ +19,5 @@
> > > >  namespace mozilla {
> > > >  namespace dom {
> > > >  
> > > > +NS_IMPL_CYCLE_COLLECTION_INHERITED(TVSource, DOMEventTargetHelper,
> > > > +                                   mTVService, mTuner, mCurrentChannel)
> > > 
> > > Upon further thought, it seems like making an XPCOM service participate in
> > > cycle collection is a mistake, since XPCOM services basically live for the
> > > lifetime of the process, so if they ever hold a reference to for example a
> > > DOM object, we'd be risking holding alive that entire document until
> > > shutdown, which is terrible.
> > > 
> > > I think we need to just use a normal XPCOM object here, perhaps by renaming
> > > nsITVService to nsITVBackend and ensuring that we create that object using
> > > do_CreateInstance as opposed to do_GetService, etc.  Then, we need to make
> > > sure that the TV backend object does participate in cycle collection.  In
> > > order to do that, we should QI the object to nsCycleCollectionISupports and
> > > report an error if that QI fails.  (Since that would be a large change which
> > > would be mostly renaming, it'd be great if you could do that in a separate
> > > patch.)
> > 
> > Will do it in a separate patch once the current implementation becomes
> > stable.
> 
> My second thought: Would it be OK if we just don't hook TV service as a
> instance member of the class? We simply call do_GetService in each method
> which needs it and use a local variable for it. Therefore TV service won't
> get involved with CC. Thoughts?!

No.  If an XPCOM component is instantiated as a service, the XPCOM component manager holds it alive and the next time you call do_GetService it just AddRefs that previous instance and returns it.  (In fact IIRC this is the only actual distinction between XPCOM components and XPCOM services.)  So it is crucial for you to create these objects using do_CreateInstance.  And once you do that, calling the interface a "Service" would be misleading since we reserve that as a convention for things you're supposed to get through do_GetService.

Comment 109 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-08 12:18:10 PDT

(In reply to Nathan Froyd (:froydnj) from comment #107)
> (In reply to Sean Lin [:seanlin] from comment #104)
> > > Comment on attachment 8500932 [details] [diff] [review]
> > > New Part 2 - TV service, diff v5 v6
> > > 
> > > Review of attachment 8500932 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > ::: dom/tv/TVTypes.cpp
> > > @@ +49,5 @@
> > > >    *aCount = mCount;
> > > >  
> > > > +  char** sourceTypes = (mCount > 0) ?
> > > > +                       static_cast<char **>(NS_Alloc(mCount * sizeof(char*))) :
> > > > +                       nullptr;
> > > 
> > > Please ask froydnj to review these bits..
> > 
> > Hi Nathan,
> > 
> > Could you help us double check this to see if the implementation above
> > (dom/tv/TVTypes.cpp) is in good shape?
> 
> Sure, looks fine to me, though I am unclear why the changes are necessary
> for the work in this bug.

The specific question is, given an XPCOM method which uses [array] and [size_is] to return an array, what is the right thing to do in order to return an array with no elements.  What Sean is doing here is setting the count to 0 and the array pointer to NULL.  Is that kosher?

Comment 110 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-08 12:21:40 PDT

Comment on attachment 8501657 [details] [diff] [review]
New Part 2 - TV service, diff v6 v7

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

This patch is lacking the part with the changes to how you create the service (comment 101) but I assume you're going to add those bits in a separate patch.  r=me if that assumption is correct.

Comment 111 Nathan Froyd [:froydnj] 2014-10-08 12:36:10 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #109)
> The specific question is, given an XPCOM method which uses [array] and
> [size_is] to return an array, what is the right thing to do in order to
> return an array with no elements.  What Sean is doing here is setting the
> count to 0 and the array pointer to NULL.  Is that kosher?

Yes.  The previous implementation will work just fine for a count of zero, too, with slightly higher memory usage.

Comment 112 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-09 02:59:46 PDT

Created attachment 8502385 [details] [diff] [review]
New Part 2 - TV service, v7

The latest version of part 2 based on r+ comments.

Comment 113 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-09 03:02:06 PDT

Created attachment 8502388 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, diff v3 v4

Updating based on previous comments.

Comment 114 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-09 03:04:54 PDT

Created attachment 8502389 [details] [diff] [review]
New Part 4 - TV channel & TV program, diff v3 v4

Updating based on previous comments.

Comment 115 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-10 14:55:15 PDT

Comment on attachment 8502388 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, diff v3 v4

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

::: dom/base/Navigator.cpp
@@ +1594,5 @@
>        aRv.Throw(NS_ERROR_UNEXPECTED);
>        return nullptr;
>      }
> +    mTVManager = new TVManager(mWindow, aRv);
> +    if (aRv.Failed()) {

FWIW if you're going to ignore the error code there is no point in having it.  Just make Create() return nullptr when it fails.

::: dom/tv/TVManager.cpp
@@ +46,4 @@
>    nsresult rv = mTVService->GetTuners(callback);
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(rv);
> +  }

FWIW everything in the body of this ctor belongs in Init().

@@ +58,5 @@
>  {
>    return TVManagerBinding::Wrap(aCx, this);
>  }
>  
> +nsresult

This should stay void.

@@ +63,5 @@
>  TVManager::SetTuners(const nsTArray<nsRefPtr<TVTuner>>& aTuners)
>  {
> +  // Should be called only when TV Manager hasn't been ready yet.
> +  if (mIsReady) {
> +    return NS_ERROR_DOM_INVALID_STATE_ERR;

You need to mark this method as [Throws] in the IDL, add an ErrorResult through that and .Throw() it.  FWIW if you had tested this you'd see that the binding layer would ignore the return value and not throw a JS exception in this case.

@@ -65,5 @@
>  
>  void
>  TVManager::RejectPendingGetTunersPromises(nsresult aRv)
>  {
> -  MOZ_ASSERT(!mIsReady);

Why did you remove this?

@@ +92,5 @@
> +
> +nsresult
> +TVManager::DispatchTVEvent(nsIDOMEvent* aEvent)
> +{
> +  return DispatchTrustedEvent(aEvent);

Is this helper function really helping? :)

@@ +155,5 @@
>      TVParentalControlChangedEvent::Constructor(this,
>                                                 NS_LITERAL_STRING("parentalcontrolchanged"),
>                                                 init);
> +  nsCOMPtr<nsIRunnable> runnable =
> +    new TVEventDispatchRunnable<TVManager>(this, event);

Couldn't you use NS_NewRunnableMethodWithArg instead of rolling your own?

::: dom/tv/TVManager.h
@@ +26,5 @@
>    NS_DECL_ISUPPORTS_INHERITED
>    NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(TVManager, DOMEventTargetHelper)
>  
> +  TVManager(nsPIDOMWindow* aWindow,
> +            ErrorResult& aRv);

As I've said before in this bug, please avoid doing fallible things in constructors.  Every time where you face something like this what you want to do is an Init() function.  A common idiom is having a public static function like:

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

and a private ctor and Init function.  The Create() function creates an object using the ctor, calls Init() on it, and if it succeeds it returns it.

::: dom/tv/TVServiceCallbacks.cpp
@@ +255,5 @@
>        continue;
>      }
>  
> +    nsRefPtr<TVTuner> tuner = new TVTuner(mManager->GetOwner());
> +    nsresult rv = tuner->Init(tunerData);

Please use the TVTuner::Create() idiom.

::: dom/tv/TVServiceRunnables.h
@@ +51,5 @@
>    uint16_t mErrorCode;
>  };
>  
> +template<class T>
> +class TVEventDispatchRunnable MOZ_FINAL : public nsRunnable

Hopefully this won't be needed.

::: dom/tv/TVSource.cpp
@@ +55,5 @@
>  }
>  
>  TVSource::~TVSource()
>  {
>  }

You need to call Shutdown() from the dtor too.

@@ +70,5 @@
> +  mTVService = do_CreateInstance(TV_SERVICE_CONTRACTID);
> +  if (!mTVService) {
> +    // Fallback to the fake service.
> +    nsresult rv;
> +    mTVService = do_CreateInstance(FAKE_TV_SERVICE_CONTRACTID, &rv);

We should share this code.

@@ +75,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  nsCOMPtr<nsITVSourceListener> sourceListener;
> +  mTVService->GetSourceListener(getter_AddRefs(sourceListener));

Did you investigate why you didn't catch the leak that was here before?

@@ +79,5 @@
> +  mTVService->GetSourceListener(getter_AddRefs(sourceListener));
> +  if (!sourceListener) {
> +    return NS_ERROR_DOM_INVALID_STATE_ERR;
> +  }
> +  ((TVSourceListener*) (sourceListener.get()))->RegisterSource(this);

static_cast, please.

@@ +96,5 @@
> +  mTVService->GetSourceListener(getter_AddRefs(sourceListener));
> +  if (!sourceListener) {
> +    return;
> +  }
> +  ((TVSourceListener*) (sourceListener.get()))->UnregisterSource(this);

static_cast.

@@ +147,5 @@
>  
> +nsresult
> +TVSource::DispatchTVEvent(nsIDOMEvent* aEvent)
> +{
> +  return DispatchTrustedEvent(aEvent);

What's the point of this helper function? :)

@@ +349,5 @@
>      TVCurrentChannelChangedEvent::Constructor(this,
>                                                NS_LITERAL_STRING("currentchannelchanged"),
>                                                init);
> +  nsCOMPtr<nsIRunnable> runnable =
> +    new TVEventDispatchRunnable<TVSource>(this, event);

Ditto here and elsewhere in this file.

::: dom/tv/TVSource.h
@@ +39,5 @@
>    // WebIDL (internal functions)
>  
>    virtual JSObject* WrapObject(JSContext *aCx) MOZ_OVERRIDE;
>  
> +  nsresult Init();

Again, please use the Create() idiom.

@@ +41,5 @@
>    virtual JSObject* WrapObject(JSContext *aCx) MOZ_OVERRIDE;
>  
> +  nsresult Init();
> +
> +  void Shutdown();

This should be private.

::: dom/tv/TVTuner.cpp
@@ +42,5 @@
>    return TVTunerBinding::Wrap(aCx, this);
>  }
>  
>  nsresult
> +TVTuner::Init(nsITVTunerData* aData)

Again you want the Create() idiom.

@@ +114,5 @@
>  
> +nsresult
> +TVTuner::DispatchTVEvent(nsIDOMEvent* aEvent)
> +{
> +  return DispatchTrustedEvent(aEvent);

Ditto.

@@ +202,5 @@
>                                               NS_LITERAL_STRING("currentsourcechanged"),
>                                               init);
> +  nsCOMPtr<nsIRunnable> runnable =
> +    new TVEventDispatchRunnable<TVTuner>(this, event);
> +  return NS_DispatchToCurrentThread(runnable);

Here too.

::: dom/tv/TVTuner.h
@@ +34,5 @@
>    // WebIDL (internal functions)
>  
>    virtual JSObject* WrapObject(JSContext *aCx) MOZ_OVERRIDE;
>  
> +  nsresult Init(nsITVTunerData* aData);

private

Comment 116 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-10 14:59:26 PDT

Comment on attachment 8502389 [details] [diff] [review]
New Part 4 - TV channel & TV program, diff v3 v4

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

::: dom/tv/TVChannel.cpp
@@ +66,5 @@
> +
> +nsresult
> +TVChannel::DispatchTVEvent(nsIDOMEvent* aEvent)
> +{
> +  return DispatchTrustedEvent(aEvent);

Ditto.

@@ +176,5 @@
>  TVChannel::GetCurrentProgram(ErrorResult& aRv)
>  {
> +  // TODO Get program data from MozStorage in follow-up patches.
> +  nsRefPtr<nsITVProgramData> programData;
> +  nsRefPtr<TVProgram> program = new TVProgram(GetOwner(), this, programData);

Can't you just pass nullptr as the 3rd arg here?

@@ +191,5 @@
>                                                 NS_LITERAL_STRING("protectionstatechanged"),
>                                                 init);
> +  nsCOMPtr<nsIRunnable> runnable =
> +    new TVEventDispatchRunnable<TVChannel>(this, event);
> +  return NS_DispatchToCurrentThread(runnable);

Ditto.

::: dom/tv/TVChannel.h
@@ +32,5 @@
>    // WebIDL (internal functions)
>  
>    virtual JSObject* WrapObject(JSContext *aCx) MOZ_OVERRIDE;
>  
> +  nsresult Init(nsITVChannelData* aData);

Again, please use the Create() idiom.

Comment 117 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-13 02:56:50 PDT

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #115)
> Comment on attachment 8502388 [details] [diff] [review]
> New Part 3 - TV manager, TV tuner, TV source, diff v3 v4
> 
> Review of attachment 8502388 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +63,5 @@
> >  TVManager::SetTuners(const nsTArray<nsRefPtr<TVTuner>>& aTuners)
> >  {
> > +  // Should be called only when TV Manager hasn't been ready yet.
> > +  if (mIsReady) {
> > +    return NS_ERROR_DOM_INVALID_STATE_ERR;
> 
> You need to mark this method as [Throws] in the IDL, add an ErrorResult
> through that and .Throw() it.  FWIW if you had tested this you'd see that
> the binding layer would ignore the return value and not throw a JS exception
> in this case.

Actually this method is not in WebIDL. And it should only be used in |TVServiceTunerGetterCallback::NotifySuccess|.

> 
> @@ -65,5 @@
> >  
> >  void
> >  TVManager::RejectPendingGetTunersPromises(nsresult aRv)
> >  {
> > -  MOZ_ASSERT(!mIsReady);
> 
> Why did you remove this?

I was thinking this check might be less useful since we could just reject the promises as long as the method got called.

> 
> @@ +92,5 @@
> > +
> > +nsresult
> > +TVManager::DispatchTVEvent(nsIDOMEvent* aEvent)
> > +{
> > +  return DispatchTrustedEvent(aEvent);
> 
> Is this helper function really helping? :)

It was added because |DispatchTrustedEvent| was protected so this helper function was to open an public entry for runnables. Will see if we can get rid of this when trying to use |NS_NewRunnableMethodWithArg|.

Comment 118 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-14 04:07:22 PDT

Created attachment 8504657 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, diff v4 v5

Updating based on latest comments.

Comment 119 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-14 04:08:16 PDT

Created attachment 8504658 [details] [diff] [review]
New Part 4 - TV channel & TV program, diff v4 v5

Updating based on latest comments.

Comment 120 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-15 03:35:00 PDT

Created attachment 8505397 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v9

Updating with the current code base.

Hi Andrea,

This patch about TV WebIDLs was initially reviewed by Ehsan and then DOM peer reviewed by you. Just wanna double check if it still requires an additional super review?

Comment 121 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-15 03:37:39 PDT

Created attachment 8505400 [details] [diff] [review]
New Part 2 - TV service, v8

Comment 122 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-15 03:38:56 PDT

Created attachment 8505401 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, diff v4 v5

Comment 123 Andrea Marchesini (:baku) 2014-10-15 03:53:34 PDT

> This patch about TV WebIDLs was initially reviewed by Ehsan and then DOM
> peer reviewed by you. Just wanna double check if it still requires an
> additional super review?

No. If you didn't change webidl files it should be fine with a normal positive review.

Comment 124 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-15 16:01:46 PDT

Comment on attachment 8505401 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, diff v4 v5

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

Instead of adding these DispatchTruestedEvent helpers, you can just use "using nsDOMEventTargetHelper::DispatchTrustedEvent;" somewhere in a public section of the derived classes in order to make the method public there.

::: dom/base/Navigator.cpp
@@ +1593,5 @@
>      if (!mWindow) {
>        aRv.Throw(NS_ERROR_UNEXPECTED);
>        return nullptr;
>      }
> +    mTVManager = TVManager::Create(mWindow, aRv);

Here again we are exposing our internal error codes through the API.  Is there any good reason to keep navigator.tv as [Throws]?  I think we can just return nullptr when we hit an error...

::: dom/tv/TVManager.cpp
@@ +48,5 @@
> +  }
> +  return manager.forget();
> +}
> +
> +nsresult

See my comment in Navigator.cpp.  If you agree, we can get rid of all of these error values and for example just return a bool here.

@@ +162,5 @@
>      TVParentalControlChangedEvent::Constructor(this,
>                                                 NS_LITERAL_STRING("parentalcontrolchanged"),
>                                                 init);
>    nsCOMPtr<nsIRunnable> runnable =
> +    NS_NewRunnableMethodWithArg<nsIDOMEvent*>(this, &TVManager::DispatchTVEvent, event);

You need to use nsCOMPtr<nsIDOMEvent> here instead.  Otherwise, the runnable object will hold a raw pointer to the DOM node, and if the DOM node gets destroyed before the runnable runs, we'll have a use after free error.

::: dom/tv/TVServiceFactory.cpp
@@ +20,5 @@
> +  return service.forget();
> +}
> +
> +/* static */ already_AddRefed<nsITVService>
> +TVServiceFactory::AutoCreateTVService(nsresult& aRv)

Taking an nsresult reference like this is not very idiomatic.  I don't think we gain anything by returning this error code, so you can just return nullptr when something goes wrong.

@@ +25,5 @@
> +{
> +  nsCOMPtr<nsITVService> service = do_CreateInstance(TV_SERVICE_CONTRACTID);
> +  if (!service) {
> +    // Fallback to the fake service.
> +    service = do_CreateInstance(FAKE_TV_SERVICE_CONTRACTID, &aRv);

Calling this "service" and using do_CreateInstance is sort of self-contradicting...  But since we're going to rename things in the future, I guess that's fine.

::: dom/tv/TVSource.cpp
@@ +361,5 @@
>      TVCurrentChannelChangedEvent::Constructor(this,
>                                                NS_LITERAL_STRING("currentchannelchanged"),
>                                                init);
>    nsCOMPtr<nsIRunnable> runnable =
> +    NS_NewRunnableMethodWithArg<nsIDOMEvent*>(this, &TVSource::DispatchTVEvent, event);

Same here and elsewhere.

::: dom/tv/TVTuner.cpp
@@ +77,5 @@
>      // Generate the source instance based on the supported source type.
> +    ErrorResult error;
> +    nsRefPtr<TVSource> source =
> +      TVSource::Create(GetOwner(), sourceType, this, error);
> +    NS_ENSURE_SUCCESS(error.ErrorCode(), error.ErrorCode());

This will leak memory if we return early from here.

Comment 125 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-15 16:03:00 PDT

Comment on attachment 8504658 [details] [diff] [review]
New Part 4 - TV channel & TV program, diff v4 v5

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

::: dom/tv/TVChannel.cpp
@@ +204,5 @@
>      TVProtectionStateChangedEvent::Constructor(this,
>                                                 NS_LITERAL_STRING("protectionstatechanged"),
>                                                 init);
>    nsCOMPtr<nsIRunnable> runnable =
> +      NS_NewRunnableMethodWithArg<nsIDOMEvent*>(this, &TVChannel::DispatchTVEvent, event);

Ditto.

Comment 126 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-16 04:12:28 PDT

Created attachment 8506070 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v10

Removing [Throws] from navigator.tv.

Comment 127 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-16 04:15:48 PDT

Created attachment 8506071 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, diff v5 v6

Updating based on most of the latest comments.

However, some build errors happened when I tried both "using DOMEventTargetHelper::DispatchTrustedEvent;" and "nsCOMPtr<nsIDOMEvent>". So keeping those part unchanged.

Comment 128 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-16 04:16:50 PDT

Created attachment 8506072 [details] [diff] [review]
New Part 4 - TV channel & TV program, diff v5 v6

Comment 129 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-16 09:29:11 PDT

Created attachment 8506228 [details] [diff] [review]
New Part 4 - TV channel & TV program, diff v5 v6

Updating based on latest comments and adding getPrograms implementation.

Comment 130 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-16 12:52:46 PDT

(In reply to Sean Lin [:seanlin] from comment #127)
> Created attachment 8506071 [details] [diff] [review]
> New Part 3 - TV manager, TV tuner, TV source, diff v5 v6
> 
> Updating based on most of the latest comments.
> 
> However, some build errors happened when I tried both "using
> DOMEventTargetHelper::DispatchTrustedEvent;" and "nsCOMPtr<nsIDOMEvent>". So
> keeping those part unchanged.

What build errors?  Keeping the first part unchanged is perhaps OK, but it just makes for uglier code.  I can help you fix the build.  The second one, however can result in UAF security sensitive bugs as previously mentioned, so your patch is unacceptable because of that.

If we can't use an nsCOMPtr with NS_NewRunnableMethodWithArg, I'm afraid you'll have to go back to the helper class you had before. :/

Comment 131 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-17 01:34:18 PDT

Created attachment 8506731 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v11

Updating with the latest code base.

Comment 132 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-17 01:35:05 PDT

Created attachment 8506732 [details] [diff] [review]
New Part 2 - TV service, v9

Updating with the latest code base.

Comment 133 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-17 01:37:56 PDT

Created attachment 8506736 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, diff v6 v7

I've figured out a way to use "nsCOMPtr<nsIDOMEvent>" for runnables.

Comment 134 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-17 01:40:22 PDT

Created attachment 8506739 [details] [diff] [review]
New Part 4 - TV channel & TV program, diff v6 v7

Using "nsCOMPtr<nsIDOMEvent>" for runnables and adding implementation to get programs.

Comment 135 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-17 02:24:24 PDT

Created attachment 8506770 [details] [diff] [review]
New Part 5 - Handle data with MozStorage, v1

Use MozStorage to handle channel data.

Comment 136 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-20 13:14:37 PDT

Comment on attachment 8506739 [details] [diff] [review]
New Part 4 - TV channel & TV program, diff v6 v7

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

::: dom/tv/TVChannel.cpp
@@ +112,5 @@
> +                    aOptions.mOffsetOfPrograms.Value() :
> +                    0;
> +  uint64_t limit = aOptions.mNumberOfPrograms.WasPassed() ?
> +                   aOptions.mNumberOfPrograms.Value() :
> +                   0;

(Note, talking in person with Sean, we agreed to remove the offset and limit notions from this API altogether, because we cannot build things such as paginated UI on top of it as the list of programs may change from one call to GetPrograms to the next.)

::: dom/tv/TVServiceCallbacks.cpp
@@ +352,5 @@
> +                                                programData);
> +    programs.AppendElement(program);
> +  }
> +
> +  mPromise->MaybeResolve(programs);

Before resolving the promise here, we should check to make sure that |programs.Length() == length|.

@@ +375,5 @@
> +    return NS_OK;
> +  }
> +
> +  mPromise->MaybeReject(NS_ERROR_DOM_INVALID_ACCESS_ERR);
> +  return NS_ERROR_ILLEGAL_VALUE;

Note for the future: we need to think about the error codes returned from this API in general and think about where we can possibly merge some of these, but this can happen later.

::: dom/tv/TVServiceCallbacks.h
@@ +100,5 @@
>    NS_DECL_CYCLE_COLLECTION_CLASS(TVServiceProgramGetterCallback)
>  
> +  TVServiceProgramGetterCallback(TVChannel* aChannel,
> +                                 Promise* aPromise,
> +                                 bool aIsSingular);

Please document aIsSingular.

Comment 137 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-20 13:35:36 PDT

Comment on attachment 8506770 [details] [diff] [review]
New Part 5 - Handle data with MozStorage, v1

We agreed to not do this for now until we decide how to implement parental controls.

Comment 138 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-21 06:08:25 PDT

Created attachment 8508649 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, v7

The aggregated r+ patch for part 3.

Comment 139 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-21 13:38:31 PDT

Created attachment 8508988 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v12

Replacing |AvailableIn=CertifiedApps| with |Func="Navigator::HasTVSupport"| to allow testing against hosted apps.
Removing parental control and channel protection attributes.
Enabling TV Manager API on B2G.
Merging with the latest code base.

Comment 140 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-21 13:40:26 PDT

Created attachment 8508990 [details] [diff] [review]
New Part 2 - TV service, v10

Updating with the latest code base.

Comment 141 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-21 13:43:15 PDT

Created attachment 8508991 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, v8

Updating with the latest code base.

Comment 142 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-21 13:44:26 PDT

Created attachment 8508992 [details] [diff] [review]
New Part 4 - TV channel & TV program, v8

Updating with the latest code base.

Comment 143 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-21 13:46:36 PDT

Created attachment 8508996 [details] [diff] [review]
Aggregated patch, v1

The reference patch aggregating new part 1 to new part 4.

Comment 144 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-21 14:51:22 PDT

Created attachment 8509038 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, diff v11 v12

The diff for the latest change in part 1.

Comment 145 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-21 20:29:28 PDT

Created attachment 8509190 [details] [diff] [review]
New Part 2 - TV service, v10

Comment 146 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-21 20:30:36 PDT

Created attachment 8509191 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, v8

Comment 147 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-21 20:31:23 PDT

Created attachment 8509192 [details] [diff] [review]
New Part 4 - TV channel & TV program, v8

Comment 148 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-22 09:46:18 PDT

Created attachment 8509608 [details] [diff] [review]
New Part 2 - TV service, v11

Comment 149 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-22 09:47:32 PDT

Created attachment 8509610 [details] [diff] [review]
New Part 3 - TV manager, TV tuner, TV source, v9

Comment 150 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-22 09:48:11 PDT

Created attachment 8509611 [details] [diff] [review]
New Part 4 - TV channel & TV program, v9

Comment 151 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-22 10:39:57 PDT

Created attachment 8509661 [details] [diff] [review]
Aggregated patch, v1

Comment 152 :Ehsan Akhgari (busy, don't ask for review please) 2014-10-22 11:38:43 PDT

Comment on attachment 8509038 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, diff v11 v12

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

r=me with the pref in b2g.js removed.

::: b2g/app/b2g.js
@@ +1043,5 @@
>  // UDPSocket API
>  pref("dom.udpsocket.enabled", true);
> +
> +// Enable TV Manager API
> +pref("dom.tv.enabled", true);

I think we should hold off for everything else to finish before turning this on by default.

Comment 153 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-22 15:12:45 PDT

Created attachment 8509847 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v13

Updating based on the latest r+ comments.

Comment 154 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-23 09:14:55 PDT

Created attachment 8510395 [details] [diff] [review]
New Part 2 - TV service, v12

Comment 155 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-24 12:14:15 PDT

Created attachment 8511239 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v14

Comment 156 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-28 22:31:33 PDT

Created attachment 8513241 [details] [diff] [review]
New part 1 - DOM API & WebIDL bindings, v15

Merging with the latest code base.

Comment 157 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-28 22:32:34 PDT

Created attachment 8513242 [details] [diff] [review]
New Part 2 - TV service, v13

Merging with the latest code base.

Comment 158 Sean Lin [:seanlin] (inactive from Mar 2016) 2014-10-28 22:51:05 PDT

The latest try-run. FYI.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ab8637d6eb51

Comment 159 Carsten Book [:Tomcat] 2014-10-29 02:44:46 PDT

https://hg.mozilla.org/integration/b2g-inbound/rev/0dc06ccc5ea2
https://hg.mozilla.org/integration/b2g-inbound/rev/1593d48890a0
https://hg.mozilla.org/integration/b2g-inbound/rev/544aa604483f
https://hg.mozilla.org/integration/b2g-inbound/rev/edf60abe62a5

Comment 160 Ryan VanderMeulen [:RyanVM] 2014-10-29 13:21:47 PDT

https://hg.mozilla.org/mozilla-central/rev/0dc06ccc5ea2
https://hg.mozilla.org/mozilla-central/rev/1593d48890a0
https://hg.mozilla.org/mozilla-central/rev/544aa604483f
https://hg.mozilla.org/mozilla-central/rev/edf60abe62a5

Comment 161 Jean-Yves Perrier [:teoli] 2014-11-17 07:09:14 PST

Though we have no writing resources to dedicate to document these interfaces, the MDN content team would be more than happy to help anybody wanted to do so. Ping me if you want tho help.