Some privileged content needs to know details about the mobile connection, so that signal strength, operator name, etc. can be shown. See https://wiki.mozilla.org/WebAPI/WebMobileConnection for a draft API. I will take a shot at implementing this.

Comment on attachment 599240 [details] [diff] [review] Part 1 (v1): IDLs Review of attachment 599240 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! Some minor comments below. ::: dom/network/interfaces/nsIDOMMobileConnection.idl @@ +14,5 @@ > + * > + * Possible values: 'unavailable', 'absent', 'pin_required', 'puk_required', > + * 'network_locked', 'not_ready', 'ready'. > + */ > + readonly attribute DOMString cardState; What's the difference between 'unavailable', 'absent' and 'not_ready'? Is the latter the state it goes into after you enter the pin while it's still authenticating against the network? @@ +50,5 @@ > + * 2...30 -109 ... -53 dBm > + * 31 -51 dBm or greater > + * null if no signal strength (or signal) is available. > + */ > + readonly attribute short signalstrength; This should be signalStrength. But onsignalstrengthchange should remain as-is. Compare to readyState/onreadystatechange. Also, in order to be able to return 'null' you unfortunately have to make this be of type jsval. @@ +57,5 @@ > + * Signal strength expressed as number of "bars". > + * > + * Possible values: 0 (no signal) to 5 (full signal). > + */ > + readonly attribute short bars; IIRC it's possible to have 0 bars while still having signal in most phone UIs. Should we make this be 0-5 or null?

(In reply to Jonas Sicking (:sicking) from comment #2) > ::: dom/network/interfaces/nsIDOMMobileConnection.idl > @@ +14,5 @@ > > + * > > + * Possible values: 'unavailable', 'absent', 'pin_required', 'puk_required', > > + * 'network_locked', 'not_ready', 'ready'. > > + */ > > + readonly attribute DOMString cardState; > > What's the difference between 'unavailable', 'absent' and 'not_ready'? Is > the latter the state it goes into after you enter the pin while it's still > authenticating against the network? "unavailable" means we don't know what the card state is. We could make that `null`. "absent" means we know what the card state is, namely there is no card. "not_ready" means we know there's a card, but the radio isn't ready to read it yet. We don't necessarily need to expose this state, we can just leave it `null` until it comes online. > @@ +50,5 @@ > > + * 2...30 -109 ... -53 dBm > > + * 31 -51 dBm or greater > > + * null if no signal strength (or signal) is available. > > + */ > > + readonly attribute short signalstrength; > > This should be signalStrength. But onsignalstrengthchange should remain > as-is. Compare to readyState/onreadystatechange. Yeah, that's a typo. Fixed! > @@ +57,5 @@ > > + * Signal strength expressed as number of "bars". > > + * > > + * Possible values: 0 (no signal) to 5 (full signal). > > + */ > > + readonly attribute short bars; > > IIRC it's possible to have 0 bars while still having signal in most phone > UIs. Should we make this be 0-5 or null? Good point, agreed. Fixed!

(In reply to Philipp von Weitershausen [:philikon] from comment #3) > > What's the difference between 'unavailable', 'absent' and 'not_ready'? Is > > the latter the state it goes into after you enter the pin while it's still > > authenticating against the network? > > "unavailable" means we don't know what the card state is. We could make that > `null`. > > "absent" means we know what the card state is, namely there is no card. > > "not_ready" means we know there's a card, but the radio isn't ready to read > it yet. We don't necessarily need to expose this state, we can just leave it > `null` until it comes online. The null for "unavailable" and "not_ready" sounds good.

Address sicking's review comments. Also define `bars` to be a value between 0 and 4 (not 5) because that's what most UIs seem to do.

Hacked together JS implementation for demo purposes. This actually works, mostly. Hot swapping the SIM card doesn't work so brilliantly, but other than that, it seems to work quite nicely! This doesn't do events properly (need C++ for this) and doesn't have any sort of permissions model. DO NOT COMMIT THIS!

Comment on attachment 600292 [details] [diff] [review] Part 1 (v2): IDLs Review of attachment 600292 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/interfaces/nsIDOMMobileConnection.idl @@ +12,5 @@ > + /** > + * Indicates the state of the device's ICC card. > + * > + * Possible values: null, 'absent', 'pin_required', 'puk_required', > + * 'network_locked', 'ready'. What does 'network_locked' means here? @@ +58,5 @@ > + * > + * Possible values: null (no signal / not registered) or a number betweeen > + * 0 (weakest signal) and 4 (full signal). > + */ > + readonly attribute jsval bars; I'm not sure why we need that. "bars" is UI-only. What we want is the signal strength (which I believe would be |signalStrength|). Then, the homescreen should show the bars in the form it prefers. It could be 4 bars or 5 bars for example.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #7) > ::: dom/network/interfaces/nsIDOMMobileConnection.idl > @@ +12,5 @@ > > + /** > > + * Indicates the state of the device's ICC card. > > + * > > + * Possible values: null, 'absent', 'pin_required', 'puk_required', > > + * 'network_locked', 'ready'. > > What does 'network_locked' means here? Phones can be locked to only accept SIM cards from specific carriers. > @@ +58,5 @@ > > + * > > + * Possible values: null (no signal / not registered) or a number betweeen > > + * 0 (weakest signal) and 4 (full signal). > > + */ > > + readonly attribute jsval bars; > > I'm not sure why we need that. "bars" is UI-only. What we want is the signal > strength (which I believe would be |signalStrength|). Then, the homescreen > should show the bars in the form it prefers. It could be 4 bars or 5 bars > for example. You'd think that that'd be case, but it turns out that some RIL stacks (relevant to us: the SGS2) don't actually give us the signal strength information. All they give us is the number of bars (0 to 4). But aside from this, I think there's value on computing this information for the UI rather than duplicating it in all UI implementations and for those who still want to do it, we expose signalStrength.

(In reply to Philipp von Weitershausen [:philikon] from comment #8) > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #7) > > ::: dom/network/interfaces/nsIDOMMobileConnection.idl > > @@ +12,5 @@ > > > + /** > > > + * Indicates the state of the device's ICC card. > > > + * > > > + * Possible values: null, 'absent', 'pin_required', 'puk_required', > > > + * 'network_locked', 'ready'. > > > > What does 'network_locked' means here? > > Phones can be locked to only accept SIM cards from specific carriers. I understand that. But is that state happens when I put a SIM card of a carrier on a locked phone? It's a bit disturbing because a phone is locked to a specific carrier or is not. Which means 'network_locked' should be a boolean on the phone for me. Though, we are speaking of |cardState| here and AFAIK, a 'networkLocked' boolean will have no meaning for a SIM card. I'm not going to fight for that because I obviously don't know enough about it to have strong arguments. Just want to point out the possible misunderstanding. But something else: states like "Absent", "PinRequired", "PukRequired", "NetworkLocked" and "Ready" would be more webby. > > @@ +58,5 @@ > > > + * > > > + * Possible values: null (no signal / not registered) or a number betweeen > > > + * 0 (weakest signal) and 4 (full signal). > > > + */ > > > + readonly attribute jsval bars; > > > > I'm not sure why we need that. "bars" is UI-only. What we want is the signal > > strength (which I believe would be |signalStrength|). Then, the homescreen > > should show the bars in the form it prefers. It could be 4 bars or 5 bars > > for example. > > You'd think that that'd be case, but it turns out that some RIL stacks > (relevant to us: the SGS2) don't actually give us the signal strength > information. All they give us is the number of bars (0 to 4). Do we have to get stuck with what the SGS2 is doing? If the SGS2 is doing something wrong, I don't think we should imitate that unless we are really forced to. > But aside from > this, I think there's value on computing this information for the UI rather > than duplicating it in all UI implementations and for those who still want > to do it, we expose signalStrength. The conversion from signalStrength to bars is as simple as |round(signalStrength / maxSignalStrength * numberOfBars)|. That's not complex... And putting that in our API forces UIs to show 4 bars. There is no reason to force UIs to get stuck with 4 bars instead of 5 (or why not 3 or 6?).

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #9) > Which means 'network_locked' should be a boolean on the phone for me. > Though, we are speaking of |cardState| here and AFAIK, a 'networkLocked' > boolean will have no meaning for a SIM card. I think you're misunderstanding something. 'cardState' specifies the state of the relationship between the device and the SIM card at any given moment. So "pin_locked" doesn't meant that the SIM card has a PIN lock on it. It just means that *right now* the radio can't use the SIM card yet until the PIN is entered. If that's successful, the 'cardState' will be "ready". If you want to know and/or change the PIN lock on the SIM card, you would use the ICC-related APIs that I haven't spec'ed here yet, but mentioned at the bottom of https://wiki.mozilla.org/WebAPI/WebMobileConnection. It would also be interesting to discuss whether these should be added to WebMobileConnection or not... Anyway, maybe 'cardState' should be 'radioCardState' or something like that to be clearer about the fact that it's a momentary representation of a relationship? I'm not feeling strongly about the name. > But something else: states like "Absent", "PinRequired", "PukRequired", > "NetworkLocked" and "Ready" would be more webby. I have no preference whatsoever, though if you have examples of uses of CamelCase in strings in other WebAPIs, I would love to see them. > Do we have to get stuck with what the SGS2 is doing? If the SGS2 is doing > something wrong, I don't think we should imitate that unless we are really > forced to. That's fair. We shall see how many other manufacturers use the chipset from the SGS2... > > But aside from > > this, I think there's value on computing this information for the UI rather > > than duplicating it in all UI implementations and for those who still want > > to do it, we expose signalStrength. > > The conversion from signalStrength to bars is as simple as > |round(signalStrength / maxSignalStrength * numberOfBars)|. Probably. If signalStrength / bars is a linear relationship. I'm kind of going back and forth between what to expose. The "signalStrength" value as described now in the interface is GSM-specific. For CDMA, we get the actual dBm values (which are easily converted to the GSM values, though). So I'm now thinking that we should just expose the dBm value. And 'bars' could be a convenient linear representation (because dBm isn't linear) as a float between 0 and 1.0. Then the UI can choose whatever number of bars and everybody wins.

Added: * emergencyCallsPossible boolean -- this is an important flag for when network registration isn't possible for some reason, but emergency calls can still be made. Phone UIs often show this. * listAvailableNetworks() function -- this is a necessary when the UI wants to enumerate operators for manual override (the override would likely be a setting in the Settings API) Changed: * Morphed 'signalStrength' (which was based on a GSM standard) and 'bars' (which arbitrarily was 0 .. 4) into 'signalStrengthDbm' (raw dBm info) and 'signalStrength' (0 .. 1.0). This should address Mounir's concerns of suggesting a particular UI representation, as well as my concern of focusing on a particular mobile standard.

(In reply to Philipp von Weitershausen [:philikon] from comment #10) > Anyway, maybe 'cardState' should be 'radioCardState' or something like that > to be clearer about the fact that it's a momentary representation of a > relationship? I'm not feeling strongly about the name. I don't care that much about the name of the property. So, I was wondering, in which state the card should be if I can't connect to any network? Let say I'm in a place where there is no coverage. > > But something else: states like "Absent", "PinRequired", "PukRequired", > > "NetworkLocked" and "Ready" would be more webby. > > I have no preference whatsoever, though if you have examples of uses of > CamelCase in strings in other WebAPIs, I would love to see them. DOM4 Errors are using that style: http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#error-types-table (In reply to Philipp von Weitershausen [:philikon] from comment #11) > * Morphed 'signalStrength' (which was based on a GSM standard) and 'bars' > (which arbitrarily was 0 .. 4) into 'signalStrengthDbm' (raw dBm info) and > 'signalStrength' (0 .. 1.0). This should address Mounir's concerns of > suggesting a particular UI representation, as well as my concern of focusing > on a particular mobile standard. Sounds good to me if |signalStrength| is renamed |signalStrengthLinear| to make it clear that it's not the raw signal strength. Feel free to say it's bikeshedding but unless you read the doc, you can't understand that's like the dbm one but from 0.0 to 1.0.

Mounir: Why do you hate short names? In general I think we for enumerated strings use all-lowercase. But we haven't had many multi-word strings...

(In reply to Jonas Sicking (:sicking) from comment #13) > Mounir: Why do you hate short names? I do not hate short names, I like clear names and trying to make a name short is rarely helping to make it clear. I want an API to be understandable without reading the documentation. > In general I think we for enumerated strings use all-lowercase. But we > haven't had many multi-word strings... DOM4 is using CamelCase as you can see. I would go with that. For sure, I haven't seen any web api with words_separated_by_undersccores.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #12) > (In reply to Philipp von Weitershausen [:philikon] from comment #10) > > Anyway, maybe 'cardState' should be 'radioCardState' or something like that > > to be clearer about the fact that it's a momentary representation of a > > relationship? I'm not feeling strongly about the name. > > I don't care that much about the name of the property. So, I was wondering, > in which state the card should be if I can't connect to any network? Let say > I'm in a place where there is no coverage. Well, presuming there's a card in the device, and either it doesn't have a PIN lock or the PIN has been entered to unlock it, the state should be "ready". This is independent of whether or not there's signal coverage in your area. That information is covered by the `connected`, `roaming`, and ultimately signal strength properties. > > > But something else: states like "Absent", "PinRequired", "PukRequired", > > > "NetworkLocked" and "Ready" would be more webby. > > > > I have no preference whatsoever, though if you have examples of uses of > > CamelCase in strings in other WebAPIs, I would love to see them. > > DOM4 Errors are using that style: > http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#error-types-table Ok. These look more like class names, but I get your point. Anyway, I was going more for the WebSMS analogy where the strings are "received" and "sent". > (In reply to Philipp von Weitershausen [:philikon] from comment #11) > > * Morphed 'signalStrength' (which was based on a GSM standard) and 'bars' > > (which arbitrarily was 0 .. 4) into 'signalStrengthDbm' (raw dBm info) and > > 'signalStrength' (0 .. 1.0). This should address Mounir's concerns of > > suggesting a particular UI representation, as well as my concern of focusing > > on a particular mobile standard. > > Sounds good to me if |signalStrength| is renamed |signalStrengthLinear| to > make it clear that it's not the raw signal strength. Feel free to say it's > bikeshedding but unless you read the doc, you can't understand that's like > the dbm one but from 0.0 to 1.0. Fair. I will defer to Jonas for both this and the CamelCase decision.

Regarding casing: In most other APIs with enumerated strings we use all lowercase (Document.readyState, XHR.responseType, WebSocket.binaryType, (arguably Event.type)). However in all those cases we have single-word names. I would probably go with camelCase to match function and attribute names given that this is basically an enum.

(In reply to Philipp von Weitershausen [:philikon] from comment #10) > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #9) > > > But aside from > > > this, I think there's value on computing this information for the UI rather > > > than duplicating it in all UI implementations and for those who still want > > > to do it, we expose signalStrength. > > > > The conversion from signalStrength to bars is as simple as > > |round(signalStrength / maxSignalStrength * numberOfBars)|. > > Probably. If signalStrength / bars is a linear relationship. > > I'm kind of going back and forth between what to expose. The > "signalStrength" value as described now in the interface is GSM-specific. > For CDMA, we get the actual dBm values (which are easily converted to the > GSM values, though). So I'm now thinking that we should just expose the dBm > value. And 'bars' could be a convenient linear representation (because dBm > isn't linear) as a float between 0 and 1.0. Then the UI can choose whatever > number of bars and everybody wins. Forgive me if I'm a little late to this conversation, but in general, cell phones don't do a straight linear conversion from signal (RSSI) strength bars. Usually the a full set of bars indicates "plenty of strengh", and fewer bars indicate increasingly degraded RSSI. So for CDMA, which might be usable down to an RSSI of -106dBm with degraded performance, but which reaches full performance at -80dBm all the way up to a radio limit of -40dBm (these numbers are a bit hand-wavy), then you would see all four bars from -40 down to -80dBm, and then start seeing them drop as the RSSI continues to drop. (As an aside: whatever conversion algorithm gets chosen, I recommend making it initially pessimistic; you don't want people seeing that a new software load gets "three bars" in the same spot on their table where it used to get four.)

Comment on attachment 601431 [details] [diff] [review] Part 1 (v3): IDLs Review of attachment 601431 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/interfaces/nsIDOMMobileConnection.idl @@ +46,5 @@ > + * > + * Possible values: 'gprs', 'edge', 'umts', 'hsdpa', 'evdo0', 'evdoa', > + * 'evdob', etc. > + */ > + readonly attribute DOMString type; There's a few properties which we could possibly merge into a .connection JSON property. This makes more sense to me if we can make those JSON objects have the same structure as the ones returned from listAvailableNetworks. @@ +64,5 @@ > + * Search for available networks. > + * > + * If successful, the request result will be an array of operator names. > + */ > + nsIDOMDOMRequest listAvailableNetworks(); I think we talked about shortening this to just getNetworks?

(In reply to Philipp von Weitershausen [:philikon] from comment #11) > Created attachment 601431 [details] [diff] [review] > Part 1 (v3): IDLs > > Added: > > * emergencyCallsPossible boolean -- this is an important flag for when > network registration isn't possible for some reason, but emergency calls can > still be made. Phone UIs often show this. Could the name of this flag be changed? It's a little bit confusing because if the phone is properly registered emergency calls are allowed as well and the current name indicates they are not. What about 'emergencyCallsOnly'? I am currently hacking around emergency calls and I would like to start a discussion about the semantics of this flags. What I understand is that when network registration isn't possible for some reason 'connected' is false and 'emergencyCallsOnly' is true (if possible I mean). In fact the `emergencyCallsOnly` flag should only ever be looked at when `connected` is false. When 'connected' is true emergency calls are allowed as any call and 'emergencyCallsOnly' is false because the user could dial any destination. Thoughts? Note: I am not sure if it is the right place for this disscussion. Sorry if I am wrong.

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #19) > (In reply to Philipp von Weitershausen [:philikon] from comment #11) > > Created attachment 601431 [details] [diff] [review] > > Part 1 (v3): IDLs > > > > Added: > > > > * emergencyCallsPossible boolean -- this is an important flag for when > > network registration isn't possible for some reason, but emergency calls can > > still be made. Phone UIs often show this. > > Could the name of this flag be changed? It's a little bit confusing because > if the phone is properly registered emergency calls are allowed as well and > the current name indicates they are not. What about 'emergencyCallsOnly'? > > I am currently hacking around emergency calls and I would like to start a > discussion about the semantics of this flags. What I understand is that when > network registration isn't possible for some reason 'connected' is false and > 'emergencyCallsOnly' is true (if possible I mean). In fact the > `emergencyCallsOnly` flag should only ever be looked at when `connected` is > false. This makes sense to me. I'll change the flag. Thanks for bringing this up.

Comment on attachment 600293 [details] [diff] [review] Part 2.71828: Hacked together JS implementation Initially we said we wouldn't land this in m-c, but there's a lot of interest in killing the B2G fork of gecko asap. So I'm going to rebase this, clean it up a little, work in the interfaces changes we discussed since it got landed on the fork, and nominate it for review. Ben, perhaps you'd like to look over this already? If you'd rather wait for the actual patch, that's fine too.

Comment on attachment 600293 [details] [diff] [review] Part 2.71828: Hacked together JS implementation Review of attachment 600293 [details] [diff] [review]: ----------------------------------------------------------------- I'd rather wait, yeah. Lots to do this week.

Separated voice and data information and modeled the naming after the wifi API.

This refactors the permissions helper from Telephony into a reusable piece, and uses it in the navigator hookup for MobileConnection.

This creates a content process bound RILContentHelper that exposes the necessary RIL state to the DOM. In the future we can build this out to do other RIL stuff, e.g. telephony. The RIL worker changes here are just some tweaks in the relevant parcel handlers and a few object creation optimizations along the way.

Comment on attachment 613064 [details] [diff] [review] Part 5 (v1): Fill in DOM implementation I should note about this patch: the observer notifications from RILContentHelper are properly received and the InternalDispatchEvent method executes just fine with no error. However, I'm not seeing the event dispatch happening in the DOM when I attach an event handler to navigator.mozMobileConnection. I'm probably missing something, but I don't know what it is.

This fetches the initial state from the chrome process via a sync message. This is necessary because the initial access to navigator.mozMobileConnection could easily be something like navigator.mozMobileConnection.cardState which would not return the right value if the state was initialized asynchronously. (This is now akin to how Wifi initializes its state on the content process.)

Comment on attachment 612719 [details] [diff] [review] Part 2 (v1): DOM boilerplate bent is PTO this week apparently, so redirecting the review to other people. Thanks!

Comment on attachment 612718 [details] [diff] [review] Part 1 (v4): IDLs Review of attachment 612718 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/interfaces/nsIDOMMobileConnection.idl @@ +26,5 @@ > + > + /** > + * Information about the data connection. > + */ > + readonly attribute nsIDOMMozMobileConnectionInfo data; Why do we have two objects, one for voice and one for data while most of the information might be duplicated? @@ +45,5 @@ > + /** > + * The 'voicechange' event is notified whenever an attribute on the voice > + * connection object changes values. > + */ > + attribute nsIDOMEventListener onvoicechange; Is there a reason why the event is sent to this object and not to the voice object? @@ +51,5 @@ > + /** > + * The 'datachange' event is notified whenever an attribute on the data > + * connection object changes values. > + */ > + attribute nsIDOMEventListener ondatachange; ditto

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #32) > ::: dom/network/interfaces/nsIDOMMobileConnection.idl > @@ +26,5 @@ > > + > > + /** > > + * Information about the data connection. > > + */ > > + readonly attribute nsIDOMMozMobileConnectionInfo data; > > Why do we have two objects, one for voice and one for data while most of the > information might be duplicated? Because in quite common cases the information is in fact not duplicated. For instance, you may be connected to the voice network, but not have a data connection (for various reasons). Also, pretty much all 3G and later phones (incl CDMA) these days, voice and data are served by entirely separate radios. Thus we need to treat them essentially as different connections with e.g. their own signal strengths etc. > @@ +45,5 @@ > > + /** > > + * The 'voicechange' event is notified whenever an attribute on the voice > > + * connection object changes values. > > + */ > > + attribute nsIDOMEventListener onvoicechange; > > Is there a reason why the event is sent to this object and not to the voice > object? This is a good point you bring up. Yes, there is a reason. The comment above it is misleading, sorry about that. I think there are good reasons to keep the 'voice' and 'data' objects as stupid as possible. For all I care they could be simple JSON objects, but I wanted to spec them out in the IDL for documentation purposes. Whenever some connection data changes, the DOM essentially should have the right to replace the whole object. In our particular implementation we don't, but that's just an implementation detail. In fact, I could see scenarios where the 'voice' and 'data' objects are null, so you need to be able to listen for events somewhere else to be notified when they're not null.

Comment on attachment 612719 [details] [diff] [review] Part 2 (v1): DOM boilerplate Review of attachment 612719 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comment below fixed. ::: dom/network/src/MobileConnection.cpp @@ +42,5 @@ > +} > + > +void > +MobileConnection::Init(nsPIDOMWindow* aWindow) > +{ I understand this is a dummy class but could you at least call |BindToOwner(aWindow)|?

Comment on attachment 613064 [details] [diff] [review] Part 5 (v1): Fill in DOM implementation Review of attachment 613064 [details] [diff] [review]: ----------------------------------------------------------------- r- because I would like to see a new patch and I would like to encourage you to fix bug 720768 ;) ::: dom/network/src/MobileConnection.cpp @@ +50,5 @@ > { > + mRIL = do_GetService(NS_RILCONTENTHELPER_CONTRACTID); > + if (!mRIL) { > + NS_ERROR("Could not acquire nsIRILContentHelper!"); > + } nit: you can write this instead: NS_ASSERTION(mRIL, "Could not acquire nsIRILContentHelper!"); @@ +62,5 @@ > + nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > + if (!obs) { > + return; > + } > + obs->AddObserver(this, kVoiceChangedTopic, false); nit: please, leave a blank line before this line. @@ +74,5 @@ > + nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > + if (!obs) { > + return; > + } > + obs->RemoveObserver(this, kVoiceChangedTopic); nit: please, leave a blank line before this line. ::: dom/network/src/MobileConnection.h @@ +14,5 @@ > #define DOM_MOBILECONNECTION_WHITELIST_PREF "dom.mobileconnection.whitelist" > > +#define VOICE_CHANGED_EVENT_NAME NS_LITERAL_STRING("voicechanged") > +#define DATA_CHANGED_EVENT_NAME NS_LITERAL_STRING("datachanged") > +#define CARDSTATE_CHANGED_EVENT_NAME NS_LITERAL_STRING("cardstatechanged") Static strings in the class would be way better. @@ +42,5 @@ > > private: > + nsCOMPtr<nsIRILContentHelper> mRIL; > + > + nsresult InternalDispatchEvent(const nsAString& aType); What about fixing bug 720768 so we don't duplicate that same code too much?

Comment on attachment 612720 [details] [diff] [review] Part 3 (v1): Hook up to navigator, refactor permission helper Review of attachment 612720 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Helpers.h @@ +25,5 @@ > + */ > +nsresult > +CheckURIWhitelistPref(nsPIDOMWindow* aWindow, > + const char* aPref, > + bool* aMatches); There is no need for a new file, you could add this in nsContentUtils.{h,cpp}. Anyhow, can't you use nsContentUtils::URIIsChromeOrInPref? ::: dom/network/src/MobileConnection.h @@ +9,5 @@ > #include "nsIDOMMobileConnection.h" > #include "nsDOMEventTargetHelper.h" > #include "nsCycleCollectionParticipant.h" > > +#define DOM_MOBILECONNECTION_WHITELIST_PREF "dom.mobileconnection.whitelist" If you really want to keep the string here, please use a static public string in the |MobileConnection| class. ::: dom/telephony/Makefile.in @@ +53,5 @@ > +LOCAL_INCLUDES = \ > + -I$(topsrcdir)/content/events/src \ > + -I$(topsrcdir)/dom/base \ > + -I$(topsrcdir)/dom/system/gonk \ > + $(NULL) Why is that needed? ::: dom/telephony/Telephony.cpp @@ +497,5 @@ > NS_NewTelephony(nsPIDOMWindow* aWindow, nsIDOMTelephony** aTelephony) > { > + bool allowed; > + nsresult rv = > + mozilla::dom::CheckURIWhitelistPref(aWindow, I would prefer the refactoring to be in a separate patch/bug.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #34) > ::: dom/network/src/MobileConnection.cpp > @@ +42,5 @@ > > +} > > + > > +void > > +MobileConnection::Init(nsPIDOMWindow* aWindow) > > +{ > > I understand this is a dummy class but could you at least call > |BindToOwner(aWindow)|? You mean do it in this patch rather than in Part 5? Seems like a lot of busywork just to shuffle some stuff around that's going to land together anyway.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #35) > r- because I would like to see a new patch and I would like to encourage you > to fix bug 720768 ;) I understand the desire to consolidate code, and I'll be happy to help out, but I would request that we do not block this bug on that. This bug is the last one that's blocking us from getting rid of the B2G gecko fork, so the sooner this bug lands, the better. I'll upload a new patch to address your other comments.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #36) > ::: dom/base/Helpers.h > @@ +25,5 @@ > > + */ > > +nsresult > > +CheckURIWhitelistPref(nsPIDOMWindow* aWindow, > > + const char* aPref, > > + bool* aMatches); > > There is no need for a new file, you could add this in > nsContentUtils.{h,cpp}. > Anyhow, can't you use nsContentUtils::URIIsChromeOrInPref? Hmm yes I suppose could! I didn't know about it. Means I don't have to refactor at all. > ::: dom/telephony/Makefile.in > @@ +53,5 @@ > > +LOCAL_INCLUDES = \ > > + -I$(topsrcdir)/content/events/src \ > > + -I$(topsrcdir)/dom/base \ > > + -I$(topsrcdir)/dom/system/gonk \ > > + $(NULL) > > Why is that needed? Because we now include dom/base/Helper.h but for some reason the default LOCAL_INCLUDES didn't include dom/base.

(In reply to Philipp von Weitershausen [:philikon] from comment #37) > You mean do it in this patch rather than in Part 5? Seems like a lot of > busywork just to shuffle some stuff around that's going to land together > anyway. If you land them in in one patch, it's okay. Otherwise, I would prefer to have the method in the first patch because it is needed at that point. (In reply to Philipp von Weitershausen [:philikon] from comment #38) > I understand the desire to consolidate code, and I'll be happy to help out, > but I would request that we do not block this bug on that. This bug is the > last one that's blocking us from getting rid of the B2G gecko fork, so the > sooner this bug lands, the better. As I said, I would like to encourage you to add that method, not force you. (In reply to Philipp von Weitershausen [:philikon] from comment #39) > > ::: dom/telephony/Makefile.in > > @@ +53,5 @@ > > > +LOCAL_INCLUDES = \ > > > + -I$(topsrcdir)/content/events/src \ > > > + -I$(topsrcdir)/dom/base \ > > > + -I$(topsrcdir)/dom/system/gonk \ > > > + $(NULL) > > > > Why is that needed? > > Because we now include dom/base/Helper.h but for some reason the default > LOCAL_INCLUDES didn't include dom/base. What about the two other -I directives?

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #40) > (In reply to Philipp von Weitershausen [:philikon] from comment #37) > > You mean do it in this patch rather than in Part 5? Seems like a lot of > > busywork just to shuffle some stuff around that's going to land together > > anyway. > > If you land them in in one patch, it's okay. Otherwise, I would prefer to > have the method in the first patch because it is needed at that point. Hmm, it compiled and worked fine without the BindToOwner, but sure, I can fold the patches together. > (In reply to Philipp von Weitershausen [:philikon] from comment #39) > > > ::: dom/telephony/Makefile.in > > > @@ +53,5 @@ > > > > +LOCAL_INCLUDES = \ > > > > + -I$(topsrcdir)/content/events/src \ > > > > + -I$(topsrcdir)/dom/base \ > > > > + -I$(topsrcdir)/dom/system/gonk \ > > > > + $(NULL) > > > > > > Why is that needed? > > > > Because we now include dom/base/Helper.h but for some reason the default > > LOCAL_INCLUDES didn't include dom/base. > > What about the two other -I directives? As soon as I overrided LOCAL_INCLUDES, I had to include all of the other directories that Telephony*.cpp imports header files from. At least that's what I think happened. Heck, I don't pretend to understand our build system :). In either case, using the helper from nsContentUtils means I don't have to do this refactoring at all, so dom/telephony won't get touched.

Comment on attachment 613264 [details] [diff] [review] Part 4 (v2): Expose the RIL state to content Review of attachment 613264 [details] [diff] [review]: ----------------------------------------------------------------- I'm not familiar with the RIL messages so I can't comment too much on this. Please fix The message manager leaks! ::: dom/system/gonk/RILContentHelper.js @@ +57,5 @@ > + this.voiceConnectionInfo = new MobileConnectionInfo(); > + this.dataConnectionInfo = new MobileConnectionInfo(); > + > + for each (let msgname in RIL_IPC_MSG_NAMES) { > + Services.cpmm.addMessageListener(msgname, this); these are never removed. You can do it in a xpcom-shutdown observer ::: dom/system/gonk/RadioInterfaceLayer.js @@ +165,2 @@ > }; > + Services.ppmm.addMessageListener("RIL:GetRadioState", this); these are never removed. ::: dom/system/gonk/nsRadioInterfaceLayer.h @@ +43,5 @@ > + > +#define NS_RILCONTENTHELPER_CID \ > + { 0x472816e1, 0x1fd6, 0x4405, \ > + { 0x99, 0x6c, 0x80, 0x6f, 0x9e, 0xa6, 0x81, 0x74 } } > +#define NS_RILCONTENTHELPER_CONTRACTID "@mozilla.org/ril/content-helper;1" nit: do we use this anywhere?

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #36) > ::: dom/network/src/MobileConnection.h > @@ +9,5 @@ > > #include "nsIDOMMobileConnection.h" > > #include "nsDOMEventTargetHelper.h" > > #include "nsCycleCollectionParticipant.h" > > > > +#define DOM_MOBILECONNECTION_WHITELIST_PREF "dom.mobileconnection.whitelist" > > If you really want to keep the string here, please use a static public > string in the |MobileConnection| class. Nah, this permission hack is gonna go away anyway, so I'll just inline for now like you did in SMS.

(In reply to Philipp von Weitershausen [:philikon] from comment #43) > Nah, this permission hack is gonna go away anyway, so I'll just inline for > now like you did in SMS. Inlining instead of #define would be okay.

Adjust documentation for `onvoicechange` and `ondatachange` (see comment 33)

Appease mounir and call BindToOwner(aWindow).

Leaves Telephony code alone and simply uses nsContentUtils::URIIsChromeOrInPref to do the permission check, as suggested by mounir. Also inlines pref name.

(In reply to Fabrice Desré [:fabrice] from comment #42) > Please fix The message manager leaks! Ooops, yes. Thanks for catching this. > ::: dom/system/gonk/nsRadioInterfaceLayer.h > @@ +43,5 @@ > > + > > +#define NS_RILCONTENTHELPER_CID \ > > + { 0x472816e1, 0x1fd6, 0x4405, \ > > + { 0x99, 0x6c, 0x80, 0x6f, 0x9e, 0xa6, 0x81, 0x74 } } > > +#define NS_RILCONTENTHELPER_CONTRACTID "@mozilla.org/ril/content-helper;1" > > nit: do we use this anywhere? Yes, the DOM uses this in Part 5 to acquire the RILContentHelper service.

Comment on attachment 613906 [details] [diff] [review] Part 3 (v2): Hook up to navigator Review of attachment 613906 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the |IsChallerChrome()| call removed. ::: dom/base/Navigator.cpp @@ +1155,5 @@ > + NS_ENSURE_TRUE(window && window->GetDocShell(), NS_OK); > + > + // Chrome is always allowed access, so do the permission check only > + // for non-chrome pages. > + if (!nsContentUtils::IsCallerChrome()) { You really don't need that check, you can rely on |nsContentUtils::URIIsChromeOrInPref|, IMO. Unless you *really* want to check if the caller is chrome instead of the URL being chrome... but that doesn't sound useful to me.

Thanks for the quick review! (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #49) > ::: dom/base/Navigator.cpp > @@ +1155,5 @@ > > + NS_ENSURE_TRUE(window && window->GetDocShell(), NS_OK); > > + > > + // Chrome is always allowed access, so do the permission check only > > + // for non-chrome pages. > > + if (!nsContentUtils::IsCallerChrome()) { > > You really don't need that check, you can rely on > |nsContentUtils::URIIsChromeOrInPref|, IMO. Unless you *really* want to > check if the caller is chrome instead of the URL being chrome... but that > doesn't sound useful to me. IIRC doc->NodePrincipal()->GetURI() won't work on chrome documents but is needed to supply nsContentUtils::URIIsChromeOrInPref with a uri. Also, nsContentUtils::URIIsChromeOrInPref just checks for the 'chrome://' scheme in the URI, but there are other URIs that could have chrome privileges (e.g. about:). So I think all in all nsContentUtils::IsCallerChrome is a good check to have here.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #35) > ::: dom/network/src/MobileConnection.h > @@ +14,5 @@ > > #define DOM_MOBILECONNECTION_WHITELIST_PREF "dom.mobileconnection.whitelist" > > > > +#define VOICE_CHANGED_EVENT_NAME NS_LITERAL_STRING("voicechanged") > > +#define DATA_CHANGED_EVENT_NAME NS_LITERAL_STRING("datachanged") > > +#define CARDSTATE_CHANGED_EVENT_NAME NS_LITERAL_STRING("cardstatechanged") > > Static strings in the class would be way better. I was just copying pretty much every other implementation (battery, network connection, etc.) out there. To be honest, it seems a bit silly to deviate from the defacto convention, but maybe I'm missing something?

(In reply to Philipp von Weitershausen [:philikon] from comment #51) > I was just copying pretty much every other implementation (battery, network > connection, etc.) out there. I did implement those ;) The difference is that the #define is in the cpp file which make it local and not available everywhere. You will find some code having some #define in headers but I'm not a big fan of that (you can easily have name conflicts for example). Having a static string on the class would make this way cleaner and more C++'y.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #52) > (In reply to Philipp von Weitershausen [:philikon] from comment #51) > > I was just copying pretty much every other implementation (battery, network > > connection, etc.) out there. > > I did implement those ;) The difference is that the #define is in the cpp > file which make it local and not available everywhere. You will find some > code having some #define in headers but I'm not a big fan of that (you can > easily have name conflicts for example). Having a static string on the class > would make this way cleaner and more C++'y. Ah, ok. So if I move the #defines to the cpp file to match your code, that's fine?

Addressed fabrice's feedback and added clean up for the message listeners. Request review from qDot for the RIL worker changes (just some mechanical reshufflings to make message passing easier, as well as signal strength computation changes.)

Addressed newline nits and moved defines from .h to .cpp file to match other DOM impls.

(In reply to Philipp von Weitershausen [:philikon] from comment #55) > Created attachment 613931 [details] [diff] [review] > Part 5 (v2): Fill in DOM implementation > > Addressed newline nits and moved defines from .h to .cpp file to match other > DOM impls. Yes.

Comment on attachment 613931 [details] [diff] [review] Part 5 (v2): Fill in DOM implementation Review of attachment 613931 [details] [diff] [review]: ----------------------------------------------------------------- I need some explanations before going forward with this patch. ::: dom/network/src/Makefile.in @@ +62,5 @@ > $(NULL) > > LOCAL_INCLUDES = \ > -I$(topsrcdir)/content/events/src \ > + -I$(topsrcdir)/dom/system/gonk \ That is scary... We shouldn't make something generic depending on a specific backend. I'm not even sure that would compile if we are not building for b2g with ril... ::: dom/network/src/MobileConnection.cpp @@ +21,5 @@ > namespace network { > > +const char* kVoiceChangedTopic = "mobile-connection-voice-changed"; > +const char* kDataChangedTopic = "mobile-connection-data-changed"; > +const char* kCardStateChangedTopic = "mobile-connection-cardstate-changed"; Could you make this static public in the class so callers will be able to do mozilla::dom::network::Class::kVoiceChangedTopic instead of re-define locally the topic name? @@ +109,1 @@ > return NS_OK; Could you add a MOZ_ASSERT() before the return, we shouldn't have a subject different from the ones we are subscribed to. @@ +147,5 @@ > + if (!mRIL) { > + *request = nsnull; > + return NS_OK; > + } > + return mRIL->GetNetworks(GetOwner(), request); Where is that implemented? Will the method handle correctly GetOwner() returning null? ::: dom/network/src/MobileConnection.h @@ +34,5 @@ > NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(MobileConnection, > nsDOMEventTargetHelper) > > private: > + nsCOMPtr<nsIRILContentHelper> mRIL; Where is that declared?

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #57) > ::: dom/network/src/Makefile.in > @@ +62,5 @@ > > $(NULL) > > > > LOCAL_INCLUDES = \ > > -I$(topsrcdir)/content/events/src \ > > + -I$(topsrcdir)/dom/system/gonk \ > > That is scary... We shouldn't make something generic depending on a specific > backend. Well, that's assuming this is meant to be generic right now. The WebTelephony API, for instance, has only one backend (gonk/ril) and so its implementation is therefore completely specific to the RIL. I think that's a fine compromise to make so long as it's unrealistic that we will have another backend any time soon. > I'm not even sure that would compile if we are not building for b2g with ril... That's a good point. I tried to make sure that the code will behave ok if the Gonk backend is disabled, but I may have overlooked this part. I see two possible options: (a) we admit that, for now, WebMobileConnection is RIL-only and simply #ifdef it with MOZ_B2G_RIL (b) we introduce an "nsIMobileConnectionProvider" (or similar) interface in dom/network. nsIRILContentHelper would extend that interface. That way dom/network doesn't have to include anything from dom/system/gonk. I'm fine with either one, with a slight preference for (a) since it would helps us land this bug more quickly; we could always do (b) as a follow-up. > @@ +147,5 @@ > > + if (!mRIL) { > > + *request = nsnull; > > + return NS_OK; > > + } > > + return mRIL->GetNetworks(GetOwner(), request); > > Where is that implemented? It's stubbed out in Part 4, but not actually implemented yet. I'm leaving this for a follow-up (bug 744344). > Will the method handle correctly GetOwner() returning null? I will make a note of that. OOC, why and when would GetOwner() return null? > ::: dom/network/src/MobileConnection.h > @@ +34,5 @@ > > NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(MobileConnection, > > nsDOMEventTargetHelper) > > > > private: > > + nsCOMPtr<nsIRILContentHelper> mRIL; > > Where is that declared? Part 4.

(In reply to Philipp von Weitershausen [:philikon] from comment #58) > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #57) > > ::: dom/network/src/Makefile.in > > @@ +62,5 @@ > > > $(NULL) > > > > > > LOCAL_INCLUDES = \ > > > -I$(topsrcdir)/content/events/src \ > > > + -I$(topsrcdir)/dom/system/gonk \ > > > > That is scary... We shouldn't make something generic depending on a specific > > backend. > > Well, that's assuming this is meant to be generic right now. The > WebTelephony API, for instance, has only one backend (gonk/ril) and so its > implementation is therefore completely specific to the RIL. I think that's a > fine compromise to make so long as it's unrealistic that we will have > another backend any time soon. We could have an Android backend. > > I'm not even sure that would compile if we are not building for b2g with ril... > > That's a good point. I tried to make sure that the code will behave ok if > the Gonk backend is disabled, but I may have overlooked this part. I see two > possible options: > > (a) we admit that, for now, WebMobileConnection is RIL-only and simply > #ifdef it with MOZ_B2G_RIL I do not like this approach. > (b) we introduce an "nsIMobileConnectionProvider" (or similar) interface in > dom/network. nsIRILContentHelper would extend that interface. That way > dom/network doesn't have to include anything from dom/system/gonk. I would prefer something in those lines. Or use hal/ or use something like dom/sms/ is doing. > I'm fine with either one, with a slight preference for (a) since it would > helps us land this bug more quickly; we could always do (b) as a follow-up. I do not like the idea of having a bad design and fixing it in a follow-up. However, this is an issue that a super-reviewer have to look at. I will follow Chris opinion here. > > Will the method handle correctly GetOwner() returning null? > > I will make a note of that. OOC, why and when would GetOwner() return null? I know it can return null but I can't tell you when nor why. Olli Pettay (smaug) could. But, as soon as you receive a pointer, you should assume it could be null.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #59) > > Well, that's assuming this is meant to be generic right now. The > > WebTelephony API, for instance, has only one backend (gonk/ril) and so its > > implementation is therefore completely specific to the RIL. I think that's a > > fine compromise to make so long as it's unrealistic that we will have > > another backend any time soon. > > We could have an Android backend. Sure. And when that time comes, we could refactor this code. I find it highly unlikely that we manage to get the abstractions 100% right now anyways. WebSMS is a good example of where we're struggling.

Comment on attachment 613931 [details] [diff] [review] Part 5 (v2): Fill in DOM implementation Review of attachment 613931 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/src/MobileConnection.cpp @@ +144,5 @@ > NS_IMETHODIMP > MobileConnection::GetNetworks(nsIDOMDOMRequest** request) > { > + if (!mRIL) { > + *request = nsnull; I don't think you want to return nsnull here. A request should always be returned. If mRil being null is unexpected and should never happen, you could throw an exception (and then return null). Otherwise, you should return a request and send an error event after going once trough the event loop.

My vote is for (b).

(In reply to Philipp von Weitershausen [:philikon] from comment #60) > Sure. And when that time comes, we could refactor this code. IMO, the DOM code should stay as much independent from the backend as possible. > I find it > highly unlikely that we manage to get the abstractions 100% right now > anyways. WebSMS is a good example of where we're struggling. That's another issue (not the code but the API). You might be right that 100% abstractions is hard to achieve but this is unfortunately something we should aim because Web APIs can't be changed easily and any adjustment to make an API working for a specific backend might be very hard to do as soon as the first B2G is shipped.

Sounds good! I'll update the patch to implement solution (b) and address mounir's other comments. Thanks!

Addressed Mounir's review comments: * Introduced an nsIMobileConnectionProvider interface. That way dom/network/ is independent of dom/systen/gonk. * Ensured to raise an exception in getNetworks() rather than return null when the provider can't be found. * Added an assertion to the bottom of Observe(). * I didn't make the topic strings available on the class because no other C++ code needs access to them. (Plus, the current definition is pretty consistent with many other modules.) This was originally part 5, but I swapped the order of part 4 and 5, so this is now part 4.

Rebased to make nsIRILContentHelper extend nsIMobileConnectionProvider.

Comment on attachment 616422 [details] [diff] [review] Part 4, ex 5 (v3): Fill in DOM implementation Review of attachment 616422 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comments applied. ::: dom/network/src/MobileConnection.cpp @@ +68,5 @@ > BindToOwner(aWindow); > + > + nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > + if (!obs) { > + return; Could you WARN or ASSERT here because that would mean observers are not added which is unlikely to be an expected behavior. @@ +81,5 @@ > MobileConnection::Shutdown() > { > + nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > + if (!obs) { > + return; You could also warn but it's likely less critical. @@ +111,5 @@ > + InternalDispatchEvent(CARDSTATE_CHANGED_EVENT_NAME); > + return NS_OK; > + } > + > + NS_ASSERTION(false, "Unknown observer topic!"); Don't use NS_ASSERTION(false, "") but NS_ERROR(""). Actually, I would prefer MOZ_ASSERT here. @@ +158,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + if (!GetOwner()) { > + return NS_ERROR_UNEXPECTED; FWIW, there are known situations where this could happen. What about just handling window = null in GetNetworks()? The request could get an error event sent to it.

Address mounir's review comments.

Forgot to qref. Here's the actual updated patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4209594a01af https://hg.mozilla.org/integration/mozilla-inbound/rev/bf86e0eb704a https://hg.mozilla.org/integration/mozilla-inbound/rev/6c0ce3fad8ae https://hg.mozilla.org/integration/mozilla-inbound/rev/466592cf0488 https://hg.mozilla.org/integration/mozilla-inbound/rev/ee7f4d1d2934 https://hg.mozilla.org/integration/mozilla-inbound/rev/880879bfc6a8

https://hg.mozilla.org/mozilla-central/rev/4209594a01af https://hg.mozilla.org/mozilla-central/rev/bf86e0eb704a https://hg.mozilla.org/mozilla-central/rev/6c0ce3fad8ae https://hg.mozilla.org/mozilla-central/rev/466592cf0488 https://hg.mozilla.org/mozilla-central/rev/ee7f4d1d2934 https://hg.mozilla.org/mozilla-central/rev/880879bfc6a8

One of those patches is missing the r= in the summary and none of those patches got approved. How could this have been landed?

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #72) > One of those patches is missing the r= in the summary Oops, my bad. > and none of those patches got approved. How could this have been landed? Patches that do not affect Fennec have blanket approval, according to https://wiki.mozilla.org/Tree_Rules.

(In reply to Philipp von Weitershausen [:philikon] from comment #73) > > and none of those patches got approved. How could this have been landed? > > Patches that do not affect Fennec have blanket approval, according to > https://wiki.mozilla.org/Tree_Rules. It depends how you define "affect". The tree rules meant patches that are not even compiled by the Native Fennec build which is not the case of those patches. There were very low risk for Native Fennec because there were simple empty classes but approval was still required.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #74) > It depends how you define "affect". The tree rules meant patches that are > not even compiled by the Native Fennec build which is not the case of those > patches. There were very low risk for Native Fennec because there were > simple empty classes but approval was still required. I see. I'll make sure to ask for approval in those cases in the future. Thanks!