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

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 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

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

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

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`.

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

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.

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

(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.

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

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

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