Closed
Bug 902783
Opened 11 years ago
Closed 11 years ago
Fx24 Beta: back out Bug 866957 and Bug 877725
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox24+ verified)
RESOLVED
FIXED
People
(Reporter: elan, Assigned: mfinkle)
References
Details
Attachments
(1 file)
(deleted),
patch
|
blassey
:
review+
liuche
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
We'll need to back out Bug 877725 of Nightly Fx26 and Aurora Fx25, too
Reporter | ||
Comment 1•11 years ago
|
||
Bug 902791 tracks the UI only back-out for Nightly and Aurora.
Assignee | ||
Comment 2•11 years ago
|
||
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.
Assignee | ||
Comment 3•11 years ago
|
||
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
Attachment #787329 -
Flags: review?(blassey.bugs)
Assignee | ||
Comment 4•11 years ago
|
||
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
Attachment #787329 -
Flags: review?(liuche)
Attachment #787329 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
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
Assignee | ||
Comment 7•11 years ago
|
||
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
Assignee | ||
Comment 8•11 years ago
|
||
The unpatched Beta would also have:
* connect and disconnect from Wi-Fi
* view Wi-Fi connections
Updated•11 years ago
|
OS: Mac OS X → Android
Hardware: x86 → All
Comment 9•11 years ago
|
||
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•11 years ago
|
||
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!
Attachment #787329 -
Flags: review?(liuche) → review+
Comment 11•11 years ago
|
||
why do we need to back this out?
Comment 12•11 years ago
|
||
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.
Attachment #787329 -
Flags: review?(blassey.bugs) → review+
Comment 13•11 years ago
|
||
> -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.
Assignee | ||
Comment 14•11 years ago
|
||
(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•11 years ago
|
||
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.
Attachment #787329 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•11 years ago
|
status-firefox24:
--- → affected
tracking-firefox24:
--- → +
Assignee | ||
Comment 16•11 years ago
|
||
Updated•11 years ago
|
Updated•11 years ago
|
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•