1070831 - Deprecate MozMobileConnection.sendMMI, cancelMMI

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Szu-Yu Chen [:aknow] (inactive)]

Once we finish Bug 1058398 and all the gaia related changes, the following interface should be deprecated.
- MozMobileConnection.sendMMI
- MozMobileConnection.cancelMMI
- MozMobileConnection.onussdreceived
- USSDReceivedEvent.sessionEnded

Comment 1

[Szu-Yu Chen [:aknow] (inactive)]

We also need to...

1. Move notifyUssdReceived from MobileConnectionService.js to TelephonyService.js. Remember to handle the interface as well.
2. Move gMobileConnectionMessenger.notifyUssdReceived from nsIMobileConnectionMessenger to nsITelephonyMessenger.

Comment 2

[Ben Hsu [:HoPang]]

Attachment: Part 1-1: Remove sendMMI() and cancelMMI() (WebIDL)

Comment 3

[Ben Hsu [:HoPang]]

Attachment: Part 1-2: Remove sendMMI() and cancelMMI() (DOM)

Comment 4

[Ben Hsu [:HoPang]]

Attachment: Part 1-3: Remove sendMMI() and cancelMMI() (IDL)

Comment 5

[Ben Hsu [:HoPang]]

Attachment: Part 1-4: Remove sendMMI() and cancelMMI() (Service)

Comment 6

[Ben Hsu [:HoPang]]

Attachment: Part 1-5: Remove sendMMI() and cancelMMI() (IPC)

Comment 7

[Ben Hsu [:HoPang]]

Attachment: Part 1-6: Remove sendMMI() and cancelMMI() (Marionette)

Comment 8

[Ben Hsu [:HoPang]]

Attachment: Part 1-7: Remove sendMMI() and cancelMMI() (Mochitest)

Comment 9

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8536485 [details] [diff] [review]
Part 1-1: Remove sendMMI() and cancelMMI() (WebIDL)

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

looks good.
r? hsinyi for the formal review

Comment 10

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

Comment on attachment 8536485 [details] [diff] [review]
Part 1-1: Remove sendMMI() and cancelMMI() (WebIDL)

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

According to comment 0, I would expect to see mobileConnection.onussdreceived and USSDReceivedEvent.sessionEnded being removed, too.

Comment 11

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

Comment on attachment 8536485 [details] [diff] [review]
Part 1-1: Remove sendMMI() and cancelMMI() (WebIDL)

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

According to comment 0, I would expect to see mobileConnection.onussdreceived and USSDReceivedEvent.sessionEnded being removed, too. 
r=me with following parts for onussdreceived and sessionEnded :)

Comment 12

[Ben Hsu [:HoPang]]

Attachment: Part 2-1: Migrate onussdreceived (WebIDL)

Comment 13

[Ben Hsu [:HoPang]]

Attachment: Part 2-2: Migrate onussdreceived (DOM)

Comment 14

[Ben Hsu [:HoPang]]

Attachment: Part 2-3: Migrate onussdreceived (IDL)

Comment 15

[Ben Hsu [:HoPang]]

Attachment: Part 2-4: Migrate onussdreceived (Service)

Comment 16

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

Comment on attachment 8537007 [details] [diff] [review]
Part 2-1: Migrate onussdreceived (WebIDL)

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

::: dom/webidl/Telephony.webidl
@@ +57,5 @@
> +  /**
> +   * The 'ussdreceived' event is notified whenever a new USSD message is
> +   * received.
> +   */
> +  attribute EventHandler onussdreceived;

Per bug 1058397 comment 15, we are fine without telephony.onussdreceived. Let's just ignore it for now, thank you.

Comment 17

[Ben Hsu [:HoPang]]

Attachment: Part 2-5: Migrate onussdreceived (IPC)

Comment 18

[Ben Hsu [:HoPang]]

Attachment: Part 2-1: Remove onussdreceived (WebIDL)

Comment 19

[Ben Hsu [:HoPang]]

Attachment: Part 2-2: Remove onussdreceived (DOM)

Comment 20

[Ben Hsu [:HoPang]]

Attachment: Part 2-3: Remove onussdreceived (IDL)

Comment 21

[Ben Hsu [:HoPang]]

Attachment: Part 2-4: Remove onussdreceived (Service)

Comment 22

[Ben Hsu [:HoPang]]

Attachment: Part 2-5: Remove onussdreceived (IPC)

Comment 23

[Ben Hsu [:HoPang]]

Attachment: Part 2-6: Remove onussdreceived (RILSystemMessengerHelper)

Comment 24

[Ben Hsu [:HoPang]]

Attachment: Part 2-7: Remove onussdreceived (BluetoothRilListener)

Comment 25

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8537081 [details] [diff] [review]
Part 2-3: Remove onussdreceived (IDL)

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

::: dom/telephony/nsITelephonyMessenger.idl
@@ +44,5 @@
> +   * @param message
> +   *        The ussd request in string format.
> +   * @param sessionEnded
> +   *        Indicates whether the session is ended.
> +   */

1. Comment is not correct. You miss one parameter and the description for message is not appropriate. Please refer to the original comment written in nsIMobileConnectionMessenger.idl.

2. You should use "aServiceId" instead of "clientId". Follow the style as notifyCallEnded and make sure the consistency of serviceId

3. Follow the coding style. Use "aXXXX" for the name of parameters

Comment 26

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

Comment on attachment 8537073 [details] [diff] [review]
Part 2-1: Remove onussdreceived (WebIDL)

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

Thank you.

Comment 27

[Ben Hsu [:HoPang]]

Attachment: Part 2-3: Remove onussdreceived (IDL) (V2)

Comment 28

[Ben Hsu [:HoPang]]

Attachment: Part 3-1: Remove sessionEnded (WebIDL)

Comment 29

[Ben Hsu [:HoPang]]

Attachment: Part 3-2: Remove sessionEnded (Service)

Comment 30

[Ben Hsu [:HoPang]]

Attachment: Part 1-4: Remove sendMMI() and cancelMMI() (Service) (V2)

Comment 31

[Ben Hsu [:HoPang]]

Attachment: Part 1-8: Remove sendMMI() and cancelMMI() (ril_worker.js)

Comment 32

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

Comment on attachment 8537760 [details] [diff] [review]
Part 3-1: Remove sessionEnded (WebIDL)

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

Looks good to me, thank you.

Comment 33

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8537614 [details] [diff] [review]
Part 2-3: Remove onussdreceived (IDL) (V2)

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

Good. Thank you.

Comment 34

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8536489 [details] [diff] [review]
Part 1-3: Remove sendMMI() and cancelMMI() (IDL)

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

You'll need to remove dialMMI from nsIGonkTelephonyService.idl

Comment 35

[Ben Hsu [:HoPang]]

Attachment: Part 1-3: Remove sendMMI() and cancelMMI() (IDL) (V2)

Comment 36

[Szu-Yu Chen [:aknow] (inactive)]

Hi Ben,

For bluetooth, remember that there are two folders. You'll need to modify the file under dom/bluetooth2 as well. Then, you can use "git blame" or "hg annotate" to find the original author (I guess it's Ben[:btian])of that piece of code and request the person for review.

Ex: http://hg.mozilla.org/mozilla-central/annotate/0e441ff66c5e/dom/bluetooth/BluetoothRilListener.cpp

I appreciate you separating the patches into several small chunks. It's really easy for me to review them and I like it. However, for the final version of patches, maybe you could merge several patches into one  and organized them by file or by functionality. I don't know the exact rule or preference for formating the patches. It's just my personal feeling because the number of patches you have now looks scary.

Comment 37

[Ben Hsu [:HoPang]]

Hi Aknow,

Thanks for your advice, and I totally agree with you that the number of the patches is scary. I'll organize those patches right after the the patch for bluetooth is review granted.

Comment 38

[Ben Hsu [:HoPang]]

Attachment: Part 2-7: Remove onussdreceived (BluetoothRilListener) (V2)

Hi, Ben

Since the internal interface of mobileConnection has been redesigned, we have to remove some code from BluetoothRilListener.cpp. Can you review this patch?

Comment 39

[Ben Tian (inactive)]

Comment on attachment 8538944 [details] [diff] [review]
Part 2-7: Remove onussdreceived (BluetoothRilListener) (V2)

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

LGTM.

Comment 40

[Ben Hsu [:HoPang]]

https://tbpl.mozilla.org/?tree=Try&rev=2811a373576e

Comment 41

[Ben Hsu [:HoPang]]

Attachment: Part 1: Clean up the WebIDL

Comment 42

[Ben Hsu [:HoPang]]

Attachment: Part 2: Clean up the DOM implementation

Comment 43

[Ben Hsu [:HoPang]]

Attachment: Part 3: Internal interface changes

Comment 44

[Ben Hsu [:HoPang]]

Attachment: Part 4: Internal architecture changes

Comment 45

[Ben Hsu [:HoPang]]

Attachment: Part 5: Clean up the related IPC files

Comment 46

[Ben Hsu [:HoPang]]

Attachment: Part 6: Clean up BluetoothRilListener

Comment 47

[Ben Hsu [:HoPang]]

Attachment: Part 7: Update related testcases

Comment 48

[Ben Hsu [:HoPang]]

https://tbpl.mozilla.org/?tree=Try&rev=2811a373576e

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f66cc942116f
https://hg.mozilla.org/integration/b2g-inbound/rev/ad85e32377a8
https://hg.mozilla.org/integration/b2g-inbound/rev/56f1f17e9ed4
https://hg.mozilla.org/integration/b2g-inbound/rev/9dab3d696c4f
https://hg.mozilla.org/integration/b2g-inbound/rev/f8ba9bea2731
https://hg.mozilla.org/integration/b2g-inbound/rev/56a25a9b0aba
https://hg.mozilla.org/integration/b2g-inbound/rev/82155015764e

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f66cc942116f
https://hg.mozilla.org/mozilla-central/rev/ad85e32377a8
https://hg.mozilla.org/mozilla-central/rev/56f1f17e9ed4
https://hg.mozilla.org/mozilla-central/rev/9dab3d696c4f
https://hg.mozilla.org/mozilla-central/rev/f8ba9bea2731
https://hg.mozilla.org/mozilla-central/rev/56a25a9b0aba
https://hg.mozilla.org/mozilla-central/rev/82155015764e

Comment 52

[Ben Hsu [:HoPang]]

Attachment: Part 7: Update related testcases (Follow)

Thanks to edgar for reminding me that I've made a mistake when merging those patches, so there is still one testcase should be removed.

Comment 53

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8541093 [details] [diff] [review]
Part 7: Update related testcases (Follow)

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

Please also remove the sendMMI() in mobileconnection head.js

r=me with comment addressed.

Comment 54

[Ben Hsu [:HoPang]]

Attachment: Part 7: Update related testcases (Follow) (V2)

Comment 55

[Ben Hsu [:HoPang]]

https://tbpl.mozilla.org/?tree=Try&rev=7396e95e6ea8

Comment 56

[Ryan VanderMeulen [:RyanVM]]

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

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1421342f823