888592 - Move Telephony, TelephonyCall to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part1 - remove nsIDOMNavigatorTelephony

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part2 - move Telephony to webidl

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part2 - move Telephony to webidl

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part2 - move Telephony to webidl

removing nsIDOMTelephony.idl ... ...

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Koi? since it blocks koi+ bug 772765.

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part2 - move Telephony to webidl (v2)

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part3 - move TelephonyCall to webidl & remove nsIDOMCallEvent.idl

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part1 - remove nsIDOMNavigatorTelephony (v2)

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part3 - move TelephonyCall to webidl & remove nsIDOMCallEvent.idl (v2)

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part4- test cases

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part1 - move Telephony to webidl (v3)

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part2 - move TelephonyCall to webidl & remove nsIDOMCallEvent (v3)

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part3 - test cases (v3)

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 778929 [details] [diff] [review]
part1 - move Telephony to webidl (v3)

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

::: dom/webidl/Telephony.webidl
@@ +18,5 @@
> +  attribute boolean speakerEnabled;
> +
> +  readonly attribute TelephonyCall? active;
> +
> +  sequence<TelephonyCall> getCalls ();

|calls| was a read-only attribute.  If there is no technical difficulty that prevents us from keeping it as an attribute, then let's leave it untouched.

Comment 15

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #14)
> Comment on attachment 778929 [details] [diff] [review]
> part1 - move Telephony to webidl (v3)
> 
> Review of attachment 778929 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/Telephony.webidl
> @@ +18,5 @@
> > +  attribute boolean speakerEnabled;
> > +
> > +  readonly attribute TelephonyCall? active;
> > +
> > +  sequence<TelephonyCall> getCalls ();
> 
> |calls| was a read-only attribute.  If there is no technical difficulty that
> prevents us from keeping it as an attribute, then let's leave it untouched.

Thanks for pointing this out. 

When I try to use "sequence<TelephonyCall> calls;" I always encounter a parse error saying "WebIDL.WebIDLError: error invalid syntax." I also see that when sequence is used in an "interface", not "dictionary," it's always used as a return type of a function, not a type of an attribute. 

I am not sure whether it's possible to use "sequence<TelephonyCall> calls;" and would need more investigation. I guess I would need to consult webidl experts. Do you have any idea about this, Ben? Thanks.

Comment 16

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #15)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #14)
> > Comment on attachment 778929 [details] [diff] [review]
> > part1 - move Telephony to webidl (v3)
> > 
> > Review of attachment 778929 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/webidl/Telephony.webidl
> > @@ +18,5 @@
> > > +  attribute boolean speakerEnabled;
> > > +
> > > +  readonly attribute TelephonyCall? active;
> > > +
> > > +  sequence<TelephonyCall> getCalls ();
> > 
> > |calls| was a read-only attribute.  If there is no technical difficulty that
> > prevents us from keeping it as an attribute, then let's leave it untouched.
> 
> Thanks for pointing this out. 
> 
> When I try to use "sequence<TelephonyCall> calls;" I always encounter a
> parse error saying "WebIDL.WebIDLError: error invalid syntax." I also see
> that when sequence is used in an "interface", not "dictionary," it's always
> used as a return type of a function, not a type of an attribute. 
> 
> I am not sure whether it's possible to use "sequence<TelephonyCall> calls;"
> and would need more investigation. I guess I would need to consult webidl
> experts. Do you have any idea about this, Ben? Thanks.

Aha, I got it. See [1], it clearly says "Sequences MUST NOT be used as the type of an attribute, constant or exception field." 

I also got an explicit error "WebDIL.WebIDLError: error: An attribute cannot be of a sequence type" when I use "readonly attribute Sequence<Telephony> calls."

[1] http://www.w3.org/TR/WebIDL/#idl-sequence

Comment 17

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part2 - move TelephonyCall to webidl & remove nsIDOMCallEvent (v4)

rm nsIDOMTelephonyCall.idl

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hm, I think I misled you when I said 'sequence'. Instead we want 'read only array' I think... I'll see what I can dig up.

Comment 19

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to ben turner [:bent] from comment #18)
> Hm, I think I misled you when I said 'sequence'. Instead we want 'read only
> array' I think... I'll see what I can dig up.

There's a similar discussion in W3C working group. I am still trying to understand the differences between sequence and read only array. It seems the working group is working on using 'sequence' instead of 'read only array.' See [1] [2].


[1] http://lists.w3.org/Archives/Public/public-script-coord/2013AprJun/0748.html
[2] https://github.com/sysapps/telephony/issues/194

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, and apparently we don't have support for arrays yet anyway. I'm sorry this is such a rabbit hole :(

The next best thing is to do the same thing that we do for NodeList. See the patch in bug 869376 for some help, but the basic idea is that you make a new webidl object that looks like this (from bz):

  [ArrayClass, NoInterfaceObject]
  interface CallsList {
    getter Call(unsigned long index);
    readonly attribute unsigned long length;
  };

Then you just implement the C++ class and use 'readonly attribute CallsList calls' on the telephony webidl.

Comment 21

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to ben turner [:bent] from comment #20)
> Yeah, and apparently we don't have support for arrays yet anyway. 

Yup, I learned that as well :(

> I'm sorry
> this is such a rabbit hole :(
> 
> The next best thing is to do the same thing that we do for NodeList. See the
> patch in bug 869376 for some help, but the basic idea is that you make a new
> webidl object that looks like this (from bz):
> 
>   [ArrayClass, NoInterfaceObject]
>   interface CallsList {
>     getter Call(unsigned long index);
>     readonly attribute unsigned long length;
>   };
> 
> Then you just implement the C++ class and use 'readonly attribute CallsList
> calls' on the telephony webidl.

Thanks for the suggestion. Let me try!

Comment 22

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 778929 [details] [diff] [review]
part1 - move Telephony to webidl (v3)

Should address comment 20 first.

Comment 23

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Patch (v4)

1) Move Telephony and TelephonyCall to webidl.
2) Introduce [ArrayClass] CallsList 
3) Remove nsIDOMCallEvent.idl

Comment 24

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 781511 [details] [diff] [review]
Patch (v4)

Comment 20 addressed. Please note that this patch is based on bug 892609 for ArrayClass support. 

Thank you.

Comment 25

[Hsin-Yi Tsai (she/her) [:hsinyi]]

try result: https://tbpl.mozilla.org/?tree=Try&rev=cefee2f12cfc, sadly encountering permission mochitest failure. 

dchan: does the permission test support WebIDL now? I tried to comment the line "idl: "nsIDOMTelephony"," but the same error appears. Any idea? Thank you :)

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #25)
> try result: https://tbpl.mozilla.org/?tree=Try&rev=cefee2f12cfc, sadly
> encountering permission mochitest failure. 
> 
> dchan: does the permission test support WebIDL now? I tried to comment the
> line "idl: "nsIDOMTelephony"," but the same error appears. Any idea? Thank
> you :)

Replace with |webidl: "Telephony",|, I believe.

Comment 27

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to :Ms2ger from comment #26)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #25)
> > try result: https://tbpl.mozilla.org/?tree=Try&rev=cefee2f12cfc, sadly
> > encountering permission mochitest failure. 
> > 
> > dchan: does the permission test support WebIDL now? I tried to comment the
> > line "idl: "nsIDOMTelephony"," but the same error appears. Any idea? Thank
> > you :)
> 
> Replace with |webidl: "Telephony",|, I believe.

Cool! Let me try that. Thank you :)

Comment 28

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Patch - fix mochitest

Comment 29

[Hsin-Yi Tsai (she/her) [:hsinyi]]

try looks good https://tbpl.mozilla.org/?tree=Try&rev=cd90ab1a78d0

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 781511 [details] [diff] [review]
Patch (v4)

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

::: dom/telephony/Telephony.cpp
@@ +361,5 @@
>    }
>  
>    nsresult rv = mProvider->StartTone(aDTMFChar);
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(rv);

Ideally we should stop using .idl for the provider interface and plain C++ classes. But that can be done later and isn't urgent at all.

Comment 31

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 781511 [details] [diff] [review]
Patch (v4)

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

This looks really good! Thank you so much for converting all of this.

Mostly nits below (sorry!) but I'd like to see the changes so r- for now.

::: dom/base/Navigator.h
@@ +87,5 @@
>  #endif
>  } // namespace Connection;
>  
> +namespace telephony {
> +#ifdef MOZ_B2G_RIL

Nit: Wrap the telephony namespace in the #ifdef too, otherwise it's just a waste.

::: dom/telephony/CallEvent.cpp
@@ +8,5 @@
> +
> +USING_TELEPHONY_NAMESPACE
> +using namespace mozilla::dom;
> +
> +CallEvent::CallEvent(mozilla::dom::EventTarget* aOwner,

Nit: 'mozilla::dom::' should not be needed here.

Also, any reason this can't be inlined in the header?

::: dom/telephony/CallEvent.h
@@ +7,5 @@
> +#include "nsDOMEvent.h"
> +
> +#include "mozilla/Attributes.h"
> +#include "mozilla/dom/CallEventBinding.h"
> +#include "mozilla/ErrorResult.h"

Can you just add Attributes.h and ErrorResult.h to TelephonyCommon.h instead of adding them to every header?

@@ +9,5 @@
> +#include "mozilla/Attributes.h"
> +#include "mozilla/dom/CallEventBinding.h"
> +#include "mozilla/ErrorResult.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsWrapperCache.h"

Hopefully you don't need this one.

@@ +12,5 @@
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsWrapperCache.h"
> +
> +#include "TelephonyCall.h"
> +#include "TelephonyCommon.h"

TelephonyCall.h should include this for you.

@@ +21,5 @@
> +{
> +  nsRefPtr<TelephonyCall> mCall;
> +
> +public:
> +  CallEvent(mozilla::dom::EventTarget* aOwner,

Can this be private along with the destructor? It should only be created via the static Create function I hope.

@@ +29,5 @@
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(CallEvent, nsDOMEvent)
> +  NS_FORWARD_TO_NSDOMEVENT
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE

Nit: 'virtual JSObject*' on its own line, similar for InitCallEvent and GetCall below.

@@ +44,5 @@
> +    mCall = aCall;
> +  }
> +
> +  static already_AddRefed<CallEvent>
> +  Create(EventTarget* aOwner, TelephonyCall* aCall);

Hm, where is this implemented?

@@ +74,5 @@
> +    return call.forget();
> +  }
> +
> +private:
> +  ~CallEvent()

Nit: Might as well add 'virtual' here.

::: dom/telephony/CallsList.cpp
@@ +10,5 @@
> +
> +USING_TELEPHONY_NAMESPACE
> +using namespace mozilla::dom;
> +
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(CallsList)

Nit: I'd put all this goop at the end of the file

@@ +11,5 @@
> +USING_TELEPHONY_NAMESPACE
> +using namespace mozilla::dom;
> +
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(CallsList)
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(CallsList)

Nit: newline between those two.

@@ +13,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(CallsList)
> +NS_IMPL_CYCLE_COLLECTING_ADDREF(CallsList)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(CallsList)
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(CallsList)

Nit: newline between those two also.

@@ +19,5 @@
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END
> +
> +CallsList::CallsList(Telephony* aTelephony)
> +: mTelephony(aTelephony)

Please MOZ_ASSERT(aTelephony)

@@ +31,5 @@
> +
> +nsPIDOMWindow*
> +CallsList::GetParentObject() const
> +{
> +  return mTelephony->GetOwner();

This can be inlined.

@@ +44,5 @@
> +already_AddRefed<TelephonyCall>
> +CallsList::Item(uint32_t aIndex)
> +{
> +  nsRefPtr<TelephonyCall> call = mTelephony->GetCalls().SafeElementAt(aIndex);
> +  return call ? call.forget() : nullptr;

call.forget() will handle nullptr just fine.

::: dom/telephony/CallsList.h
@@ +11,5 @@
> +#include "mozilla/ErrorResult.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsWrapperCache.h"
> +#include "Telephony.h"
> +#include "TelephonyCall.h"

Nit: These two should just be forward-declared, but that's handled in TelephonyCommon.h

@@ +24,5 @@
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(CallsList)
> +
> +public:

Nit: remove

@@ +27,5 @@
> +
> +public:
> +  CallsList(Telephony* aTelephony);
> +
> +  ~CallsList();

Refcounted things should always have non-public destructors.

@@ +30,5 @@
> +
> +  ~CallsList();
> +
> +  // WrapperCache
> +  nsPIDOMWindow* GetParentObject() const;

Nit: return types on their own lines here and below.

@@ +35,5 @@
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  // CallsList WebIDL
> +  already_AddRefed<TelephonyCall> Item(uint32_t aIndex);

This (and IndexedGetter) can be const too.

@@ +40,5 @@
> +
> +  uint32_t Length() const;
> +
> +  already_AddRefed<TelephonyCall>
> +  IndexedGetter(uint32_t aIndex, bool &aFound);

Nit: & on the left

::: dom/telephony/Telephony.cpp
@@ +115,5 @@
> +  return TelephonyBinding::Wrap(aCx, aScope, this);
> +}
> +
> +nsPIDOMWindow*
> +Telephony::GetParentObject() const

This should be inlined.

@@ +279,3 @@
>  {
> +  nsRefPtr<TelephonyCall> call;
> +  call = DialInternal(false, aNumber, aRv);

Nit: Here and below let's just keep the declaration and assignment on the same line.

@@ +308,4 @@
>  {
>    nsresult rv = mProvider->SetMicrophoneMuted(aMuted);
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(rv);

I think you can just do:

  aRv = mProvider->SetMicrophoneMuted(aMuted);

Similar simplifications can be made in some of the other methods below.

::: dom/telephony/Telephony.h
@@ +25,5 @@
>  class nsPIDOMWindow;
>  
>  BEGIN_TELEPHONY_NAMESPACE
>  
> +class Telephony : public nsDOMEventTargetHelper

Nit: Can you add MOZ_FINAL here?

@@ +62,5 @@
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  // WebIDL interface
> +  already_AddRefed<TelephonyCall> Dial(const nsAString& aNumber,
> +                                             ErrorResult& aRv);

Nit: alignment is weird here. Below too.

@@ +114,5 @@
>      return mProvider;
>    }
>  
> +  const nsTArray<nsRefPtr<TelephonyCall> >&
> +  GetCalls()

Normally we use "Get" to indicate that the return value could be null. How about you change this to "CallsArray"?

::: dom/telephony/TelephonyCall.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "TelephonyCall.h"
>  
> +#include "CallEvent.h"

Nit: The pattern that exists in dom/telephony for include order is:

  1. Header for specific cpp file
  2. Interfaces (alphabetized)
  3. Other non-local includes (alphabetized)
  4. Includes in the same directory (alphabetized)

Since you're touching basically all of them in this patch we could change the pattern but we should be consistent either way. The other pattern I would be happy with is:

  1. Header for specific cpp file
  2. All others (alphabetized)

It doesn't really matter to me so long as they all do the same thing.

@@ +145,3 @@
>  {
>    MOZ_ASSERT(aCall);
> +  nsRefPtr<CallEvent> event = new CallEvent(this, nullptr, nullptr);

Nit: newline between those lines

@@ +182,3 @@
>  
> +void
> +TelephonyCall::GetNumber(nsString& aNumber) const

A whole lot of these could be inlined.

::: dom/telephony/TelephonyCall.h
@@ +13,3 @@
>  #include "mozilla/dom/DOMError.h"
> +#include "mozilla/ErrorResult.h"
> +#include "nsWrapperCache.h"

You shouldn't need this

@@ +17,5 @@
>  class nsPIDOMWindow;
>  
>  BEGIN_TELEPHONY_NAMESPACE
>  
> +class TelephonyCall : public nsDOMEventTargetHelper

Nit: Can you add MOZ_FINAL here?

@@ +38,5 @@
>    NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(TelephonyCall,
>                                             nsDOMEventTargetHelper)
>  
> +  // WrapperCache
> +  nsPIDOMWindow* GetParentObject() const;

Nit: GetParentObject is a requirement of the bindings code, not nsWrapperCache.

@@ +41,5 @@
> +  // WrapperCache
> +  nsPIDOMWindow* GetParentObject() const;
> +
> +  virtual JSObject* WrapObject(JSContext* aCx,
> +                               JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;

Nit: return types on their own lines, here and below.

@@ +121,5 @@
>  
>  private:
>    TelephonyCall();
>  
> +  ~TelephonyCall();

Hm, why did you have to un-inline this?

::: dom/webidl/CallEvent.webidl
@@ -7,2 @@
>  
> -[Constructor(DOMString type, optional CallEventInit eventInitDict), HeaderFile="GeneratedEventClasses.h"]

Hm, why are you removing the constructor here? As far as I know all events are still supposed to have one.

::: dom/webidl/CallsList.webidl
@@ +1,1 @@
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Nit: Your other files have a VIM modeline too...

@@ +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/.
> + *
> + * The origin of this IDL file is
> + * http://www.w3.org/TR/2012/WD-dom-20120105/

I don't think you should copy this. Just use the standard license stuff.

::: dom/webidl/TelephonyCall.webidl
@@ +8,5 @@
> +  readonly attribute DOMString number;
> +
> +  readonly attribute DOMString state;
> +
> +  // The property "emergency" indicate whether the call number is an emergency

Nit: s/indicate/indicates.

@@ +13,5 @@
> +  // number. Only the outgoing call could have a value with true and it is
> +  // available after dialing state.
> +  readonly attribute boolean emergency;
> +
> +  readonly attribute DOMError error;

I think this should be 'DOMError?' to indicate that it can be null.

::: dom/webidl/WebIDL.mk
@@ +474,5 @@
>  
>  ifdef MOZ_B2G_RIL
>  webidl_files += \
>    CallEvent.webidl \
> +  CallsList.webidl \

This should go in the other block, it's not related to events.

Comment 32

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Jonas Sicking (:sicking) from comment #30)
> Comment on attachment 781511 [details] [diff] [review]
> Patch (v4)
> 
> Review of attachment 781511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/Telephony.cpp
> @@ +361,5 @@
> >    }
> >  
> >    nsresult rv = mProvider->StartTone(aDTMFChar);
> > +  if (NS_FAILED(rv)) {
> > +    aRv.Throw(rv);
> 
> Ideally we should stop using .idl for the provider interface and plain C++
> classes. But that can be done later and isn't urgent at all.

Thank you for the review, Jonas!

I am not sure I got your comment here. Were you saying that we should use .webidl even for internal interfaces?

Comment 33

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to ben turner [:bent] from comment #31)
> Comment on attachment 781511 [details] [diff] [review]
> Patch (v4)
> 
> Review of attachment 781511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks really good! Thank you so much for converting all of this.
> 
> Mostly nits below (sorry!) but I'd like to see the changes so r- for now.
> 

Thanks for the review, Ben! I will go through all the details and address your comments in the next version.


> > -[Constructor(DOMString type, optional CallEventInit eventInitDict), HeaderFile="GeneratedEventClasses.h"]
> 
> Hm, why are you removing the constructor here? As far as I know all events
> are still supposed to have one.
> 

Per my understanding, [constructor] appearing on the interface means the interface would have a [[Construct]] property on the CallEvent interface object. So a web page can do |var x = new CallEvet();| to get a new object that implements the interface.
And in telephony API, the event should be created and fired by Telephony or TelephonyCall object. That's the way to get CallEvent object. So I removed the constructor. Please let me know if  I misunderstand something.

I am not sure about whether all events are supposed to have [constructor], but I see some, like CommandEvent, CompositionEvent, MessageEvent, don't.

Comment 34

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #33)
> (In reply to ben turner [:bent] from comment #31)
> > Comment on attachment 781511 [details] [diff] [review]
> > Patch (v4)
> > 
> > Review of attachment 781511 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks really good! Thank you so much for converting all of this.
> > 
> > Mostly nits below (sorry!) but I'd like to see the changes so r- for now.
> > 
> 
> Thanks for the review, Ben! I will go through all the details and address
> your comments in the next version.
> 
> 
> > > -[Constructor(DOMString type, optional CallEventInit eventInitDict), HeaderFile="GeneratedEventClasses.h"]
> > 
> > Hm, why are you removing the constructor here? As far as I know all events
> > are still supposed to have one.
> > 
> 
> Per my understanding, [constructor] appearing on the interface means the
> interface would have a [[Construct]] property on the CallEvent interface
> object. So a web page can do |var x = new CallEvet();| to get a new object
> that implements the interface.
> And in telephony API, the event should be created and fired by Telephony or
> TelephonyCall object. That's the way to get CallEvent object. So I removed
> the constructor. Please let me know if  I misunderstand something.
> 

Seems missing one word -- That's the *only* way to get CallEvent object.

> I am not sure about whether all events are supposed to have [constructor],
> but I see some, like CommandEvent, CompositionEvent, MessageEvent, don't.

Comment 35

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yes, all events should have constructors.

Comment 36

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to :Ms2ger from comment #35)
> Yes, all events should have constructors.

Thanks for clarification! Will do that in the next patch. But may I ask why it should be designed in this way? Any problems if no constructors?

Comment 37

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to ben turner [:bent] from comment #31)
> Comment on attachment 781511 [details] [diff] [review]
> Patch (v4)
> 
> Review of attachment 781511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks really good! Thank you so much for converting all of this.
> 
> Mostly nits below (sorry!) but I'd like to see the changes so r- for now.

Thank you very much for the review and picking all the nits.

> 
> ::: dom/base/Navigator.h
> @@ +87,5 @@
> >  #endif
> >  } // namespace Connection;
> >  
> > +namespace telephony {
> > +#ifdef MOZ_B2G_RIL
> 
> Nit: Wrap the telephony namespace in the #ifdef too, otherwise it's just a
> waste.
Done.

> 
> ::: dom/telephony/CallEvent.cpp
> @@ +8,5 @@
> > +
> > +USING_TELEPHONY_NAMESPACE
> > +using namespace mozilla::dom;
> > +
> > +CallEvent::CallEvent(mozilla::dom::EventTarget* aOwner,
> 
> Nit: 'mozilla::dom::' should not be needed here.
> 
Done.
> Also, any reason this can't be inlined in the header?
> 
No, moved.
> ::: dom/telephony/CallEvent.h
> @@ +7,5 @@
> > +#include "nsDOMEvent.h"
> > +
> > +#include "mozilla/Attributes.h"
> > +#include "mozilla/dom/CallEventBinding.h"
> > +#include "mozilla/ErrorResult.h"
> 
> Can you just add Attributes.h and ErrorResult.h to TelephonyCommon.h instead
> of adding them to every header?
> 
Good idea.
> @@ +9,5 @@
> > +#include "mozilla/Attributes.h"
> > +#include "mozilla/dom/CallEventBinding.h"
> > +#include "mozilla/ErrorResult.h"
> > +#include "nsCycleCollectionParticipant.h"
> > +#include "nsWrapperCache.h"
> 
> Hopefully you don't need this one.
> 
No, we don't. Cleaned it up.
> @@ +12,5 @@
> > +#include "nsCycleCollectionParticipant.h"
> > +#include "nsWrapperCache.h"
> > +
> > +#include "TelephonyCall.h"
> > +#include "TelephonyCommon.h"
> 
> TelephonyCall.h should include this for you.
> 

Already cleaned the unnecessary headers up.

> @@ +21,5 @@
> > +{
> > +  nsRefPtr<TelephonyCall> mCall;
> > +
> > +public:
> > +  CallEvent(mozilla::dom::EventTarget* aOwner,
> 
> Can this be private along with the destructor? It should only be created via
> the static Create function I hope.
> 

Good idea. Revised.

> @@ +29,5 @@
> > +  NS_DECL_ISUPPORTS_INHERITED
> > +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(CallEvent, nsDOMEvent)
> > +  NS_FORWARD_TO_NSDOMEVENT
> > +
> > +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE
> 
> Nit: 'virtual JSObject*' on its own line, similar for InitCallEvent and
> GetCall below.
>

Thanks for pointing the nits out. Revised. 
> @@ +44,5 @@
> > +    mCall = aCall;
> > +  }
> > +
> > +  static already_AddRefed<CallEvent>
> > +  Create(EventTarget* aOwner, TelephonyCall* aCall);
> 
> Hm, where is this implemented?
> 

My fault. The previous patch didn't call Create() so the patch passed compile and test.
Already implemented Create function.

> @@ +74,5 @@
> > +    return call.forget();
> > +  }
> > +
> > +private:
> > +  ~CallEvent()
> 
> Nit: Might as well add 'virtual' here.
Done.
> 
> ::: dom/telephony/CallsList.cpp
> @@ +10,5 @@
> > +
> > +USING_TELEPHONY_NAMESPACE
> > +using namespace mozilla::dom;
> > +
> > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(CallsList)
> 
> Nit: I'd put all this goop at the end of the file
> 
I will follow the pattern used by other telephony files. I.e., make the macros followed by idl functions.
> @@ +11,5 @@
> > +USING_TELEPHONY_NAMESPACE
> > +using namespace mozilla::dom;
> > +
> > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(CallsList)
> > +NS_IMPL_CYCLE_COLLECTING_ADDREF(CallsList)
> 
> Nit: newline between those two.
> 
OK.
> @@ +13,5 @@
> > +
> > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(CallsList)
> > +NS_IMPL_CYCLE_COLLECTING_ADDREF(CallsList)
> > +NS_IMPL_CYCLE_COLLECTING_RELEASE(CallsList)
> > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(CallsList)
> 
> Nit: newline between those two also.
> 
OK.
> @@ +19,5 @@
> > +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> > +NS_INTERFACE_MAP_END
> > +
> > +CallsList::CallsList(Telephony* aTelephony)
> > +: mTelephony(aTelephony)
> 
> Please MOZ_ASSERT(aTelephony)
> 
OK, but I guess you meant MOZ_ASSER(mTelephony).
> @@ +31,5 @@
> > +
> > +nsPIDOMWindow*
> > +CallsList::GetParentObject() const
> > +{
> > +  return mTelephony->GetOwner();
> 
> This can be inlined.
> 
Not if we want to have Telephony class forward-declared in CallsList.h.

> @@ +44,5 @@
> > +already_AddRefed<TelephonyCall>
> > +CallsList::Item(uint32_t aIndex)
> > +{
> > +  nsRefPtr<TelephonyCall> call = mTelephony->GetCalls().SafeElementAt(aIndex);
> > +  return call ? call.forget() : nullptr;
> 
> call.forget() will handle nullptr just fine.
Good to know that.
> 
> ::: dom/telephony/CallsList.h
> @@ +11,5 @@
> > +#include "mozilla/ErrorResult.h"
> > +#include "nsCycleCollectionParticipant.h"
> > +#include "nsWrapperCache.h"
> > +#include "Telephony.h"
> > +#include "TelephonyCall.h"
> 
> Nit: These two should just be forward-declared, but that's handled in
> TelephonyCommon.h
> 
Done.
> @@ +24,5 @@
> > +public:
> > +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> > +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(CallsList)
> > +
> > +public:
> 
> Nit: remove
> 
Done.
> @@ +27,5 @@
> > +
> > +public:
> > +  CallsList(Telephony* aTelephony);
> > +
> > +  ~CallsList();
> 
> Refcounted things should always have non-public destructors.
> 
Revised.

> @@ +30,5 @@
> > +
> > +  ~CallsList();
> > +
> > +  // WrapperCache
> > +  nsPIDOMWindow* GetParentObject() const;
> 
> Nit: return types on their own lines here and below.
Done.
> 
> @@ +35,5 @@
> > +
> > +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> > +
> > +  // CallsList WebIDL
> > +  already_AddRefed<TelephonyCall> Item(uint32_t aIndex);
> 
> This (and IndexedGetter) can be const too.
> 
Done.
> @@ +40,5 @@
> > +
> > +  uint32_t Length() const;
> > +
> > +  already_AddRefed<TelephonyCall>
> > +  IndexedGetter(uint32_t aIndex, bool &aFound);
> 
> Nit: & on the left
> 
Done.
> ::: dom/telephony/Telephony.cpp
> @@ +115,5 @@
> > +  return TelephonyBinding::Wrap(aCx, aScope, this);
> > +}
> > +
> > +nsPIDOMWindow*
> > +Telephony::GetParentObject() const
> 
> This should be inlined.
> 
Ok.
> @@ +279,3 @@
> >  {
> > +  nsRefPtr<TelephonyCall> call;
> > +  call = DialInternal(false, aNumber, aRv);
> 
> Nit: Here and below let's just keep the declaration and assignment on the
> same line.
> 
Done.
> @@ +308,4 @@
> >  {
> >    nsresult rv = mProvider->SetMicrophoneMuted(aMuted);
> > +  if (NS_FAILED(rv)) {
> > +    aRv.Throw(rv);
> 
> I think you can just do:
> 
>   aRv = mProvider->SetMicrophoneMuted(aMuted);
> 
> Similar simplifications can be made in some of the other methods below.
> 
Good to know that.
> ::: dom/telephony/Telephony.h
> @@ +25,5 @@
> >  class nsPIDOMWindow;
> >  
> >  BEGIN_TELEPHONY_NAMESPACE
> >  
> > +class Telephony : public nsDOMEventTargetHelper
> 
> Nit: Can you add MOZ_FINAL here?
> 
Sure.
> @@ +62,5 @@
> > +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> > +
> > +  // WebIDL interface
> > +  already_AddRefed<TelephonyCall> Dial(const nsAString& aNumber,
> > +                                             ErrorResult& aRv);
> 
> Nit: alignment is weird here. Below too.
> 
Revised.

> @@ +114,5 @@
> >      return mProvider;
> >    }
> >  
> > +  const nsTArray<nsRefPtr<TelephonyCall> >&
> > +  GetCalls()
> 
> Normally we use "Get" to indicate that the return value could be null. How
> about you change this to "CallsArray"?
Sure.
> 
> ::: dom/telephony/TelephonyCall.cpp
> @@ +5,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  #include "TelephonyCall.h"
> >  
> > +#include "CallEvent.h"
> 
> Nit: The pattern that exists in dom/telephony for include order is:
> 
>   1. Header for specific cpp file
>   2. Interfaces (alphabetized)
>   3. Other non-local includes (alphabetized)
>   4. Includes in the same directory (alphabetized)
> 
> Since you're touching basically all of them in this patch we could change
> the pattern but we should be consistent either way. The other pattern I
> would be happy with is:
> 
>   1. Header for specific cpp file
>   2. All others (alphabetized)
> 
> It doesn't really matter to me so long as they all do the same thing.
> 
I'll follow the current pattern. Thank you.
> @@ +145,3 @@
> >  {
> >    MOZ_ASSERT(aCall);
> > +  nsRefPtr<CallEvent> event = new CallEvent(this, nullptr, nullptr);
> 
> Nit: newline between those lines
> 
OK.
> @@ +182,3 @@
> >  
> > +void
> > +TelephonyCall::GetNumber(nsString& aNumber) const
> 
> A whole lot of these could be inlined.
> 
Done.
> ::: dom/telephony/TelephonyCall.h
> @@ +13,3 @@
> >  #include "mozilla/dom/DOMError.h"
> > +#include "mozilla/ErrorResult.h"
> > +#include "nsWrapperCache.h"
> 
> You shouldn't need this
>
Cleared.
> @@ +17,5 @@
> >  class nsPIDOMWindow;
> >  
> >  BEGIN_TELEPHONY_NAMESPACE
> >  
> > +class TelephonyCall : public nsDOMEventTargetHelper
> 
> Nit: Can you add MOZ_FINAL here?
> 
Sure.

> @@ +38,5 @@
> >    NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(TelephonyCall,
> >                                             nsDOMEventTargetHelper)
> >  
> > +  // WrapperCache
> > +  nsPIDOMWindow* GetParentObject() const;
> 
> Nit: GetParentObject is a requirement of the bindings code, not
> nsWrapperCache.
> 
Corrected.
> @@ +41,5 @@
> > +  // WrapperCache
> > +  nsPIDOMWindow* GetParentObject() const;
> > +
> > +  virtual JSObject* WrapObject(JSContext* aCx,
> > +                               JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> 
> Nit: return types on their own lines, here and below.
> 
Done. Sorry for letting you repeat this in the review. Thank you.
> @@ +121,5 @@
> >  
> >  private:
> >    TelephonyCall();
> >  
> > +  ~TelephonyCall();
> 
> Hm, why did you have to un-inline this?

Because of the compile error:
In file included from ../../dist/include/mozilla/dom/BindingDeclarations.h:24,
                 from ../../dist/include/mozilla/dom/EventBinding.h:8,
                 from ../../dist/include/mozilla/dom/CallEventBinding.h:7,
                 from ~/src/mozilla/mozilla-central/dom/telephony/CallEvent.h:10,
                 from ~/src/mozilla/mozilla-central/dom/telephony/CallEvent.cpp:7:
../../dist/include/nsAutoPtr.h: In destructor 'nsRefPtr<T>::~nsRefPtr() [with T = mozilla::dom::telephony::Telephony]':
~/src/mozilla/mozilla-central/dom/telephony/TelephonyCall.h:139:   instantiated from here
../../dist/include/nsAutoPtr.h:880: error: invalid use of incomplete type 'struct mozilla::dom::telephony::Telephony'
~/src/mozilla/mozilla-central/dom/telephony/TelephonyCommon.h:33: error: forward declaration of 'struct mozilla::dom::telephony::Telephony'

> 
> ::: dom/webidl/CallEvent.webidl
> @@ -7,2 @@
> >  
> > -[Constructor(DOMString type, optional CallEventInit eventInitDict), HeaderFile="GeneratedEventClasses.h"]
> 
> Hm, why are you removing the constructor here? As far as I know all events
> are still supposed to have one.

Revised.

> 
> ::: dom/webidl/CallsList.webidl
> @@ +1,1 @@
> > +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> Nit: Your other files have a VIM modeline too...

Cleaned them up.
> 
> @@ +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/.
> > + *
> > + * The origin of this IDL file is
> > + * http://www.w3.org/TR/2012/WD-dom-20120105/
> 
> I don't think you should copy this. Just use the standard license stuff.
> 
Removed.

> ::: dom/webidl/TelephonyCall.webidl
> @@ +8,5 @@
> > +  readonly attribute DOMString number;
> > +
> > +  readonly attribute DOMString state;
> > +
> > +  // The property "emergency" indicate whether the call number is an emergency
> 
> Nit: s/indicate/indicates.
> 
Done.
> @@ +13,5 @@
> > +  // number. Only the outgoing call could have a value with true and it is
> > +  // available after dialing state.
> > +  readonly attribute boolean emergency;
> > +
> > +  readonly attribute DOMError error;
> 
> I think this should be 'DOMError?' to indicate that it can be null.
> 
Good point.
> ::: dom/webidl/WebIDL.mk
> @@ +474,5 @@
> >  
> >  ifdef MOZ_B2G_RIL
> >  webidl_files += \
> >    CallEvent.webidl \
> > +  CallsList.webidl \
> 
> This should go in the other block, it's not related to events.
Moved.

Comment 38

[Hsin-Yi Tsai (she/her) [:hsinyi]]

> > @@ +121,5 @@
> > >  
> > >  private:
> > >    TelephonyCall();
> > >  
> > > +  ~TelephonyCall();
> > 
> > Hm, why did you have to un-inline this?
> 
> Because of the compile error:
> In file included from
> ../../dist/include/mozilla/dom/BindingDeclarations.h:24,
>                  from ../../dist/include/mozilla/dom/EventBinding.h:8,
>                  from ../../dist/include/mozilla/dom/CallEventBinding.h:7,
>                  from
> ~/src/mozilla/mozilla-central/dom/telephony/CallEvent.h:10,
>                  from
> ~/src/mozilla/mozilla-central/dom/telephony/CallEvent.cpp:7:
> ../../dist/include/nsAutoPtr.h: In destructor 'nsRefPtr<T>::~nsRefPtr()
> [with T = mozilla::dom::telephony::Telephony]':
> ~/src/mozilla/mozilla-central/dom/telephony/TelephonyCall.h:139:  
> instantiated from here
> ../../dist/include/nsAutoPtr.h:880: error: invalid use of incomplete type
> 'struct mozilla::dom::telephony::Telephony'
> ~/src/mozilla/mozilla-central/dom/telephony/TelephonyCommon.h:33: error:
> forward declaration of 'struct mozilla::dom::telephony::Telephony'
> 
> > 

To be clear, in the current code the only places that include TelephonyCall.h now are TelephonyCall.cpp, Telephony.cpp, and nsDOMClassInfo.cpp. They all include TelephonyCall.h. But with the patch, we will have some generated code such as CallEventBinding.cpp and CallsListBinding.cpp including TelephonyCall.h without Telephony.h. So we either include Telephony.h in TelephonyCall.h or un-inline ~TelephonyCall().

Comment 39

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Patch (v5)

Comments addressed. Thank you.

Comment 40

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Patch (v5) -- rebased

Rebased.

Comment 41

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 786233 [details] [diff] [review]
Patch (v5) -- rebased

Thanks again.

Comment 42

[Hsin-Yi Tsai (she/her) [:hsinyi]]

try: https://tbpl.mozilla.org/?tree=Try&rev=8c7a210af868

Comment 43

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 786233 [details] [diff] [review]
Patch (v5) -- rebased

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

This looks great! Thanks so much for your hard work here.

Only one actual bug remains here, the rest are nits. r=me with this all addressed!

::: dom/telephony/CallEvent.h
@@ +5,5 @@
> +
> +#ifndef mozilla_dom_telephony_callevent_h
> +#define mozilla_dom_telephony_callevent_h
> +
> +#include "mozilla/dom/CallEventBinding.h"

Nit: Is this needed here? Probably just in the cpp, right?

@@ +12,5 @@
> +#include "nsDOMEvent.h"
> +
> +BEGIN_TELEPHONY_NAMESPACE
> +
> +class CallEvent : public nsDOMEvent

Nit: MOZ_FINAL please

::: dom/telephony/CallsList.cpp
@@ +11,5 @@
> +
> +USING_TELEPHONY_NAMESPACE
> +using namespace mozilla::dom;
> +
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(CallsList)

You need to traverse/unlink mTelephony, so:

  NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(CallsList, mTelephony)

Please double-check all your classes that have strong references (nsCOMPtr/nsRefPtr) and make sure they're accounted for in the CC macros.

::: dom/telephony/CallsList.h
@@ +33,5 @@
> +  // CallsList WebIDL
> +  already_AddRefed<TelephonyCall>
> +  Item(uint32_t aIndex) const;
> +
> +  uint32_t Length() const;

Nit: return type on its own line.

::: dom/telephony/Telephony.cpp
@@ +258,5 @@
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(Telephony,
>                                                  nsDOMEventTargetHelper)
>    tmp->mCalls.Clear();
>    tmp->mActiveCall = nullptr;
> +  tmp->mCallsList = nullptr;

Nit: Please use NS_IMPL_CYCLE_COLLECTION_UNLINK for mCalls and mCallsList.

::: dom/telephony/Telephony.h
@@ +120,5 @@
>      return mProvider;
>    }
>  
> +  const nsTArray<nsRefPtr<TelephonyCall> >&
> +  CallsArray()

Nit: This can be const too.

::: dom/telephony/TelephonyCall.cpp
@@ +53,5 @@
> +
> +nsPIDOMWindow*
> +TelephonyCall::GetParentObject() const
> +{
> +  return mTelephony->GetOwner();

Nit: TelephonyCall inherits nsDOMEventTargetHelper, you should move this to the header and then just call 'GetOwner()' like Telephony does.

::: dom/telephony/TelephonyCommon.h
@@ -1,2 @@
> -/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> -/* vim: set ts=2 et sw=2 tw=40: */

Nit: Hm, why remove this from all these files?

::: dom/telephony/moz.build
@@ +17,5 @@
>  ]
>  
>  MODULE = 'dom'
>  
> +EXPORTS.mozilla.dom.telephony += [

Nit: Do we actually need to export all these files? It won't really hurt anything but I wouldn't do it if we can avoid it.

Comment 44

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to ben turner [:bent] from comment #43)
> Comment on attachment 786233 [details] [diff] [review]
> Patch (v5) -- rebased
> 
> Review of attachment 786233 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great! Thanks so much for your hard work here.
> 
> Only one actual bug remains here, the rest are nits. r=me with this all
> addressed!
> 
> ::: dom/telephony/CallEvent.h
> @@ +5,5 @@
> > +
> > +#ifndef mozilla_dom_telephony_callevent_h
> > +#define mozilla_dom_telephony_callevent_h
> > +
> > +#include "mozilla/dom/CallEventBinding.h"
> 
> Nit: Is this needed here? Probably just in the cpp, right?

We need it for CallEventInit type when declaring |static already_AddRefed<CallEvent> Constructor(const GlobalObject& aGlobal, const nsAString& aType, const CallEventInit& aOptions, ErrorResult& aRv);|

> 
> @@ +12,5 @@
> > +#include "nsDOMEvent.h"
> > +
> > +BEGIN_TELEPHONY_NAMESPACE
> > +
> > +class CallEvent : public nsDOMEvent
> 
> Nit: MOZ_FINAL please

Sure.
> 
> ::: dom/telephony/CallsList.cpp
> @@ +11,5 @@
> > +
> > +USING_TELEPHONY_NAMESPACE
> > +using namespace mozilla::dom;
> > +
> > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(CallsList)
> 
> You need to traverse/unlink mTelephony, so:
> 
>   NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(CallsList, mTelephony)
> 
> Please double-check all your classes that have strong references
> (nsCOMPtr/nsRefPtr) and make sure they're accounted for in the CC macros.
> 

Thank you for pointing this out.

> ::: dom/telephony/CallsList.h
> @@ +33,5 @@
> > +  // CallsList WebIDL
> > +  already_AddRefed<TelephonyCall>
> > +  Item(uint32_t aIndex) const;
> > +
> > +  uint32_t Length() const;
> 
> Nit: return type on its own line.
> 
OK.

> ::: dom/telephony/Telephony.cpp
> @@ +258,5 @@
> >  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(Telephony,
> >                                                  nsDOMEventTargetHelper)
> >    tmp->mCalls.Clear();
> >    tmp->mActiveCall = nullptr;
> > +  tmp->mCallsList = nullptr;
> 
> Nit: Please use NS_IMPL_CYCLE_COLLECTION_UNLINK for mCalls and mCallsList.
Done.
> 
> ::: dom/telephony/Telephony.h
> @@ +120,5 @@
> >      return mProvider;
> >    }
> >  
> > +  const nsTArray<nsRefPtr<TelephonyCall> >&
> > +  CallsArray()
> 
> Nit: This can be const too.
> 

OK
> ::: dom/telephony/TelephonyCall.cpp
> @@ +53,5 @@
> > +
> > +nsPIDOMWindow*
> > +TelephonyCall::GetParentObject() const
> > +{
> > +  return mTelephony->GetOwner();
> 
> Nit: TelephonyCall inherits nsDOMEventTargetHelper, you should move this to
> the header and then just call 'GetOwner()' like Telephony does.

Done.
> 
> ::: dom/telephony/TelephonyCommon.h
> @@ -1,2 @@
> > -/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> > -/* vim: set ts=2 et sw=2 tw=40: */
> 
> Nit: Hm, why remove this from all these files?
> 

I'll keep them untouched, and make the .cpp/.h follow the same header style.

> ::: dom/telephony/moz.build
> @@ +17,5 @@
> >  ]
> >  
> >  MODULE = 'dom'
> >  
> > +EXPORTS.mozilla.dom.telephony += [
> 
> Nit: Do we actually need to export all these files? It won't really hurt
> anything but I wouldn't do it if we can avoid it.

Sounds good.
Okay, removed.

Comment 45

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #44)
> (In reply to ben turner [:bent] from comment #43)
> > Comment on attachment 786233 [details] [diff] [review]
> > Patch (v5) -- rebased
> > 
> > Review of attachment 786233 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks great! Thanks so much for your hard work here.
> > 
> > Only one actual bug remains here, the rest are nits. r=me with this all
> > addressed!
> > 
> > ::: dom/telephony/CallEvent.h
> > @@ +5,5 @@
> > > +
> > > +#ifndef mozilla_dom_telephony_callevent_h
> > > +#define mozilla_dom_telephony_callevent_h
> > > +
> > > +#include "mozilla/dom/CallEventBinding.h"
> > 
> > Nit: Is this needed here? Probably just in the cpp, right?
> 
> We need it for CallEventInit type when declaring |static
> already_AddRefed<CallEvent> Constructor(const GlobalObject& aGlobal, const
> nsAString& aType, const CallEventInit& aOptions, ErrorResult& aRv);|

You should be able to forward declare it.

Comment 46

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: v6.patch - sr=sicking, r=bent

comment 43 & comment 45 addressed.

Comment 47

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Ｔhanks for the review and comments!
try: https://tbpl.mozilla.org/?tree=Try&rev=ce7441ffd264


https://hg.mozilla.org/integration/b2g-inbound/rev/5726180e4834

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5726180e4834