713377 - Don't ask for SMS-related Android permissions in Firefox and Firefox Beta

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Where "Firefox" means official release.

We unfortunately can't do:
#if MOZ_UPDATE_CHANNEL != release && MOZ_UPDATE_CHANNEL != beta
(or #if !defined(MOZ_UPDATE_CHANNEL) || MOZ_UPDATE_CHANNEL == nightly || MOZ_UPDATE_CHANNEL == aurora)

GeckoSmsManager.java handles correctly the absence of those permissions so there is nothing to do in this area.

Comment 1

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

Comment on attachment 584206 [details] [diff] [review]
Patch v1

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

make this a configure flag

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

Actually, opt-out seems better so devs will have the permissions enabled for their own builds.

Comment 4

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

Comment on attachment 584234 [details] [diff] [review]
Patch v2.1

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

I don't want this based on branding. All Fennec builds should default to having SMS disabled because want to test in nightly and aurora what we're going to ship in beta and release.

I would suggest setting it to the disabled state in mobile/android/confvars.sh. The actual configure flags (--enable or --disable) are optional as far as I'm concerned, but I'd assume you want them for testing.

::: configure.in
@@ +4640,5 @@
>  USE_ARM_KUSER=
>  BUILD_CTYPES=1
>  MOZ_USE_NATIVE_POPUP_WINDOWS=
>  MOZ_ANDROID_HISTORY=
> +MOZ_ANDROID_SMS_PERMISSIONS=

As I've said, I much prefer disabling the whole implementation over just disabling the permissions and allowing the code to throw. Given that, this should be MOZ_ANDROID_SMS

@@ +5497,5 @@
> +MOZ_ARG_DISABLE_BOOL(android-sms-permissions,
> +[  --disable-android-sms-permissions
> +                          Disable Android SMS-related permissions (send/receive/read/write)].
> +    MOZ_ANDROID_SMS_PERMISSIONS=,
> +    MOZ_ANDROID_SMS_PERMISSIONS=1 )

set the default to disabled in mobile/android/confvars.sh and make this flag --enable-android-sms to override that

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Mounir, one quick question for you --- does the DOM spec say anything about error behavior when SMS capabilities are present on device, but possibly disabled?  If we continue going the route of disabling sms at the last possible second here, that's going to allow script to see a navigator.sms object, but it'll be useless.  So a user might run an sms app that looks like it should work, but doesn't ever do anything.

I'm asking because, I wonder if we should make navigator.sms return null for the sms-disabled case here.

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> Mounir, one quick question for you --- does the DOM spec say anything about
> error behavior when SMS capabilities are present on device, but possibly
> disabled?  If we continue going the route of disabling sms at the last
> possible second here, that's going to allow script to see a navigator.sms
> object, but it'll be useless.  So a user might run an sms app that looks
> like it should work, but doesn't ever do anything.

We do not handle that case. My guess is that should behave like SMS permissions not being granted (in the Web App sense, not Android one) but we don't know yet if we are going to have a one SMS permission or multiple ones. In the former case, it navigator.sms might return null, in the later, we will very likely throw error events like geolocation is doing.
I do not mind having all SMS methods throwing an error event even if there is an SMS object. What I would mind is to write very specific code just to handle that very specific case.

> I'm asking because, I wonder if we should make navigator.sms return null for
> the sms-disabled case here.

If Brad is okay with the code being built but never used, that is a viable solution: it doesn't add any new code path, it just requires a #ifdef somewhere.

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to Brad Lassey [:blassey] from comment #4)
> I don't want this based on branding. All Fennec builds should default to
> having SMS disabled because want to test in nightly and aurora what we're
> going to ship in beta and release.

There have been discussions to have that kind of behavior for web facing experimental API instead of prefixing. The rational is that if the feature is available in Aurora and Nightly we can have some testing but we still don't ship it so websites can't rely on it.
We can reduce to Nigthly only though.

> I would suggest setting it to the disabled state in
> mobile/android/confvars.sh. The actual configure flags (--enable or
> --disable) are optional as far as I'm concerned, but I'd assume you want
> them for testing.

Pushing code that is not built nor tested would be quite sad :( (and shooting ourselves in the foot)

> ::: configure.in
> @@ +4640,5 @@
> >  USE_ARM_KUSER=
> >  BUILD_CTYPES=1
> >  MOZ_USE_NATIVE_POPUP_WINDOWS=
> >  MOZ_ANDROID_HISTORY=
> > +MOZ_ANDROID_SMS_PERMISSIONS=
> 
> As I've said, I much prefer disabling the whole implementation over just
> disabling the permissions and allowing the code to throw. Given that, this
> should be MOZ_ANDROID_SMS

To give all the information needed, we have try/catch blocks not only for this we need them because we are calling methods that can fail (like DB-related methods). We do not allow the code to throw, it can throw. If it throws, we send an error event to the DOM, there is no hack.
If by disabling the whole implementation, you just want to be sure that we never run that code (without making it not compiled), we could do that but I do not understand why you prefer this over removing the permissions.

Comment 8

[Mounir Lamouri (:mounir)]

To make things clearer and improve the decision making process, I've distinguish the two issues we have here and the different solutions.

The first issue is on which builds/channels we want this to be enabled. The different solutions are:

A. Disable everywhere except when building for no channel (MOZ_UPDATE_CHANNEL=default)
 - very hard for anyone to test the feature

B. Allow also in Nightly
 + feature is testable in Nightly (behind two prefs)

C. Allow also in Aurora
 + we increase number of people who can test the feature

D. Allow in Beta and Final
 - it reaches the market, people are going to scream...

I agree that D is clearly not doable.
I do not see any reason to no try B or C. I admit that having Aurora coverage isn't highly required so I would be fine with B. I really don't like A because that would highly reduces the chance of anyone trying out this experimental feature (getting a build from our build bots or running a build on try is too complex).
I think B is a good compromise between making clear this feature isn't going to ship and still allowing it to be testable.


The second issue is *how* to disable this feature. The different solutions are:

1. Do not grant Android SMS-related permissions
   -> the API is usable but will always produce an error
 + very easy to maintain
 - binary has code for something that isn't allowed to success but can run

2. Make Android not supporting SMS (ie. treated like other platforms)
   -> the device will be shown as not capable of handling sms (navigotr.mozSms will be null)
 + easy to maintain
 - binary has some consequent amount of code that can't run

3. Disable the entire feature
   -> WebSMS will not appear to be implemented by the UA
 - hard to maintain (lot of #ifdef MOZ_WEBSMS)
 + binary do not bloat

My first take was to go for (1) because it was the best way to show that WebSMS isn't working on Beta and final because the permissions are not granted. If we stick with solutions A or B, we don't need that kind of feedback.
In that case (2) or (3) could make sense. I think (3)'s major disadvantages can be fixed if we have both code paths being run. I mean that on m-c we could have Android builds building with --enable-websms and non-Android builds building with --disable-websms. Thus, we are sure that both paths are always maintained.
Between (2) and (3), I'm not sure which one would be the best pick. Usually, we try to have Gecko for Firefox Desktop and Gecko for Firefox Mobile to be the same but here, that might be useless to have a feature being built for all platforms while only one will use it (two, with B2G). So maybe we could try (3)?


So, Brad, can we try to find a common ground with these data?
B3 seems like a good compromise between your concerns and mine.

Comment 9

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

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #7)
> (In reply to Brad Lassey [:blassey] from comment #4)
> > I don't want this based on branding. All Fennec builds should default to
> > having SMS disabled because want to test in nightly and aurora what we're
> > going to ship in beta and release.
> 
> There have been discussions to have that kind of behavior for web facing
> experimental API instead of prefixing. The rational is that if the feature
> is available in Aurora and Nightly we can have some testing but we still
> don't ship it so websites can't rely on it.
> We can reduce to Nigthly only though.
Regardless you shouldn't enable or disable it based on branding in the build system. Instead you'll need to provide an --enable/--disable flag for configure so the nightly/aurora/beta/final mozconfigs can set it 
> 
> > I would suggest setting it to the disabled state in
> > mobile/android/confvars.sh. The actual configure flags (--enable or
> > --disable) are optional as far as I'm concerned, but I'd assume you want
> > them for testing.
> 
> Pushing code that is not built nor tested would be quite sad :( (and
> shooting ourselves in the foot)
Why are you trying to push this? I had been assuming this was for b2g, in which case this would be built and tested there. Code is pushed that isn't built or tested under various configurations all the time.

> 
> > ::: configure.in
> > @@ +4640,5 @@
> > >  USE_ARM_KUSER=
> > >  BUILD_CTYPES=1
> > >  MOZ_USE_NATIVE_POPUP_WINDOWS=
> > >  MOZ_ANDROID_HISTORY=
> > > +MOZ_ANDROID_SMS_PERMISSIONS=
> > 
> > As I've said, I much prefer disabling the whole implementation over just
> > disabling the permissions and allowing the code to throw. Given that, this
> > should be MOZ_ANDROID_SMS
> 
> To give all the information needed, we have try/catch blocks not only for
> this we need them because we are calling methods that can fail (like
> DB-related methods). We do not allow the code to throw, it can throw. If it
> throws, we send an error event to the DOM, there is no hack.
The message passing function has a return value, you could just check that return value from the caller and throw the error there. In fact you should anyway, this really shouldn't be any additional code in the end.

> If by disabling the whole implementation, you just want to be sure that we
> never run that code (without making it not compiled), we could do that but I
> do not understand why you prefer this over removing the permissions.

I see no need to build code for a feature that we don't want in the product. Your explanation of the error handling further convinces me of that.

Comment 10

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

> So, Brad, can we try to find a common ground with these data?
> B3 seems like a good compromise between your concerns and mine.

3 certainly seems to be the way to go here. The A, B, C, D discussion doesn't really have to happen in this bug. There should be a configure flag which the mozconfigs that control what's in nightly/aurora/beta/final can set.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #8)
> The second issue is *how* to disable this feature. The different solutions
> are:
> 
> 1. Do not grant Android SMS-related permissions

> 2. Make Android not supporting SMS (ie. treated like other platforms)

> 3. Disable the entire feature

This decision is an issue of web API semantics when the API isn't implemented by a particular platform, and that semantic decision is orthogonal to code maintenance.  I'm told that semantic preference is to return null from unimplemented APIs instead of ifdef'ing it out to the point where it's |undefined|.  So that argues for (2) above.  Jonas needs to sr that, since it's a problem of the DOM module.

On android, we obviously need to ifdef out the permission request with any of those three options.  Beyond that, with option (2) it's not terribly critical what else is ifdef'd out, because all the android-backend code is unreachable.  What's the minimal patch maintenance-wise that gets rid of geckosmsmanager?  Brad, if we use the isSmsEnabled() helper you proposed in bug 674725, and then never create the geckosmsmanager singleton (so it remains null and any attempt to access it throws), does that satisfy your concerns?

Mounir, that should be simple to hack up --- can you give that a shot while we keep discussing here?

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Mounir, also we need tests for the sms-disabled state, that can start running on tinderbox now.

Comment 13

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #8)
> > The second issue is *how* to disable this feature. The different solutions
> > are:
> > 
> > 1. Do not grant Android SMS-related permissions
> 
> > 2. Make Android not supporting SMS (ie. treated like other platforms)
> 
> > 3. Disable the entire feature
> 
> This decision is an issue of web API semantics when the API isn't
> implemented by a particular platform, and that semantic decision is
> orthogonal to code maintenance.  I'm told that semantic preference is to
> return null from unimplemented APIs instead of ifdef'ing it out to the point
> where it's |undefined|.  So that argues for (2) above.  Jonas needs to sr
> that, since it's a problem of the DOM module.
> 
> On android, we obviously need to ifdef out the permission request with any
> of those three options.  Beyond that, with option (2) it's not terribly
> critical what else is ifdef'd out, because all the android-backend code is
> unreachable.  What's the minimal patch maintenance-wise that gets rid of
> geckosmsmanager?  Brad, if we use the isSmsEnabled() helper you proposed in
> bug 674725, and then never create the geckosmsmanager singleton (so it
> remains null and any attempt to access it throws), does that satisfy your
> concerns?

another (possibly cleaner) way to do this is to override handleMessage() in App.java.in. Something like this:

App.java.in:

    public void handleMessage(String event, JSONObject message) {
#ifdef MOZ_WEBSMS
    if (event.equals("WebSMS")) {
        GeckoSmsManager.handleMessage(event, message); // <-- static method
        return;
    }
#endif
        super.handleMessage(event, message);
    }

and then the makefile can build GeckoSmsManager.java conditionally

Makefile.in:

ifdef MOZ_WEBSMS
JAVAFILES += GeckoSmsManager.java
endif

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Patch v3

Comment 15

[Mounir Lamouri (:mounir)]

Basically this makes navigator.mozSms returns null if --disable-android-websms is used.

Whether we do and how we do prevent building GeckoSmsManager.java should be done in a follow-up I believe.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #12)
> Mounir, also we need tests for the sms-disabled state, that can start
> running on tinderbox now.

All tests I wrote are assuming sms isn't supported because Android mochitests are only a subset of mochitests (so they don't run this test unfortunately) and testing WebSMS methods isn't doable yet and should be in the scope of Marionette I believe.

Comment 16

[Mounir Lamouri (:mounir)]

Comment on attachment 584725 [details] [diff] [review]
Patch v3

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

::: config/autoconf.mk.in
@@ +143,5 @@
>  MOZ_DISABLE_PARENTAL_CONTROLS = @MOZ_DISABLE_PARENTAL_CONTROLS@
>  NS_ENABLE_TSF = @NS_ENABLE_TSF@
>  MOZ_SPELLCHECK = @MOZ_SPELLCHECK@
>  MOZ_ANDROID_HISTORY = @MOZ_ANDROID_HISTORY@
> +MOZ_ANDROID_WEBSMS = @MOZ_ANDROID_SMS_PERMISSIONS@

This typo is fixed locally.

Comment 17

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

Comment on attachment 584725 [details] [diff] [review]
Patch v3

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

r=blassey with comments addressed

::: configure.in
@@ +5497,5 @@
> +MOZ_ARG_DISABLE_BOOL(android-websms,
> +[  --disable-android-websms
> +                          Disable WebSMS backend for Android].
> +    MOZ_ANDROID_WEBSMS=,
> +    MOZ_ANDROID_WEBSMS=1 )

flip this to --enable-android-websms, because if I understand it correctly it will default to enabled if you don't pass the flag.

(khuey correct me if I'm wrong here)

Also, it makes more sense to me for this to be --enable-websms, no need for it to be android specific.

::: dom/sms/src/android/SmsService.cpp
@@ +51,5 @@
> +#ifdef MOZ_ANDROID_WEBSMS
> +    true;
> +#else
> +    false;
> +#endif

I'd prefer:

#ifdef MOZ_ANDROID_WEBSMS
    *aHasSupport = true;
#else
    *aHasSupport = false;
#endif

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 584725 [details] [diff] [review]
Patch v3

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

I'm cool with blassey handling this one.

::: configure.in
@@ +5497,5 @@
> +MOZ_ARG_DISABLE_BOOL(android-websms,
> +[  --disable-android-websms
> +                          Disable WebSMS backend for Android].
> +    MOZ_ANDROID_WEBSMS=,
> +    MOZ_ANDROID_WEBSMS=1 )

Correct.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Brad Lassey [:blassey] from comment #17)
> Comment on attachment 584725 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 584725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=blassey with comments addressed
> 
> ::: configure.in
> @@ +5497,5 @@
> > +MOZ_ARG_DISABLE_BOOL(android-websms,
> > +[  --disable-android-websms
> > +                          Disable WebSMS backend for Android].
> > +    MOZ_ANDROID_WEBSMS=,
> > +    MOZ_ANDROID_WEBSMS=1 )
> 
> flip this to --enable-android-websms, because if I understand it correctly
> it will default to enabled if you don't pass the flag.
> 
> (khuey correct me if I'm wrong here)

Correct.

Also, Splinter is dumb.

Comment 20

[Mounir Lamouri (:mounir)]

(In reply to Brad Lassey [:blassey] from comment #17)
> ::: configure.in
> @@ +5497,5 @@
> > +MOZ_ARG_DISABLE_BOOL(android-websms,
> > +[  --disable-android-websms
> > +                          Disable WebSMS backend for Android].
> > +    MOZ_ANDROID_WEBSMS=,
> > +    MOZ_ANDROID_WEBSMS=1 )
> 
> flip this to --enable-android-websms, because if I understand it correctly
> it will default to enabled if you don't pass the flag.

--disable-foo is used when foo is enabled by default and this is what I want to do.

> Also, it makes more sense to me for this to be --enable-websms, no need for
> it to be android specific.

I prefer to avoid any confusion with WebSMS feature being completely disabled. This is only for Android and as long as we don't try to disable the backend of another platform I don't see any reason to make it wider. Also, Gonk backend is not affected by this flag.

> ::: dom/sms/src/android/SmsService.cpp
> @@ +51,5 @@
> > +#ifdef MOZ_ANDROID_WEBSMS
> > +    true;
> > +#else
> > +    false;
> > +#endif
> 
> I'd prefer:
> 
> #ifdef MOZ_ANDROID_WEBSMS
>     *aHasSupport = true;
> #else
>     *aHasSupport = false;
> #endif

I actually did that to make sure that if #else is missing we do not compile. We need to set *aHasSupport and not doing so might result in unexpected behavior. Does that make sense?

Comment 21

[Mounir Lamouri (:mounir)]

Attachment: Patch v3.1

With, I hope, a correct use of MOZ_DISABLE_ARG_BOOL.

Comment 22

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

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #20)
> (In reply to Brad Lassey [:blassey] from comment #17)
> > ::: configure.in
> > @@ +5497,5 @@
> > > +MOZ_ARG_DISABLE_BOOL(android-websms,
> > > +[  --disable-android-websms
> > > +                          Disable WebSMS backend for Android].
> > > +    MOZ_ANDROID_WEBSMS=,
> > > +    MOZ_ANDROID_WEBSMS=1 )
> > 
> > flip this to --enable-android-websms, because if I understand it correctly
> > it will default to enabled if you don't pass the flag.
> 
> --disable-foo is used when foo is enabled by default and this is what I want
> to do.
but this is disabled by default. In the future, if we change this to enabled by default we can flip it around to --disable-foo

> 
> > Also, it makes more sense to me for this to be --enable-websms, no need for
> > it to be android specific.
> 
> I prefer to avoid any confusion with WebSMS feature being completely
> disabled. This is only for Android and as long as we don't try to disable
> the backend of another platform I don't see any reason to make it wider.
> Also, Gonk backend is not affected by this flag.
--enable-websms-backend then. There really is no reason for it to be platform specific

> > ::: dom/sms/src/android/SmsService.cpp
> > @@ +51,5 @@
> > > +#ifdef MOZ_ANDROID_WEBSMS
> > > +    true;
> > > +#else
> > > +    false;
> > > +#endif
> > 
> > I'd prefer:
> > 
> > #ifdef MOZ_ANDROID_WEBSMS
> >     *aHasSupport = true;
> > #else
> >     *aHasSupport = false;
> > #endif
> 
> I actually did that to make sure that if #else is missing we do not compile.
> We need to set *aHasSupport and not doing so might result in unexpected
> behavior. Does that make sense?

Not really. Its pretty clear that the else is there.

Comment 23

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

(In reply to Brad Lassey [:blassey] from comment #22)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #20)
> > (In reply to Brad Lassey [:blassey] from comment #17)
> > > ::: configure.in
> > > @@ +5497,5 @@
> > > > +MOZ_ARG_DISABLE_BOOL(android-websms,
> > > > +[  --disable-android-websms
> > > > +                          Disable WebSMS backend for Android].
> > > > +    MOZ_ANDROID_WEBSMS=,
> > > > +    MOZ_ANDROID_WEBSMS=1 )
> > > 
> > > flip this to --enable-android-websms, because if I understand it correctly
> > > it will default to enabled if you don't pass the flag.
> > 
> > --disable-foo is used when foo is enabled by default and this is what I want
> > to do.
> but this is disabled by default. In the future, if we change this to enabled
> by default we can flip it around to --disable-foo
The other alternative is to provide both and have the lack of a flag not change the default for the platform. So:

MOZ_ARG_DISABLE_BOOL(websms-backend,
[  --disable-websms-backend
                          Disable WebSMS backend].
    MOZ_WEBSMS_BACKEND=,)

MOZ_ARG_ENABLE_BOOL(websms-backend,
[  --enable-websms-backend
                          Enable WebSMS backend].
    MOZ_WEBSMS_BACKEND=1,)

Comment 24

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Mechanism to disable WebSMS backends + don't ask for Android permissions

Comment 25

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Don't build GeckoSmsManager.java is WebSMS backends are disabled

Comment 26

[Mounir Lamouri (:mounir)]

I prefer to keep WebSMS backends enabled by default. If Firefox Mobile doesn't want that feature in any channel, that's another story and we can add --disable-websms-backends in the appropriate mozconfigs. At least, keeping enabled by default guarantees us that the code doesn't bitrot...

Comment 27

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

Comment on attachment 584791 [details] [diff] [review]
Part 2 - Don't build GeckoSmsManager.java is WebSMS backends are disabled

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

1) I don't want to preprocess GeckoAppShell.java
2) use the message passing API

Also, MOZ_WEBSMS_BACKEND sounds better than MOZ_WEBSMS_BACKENDS since we'd presumably only have one backend per platform

Comment 28

[Mounir Lamouri (:mounir)]

(In reply to Brad Lassey [:blassey] from comment #27)
> Comment on attachment 584791 [details] [diff] [review]
> Part 2 - Don't build GeckoSmsManager.java is WebSMS backends are disabled
> 
> Review of attachment 584791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 1) I don't want to preprocess GeckoAppShell.java

I see only two ways to prevent building GeckoSmsManager.java without using the message passing API:
- Preprocess GeckoAppShell.java like I did.
- Call methods in App.java when sms-related methods in GeckoAppShell.java are called. Which means we are going to require App.java to call GeckoSmsManager. That design should work but seem odd to me because we would just move methods from GeckoAppShell.java in App.java thus increase the boilerplate.

Why GeckoAppShell.java shouldn't be preprocessed?

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #28)
> (In reply to Brad Lassey [:blassey] from comment #27)
> > Comment on attachment 584791 [details] [diff] [review]
> > Part 2 - Don't build GeckoSmsManager.java is WebSMS backends are disabled
> > 
> > Review of attachment 584791 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > 1) I don't want to preprocess GeckoAppShell.java
> 
> I see only two ways to prevent building GeckoSmsManager.java without using
> the message passing API

Here's a solution: make abstract ISmsManager.java.in, with a static get() method.  GeckoSmsManager implements ISmsManager.  When ENABLE_SMS, have get() return the GeckoSmsManager singleton.  Otherwise, return null.

Comment 30

[Mounir Lamouri (:mounir)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #29)
> Here's a solution: make abstract ISmsManager.java.in, with a static get()
> method.  GeckoSmsManager implements ISmsManager.  When ENABLE_SMS, have
> get() return the GeckoSmsManager singleton.  Otherwise, return null.

I do not understand how this is going to solve the problem because we are still going to do a runtime check:
if (ISmsManager.get()) {
  ISmsManager.get().foo();
}
(or something similar)

In addition get() is going to be declared as returning a GeckoSmsManager object but this declaration will fail if GeckoSmsManager.java isn't compiled.
In addition all methods colled in GeckoAppShell.java and GekoApp.java will not be declared in ISmsManager.java but in GeckoSmsManager.java.

We could fix that by havinig ISmsManager declaring all methods and forwarding them to GeckoSmsManager if MOZ_WEBSMS_BACKEND is defined. Though, that would be quite a huge boilerplate.
An alternative would be to have a GeckoSmsManager.java file and another GeckoSmsManagerFallback.java and we would compile one or the other depending on MOZ_WEBSMS_BACKEND. This solution might work but this is going to grow significantly the amount of code and paths to maintain so I really wonder if it's really worth doing it compared to just always compile GeckoSmsManager.java.

Comment 31

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #30)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #29)
> > Here's a solution: make abstract ISmsManager.java.in, with a static get()
> > method.  GeckoSmsManager implements ISmsManager.  When ENABLE_SMS, have
> > get() return the GeckoSmsManager singleton.  Otherwise, return null.
> 
> I do not understand how this is going to solve the problem because we are
> still going to do a runtime check:
> if (ISmsManager.get()) {
>   ISmsManager.get().foo();
> }
> (or something similar)
> 

The problem to solve was preprocessing.  This converts the #ifdef into a runtime check.

> In addition get() is going to be declared as returning a GeckoSmsManager
> object 

It returns ISmsManager.

> We could fix that by havinig ISmsManager declaring all methods and
> forwarding them to GeckoSmsManager if MOZ_WEBSMS_BACKEND is defined. Though,
> that would be quite a huge boilerplate.

ISmsManager declares abstract methods with no impl.  GeckoSmsManager implements them when enabled.  No worse than C++.

> An alternative would be to have a GeckoSmsManager.java file and another
> GeckoSmsManagerFallback.java and we would compile one or the other depending
> on MOZ_WEBSMS_BACKEND.

I don't understand how that would work.  javac will only emit Foo.class from a Foo.java source file.

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> The problem to solve was preprocessing.  This converts the #ifdef into a
> runtime check.
> 

And since this code is unreachable when SMS is disabled, the runtime check doesn't affect perf.

Comment 33

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Don't build GeckoSmsManager.java is WebSMS backends are disabled

This is following Chris' plan.

Comment 34

[Doug Turner (:dougt)]

Comment on attachment 586076 [details] [diff] [review]
Part 2 - Don't build GeckoSmsManager.java is WebSMS backends are disabled

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

I am assuming that the changes to mobile/android and mobile/xul are the same.

When ISmsManager is null, I wasn't completely sure what the return codes should be from GeckoAppShell.

I attempted to apply this patch and it has bitrotted a bunch.  I couldn't build or test with it.  Please update the patch and make sure you don't break the build or anything fun like that.

So, r+ with:

The copyright date fixed.
Renaming ISmsManager.java.in -> SmsManager.java.in (or similar)  -- or a follow up bug -- or an argument explaining why I* is fine even though we have an interface in the code without this prefix.

::: embedding/android/GeckoApp.java
@@ +413,5 @@
>          mBatteryReceiver = new GeckoBatteryManager();
>          registerReceiver(mBatteryReceiver, batteryFilter);
>  
> +        if (ISmsManager.getInstance() != null) {
> +            ISmsManager.getInstance().init();

Consider storing the value of getInstance() in this class to avoid the subsequent getInstance() calls.

::: embedding/android/GeckoAppShell.java
@@ +1700,5 @@
>      /*
>       * WebSMS related methods.
>       */
>      public static int getNumberOfMessagesForText(String aText) {
> +        if (ISmsManager.getInstance() == null) {

You also want a static class so that you can abstract/hide these if checks.

::: embedding/android/ISmsManager.java.in
@@ +14,5 @@
> + *
> + * The Original Code is Mozilla Android code.
> + *
> + * The Initial Developer of the Original Code is Mozilla Foundation.
> + * Portions created by the Initial Developer are Copyright (C) 2011

OMG 2012!

@@ +36,5 @@
> + * ***** END LICENSE BLOCK ***** */
> +
> +package org.mozilla.gecko;
> +
> +abstract class ISmsManager

not sure about "our" style guide, but (as a nit -- feel free to fight back), i don't like I prefixed to this class.  We already have real interfaces in our code that aren't prefixed:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/InputConnectionHandler.java

Also, why not just use a real interface type?

Comment 35

[Matt Brubeck (:mbrubeck)]

Requesting tracking-firefox11.  We need to fix this before Firefox 11 Beta is published to the market.

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/544fd25dfc61
https://hg.mozilla.org/mozilla-central/rev/4deac0f12db9

Comment 37

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

it looks like aurora doesn't have the SMS related permission requests, so we don't need this there