833278 - Implement MozVoicemailEvent using codegenerator

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Comment 1

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

Attachment: Part 1/2: move voicemail sources to dom/voicemail

Comment 2

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

Attachment: Part 2/2: Implement MozVoicemailEvent using codegenerator

Comment 3

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

Let's have some code cleanups before rewriting its IPC parts.

Comment 4

[Gregor Wagner [:gwagner]]

Comment on attachment 704809 [details] [diff] [review]
Part 2/2: Implement MozVoicemailEvent using codegenerator

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

Looks good to me but I think smaug should also look over it.

::: dom/voicemail/Voicemail.cpp
@@ +97,5 @@
> +  NS_NewDOMMozVoicemailEvent(getter_AddRefs(event), nullptr, nullptr);
> +
> +  nsCOMPtr<nsIDOMMozVoicemailEvent> ce = do_QueryInterface(event);
> +  nsresult rv = ce->InitMozVoicemailEvent(NS_LITERAL_STRING("statuschanged"),
> +                                          true, false, aStatus);

is the aCanBubbleArg change intentional?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 704809 [details] [diff] [review]
Part 2/2: Implement MozVoicemailEvent using codegenerator

>@@ -3918,17 +3916,17 @@ nsDOMClassInfo::Init()
> 
>   DOM_CLASSINFO_MAP_BEGIN(MozVoicemail, nsIDOMMozVoicemail)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMMozVoicemail)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget)
>   DOM_CLASSINFO_MAP_END
> 
>   DOM_CLASSINFO_MAP_BEGIN(MozVoicemailEvent, nsIDOMMozVoicemailEvent)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMMozVoicemailEvent)
>-    DOM_CLASSINFO_MAP_ENTRY(nsIDOMEvent)
>+    DOM_CLASSINFO_EVENT_MAP_ENTRIES
>   DOM_CLASSINFO_MAP_END

You should remove the whole 
DOM_CLASSINFO_MAP_BEGIN(MozVoicemailEvent, nsIDOMMozVoicemailEvent)
...
DOM_CLASSINFO_MAP_END

>-  nsresult rv = event->InitVoicemailEvent(NS_LITERAL_STRING("statuschanged"),
>-                                          false, false, aStatus);
>+  nsCOMPtr<nsIDOMEvent> event;
>+  NS_NewDOMMozVoicemailEvent(getter_AddRefs(event), nullptr, nullptr);
>+
>+  nsCOMPtr<nsIDOMMozVoicemailEvent> ce = do_QueryInterface(event);
>+  nsresult rv = ce->InitMozVoicemailEvent(NS_LITERAL_STRING("statuschanged"),
>+                                          true, false, aStatus);
false, false
as in the old code

Comment 6

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

Attachment: Part 2/2: Implement MozVoicemailEvent using codegenerator : v2

Address review comment #4 & comment #5. Verified again locally.

Comment 7

[Mounir Lamouri (:mounir)]

Comment on attachment 704808 [details] [diff] [review]
Part 1/2: move voicemail sources to dom/voicemail

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

Vicamo, could you re-attach this patch with the removed-files.sh fixed and using `hg mv` to move the files. It would make the patch way smaller and will ease the review process.

Sorry that it took so long before asking you to do that. FWIW, I tried to review files that didn't look like simple file move and they seem okay.

::: browser/installer/removed-files.in
@@ +75,5 @@
>  components/pluginGlue.js
>  components/sidebar.xpt
>  #ifdef MOZ_B2G_RIL
>  components/dom_telephony.xpt
> +components/dom_voicemail.xpt

Why do you mark this file as to be removed if you are just adding it?

Please, don't add the file in this list.

@@ +1192,5 @@
>    components/dom_apps.xpt
>    components/dom_base.xpt
>  #ifdef MOZ_B2G_RIL
>    components/dom_telephony.xpt
> +  components/dom_voicemail.xpt

ditto

Comment 8

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

Attachment: Part 1/2: move voicemail sources to dom/voicemail : v2

1) Re-gen patch with `hg mv` & `hg cp`. I didn't know that helps reducing patch size. Sorry.
2) Remove entries in `removed-files.in`.

Comment 9

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

Attachment: Part 2/2: Implement MozVoicemailEvent using codegenerator : v3

1) Rebase
2) Don't include nsIDOMMozVoicemailEvent.h in nsDOMClassInfo.cpp as requested in bug 834193 comment #3.

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 708021 [details] [diff] [review]
Part 1/2: move voicemail sources to dom/voicemail : v2

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

I'm not sure why we want to move all of that to dom/voicemail but the patch looks good.

r=me, and sorry for the delay.

Comment 11

[Dietrich Ayala (:dietrich)]

Blocking- without any clear user impact or product requirement. But tree is open, land away.

Comment 12

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

(In reply to Mounir Lamouri (:mounir) from comment #10)
> I'm not sure why we want to move all of that to dom/voicemail
> but the patch looks good.

In order to deprecate RILContentHelper, the ideal/only replacement will be IPDL. We will then also copy directory structure of SMS, placing backend glue code in dom/voicemail. dom/telephony will have the same stuff, so separating them here isn't a bad idea. Beside, considering voice mail is not an mandatory element for WebTelephony, having another standalone folder reflects this, too.

Comment 13

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

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

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f741c36cc20
https://hg.mozilla.org/integration/mozilla-inbound/rev/1057dfda57a5

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f741c36cc20
https://hg.mozilla.org/mozilla-central/rev/1057dfda57a5