Closed Bug 833278 Opened 7 years ago Closed 7 years ago

Implement MozVoicemailEvent using codegenerator

Categories

(Core :: DOM: Events, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
blocking-b2g -

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 3 obsolete files)

No description provided.
Assignee: nobody → vyang
Attachment #704808 - Flags: review?(mounir)
Let's have some code cleanups before rewriting its IPC parts.
Blocks: 833229
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?
Attachment #704809 - Flags: review?(bugs)
Attachment #704809 - Flags: review?(anygregor)
Attachment #704809 - Flags: review+
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
Attachment #704809 - Flags: review?(bugs) → review+
Address review comment #4 & comment #5. Verified again locally.
Attachment #704809 - Attachment is obsolete: true
Attachment #705735 - Flags: review+
Blocks: 834160
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
Attachment #704808 - Flags: review?(mounir) → review-
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`.
Attachment #704808 - Attachment is obsolete: true
Attachment #708021 - Flags: review?(mounir)
1) Rebase
2) Don't include nsIDOMMozVoicemailEvent.h in nsDOMClassInfo.cpp as requested in bug 834193 comment #3.
Attachment #705735 - Attachment is obsolete: true
Attachment #708023 - Flags: review+
blocking-b2g: --- → leo?
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.
Attachment #708021 - Flags: review?(mounir) → review+
Blocking- without any clear user impact or product requirement. But tree is open, land away.
blocking-b2g: leo? → -
(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.
https://hg.mozilla.org/mozilla-central/rev/4f741c36cc20
https://hg.mozilla.org/mozilla-central/rev/1057dfda57a5
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.