To make the data structure simpler and more generic, it's better to using array for data call settings and RILNetworkInterface. Reference: [1] https://bugzilla.mozilla.org/show_bug.cgi?id=782513#c12 [2] https://bugzilla.mozilla.org/show_bug.cgi?id=772747#c22

Comment on attachment 678653 [details] [diff] [review] Bug 794336 patch Review of attachment 678653 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your work, but, IMHO this patch does two things: 1) make the line longer than before, 2) carelessly removes usingDefaultAPN() without providing any replacement logic. Besides, it's not as generic as claimed in issue description in some places like handleDataCallError(). Refactoring is good, but 2) introduces bugs when MMS/SUPL share default data connection.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #2) > Comment on attachment 678653 [details] [diff] [review] > Bug 794336 patch > > Review of attachment 678653 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for your work, but, IMHO this patch does two things: 1) make the line > longer than before, 2) carelessly removes usingDefaultAPN() without > providing any replacement logic. Besides, it's not as generic as claimed in > issue description in some places like handleDataCallError(). Refactoring is > good, but 2) introduces bugs when MMS/SUPL share default data connection. I think the item 2 is fixed in the patch of 794342. I will double check it.

Do 2 modifications. 1. Avoid lines longer than 80 characters. 2. Add usingDefaultAPN().

Comment on attachment 679493 [details] [diff] [review] Bug 794336 patch V2 Talked to Ken, I'd rather have a more generic solution here and already had a draft patch in bug 782513. Besides, separating data call related parameters into several Settings API set() calls is really something we should refactor first. It brings too much uncertainty and complexity while no obvious benefits gained.