902783 - Fx24 Beta: back out Bug 866957 and Bug 877725

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Erin Lancaster [:elan]]

We'll need to back out Bug 877725 of Nightly Fx26 and Aurora Fx25, too

Comment 1

[Erin Lancaster [:elan]]

Bug 902791 tracks the UI only back-out for Nightly and Aurora.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: backout-wifi-cell-tracking

I am testing a build with this backout patch. I am basically backing out:
* bug 866957, the main bug (3 csets)
* bug 877725, the setting ui (1 cset)
* bug 882959, a crash fix (1 cset)
* bug 886921, a crash fix (1 cset)

The patch leaves the strings alone. They won't hurt anything and will reduce l10n churn.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 787329 [details] [diff] [review]
backout-wifi-cell-tracking

The beta build went fine. I tested the build. It launched fine. It displayed normal geolocation webpages fine. The settings for wifi tracking were removed, but otherwise things continued to work fine.

Ready for review

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 787329 [details] [diff] [review]
backout-wifi-cell-tracking

Preemptive approval request

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Needed for Product reasons
Testing completed (on m-c, etc.): None. This is a straight to beta patch
Risk to taking this patch (and alternatives if risky): Some risk. We'll need to do some general testing.
String or IDL/UUID changes made by this patch: I left the strings alone

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Here is my build in case anyone else can test:
http://people.mozilla.com/~mfinkle/fennec/fennec-24.0.en-US.android-arm.backout.apk

It is a mozilla-beta build using Beta branding. You might need to remove any Beta or Release builds from your device if you have issues with installing.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Note to testers: The backout affects the Settings UI and code fired during a geolocation reading. Of course, the app needs to load correctly, and without any new errors appearing in logcat.

In general:

1. Check that Settings still functions correctly for the rest of the settings.
2. Check that geo-location test pages still work and request permissions.
3. Check that no logcat errors appear while testing #1 or #2

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Another thing to look for is permissions. Compare the patched build (Fx 24 Beta) to the release version (Fx 23). You can find the permissions by going to the Android Settings > Apps > Firefox (or Firefox Beta)

The only new permissions should be:
* record audio
* control Near Field Communication

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

The unpatched Beta would also have:
* connect and disconnect from Wi-Fi
* view Wi-Fi connections

Comment 9

[Adrian Tamas (:AdrianT)]

Tested on Samsung Galaxy Tab 2 (Android 4.1.1) and HTC Desire HD (Android 2.3.5):
* The permissions: "connect and disconnect from Wi-Fi" and "view Wi-Fi connections" are not present in the test build
* I do not see any errors in logcat related to data reporting
* There is no crash when enabling location share
* Location share works over wifi and the share request doorhanger is displayed on maps.google.com and http://mozqa.com/data/firefox/geolocation/wifi.html

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 787329 [details] [diff] [review]
backout-wifi-cell-tracking

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

The UI stuff definitely looks good to me, they pretty much exactly match the patch I added the UI in, and I see you're intentionally leaving in the strings (from an earlier comment). I looked at the GeckoApp.java changes, and those seem fine, but should also be formally reviewed by blassey.

Thanks for doing this backout, mfinkle, especially at whatever hellish time zone you're in!

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

why do we need to back this out?

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 787329 [details] [diff] [review]
backout-wifi-cell-tracking

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

::: mobile/android/base/GeckoApp.java
@@ -2355,5 @@
>      public void onLocationChanged(Location location) {
>          // No logging here: user-identifying information.
>          GeckoAppShell.sendEventToGecko(GeckoEvent.createLocationEvent(location));
> -        if (mShouldReportGeoData)
> -            collectAndReportLocInfo(location);

If we want a lower-touch patch we can just remove these two lines and the permissions from the manifest.

Comment 13

[Aaron Train [:aaronmt]]

> -import java.io.InputStreamReader;

Was that an unused import?

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Another thing to look for is permissions. Compare the patched build (Fx 24
> Beta) to the release version (Fx 23). You can find the permissions by going
> to the Android Settings > Apps > Firefox (or Firefox Beta)
> 
> The only new permissions should be:
> * record audio
> * control Near Field Communication

Confirmed.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Aaron Train [:aaronmt] from comment #13)
> > -import java.io.InputStreamReader;
> 
> Was that an unused import?

It was added here:
https://hg.mozilla.org/mozilla-central/rev/92178edfeb6e (in bug 866957)

Must be unused or Java would yell at me.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 787329 [details] [diff] [review]
backout-wifi-cell-tracking

Approving for landing directly to the beta branch for a 24.0beta1 (build #3) respin.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d62df37a3891