Closed
Bug 818353
Opened 12 years ago
Closed 11 years ago
B2G Multi-SIM: mobileconnection - add client id in nsIMobileConnectionProvider
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(blocking-b2g:1.3+, firefox28 fixed)
Tracking | Status | |
---|---|---|
firefox28 | --- | fixed |
People
(Reporter: hsinyi, Assigned: jessica)
References
Details
(Whiteboard: [FT:RIL])
Attachments
(5 files, 21 obsolete files)
7.49 KB,
patch
|
hsinyi
:
review+
|
Details | Diff | Splinter Review |
12.28 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
3.47 KB,
patch
|
gyeh
:
review+
|
Details | Diff | Splinter Review |
991 bytes,
patch
|
jessica
:
review+
|
Details | Diff | Splinter Review |
48.54 KB,
patch
|
jessica
:
review+
|
Details | Diff | Splinter Review |
This bug was originally reported in Bug 814584. This bug focuses on re-factoring 'mobileconnection' related functions and messages. What we probably need to do is: a) nsIMobileConnectionProvider IDL change: add an attribute 'subscriptionId' b) DOM changes: assign '0' (default subscriptionId) in every MobileConnectionProvider function call based on a) c) RIL impl: i) add a property 'subscriptionId' in IPC messages ii) notify observer of a specific Id ...
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → kchang
Reporter | ||
Updated•12 years ago
|
Blocks: b2g-multi-sim
Comment 1•12 years ago
|
||
Done. Here is the branch path. https://github.com/multi-sim/releases-mozilla-central/tree/bugzilla/818353/master
Updated•11 years ago
|
Assignee: kchang → echen
Updated•11 years ago
|
Assignee: echen → jjong
Assignee | ||
Comment 2•11 years ago
|
||
add client id in nsIMobileConnectionProvider.idl
Attachment #810430 -
Flags: feedback?(htsai)
Attachment #810430 -
Flags: feedback?(echen)
Assignee | ||
Comment 3•11 years ago
|
||
use default client id (0) in function calls
Attachment #810431 -
Flags: feedback?(htsai)
Attachment #810431 -
Flags: feedback?(echen)
Assignee | ||
Comment 4•11 years ago
|
||
use default client id (0) in BT function calls
Attachment #810433 -
Flags: feedback?(htsai)
Attachment #810433 -
Flags: feedback?(echen)
Assignee | ||
Comment 5•11 years ago
|
||
I have modified some of the function definitions in RILContentHelper.js (e.g. registerListener(), unregisterListener(), _deliverEvent(), etc.), so I needed to touch some of the other functions (icc, voicemail, etc.) for them to work. I marked them with "//TODO: fixme", not sure it it's okay...?
Attachment #810436 -
Flags: feedback?(htsai)
Attachment #810436 -
Flags: feedback?(echen)
Comment 6•11 years ago
|
||
Comment on attachment 810430 [details] [diff] [review] Part 1: idl changes Review of attachment 810430 [details] [diff] [review]: ----------------------------------------------------------------- Please see my below comments. BTW, I guess you miss the getter function for iccId. :) Thanks ::: dom/network/interfaces/nsIMobileConnectionProvider.idl @@ +43,5 @@ > */ > + void registerMobileConnectionMsg(in nsIMobileConnectionListener listener, > + in unsigned long clientId); > + void unregisterMobileConnectionMsg(in nsIMobileConnectionListener listener, > + in unsigned long clientId); As the conclusion we have made today, please move |clientId| to the first.
Attachment #810430 -
Flags: feedback?(echen)
Reporter | ||
Updated•11 years ago
|
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Reporter | ||
Comment 7•11 years ago
|
||
Comment on attachment 810430 [details] [diff] [review] Part 1: idl changes Review of attachment 810430 [details] [diff] [review]: ----------------------------------------------------------------- Towards the right direction! Also agree with Edgar's comment. :)
Attachment #810430 -
Flags: feedback?(htsai) → feedback+
Reporter | ||
Comment 8•11 years ago
|
||
Comment on attachment 810431 [details] [diff] [review] Part 2: dom changes Review of attachment 810431 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/src/MobileConnection.cpp @@ +81,5 @@ > MobileConnection::MobileConnection() > { > mProvider = do_GetService(NS_RILCONTENTHELPER_CONTRACTID); > mWindow = nullptr; > + mClientId = 0; Would be better to leave a comment here. @@ +101,5 @@ > mListener = new Listener(this); > > if (!CheckPermission("mobilenetwork") && > CheckPermission("mobileconnection")) { > + DebugOnly<nsresult> rv = mProvider->RegisterMobileConnectionMsg(mListener, mClientId); Taking care of the position of 'mClientId,' here and below.
Attachment #810431 -
Flags: feedback?(htsai) → feedback+
Reporter | ||
Comment 9•11 years ago
|
||
Comment on attachment 810433 [details] [diff] [review] Part 3: dom changes for BT Review of attachment 810433 [details] [diff] [review]: ----------------------------------------------------------------- Don't forget to move mClientId to the 1st place in your next version. Rest looks good to me. I also invite BT peers to make sure they are happy with these as well.
Attachment #810433 -
Flags: feedback?(htsai)
Attachment #810433 -
Flags: feedback?(gyeh)
Attachment #810433 -
Flags: feedback+
Reporter | ||
Comment 10•11 years ago
|
||
Comment on attachment 810436 [details] [diff] [review] Part 4: RIL implementation Review of attachment 810436 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for drafting the patch, and sorry that the scope of this bug isn't clear enough so that you would need to address icc and voicemail stuff. Also, as we are here touching mobileconnection API and RIL implementation, it looks a good chance for us to correct inconsistent coding style and namings. Please help rename option to options. In addition to the specific ones in [1] & [2], we should take care of other methods *Option(), e.g. setCallWaitingOption. Thank you! [1]:https://bugzilla.mozilla.org/show_bug.cgi?id=818393#c43 [2]:https://bugzilla.mozilla.org/show_bug.cgi?id=818393#c44 ::: dom/system/gonk/RILContentHelper.js @@ +563,5 @@ > * nsIMobileConnectionProvider > */ > > get iccInfo() { > + let context = this.getRilContext(0); //TODO: fixme TODO flag is followed by a bug number. Saying: //TODO: Bug 814637 - WebIccManager API: support multiple sim cards. @@ +568,5 @@ > return context && context.iccInfo; > }, > > + get cardState() { > + let context = this.getRilContext(0); //TODO: fixme TODO flag is followed by a bug number. Saying: //TODO: Bug 814637 - WebIccManager API: support multiple sim cards. @@ +690,5 @@ > }); > return request; > }, > > + setRoamingPreference: function setRoamingPreference(window, mode, clientId) { position of 'clientId' @@ +733,5 @@ > }); > return request; > }, > > + setVoicePrivacyMode: function setVoicePrivacyMode(window, enabled, clientId) { ditto. @@ +832,5 @@ > }); > return request; > }, > > + sendMMI: function sendMMI(window, mmi, clientId) { ditto. @@ +1057,5 @@ > > return request; > }, > > + getCallForwardingOption: function getCallForwardingOption(window, reason, clientId) { ditto. @@ +1082,5 @@ > > return request; > }, > > + setCallForwardingOption: function setCallForwardingOption(window, cfInfo, clientId) { ditto. @@ +1113,5 @@ > > return request; > }, > > + getCallBarringOption: function getCallBarringOption(window, option, clientId) { ditto. @@ +1140,5 @@ > }); > return request; > }, > > + setCallBarringOption: function setCallBarringOption(window, option, clientId) { ditto. @@ +1168,5 @@ > }); > return request; > }, > > + changeCallBarringPassword: function changeCallBarringPassword(window, info, clientId) { ditto. @@ +1211,5 @@ > > return request; > }, > > + setCallWaitingOption: function setCallWaitingOption(window, enabled, clientId) { ditto. @@ +1249,5 @@ > return request; > }, > > setCallingLineIdRestriction: > + function setCallingLineIdRestriction(window, clirMode, clientId) { ditto. @@ +1315,5 @@ > get voicemailDisplayName() { > return this.getVoicemailInfo().displayName; > }, > > + registerListener: function registerListener(listenerType, clientId, listener) { ditto. @@ +1329,5 @@ > listeners.push(listener); > if (DEBUG) debug("Registered " + listenerType + " listener: " + listener); > }, > > + unregisterListener: function unregisterListener(listenerType, clientId, listener) { ditto. @@ +1347,2 @@ > debug("Registering for mobile connection related messages"); > + this.registerListener("_mobileConnectionListeners", clientId, listener); ditto. @@ +1347,4 @@ > debug("Registering for mobile connection related messages"); > + this.registerListener("_mobileConnectionListeners", clientId, listener); > + cpmm.sendAsyncMessage("RIL:RegisterMobileConnectionMsg", > + {clientId: clientId}); nit: indention, make " and { align. @@ +1351,4 @@ > }, > > + unregisterMobileConnectionMsg: function unregisteMobileConnectionMsg(listener, clientId) { > + this.unregisterListener("_mobileConnectionListeners", clientId, listener); ditto. @@ +1355,5 @@ > }, > > registerVoicemailMsg: function registerVoicemailMsg(listener) { > debug("Registering for voicemail-related messages"); > + this.registerListener("_voicemailListeners", 0, listener); // TODO: fixme //TODO: Bug 814634 - WebVoicemail API: support multiple sim cards. @@ +1360,5 @@ > cpmm.sendAsyncMessage("RIL:RegisterVoicemailMsg"); > }, > > unregisterVoicemailMsg: function unregisteVoicemailMsg(listener) { > + this.unregisterListener("_voicemailListeners", 0, listener); // TODO: fixme ditto. @@ +1365,5 @@ > }, > > registerCellBroadcastMsg: function registerCellBroadcastMsg(listener) { > debug("Registering for Cell Broadcast related messages"); > + this.registerListener("_cellBroadcastListeners", 0, listener); // TODO: fixme Aha, we miss a bug for multisim cellbroadcast! Filed it! //TODO: Bug 921326 - Cellbroadcast API: support multiple sim cards @@ +1370,5 @@ > cpmm.sendAsyncMessage("RIL:RegisterCellBroadcastMsg"); > }, > > unregisterCellBroadcastMsg: function unregisterCellBroadcastMsg(listener) { > + this.unregisterListener("_cellBroadcastListeners", 0, listener); // TODO: fixme ditto. @@ +1375,5 @@ > }, > > registerIccMsg: function registerIccMsg(listener) { > debug("Registering for ICC related messages"); > + this.registerListener("_iccListeners", 0, listener); // TODO: fixme ditto. @@ +1380,5 @@ > cpmm.sendAsyncMessage("RIL:RegisterIccMsg"); > }, > > unregisterIccMsg: function unregisterIccMsg(listener) { > + this.unregisterListener("_iccListeners", 0, listener); // TODO: fixme ditto. @@ +1448,5 @@ > debug("Received message '" + msg.name + "': " + JSON.stringify(msg.json)); > switch (msg.name) { > case "RIL:CardStateChanged": { > let data = msg.json.data; > + if (this.rilContext[0].cardState != data.cardState) { //TODO: fixme We are fine using |msg.json.clientId| here. @@ +1450,5 @@ > case "RIL:CardStateChanged": { > let data = msg.json.data; > + if (this.rilContext[0].cardState != data.cardState) { //TODO: fixme > + this.rilContext[0].cardState = data.cardState; //TODO: fixme > + this._deliverEvent(0, //TODO: fixme ditto. In _deliverEvent(), if listener of clientId isn't found, the function returns immediately, no harm. @@ +1458,5 @@ > } > break; > } > case "RIL:IccInfoChanged": > + this.updateIccInfo(0, msg.json.data); //TODO: fixme ditto. @@ +1513,5 @@ > this.fireRequestSuccess(msg.json.requestId, result); > } else { > if (msg.json.rilMessageType == "iccSetCardLock" || > msg.json.rilMessageType == "iccUnlockCardLock") { > + this._deliverEvent(0, //TODO: fixme ditto. @@ +1542,5 @@ > case "RIL:CancelMMI": > this.handleSendCancelMMI(msg.json); > break; > case "RIL:StkCommand": > + this._deliverEvent(0, "_iccListeners", "notifyStkCommand", //TODO: fixme ditto. @@ +1547,4 @@ > [JSON.stringify(msg.json.data)]); > break; > case "RIL:StkSessionEnd": > + this._deliverEvent(0, "_iccListeners", "notifyStkSessionEnd", null); //TODO: fixme ditto. @@ +1611,5 @@ > this.handleSimpleRequest(msg.json.requestId, msg.json.errorMsg, null); > break; > case "RIL:CellBroadcastReceived": { > let message = new CellBroadcastMessage(msg.json.data); > + this._deliverEvent(0, //TODO: fixme ditto. @@ +1753,5 @@ > this.voicemailStatus.returnMessage = message.returnMessage; > } > > if (changed) { > + this._deliverEvent(0, // TODO: fixme ditto.
Attachment #810436 -
Flags: feedback?(htsai)
Reporter | ||
Comment 11•11 years ago
|
||
Comment on attachment 810436 [details] [diff] [review] Part 4: RIL implementation Review of attachment 810436 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RILContentHelper.js @@ +421,5 @@ > function RILContentHelper() { > + > + this.numClients = (function() { > + try { > + return Services.prefs.getIntPref("ril.numRadioInterfaces"); Here, let's only have one entry to get the number of clients, i.e. via nsIRadioInterfaceLayer.numRadioInterfaces.
Comment 12•11 years ago
|
||
Comment on attachment 810436 [details] [diff] [review] Part 4: RIL implementation Review of attachment 810436 [details] [diff] [review]: ----------------------------------------------------------------- Please see my below comment, thanks. ::: dom/system/gonk/RILContentHelper.js @@ +425,5 @@ > + return Services.prefs.getIntPref("ril.numRadioInterfaces"); > + } catch (e) { > + return 1; > + } > + })(); Please use |RadioInterfaceLayer.numRadioInterfaces| instead of accessing preference directly. @@ +545,1 @@ > }; I think there are some problems here for multi-sim. We override |getRilContext| immediately once it is been called. In multi-sim situation, it may cause other client does not send synced message to get RilContext, but access local rilContext directly. @@ +560,5 @@ > }, > > /** > * nsIMobileConnectionProvider > */ Could you help on the comment as well? |iccInfo| and |cardState| now is in nsIIccProvider, thanks. @@ +593,5 @@ > * This helps ensure that only one network is selected at a time. > */ > _selectingNetwork: null, > > + getNetworks: function getNetworks(window, clientId) { Do forget the order of |clientId| :). @@ +1323,1 @@ > } I think we may need to have more check here. Because if the listenerType, 'foo', does not register yet before, the this['foo'][|clientId|] will throw error. @@ +1335,3 @@ > if (!listeners) { > return; > } ditto @@ +1864,3 @@ > if (!thisListeners) { > return; > } Same as above, I think we may need to have more check here.
Attachment #810436 -
Flags: feedback?(echen) → feedback?
Updated•11 years ago
|
Attachment #810436 -
Flags: feedback?
Comment 13•11 years ago
|
||
Comment on attachment 810436 [details] [diff] [review] Part 4: RIL implementation Review of attachment 810436 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for comment #12 does not clear enough. Please see my below update. Thanks :) ::: dom/system/gonk/RILContentHelper.js @@ +1321,2 @@ > if (!listeners) { > + listeners = this[listenerType][clientId] = []; I think we may need to have more check here. Because if the listenerType, 'foo', does not register yet before, the this['foo'][|clientId|] will throw error. @@ +1335,2 @@ > if (!listeners) { > return; ditto @@ +1864,2 @@ > if (!thisListeners) { > return; Same as above, I think we may need to have more check here.
Comment 14•11 years ago
|
||
Comment on attachment 810431 [details] [diff] [review] Part 2: dom changes Review of attachment 810431 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thanks.
Attachment #810431 -
Flags: feedback?(echen) → feedback+
Comment 15•11 years ago
|
||
Comment on attachment 810433 [details] [diff] [review] Part 3: dom changes for BT Review of attachment 810433 [details] [diff] [review]: ----------------------------------------------------------------- Please see my below comments, reset part looks good to me, thanks. :) ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +544,5 @@ > { > nsCOMPtr<nsIMobileConnectionProvider> connection = > do_GetService(NS_RILCONTENTHELPER_CONTRACTID); > NS_ENSURE_TRUE_VOID(connection); > + uint32_t mClientId = 0; I would suggest add a TODO comment here. ::: dom/bluetooth/BluetoothRilListener.cpp @@ +234,5 @@ > mIccListener = new IccListener(); > mMobileConnectionListener = new MobileConnectionListener(); > mTelephonyListener = new TelephonyListener(); > + > + mClientId = 0; ditto
Attachment #810433 -
Flags: feedback?(echen) → feedback+
Comment 16•11 years ago
|
||
BTW, after grep the source code, I found nsIMobileConnecionProvider is been used in some other modules, like dom/phonenumberutils/PhoneNumberUtils.jsm dom/push/src/PushService.jsm dom/apps/src/OperatorApps.jsm Some of them even does not use nsIMobileConnectionProvider correctly, for example, trying to access iccInfo from nsIMobileConnectionProvider [1][2], but actually iccInfo was already been moved to nsIIccProvider (I will file a bug for that). In PhoneNumberUtils.jsm, it uses nsIMobileConnectionProvider to get voiceConnectionInfo [3]. Please help to take care of this in this bug as well, thank you. [1] http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/OperatorApps.jsm#93 [2] http://mxr.mozilla.org/mozilla-central/source/dom/push/src/PushService.jsm#1489 [3] http://mxr.mozilla.org/mozilla-central/source/dom/phonenumberutils/PhoneNumberUtils.jsm#44
Assignee | ||
Comment 17•11 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #10) > Comment on attachment 810436 [details] [diff] [review] > Part 4: RIL implementation > > Review of attachment 810436 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for drafting the patch, and sorry that the scope of this bug isn't > clear enough so that you would need to address icc and voicemail stuff. > > Also, as we are here touching mobileconnection API and RIL implementation, > it looks a good chance for us to correct inconsistent coding style and > namings. Please help rename option to options. In addition to the specific > ones in [1] & [2], we should take care of other methods *Option(), e.g. > setCallWaitingOption. Thank you! > > [1]:https://bugzilla.mozilla.org/show_bug.cgi?id=818393#c43 > [2]:https://bugzilla.mozilla.org/show_bug.cgi?id=818393#c44 > (comments omitted...) Thanks a lot for the review effort! Will add bug number to all of the TODOs and do the renaming stuff. Just one thing about _deliverEvent(), if listener of clientId isn't found, the function returns immediately so listeners will not receive the notifications. This will cause some of the tests to fail... Is it okay if I leave it with 0 for now?
Assignee | ||
Comment 18•11 years ago
|
||
Hsinyi, My last comment is not quite right. The reason why msg.json.clientId can not be found is because RadioInterfaceLayer sendWithIPCMessage() [1] only sends the content of "msg.json.data" to RILContentHelper, so clientId is missing. This affects all the functions that use sendWithIPCMessage(). I will file another bug for this. [1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#638
Reporter | ||
Comment 19•11 years ago
|
||
Comment on attachment 810436 [details] [diff] [review] Part 4: RIL implementation Review of attachment 810436 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RILContentHelper.js @@ +421,5 @@ > function RILContentHelper() { > + > + this.numClients = (function() { > + try { > + return Services.prefs.getIntPref("ril.numRadioInterfaces"); If we are in content process, use 'sendSyncMessage("RIL:GetNumRadioInterfaces")' to get numRadioInterfaces. Be careful that we need to add this IPC message to all the permission lists, otherwise the process might be killed in permission checking. If we are in chrome process, we should use 'getService(Ci.nsIRadioInterfaceLayer).numRadioInterfaces.' We could use the following to know the process type: this._inChild = Cc["@mozilla.org/xreappinfo;1"].getService(Ci.nsIXULRuntime) .processType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;
Comment 20•11 years ago
|
||
Comment on attachment 810433 [details] [diff] [review] Part 3: dom changes for BT Review of attachment 810433 [details] [diff] [review]: ----------------------------------------------------------------- Please take a look at my comment below. Thanks. ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +544,5 @@ > { > nsCOMPtr<nsIMobileConnectionProvider> connection = > do_GetService(NS_RILCONTENTHELPER_CONTRACTID); > NS_ENSURE_TRUE_VOID(connection); > + uint32_t mClientId = 0; since it's a local variable, I would recommend that the variable name shouldn't start with 'm'.
Attachment #810433 -
Flags: feedback?(gyeh) → feedback+
Reporter | ||
Comment 21•11 years ago
|
||
(In reply to Hsin-Yi Tsai (OOO 10/2 ~ 10/13) [:hsinyi] from comment #19) > Comment on attachment 810436 [details] [diff] [review] > Part 4: RIL implementation > > Review of attachment 810436 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/RILContentHelper.js > @@ +421,5 @@ > > function RILContentHelper() { > > + > > + this.numClients = (function() { > > + try { > > + return Services.prefs.getIntPref("ril.numRadioInterfaces"); > > If we are in content process, use > 'sendSyncMessage("RIL:GetNumRadioInterfaces")' to get numRadioInterfaces. Be > careful that we need to add this IPC message to all the permission lists, > otherwise the process might be killed in permission checking. > > If we are in chrome process, we should use > 'getService(Ci.nsIRadioInterfaceLayer).numRadioInterfaces.' > > We could use the following to know the process type: > > this._inChild = Cc["@mozilla.org/xreappinfo;1"].getService(Ci.nsIXULRuntime) > .processType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT; I have a patch in bug 842239 for getting the number of radio interfaces so that voicemail, mobileconnection related bugs benefit. :)
Assignee | ||
Comment 22•11 years ago
|
||
(In reply to Hsin-Yi Tsai (OOO 10/2 ~ 10/13) [:hsinyi] from comment #21) > (In reply to Hsin-Yi Tsai (OOO 10/2 ~ 10/13) [:hsinyi] from comment #19) > > Comment on attachment 810436 [details] [diff] [review] > > Part 4: RIL implementation > > > > Review of attachment 810436 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: dom/system/gonk/RILContentHelper.js > > @@ +421,5 @@ > > > function RILContentHelper() { > > > + > > > + this.numClients = (function() { > > > + try { > > > + return Services.prefs.getIntPref("ril.numRadioInterfaces"); > > > > If we are in content process, use > > 'sendSyncMessage("RIL:GetNumRadioInterfaces")' to get numRadioInterfaces. Be > > careful that we need to add this IPC message to all the permission lists, > > otherwise the process might be killed in permission checking. > > > > If we are in chrome process, we should use > > 'getService(Ci.nsIRadioInterfaceLayer).numRadioInterfaces.' > > > > We could use the following to know the process type: > > > > this._inChild = Cc["@mozilla.org/xreappinfo;1"].getService(Ci.nsIXULRuntime) > > .processType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT; > > I have a patch in bug 842239 for getting the number of radio interfaces so > that voicemail, mobileconnection related bugs benefit. :) Thank you Hsin-Yi, works like a charm! :)
Assignee | ||
Comment 23•11 years ago
|
||
Address comments in Comment 6 and Comment 10: 1. add getter function for iccId 2. move clientId order 3. Rename option to options and *Option() to Options() Since we are also renaming web APIs, should file another bug for handling gaia changes? or should we do it here?
Attachment #810430 -
Attachment is obsolete: true
Attachment #815793 -
Flags: review?(htsai)
Assignee | ||
Comment 24•11 years ago
|
||
Address comments in Comment 5: 1. Leave TODO comment with bug number 2. move clientId order
Attachment #810431 -
Attachment is obsolete: true
Attachment #815795 -
Flags: review?(htsai)
Assignee | ||
Comment 25•11 years ago
|
||
Address comments in Comment 15 and Comment 20: 1. Change local variable name to one that does not start with "m" 2. Add TODO comment with bug number
Attachment #810433 -
Attachment is obsolete: true
Attachment #815796 -
Flags: review?(htsai)
Assignee | ||
Comment 26•11 years ago
|
||
Address comments in Comment 10, Comment 11 and Comment 12: 1. use number of clients from RadioInterfaceLayer 2. fix getRilContext() for multi-sim 3. add TODO comments with bug number 4. check for listenerType before using it 5. Use msg.json.clientId wherever possible 6. Rename option to options and *Option() to Options() 7. Move clientId order
Attachment #810436 -
Attachment is obsolete: true
Attachment #815801 -
Flags: review?(htsai)
Assignee | ||
Comment 27•11 years ago
|
||
Since we have done some renaming, we should modify the test scripts as well.
Attachment #815802 -
Flags: review?(htsai)
Assignee | ||
Comment 28•11 years ago
|
||
Hsinyi, Edgar, Thank you so much for the review effort! :) Since we have renamed some web APIs, should file another bug for handling gaia changes? or should we do it here? And for PhoneNumberUtils mentioned in Comment 16, should we insert a default index (0) for now and file another bug for owner to take care of it properly? Or any other suggestion?
Assignee | ||
Updated•11 years ago
|
Attachment #815793 -
Flags: review?(echen)
Assignee | ||
Updated•11 years ago
|
Attachment #815795 -
Flags: review?(echen)
Assignee | ||
Updated•11 years ago
|
Attachment #815796 -
Flags: review?(echen)
Assignee | ||
Updated•11 years ago
|
Attachment #815801 -
Flags: review?(echen)
Reporter | ||
Comment 29•11 years ago
|
||
(In reply to Jessica Jong [:jjong] [:jessica] from comment #28) > Hsinyi, Edgar, > > Thank you so much for the review effort! :) > Since we have renamed some web APIs, should file another bug for handling > gaia changes? or should we do it here? > And for PhoneNumberUtils mentioned in Comment 16, should we insert a default > index (0) for now and file another bug for owner to take care of it > properly? Or any other suggestion? This bug touches internal architecture, not supposed to influence Gaia. Nevertheless, I filed Bug 926169 for addressing gaia modification due to DSDS MobileConnection API changes.
Assignee | ||
Comment 30•11 years ago
|
||
(In reply to Hsin-Yi Tsai (OOO 10/2 ~ 10/13) [:hsinyi] from comment #29) > (In reply to Jessica Jong [:jjong] [:jessica] from comment #28) > > Hsinyi, Edgar, > > > > Thank you so much for the review effort! :) > > Since we have renamed some web APIs, should file another bug for handling > > gaia changes? or should we do it here? > > And for PhoneNumberUtils mentioned in Comment 16, should we insert a default > > index (0) for now and file another bug for owner to take care of it > > properly? Or any other suggestion? > > This bug touches internal architecture, not supposed to influence Gaia. > Nevertheless, I filed Bug 926169 for addressing gaia modification due to > DSDS MobileConnection API changes. Hsinyi, Sorry, I wasn't clear enough. DSDS modifications in this bug do not affect Gaia. What affect Gaia are the renamings from option to options/*Option() to *Options().
Assignee | ||
Comment 31•11 years ago
|
||
Comment on attachment 815793 [details] [diff] [review] Part 1: idl changes (v2) This idl part is fine, I just forgot to change dictionary MozCallBarringOption to MozCallBarringOptions, will complete it when I get to the office tomorrow.
Attachment #815793 -
Flags: review?(htsai)
Attachment #815793 -
Flags: review?(echen)
Comment 32•11 years ago
|
||
Comment on attachment 815793 [details] [diff] [review] Part 1: idl changes (v2) Review of attachment 815793 [details] [diff] [review]: ----------------------------------------------------------------- I will suggest to move the naming changes of web api to bug 814629 and keep this bug only affects the internal interface. Then we could address the gaia changes in bug 926169 together. ::: dom/network/interfaces/nsIDOMMobileConnection.idl @@ +71,5 @@ > > /** > + * The Integrated Circuit Card Identifier of this mobile connection. > + */ > + readonly attribute DOMString iccId; Move to bug 814629 as well and keep this bug only for the internal interface, thank you. ::: dom/network/interfaces/nsIMobileConnectionProvider.idl @@ +53,3 @@ > > + nsIDOMDOMRequest getNetworks(in nsIDOMWindow window, > + in unsigned long clientId); According to the last discussion, please move clientId to the first parameter, here and below, thank you.
Comment 33•11 years ago
|
||
Comment on attachment 815795 [details] [diff] [review] Part 2: dom changes (v2) Review of attachment 815795 [details] [diff] [review]: ----------------------------------------------------------------- Please see my below comments. Other parts looks good to me, thank you. ::: dom/network/src/MobileConnection.cpp @@ +224,5 @@ > if (!mProvider) { > return NS_ERROR_FAILURE; > } > > + return mProvider->GetNetworks(GetOwner(), mClientId, request); Don't forget to move mClientId to the first parameter, here and below, thank you. @@ +354,4 @@ > } > > NS_IMETHODIMP > +MobileConnection::GetCallForwardingOptions(uint16_t aReason, Please move the changes for webapi to bug 814629, thank you.
Attachment #815795 -
Flags: review?(echen)
Comment 34•11 years ago
|
||
Comment on attachment 815796 [details] [diff] [review] Part 3: dom changes for BT (v2) Review of attachment 815796 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, but we need BT peers to review this, forward r? to BT peers, thanks.
Attachment #815796 -
Flags: review?(echen) → review?(gyeh)
Comment on attachment 815795 [details] [diff] [review] Part 2: dom changes (v2) Review of attachment 815795 [details] [diff] [review]: ----------------------------------------------------------------- Please ask r? also for a DOM-peer, we've been asked by a DOM-peer why those DOM changes didn't go through them.
Reporter | ||
Comment 36•11 years ago
|
||
(In reply to Edgar Chen [:edgar][:echen] from comment #32) > Comment on attachment 815793 [details] [diff] [review] > Part 1: idl changes (v2) > > Review of attachment 815793 [details] [diff] [review]: > ----------------------------------------------------------------- > > I will suggest to move the naming changes of web api to bug 814629 and keep > this bug only affects the internal interface. Then we could address the gaia > changes in bug 926169 together. I am fine here since it could reduce some gaia noises. :) > > ::: dom/network/interfaces/nsIDOMMobileConnection.idl > @@ +71,5 @@ > > > > /** > > + * The Integrated Circuit Card Identifier of this mobile connection. > > + */ > > + readonly attribute DOMString iccId; > > Move to bug 814629 as well and keep this bug only for the internal > interface, thank you. > > ::: dom/network/interfaces/nsIMobileConnectionProvider.idl > @@ +53,3 @@ > > > > + nsIDOMDOMRequest getNetworks(in nsIDOMWindow window, > > + in unsigned long clientId); > > According to the last discussion, please move clientId to the first > parameter, here and below, thank you.
Reporter | ||
Comment 37•11 years ago
|
||
Comment on attachment 815795 [details] [diff] [review] Part 2: dom changes (v2) Review of attachment 815795 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/src/MobileConnection.cpp @@ +190,5 @@ > + return mProvider->GetDataConnectionInfo(mClientId, data); > +} > + > +NS_IMETHODIMP > +MobileConnection::GetIccId(nsAString& iccId) Do this in bug 814629, thank you. @@ +224,5 @@ > if (!mProvider) { > return NS_ERROR_FAILURE; > } > > + return mProvider->GetNetworks(GetOwner(), mClientId, request); Please take care of the position of 'mClientId' here and below as our offline discussion.
Attachment #815795 -
Flags: review?(htsai)
Reporter | ||
Comment 38•11 years ago
|
||
Comment on attachment 815796 [details] [diff] [review] Part 3: dom changes for BT (v2) Review of attachment 815796 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothRilListener.cpp @@ +235,5 @@ > mMobileConnectionListener = new MobileConnectionListener(); > mTelephonyListener = new TelephonyListener(); > + > + // TODO: Bug 921991 - B2G BT: support multiple sim cards > + mClientId = 0; I am not sure how the architecture of BluetoothRilListener in multisim will be. Will there be several objects (as MobileConnections/Icc) or only one (as Telephony)? If the former, it could be fine to add a private member 'mClientId' but be sure to take the different structure of Telephony into account. I would suggest removing this private member in this patch, and leave the decision to BT developers in a BT follow-up bug. r? Gina! @@ +299,5 @@ > do_GetService(NS_RILCONTENTHELPER_CONTRACTID); > NS_ENSURE_TRUE(provider, false); > > nsresult rv = provider-> > + RegisterMobileConnectionMsg(mClientId, mMobileConnectionListener); I would suggest: // TODO: Bug 921991 - B2G BT: support multiple sim cards RegisterMobileConnectionMsg(0, mMobileConnectionListener); @@ +311,5 @@ > do_GetService(NS_RILCONTENTHELPER_CONTRACTID); > NS_ENSURE_TRUE(provider, false); > > nsresult rv = provider-> > + UnregisterMobileConnectionMsg(mClientId, mMobileConnectionListener); ditto.
Attachment #815796 -
Flags: review?(htsai) → review?(gyeh)
Comment 39•11 years ago
|
||
Comment on attachment 815801 [details] [diff] [review] Part 4: RIL implementation (v2) Review of attachment 815801 [details] [diff] [review]: ----------------------------------------------------------------- Please see my below comments and remember to move |clientId| to first parameter for all nsIMboileConnectionProvider interface. Thank you. :) ::: dom/system/gonk/RILContentHelper.js @@ +528,5 @@ > return; > } > > // If iccInfo is null, new corresponding object based on iccType. > + if (!this.rilContext[clientId].iccInfo) { You could consider to add a local variable to cache rilContext. let rilContext = this.rilContext[clientId]; Then you don't need to have array accessing every time. @@ +614,5 @@ > * This helps ensure that only one network is selected at a time. > */ > _selectingNetwork: null, > > + getNetworks: function getNetworks(window, clientId) { Please move |clientId| to the first parameter for all nsIMobileConnecitonProvier interface. @@ +632,5 @@ > }); > return request; > }, > > + selectNetwork: function selectNetwork(window, clientId, network) { Ditto. @@ +638,5 @@ > throw Components.Exception("Can't get window object", > Cr.NS_ERROR_UNEXPECTED); > } > > if (this._selectingNetwork) { We need to handle |_selectingNetwork| for multiple SIM scenario. @@ +687,5 @@ > throw Components.Exception("Can't get window object", > Cr.NS_ERROR_UNEXPECTED); > } > > if (this._selectingNetwork) { Ditto. @@ +1143,5 @@ > let request = Services.DOMRequest.createRequest(window); > let requestId = this.getRequestId(request); > > + if (DEBUG) debug("getCallBarringOptions: " + JSON.stringify(options)); > + if (!this._isValidCallBarringOption(options)) { Please also help to correct the naming for _isValidCallBarringOption as well. s/_isValidCallBarringOption/_isValidCallBarringOptions thank you. @@ +1170,5 @@ > let request = Services.DOMRequest.createRequest(window); > let requestId = this.getRequestId(request); > > + if (DEBUG) debug("setCallBarringOptions: " + JSON.stringify(options)); > + if (!this._isValidCallBarringOption(options, true)) { Ditto. @@ +1483,5 @@ > switch (msg.name) { > case "RIL:CardStateChanged": > + if (this.rilContext[msg.json.clientId].cardState != data.cardState) { > + this.rilContext[msg.json.clientId].cardState = data.cardState; > + this._deliverEvent(msg.json.clientId, It seems clientId will be used for almost all message, I suggest to add a local variable to cache the |msg.json.clientId|. (Just like what we did for |msg.json.data|) let clientId = msg.json.clientId; Then we can use |clientId| here and below. @@ +1551,1 @@ > "notifyIccCardLockError", IccCardLockError event was already been removed in bug 873380. You won't need to have this change after rebase. :) @@ +1701,1 @@ > this._selectingNetwork = null; Handle |_selectingNetwork| for multiple SIM scenario. :)
Attachment #815801 -
Flags: review?(echen)
Reporter | ||
Comment 40•11 years ago
|
||
Comment on attachment 815801 [details] [diff] [review] Part 4: RIL implementation (v2) Review of attachment 815801 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RILContentHelper.js @@ +525,3 @@ > // Card is not detected, clear iccInfo to null. > if (!newInfo || !newInfo.iccType) { > + this.rilContext[clientId].iccInfo = null; How about a new local variable for this.rilContext[clientId]? @@ +612,5 @@ > /** > * The network that is currently trying to be selected (or "automatic"). > * This helps ensure that only one network is selected at a time. > */ > _selectingNetwork: null, We need _selectingNework[] for multi-sims. @@ +1143,5 @@ > let request = Services.DOMRequest.createRequest(window); > let requestId = this.getRequestId(request); > > + if (DEBUG) debug("getCallBarringOptions: " + JSON.stringify(options)); > + if (!this._isValidCallBarringOption(options)) { this._isValidCallBarringOptions @@ +1170,5 @@ > let request = Services.DOMRequest.createRequest(window); > let requestId = this.getRequestId(request); > > + if (DEBUG) debug("setCallBarringOptions: " + JSON.stringify(options)); > + if (!this._isValidCallBarringOption(options, true)) { this._isValidCallBarringOptions @@ +1546,5 @@ > } else { > if (data.rilMessageType == "iccSetCardLock" || > data.rilMessageType == "iccUnlockCardLock") { > + this._deliverEvent(msg.json.clientId, > + "_iccListeners", Please rebase on the latest m-c. You will find we don't need this anymore. :) @@ +1809,4 @@ > if (!message.success) { > this.fireRequestError(message.requestId, message.errorMsg); > } else { > let option = new CallBarringOption(message); let options = new CallBarringOptions(...)
Attachment #815801 -
Flags: review?(htsai)
Reporter | ||
Comment 41•11 years ago
|
||
Comment on attachment 815802 [details] [diff] [review] Part 5: test scripts Review of attachment 815802 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but please help move these to bug 814629. Sorry about that :(
Attachment #815802 -
Flags: review?(htsai)
Comment 42•11 years ago
|
||
Comment on attachment 815801 [details] [diff] [review] Part 4: RIL implementation (v2) Review of attachment 815801 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RILContentHelper.js @@ +858,3 @@ > // We need to save the global window to get the proper MMIError > // constructor once we get the reply from the parent process. > this._window = window; BTW, I found there is a potential issue here. Actually this potential issue is not only existed in multi-sim scenario, but also single sim. We use |_window| to cache the context window of requester. But if there are two requests come at the same time, the |_window| will be override by latter one. We should use |_windowsMap| to cache window, just like what we did in CardLock related function. I will file a bug for this. Thanks
Assignee | ||
Comment 43•11 years ago
|
||
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #35) > Comment on attachment 815795 [details] [diff] [review] > Part 2: dom changes (v2) > > Review of attachment 815795 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please ask r? also for a DOM-peer, > we've been asked by a DOM-peer why those DOM changes didn't go through them. Thanks for the reminder. Is it okay if I ask r? for DOM-peer after Hsin-Yi is comfortable with the patches?
(In reply to Jessica Jong [:jjong] [:jessica] from comment #43) > > Please ask r? also for a DOM-peer, > > we've been asked by a DOM-peer why those DOM changes didn't go through them. > > Thanks for the reminder. > Is it okay if I ask r? for DOM-peer after Hsin-Yi is comfortable with the > patches? Hsinyi isn't a DOM-peer either https://wiki.mozilla.org/Modules/Core#Document_Object_Model Unless you got some confirmation from a DOM-peer through IRC or f2f talk she could help to review that, then please note it here on Bugzilla. Othewise she cannot review it. But it's okay you ask feedback? from her. Mozilla does have a strict policy for reviewing. Even you try to modify the build files in dom/system/gonk, that changes should go through Build-peer, not a RIL-peer. We have been asked several times why some patches are reviewed by a non-Peer/non-Owner.
Assignee | ||
Comment 45•11 years ago
|
||
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #44) > (In reply to Jessica Jong [:jjong] [:jessica] from comment #43) > > > Please ask r? also for a DOM-peer, > > > we've been asked by a DOM-peer why those DOM changes didn't go through them. > > > > Thanks for the reminder. > > Is it okay if I ask r? for DOM-peer after Hsin-Yi is comfortable with the > > patches? > > Hsinyi isn't a DOM-peer either > https://wiki.mozilla.org/Modules/Core#Document_Object_Model > Unless you got some confirmation from a DOM-peer through IRC or f2f talk she > could help to review that, then please note it here on Bugzilla. > Othewise she cannot review it. > But it's okay you ask feedback? from her. > > Mozilla does have a strict policy for reviewing. > Even you try to modify the build files in dom/system/gonk, > that changes should go through Build-peer, not a RIL-peer. > > We have been asked several times why some patches are reviewed by a > non-Peer/non-Owner. Got it! Thanks again for the reminder.
Assignee | ||
Comment 46•11 years ago
|
||
Address comments in Comment 32: 1. No web api changes in this bug 2. Move clientId as the first parameter
Attachment #815793 -
Attachment is obsolete: true
Attachment #816979 -
Flags: feedback?(htsai)
Attachment #816979 -
Flags: feedback?(echen)
Assignee | ||
Comment 47•11 years ago
|
||
Address comments in Comment 33 and Comment 37: 1. Move iccid getter function to bug 814629 2. Move clientId as the first parameter
Attachment #815795 -
Attachment is obsolete: true
Attachment #816981 -
Flags: feedback?(htsai)
Attachment #816981 -
Flags: feedback?(echen)
Assignee | ||
Comment 48•11 years ago
|
||
Attachment #815801 -
Attachment is obsolete: true
Attachment #816987 -
Flags: feedback?(echen)
Assignee | ||
Comment 49•11 years ago
|
||
Address comments in Comment 39 and Comment 40: 1. Move clientId as the first parameter 2. Use local variable in updateIccInfo() 3. Handle _selectingNetwork for multi-SIM scenario 4. Use local variable for msg.json.clientId in receiveMessage() 5. Other renamings from option to options / *Option() to *Options()
Attachment #815802 -
Attachment is obsolete: true
Attachment #816991 -
Flags: review?(htsai)
Attachment #816991 -
Flags: review?(echen)
Reporter | ||
Comment 50•11 years ago
|
||
Comment on attachment 816979 [details] [diff] [review] Part 1: idl changes (v3) Review of attachment 816979 [details] [diff] [review]: ----------------------------------------------------------------- This looks great and it deserves r+ :)
Attachment #816979 -
Flags: feedback?(htsai) → review+
Reporter | ||
Comment 51•11 years ago
|
||
Comment on attachment 816981 [details] [diff] [review] Part 2: dom changes (v3) Review of attachment 816981 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.
Attachment #816981 -
Flags: feedback?(htsai) → feedback+
Reporter | ||
Comment 52•11 years ago
|
||
Comment on attachment 816991 [details] [diff] [review] Part 5: RIL implementation (v3) Review of attachment 816991 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thank you. ::: dom/system/gonk/RILContentHelper.js @@ +631,5 @@ > /** > * The network that is currently trying to be selected (or "automatic"). > * This helps ensure that only one network is selected at a time. > */ > _selectingNetwork: null, _selectingNetworks & please change the comment a little bit due to renaming.
Attachment #816991 -
Flags: review?(htsai) → review+
Comment 53•11 years ago
|
||
Comment on attachment 816979 [details] [diff] [review] Part 1: idl changes (v3) Review of attachment 816979 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thank you.
Attachment #816979 -
Flags: feedback?(echen) → feedback+
Updated•11 years ago
|
Attachment #816981 -
Flags: feedback?(echen) → feedback+
Comment 54•11 years ago
|
||
Comment on attachment 816991 [details] [diff] [review] Part 5: RIL implementation (v3) Review of attachment 816991 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, f+. (I am not a RIL peer, so I can not review :))
Attachment #816991 -
Flags: review?(echen) → feedback+
Comment 55•11 years ago
|
||
Comment on attachment 816987 [details] [diff] [review] Part 4: dom changes for PhoneNumberUtils (v1) Review of attachment 816987 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, and I would like to have Gregor's feedback as well. Hi Gregor, nsIMobileConnectionProvider is changed to support multi-sim, in this patch we always use the first client in PhoneNumberUtils to be backward compatible with signal sim. And we file a follow up, bug 926740, for multi-sim support in PhoneNumberUtils.
Attachment #816987 -
Flags: feedback?(echen)
Attachment #816987 -
Flags: feedback?(anygregor)
Attachment #816987 -
Flags: feedback+
Comment 56•11 years ago
|
||
Comment on attachment 816987 [details] [diff] [review] Part 4: dom changes for PhoneNumberUtils (v1) Review of attachment 816987 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me!
Attachment #816987 -
Flags: feedback?(anygregor) → feedback+
Please update the title if you'd like to call it client Id instead of subscription Id.
Assignee | ||
Updated•11 years ago
|
Summary: B2G Multi-SIM: mobileconnection - add subscription id in nsIMobileConnectionProvider → B2G Multi-SIM: mobileconnection - add client id in nsIMobileConnectionProvider
Assignee | ||
Comment 58•11 years ago
|
||
Comment on attachment 816981 [details] [diff] [review] Part 2: dom changes (v3) Olli, Can you please help take a look? Thanks!
Attachment #816981 -
Flags: review?(bugs)
Assignee | ||
Comment 59•11 years ago
|
||
Comment on attachment 816987 [details] [diff] [review] Part 4: dom changes for PhoneNumberUtils (v1) Gregor, Need you to review it as well. Thanks!
Attachment #816987 -
Flags: review?(anygregor)
Updated•11 years ago
|
Attachment #816987 -
Flags: review?(anygregor) → review+
Updated•11 years ago
|
Attachment #816981 -
Flags: review?(bugs) → review+
Comment 60•11 years ago
|
||
Comment on attachment 815796 [details] [diff] [review] Part 3: dom changes for BT (v2) Review of attachment 815796 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the late reply. Agree with Hsin-yi. I think we might need a new structure to keep these information for multi-sim. It would be great to land this patch first, and we'll file a follow-up for BT to handle these cases. Thanks.
Attachment #815796 -
Flags: review?(gyeh)
Attachment #815796 -
Flags: review+
Comment 61•11 years ago
|
||
Comment on attachment 815796 [details] [diff] [review] Part 3: dom changes for BT (v2) Review of attachment 815796 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothRilListener.cpp @@ +235,5 @@ > mMobileConnectionListener = new MobileConnectionListener(); > mTelephonyListener = new TelephonyListener(); > + > + // TODO: Bug 921991 - B2G BT: support multiple sim cards > + mClientId = 0; Agree with Hsin-yi. I'll file a follow-up for BT. Thanks. ::: dom/bluetooth/BluetoothRilListener.h @@ +40,5 @@ > nsCOMPtr<nsIIccListener> mIccListener; > nsCOMPtr<nsIMobileConnectionListener> mMobileConnectionListener; > nsCOMPtr<nsITelephonyListener> mTelephonyListener; > + > + uint32_t mClientId; Please remove it.
Assignee | ||
Comment 62•11 years ago
|
||
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #61) > Comment on attachment 815796 [details] [diff] [review] > Part 3: dom changes for BT (v2) > > Review of attachment 815796 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/bluetooth/BluetoothRilListener.cpp > @@ +235,5 @@ > > mMobileConnectionListener = new MobileConnectionListener(); > > mTelephonyListener = new TelephonyListener(); > > + > > + // TODO: Bug 921991 - B2G BT: support multiple sim cards > > + mClientId = 0; > > Agree with Hsin-yi. I'll file a follow-up for BT. Thanks. > > ::: dom/bluetooth/BluetoothRilListener.h > @@ +40,5 @@ > > nsCOMPtr<nsIIccListener> mIccListener; > > nsCOMPtr<nsIMobileConnectionListener> mMobileConnectionListener; > > nsCOMPtr<nsITelephonyListener> mTelephonyListener; > > + > > + uint32_t mClientId; > > Please remove it. Thank you, Gina! As discussed, I will mark with Bug 921991 for now.
Assignee | ||
Comment 63•11 years ago
|
||
Add f=edgar r=hsinyi after f+/r+.
Attachment #816979 -
Attachment is obsolete: true
Attachment #818341 -
Flags: review+
Assignee | ||
Comment 64•11 years ago
|
||
Add f=hsinyi,edgar r=smaug after f+/r+.
Attachment #816981 -
Attachment is obsolete: true
Attachment #818342 -
Flags: review+
Assignee | ||
Comment 65•11 years ago
|
||
1. Remove private members in the patch as stated in Comment 38 and Comment 61. 2. Add r=gyeh after r+
Attachment #815796 -
Attachment is obsolete: true
Attachment #818345 -
Flags: review+
Assignee | ||
Comment 66•11 years ago
|
||
Add f=edgar r=gwagner after f+/r+.
Attachment #816987 -
Attachment is obsolete: true
Attachment #818348 -
Flags: review+
Assignee | ||
Comment 67•11 years ago
|
||
1. Replace _selectingNetwork with _selectingNetworks and adjust comment, as suggested in Comment 52. 2. Add f=edgar r=hsinyi after f+/r+.
Attachment #816991 -
Attachment is obsolete: true
Attachment #818350 -
Flags: review+
Assignee | ||
Comment 68•11 years ago
|
||
Try results: https://tbpl.mozilla.org/?tree=Try&rev=dc2970772896
Updated•11 years ago
|
blocking-b2g: --- → 1.3+
Assignee | ||
Updated•11 years ago
|
Target Milestone: --- → 1.3 Sprint 3 - 10/25
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 69•11 years ago
|
||
Jessica, Patches for part 3 and part 5 cannot be cleanly applied to b2g-inbound. Please provide new ones based the latest b2g-inbound for checking in. Thanks!
Assignee | ||
Comment 70•11 years ago
|
||
rebased.
Attachment #818345 -
Attachment is obsolete: true
Attachment #818861 -
Flags: review+
Assignee | ||
Comment 71•11 years ago
|
||
rebased.
Attachment #818350 -
Attachment is obsolete: true
Attachment #818862 -
Flags: review+
Assignee | ||
Comment 72•11 years ago
|
||
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #69) > Jessica, > > Patches for part 3 and part 5 cannot be cleanly applied to b2g-inbound. > Please provide new ones based the latest b2g-inbound for checking in. Thanks! Sorry, it should be fine now. Thank you!
Reporter | ||
Comment 73•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/32307c4a20bc https://hg.mozilla.org/integration/b2g-inbound/rev/c6a0d5f696ec https://hg.mozilla.org/integration/b2g-inbound/rev/033a0cdb8c03 https://hg.mozilla.org/integration/b2g-inbound/rev/3867f316204e https://hg.mozilla.org/integration/b2g-inbound/rev/e7a4fe2d9e5e
Keywords: checkin-needed
Comment 74•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/32307c4a20bc https://hg.mozilla.org/mozilla-central/rev/c6a0d5f696ec https://hg.mozilla.org/mozilla-central/rev/033a0cdb8c03 https://hg.mozilla.org/mozilla-central/rev/3867f316204e https://hg.mozilla.org/mozilla-central/rev/e7a4fe2d9e5e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 75•11 years ago
|
||
The patches in this bug seem to be causing bug 929815. Since we can't launch Marketplace at all, I'm going to look at backing this out.
Blocks: 929815
Reporter | ||
Comment 76•11 years ago
|
||
(In reply to Dave Hylands [:dhylands] from comment #75) > The patches in this bug seem to be causing bug 929815. Since we can't launch > Marketplace at all, I'm going to look at backing this out. Sorry for the regression. It looks this is the cause for bug 929815. Bug 926302 could fix this.
Comment 77•11 years ago
|
||
I tried to apply the patches from bug 929815, but my buri wouldn't boot to the homescreen. So I've backed out these patches and reopened the bug. https://hg.mozilla.org/integration/b2g-inbound/rev/4c7a2feefb4a https://hg.mozilla.org/integration/b2g-inbound/rev/828da5ad11d8 https://hg.mozilla.org/integration/b2g-inbound/rev/e6c73dda3830 https://hg.mozilla.org/integration/b2g-inbound/rev/fc0148c0b999 https://hg.mozilla.org/integration/b2g-inbound/rev/53b3db5d5218
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 78•11 years ago
|
||
Given that this caused a smoketest regression, can we add some automated tests here to make sure this doesn't regress again?
Reporter | ||
Comment 79•11 years ago
|
||
Bug 929815, marketplace has 'mobilenetwork' permission so it has access to navigator.mozMobileConnection. Since it doesn't have 'mobileconnection' permission, it got killed when a message is somehow sent to chrome. To test this, we first will need OOP support in marionette (Bug 926280). Without OOP support, permission check in RadioInterfaceLayer.js (in chrome) has no effects. The test case can't really mean something... We would file a follow-up bug, dependent to bug 926280, to see how we could have automated tests for this scenario.
Updated•11 years ago
|
Whiteboard: [FT:RIL]
Comment 80•11 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/53b3db5d5218 http://hg.mozilla.org/mozilla-central/rev/fc0148c0b999 http://hg.mozilla.org/mozilla-central/rev/e6c73dda3830 http://hg.mozilla.org/mozilla-central/rev/828da5ad11d8 http://hg.mozilla.org/mozilla-central/rev/4c7a2feefb4a
Assignee | ||
Comment 81•11 years ago
|
||
Rebased and added iccchange event. (asking for review again as I added a new event)
Attachment #818341 -
Attachment is obsolete: true
Attachment #822198 -
Flags: review?(htsai)
Assignee | ||
Comment 82•11 years ago
|
||
Rebased and added iccchange event. (asking for review again as I added a new event)
Attachment #818342 -
Attachment is obsolete: true
Attachment #822200 -
Flags: review?(bugs)
Assignee | ||
Comment 83•11 years ago
|
||
Rebased and added iccchange event. (asking for review again as I added a new event)
Attachment #818861 -
Attachment is obsolete: true
Attachment #822201 -
Flags: review?(gyeh)
Assignee | ||
Comment 84•11 years ago
|
||
Rebased only, setting r+ directly.
Attachment #818348 -
Attachment is obsolete: true
Attachment #822204 -
Flags: review+
Assignee | ||
Comment 85•11 years ago
|
||
Rebased and added iccchange event implementation. (asking for review again as I added a new event)
Attachment #818862 -
Attachment is obsolete: true
Attachment #822206 -
Flags: review?(htsai)
Assignee | ||
Comment 86•11 years ago
|
||
Bug 926302 has modified the way we get number of radio interfaces. The patches uploaded today were rebased based on Bug 926302, so there should be no more permission issues. However, is there any way we can know before landing, if these patches breaks anything, besides from the try server results? Thanks.
Flags: needinfo?(jsmith)
Comment 87•11 years ago
|
||
Comment on attachment 822201 [details] [diff] [review] Part 3: dom changes for BT (reland) r=gyeh Review of attachment 822201 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Jessica. ::: dom/bluetooth/BluetoothRilListener.cpp @@ +126,5 @@ > } > > +NS_IMETHODIMP > +MobileConnectionListener::NotifyIccChanged() > +{ Just in case we forget, please help to add a comment here. // TODO: Bug 921991 - B2G BT: support multiple sim cards
Attachment #822201 -
Flags: review?(gyeh) → review+
Comment 88•11 years ago
|
||
(In reply to Jessica Jong [:jjong] [:jessica] from comment #86) > Bug 926302 has modified the way we get number of radio interfaces. The > patches uploaded today were rebased based on Bug 926302, so there should be > no more permission issues. However, is there any way we can know before > landing, if these patches breaks anything, besides from the try server > results? Thanks. An easy way to verify this doesn't break the smoketest in question is just to build a device build with the provided patch & verify that you can launch marketplace with & without a SIM included.
Flags: needinfo?(jsmith)
Reporter | ||
Comment 89•11 years ago
|
||
Comment on attachment 822198 [details] [diff] [review] Part 1: idl changes (reland) f=edgar r=hsinyi Review of attachment 822198 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #822198 -
Flags: review?(htsai) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #822206 -
Flags: review?(htsai) → review+
Comment 90•11 years ago
|
||
Comment on attachment 822200 [details] [diff] [review] Part 2: dom changes (reland) f=hsinyi,edgar r=smaug >+NS_IMETHODIMP >+MobileConnection::NotifyIccChanged() >+{ >+ // TODO: Bug 814629 - WebMobileConnection API: support multiple sim cards >+ // Return NS_OK for now, will be implemented in Bug 814629. >+ return NS_OK; >+} Why are we adding this method in this bug and not in Bug 814629? Explain and re-ask review, or remove the method.
Attachment #822200 -
Flags: review?(bugs)
Assignee | ||
Comment 91•11 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #90) > Comment on attachment 822200 [details] [diff] [review] > Part 2: dom changes (reland) f=hsinyi,edgar r=smaug > > > >+NS_IMETHODIMP > >+MobileConnection::NotifyIccChanged() > >+{ > >+ // TODO: Bug 814629 - WebMobileConnection API: support multiple sim cards > >+ // Return NS_OK for now, will be implemented in Bug 814629. > >+ return NS_OK; > >+} > Why are we adding this method in this bug and not in Bug 814629? > > Explain and re-ask review, or remove the method. Olli, this bug touches mainly the internal APIs and the corresponding implementations. We have added the method notifyIccChanged() in nsIMobileConnectionListener (see attachment 822198 [details] [diff] [review]), so we need to add this dummy implementation in order to build without errors. Please correct me if I am wrong. Thanks.
Assignee | ||
Updated•11 years ago
|
Attachment #822200 -
Flags: review?(bugs)
Assignee | ||
Comment 92•11 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #88) > (In reply to Jessica Jong [:jjong] [:jessica] from comment #86) > > Bug 926302 has modified the way we get number of radio interfaces. The > > patches uploaded today were rebased based on Bug 926302, so there should be > > no more permission issues. However, is there any way we can know before > > landing, if these patches breaks anything, besides from the try server > > results? Thanks. > > An easy way to verify this doesn't break the smoketest in question is just > to build a device build with the provided patch & verify that you can launch > marketplace with & without a SIM included. The weird thing is I wasn't able to reproduce the issue with or without the old patches. I was able to launch marketplace with the old patches, with and without SIM included, in my builds from master branch. Do you know anyone who can help on this? Thanks.
Comment 93•11 years ago
|
||
(In reply to Jessica Jong [:jjong] [:jessica] from comment #92) > (In reply to Jason Smith [:jsmith] from comment #88) > > (In reply to Jessica Jong [:jjong] [:jessica] from comment #86) > > > Bug 926302 has modified the way we get number of radio interfaces. The > > > patches uploaded today were rebased based on Bug 926302, so there should be > > > no more permission issues. However, is there any way we can know before > > > landing, if these patches breaks anything, besides from the try server > > > results? Thanks. > > > > An easy way to verify this doesn't break the smoketest in question is just > > to build a device build with the provided patch & verify that you can launch > > marketplace with & without a SIM included. > > The weird thing is I wasn't able to reproduce the issue with or without the > old patches. I was able to launch marketplace with the old patches, with and > without SIM included, in my builds from master branch. > Do you know anyone who can help on this? Thanks. Strange. The above comment indicates this was caused by bug 926302, which is fixed now. I'm not sure why you couldn't reproduce the original issue - some questions I have then are: 1. Was your mozilla central tree containing other patches when you applied the old patches here? If so, did it contain the patch in bug 926302? 2. Do you know if your mozilla central tree was out of date when you tested your applied patch or not?
Assignee | ||
Comment 94•11 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #93) > (In reply to Jessica Jong [:jjong] [:jessica] from comment #92) > > (In reply to Jason Smith [:jsmith] from comment #88) > > > (In reply to Jessica Jong [:jjong] [:jessica] from comment #86) > > > > Bug 926302 has modified the way we get number of radio interfaces. The > > > > patches uploaded today were rebased based on Bug 926302, so there should be > > > > no more permission issues. However, is there any way we can know before > > > > landing, if these patches breaks anything, besides from the try server > > > > results? Thanks. > > > > > > An easy way to verify this doesn't break the smoketest in question is just > > > to build a device build with the provided patch & verify that you can launch > > > marketplace with & without a SIM included. > > > > The weird thing is I wasn't able to reproduce the issue with or without the > > old patches. I was able to launch marketplace with the old patches, with and > > without SIM included, in my builds from master branch. > > Do you know anyone who can help on this? Thanks. > > Strange. The above comment indicates this was caused by bug 926302, which is > fixed now. I'm not sure why you couldn't reproduce the original issue - some > questions I have then are: > > 1. Was your mozilla central tree containing other patches when you applied > the old patches here? If so, did it contain the patch in bug 926302? > 2. Do you know if your mozilla central tree was out of date when you tested > your applied patch or not? From the description in Bug 929815, we determined it was a security/permission issue, so we assume Bug 926302 would solve it, but we weren't really able to reproduce the issue from our local builds. To answer your questions... what I did was to rollback my gecko revision to 7823e2985daf (same from the 20131021040204 build where the smoke test fail), so the patches in bug 926302 was not there yet. I wonder if there is any difference between PVT builds and our own local builds?
Updated•11 years ago
|
Attachment #822200 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 95•11 years ago
|
||
Rebased only.
Attachment #822206 -
Attachment is obsolete: true
Attachment #823794 -
Flags: review+
Assignee | ||
Comment 96•11 years ago
|
||
Try server results: https://tbpl.mozilla.org/?tree=Try&rev=d00b01db2425 The Mnw failure was a intermittent fail and passed on the second run: https://tbpl.mozilla.org/?tree=Try&rev=a5f6befa4203
Assignee | ||
Comment 97•11 years ago
|
||
Since we can't verify any further, I say we land the patches and feel free to reopen/backout if they break anything. Thanks.
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 98•11 years ago
|
||
(In reply to Jessica Jong [:jjong] [:jessica] from comment #97) > Since we can't verify any further, I say we land the patches and feel free > to reopen/backout if they break anything. Thanks. I also faced the same situation as comment #94. I could launch marketplace app without any problem even with the previous problematic patches ... We've tried our best to test and verify Jessica's latest patches, and it looks we couldn't do more. As the try result looks good and we didn't detect problems by our manual test, I am going to push the patches.
Reporter | ||
Comment 99•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/ed8470f3f7c1 https://hg.mozilla.org/integration/b2g-inbound/rev/b5c35a59ac50 https://hg.mozilla.org/integration/b2g-inbound/rev/8adc3e66e6c2 https://hg.mozilla.org/integration/b2g-inbound/rev/5a43c2099e39 https://hg.mozilla.org/integration/b2g-inbound/rev/ed37f98ae0fe
Keywords: checkin-needed
Comment 100•11 years ago
|
||
Because Sprint 3 was already passed, should we assign new target milestone? Thanks.
Assignee | ||
Updated•11 years ago
|
Target Milestone: 1.3 Sprint 3 - 10/25 → 1.3 Sprint 4 - 11/8
Comment 101•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ed8470f3f7c1 https://hg.mozilla.org/mozilla-central/rev/b5c35a59ac50 https://hg.mozilla.org/mozilla-central/rev/8adc3e66e6c2 https://hg.mozilla.org/mozilla-central/rev/5a43c2099e39 https://hg.mozilla.org/mozilla-central/rev/ed37f98ae0fe
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 102•11 years ago
|
||
\o/
Updated•11 years ago
|
status-firefox28:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•