Closed Bug 720614 Opened 12 years ago Closed 12 years ago

SMS android permissions being requested on trunk builds

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox11 unaffected, firefox12 fixed)

RESOLVED FIXED
Firefox 12
Tracking Status
firefox11 --- unaffected
firefox12 --- fixed

People

(Reporter: blassey, Assigned: mbrubeck)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
Disable WebSMS by default.
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED
Attachment #591340 - Flags: review?(mounir)
(In reply to Brad Lassey [:blassey] from comment #0)
> what landed: http://mxr.mozilla.org/mozilla-central/source/configure.in#4637
> 
> isn't what got posted in the bug (and apparently not reviewed):
> https://bugzilla.mozilla.org/attachment.cgi?id=584789&action=diff#a/
> configure.in_sec1

What landed is what I said I would do. The patch in the bug wasn't doing what I said (it was untested).

Anyhow, if having websms enabled in Nightly is an issue, we should disable that in mozconfigs.
Attachment #591340 - Flags: review?(mounir) → review-
Comment on attachment 591340 [details] [diff] [review]
patch

r=blassey assuming you have tested this
Attachment #591340 - Flags: review- → review+
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #2)
> (In reply to Brad Lassey [:blassey] from comment #0)
> > what landed: http://mxr.mozilla.org/mozilla-central/source/configure.in#4637
> > 
> > isn't what got posted in the bug (and apparently not reviewed):
> > https://bugzilla.mozilla.org/attachment.cgi?id=584789&action=diff#a/
> > configure.in_sec1
> 
> What landed is what I said I would do. The patch in the bug wasn't doing
> what I said (it was untested).
> 
> Anyhow, if having websms enabled in Nightly is an issue, we should disable
> that in mozconfigs.

Mounir you landed that patch with out any review at all, not just your customary insufficient review. On top of that, I was pretty explicit in discussing this that we don't want it enabled for any fennec builds, including trunk. There was no way that wasn't clear to you.
(In reply to Brad Lassey [:blassey] from comment #4)
> Mounir you landed that patch with out any review at all, not just your
> customary insufficient review. On top of that, I was pretty explicit in
> discussing this that we don't want it enabled for any fennec builds,
> including trunk. There was no way that wasn't clear to you.

IIRC, I did ask a review for this patch saying that I wanted to have this enabled by default and being able to get disabled. khuey and you told me it was disabled by default and I must have attached another patch saying it will be ok like that. No one said nothing so I assumed it was. I guess there was a confusion with me thinking "it's now enabled by default" and you guys thinking "it's something I will be okay with". I did try to use the 'else' part of the autoconf macro but that doesn't seem to work.
I realize that it was actually disabled by default a few days before landing and I thought it wasn't worth re-asking a review given that I was changing the behavior for what I told it would be. I agree that I should ask a review for this change and I apologize for not doing this correctly.

However, if you want to have WebSMS disabled by default in all fennec builds, you should use --disable-websms-backend in the fennec mozconfigs. This patch is disabling this by default for all builds (including b2g builds). You can also add a condition in the configure.in if having this enabled in developer builds seems harmful.
Comment on attachment 591340 [details] [diff] [review]
patch

r- based on my previous comment: this is doing more than disabling WebSMS for Fennec.
Attachment #591340 - Flags: review-
Comment on attachment 591340 [details] [diff] [review]
patch

Mounir, b2g should enable it in its mozconfig if they want it

Basically since you landed without review, either stop r-'ing this patch or back all of WebSMS out. Resetting your r- to a ? and I'll take your response as a decision.
Attachment #591340 - Flags: review- → review?(mounir)
First of all, let's please leave personal feelings at the door, and interact like professionals.  Grievances should be aired through appropriate channels.

All "desktop" platforms are unaffected by the changes here, so we don't need to discuss them.

I think we all agree on these goals
 - SMS should be enabled for b2g
 - SMS should be disabled for firefox-android builds shipped to users (pending strategic discussion to take place elsewhere)

We want this SMS-android code for non-firefox uses of gecko-android, and likely firefox-android pending discussion.  Because of that, it would be nice to
 - leave SMS enabled for developer builds, so it doesn't bitrot

What comprises "developer builds" is less clear.  It's not really fair to bounce tryserver/m-i builds for breaking a feature that's not shipping yet.

Also, the SMS code is pretty well isolated, so the risk of bitrot seems fairly low to me.  Mounir, do you have more specific concerns about bitrottage?

To achieve all these goals, the options are
 1. Globally disable SMS by default, specifically enable it for b2g and local devs.
 2. Globally enable SMS by default, specifically disable it for all android mozconfigs in buildbot.

I lean towards (1) just because it's simpler.  Any objections?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #8)

> To achieve all these goals, the options are
>  1. Globally disable SMS by default, specifically enable it for b2g and
> local devs.
>  2. Globally enable SMS by default, specifically disable it for all android
> mozconfigs in buildbot.
> 
> I lean towards (1) just because it's simpler.  Any objections?

#1 seems ok to me as well. Unless I am missing something (I am not the smartest build stuff guy) Brubeck's patch would make #1 work, except we would want to add MOZ_WEBSMS_BACKEND=1 to b2g/confvars.sh, right?

http://mxr.mozilla.org/mozilla-central/source/b2g/confvars.sh

That would enable SMS for b2g and the b2g devs, I think.
I'm not intimately familiar with confvars.sh either, but that or changing this mozconfig option to --enable-websms and using that in b2g would be fine too.  Your idea seems simpler, so I like it better.
Comment on attachment 591340 [details] [diff] [review]
patch

I guess one r+ is enough.
Attachment #591340 - Flags: review?(mounir)
Blocks: 721459
Backed out because of Android build bustage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/547cea3b54fd

/builds/slave/m-in-andrd-xul/build/obj-firefox/config/nsinstall -D classes
"/tools/jdk6/bin/javac" -target 1.5 -source 1.5 -classpath /tools/android-sdk-r13/platforms/android-13/android.jar -bootclasspath /tools/android-sdk-r13/platforms/android-13/android.jar -encoding UTF8 -g  -d classes  /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoApp.java /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoAppShell.java /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoConnectivityReceiver.java /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoEvent.java /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoSurfaceView.java /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoInputConnection.java /builds/slave/m-in-andrd-xul/build/embedding/android/AlertNotification.java /builds/slave/m-in-andrd-xul/build/embedding/android/SurfaceInfo.java /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoBatteryManager.java /builds/slave/m-in-andrd-xul/build/embedding/android/VideoPlayer.java /builds/slave/m-in-andrd-xul/build/embedding/android/GeckoNetworkManager.java App.java Restarter.java NotificationHandler.java LauncherShortcuts.java SmsManager.java  CrashReporter.java R.java
SmsManager.java:41: cannot find symbol
symbol  : class GeckoSmsManager
location: package org.mozilla.gecko
import org.mozilla.gecko.GeckoSmsManager;
                        ^
SmsManager.java:51: cannot find symbol
symbol  : class GeckoSmsManager
location: class org.mozilla.gecko.SmsManager
      sInstance = new GeckoSmsManager();
                      ^
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
2 errors
Target Milestone: Firefox 12 → ---
Actually, this built successfully some of the time after the original landing; maybe it just requires a clobber.
I've clobbered Android on inbound and retriggered the failed build.  If it goes green, I will re-push with a notice that clobbering is required.
Re-landed after clobbering Android on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87a57afa0226
Target Milestone: --- → Firefox 12
https://hg.mozilla.org/mozilla-central/rev/87a57afa0226
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
> I think we all agree on these goals
>  - SMS should be enabled for b2g
>  - SMS should be disabled for firefox-android builds shipped to users
> (pending strategic discussion to take place elsewhere)

I notice that the current Firefox (10.0.0.2) and Beta (11.0) versions in the Android Marketplace both request permission to send and receive SMS. From looking at some marketplace reviews, it seems some other people have noticed this too...

Has this discussion taken place yet? Is there a reason this should be enabled for firefox-android currently?
(In reply to Mark Goodwin [:mgoodwin] from comment #18)
> I notice that the current Firefox (10.0.0.2) and Beta (11.0) versions in the
> Android Marketplace both request permission to send and receive SMS. From
> looking at some marketplace reviews, it seems some other people have noticed
> this too...
> 
> Has this discussion taken place yet? Is there a reason this should be
> enabled for firefox-android currently?

it is only 11.0 Beta 3 and it was a mistake: bug 728327
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.