Closed Bug 1212679 Opened 4 years ago Closed 4 years ago

Make sure we handle being the default SMS app correctly

Categories

(B2GDroid Graveyard :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: reuben, Assigned: reuben)

References

Details

Attachments

(2 files, 2 obsolete files)

KK makes all the Telephony stuff read-only unless you're the default SMS app on the system. This bug is specifically to make sure everything works when we are the default app.
Depends on: 748391
Can we just import this code like this? Is the Apache license a problem at all?
Attachment #8683790 - Flags: feedback?(fabrice)
Sorry, I hit submit too early. We need those definitions to be able to parse downloaded MMS's, they're not exported so the only way to get them is by copying from the Android source code.

Code was imported from https://android.googlesource.com/platform/frameworks/opt/telephony/
Attached patch Handle default SMS/MMS intents (obsolete) — Splinter Review
SMS works, MMS still needs work. SMS is hard to test locally (you can do it on the emulator[0]), MMS is not testable at all, you need to send a real MMS from another phone to be able to test your code :(

Not to mention that this stuff is barely documented. The good references are the MMS demo[1] and the Telephony source code[2].

[0] http://developer.android.com/tools/devices/emulator.html#sms
[1] https://github.com/android/platform_development/tree/master/samples/ApiDemos/src/com/example/android/apis/os
[2] http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/5.1.1_r1/android/provider/Telephony.java
Comment on attachment 8683790 [details] [diff] [review]
Import MMS PDU definitions and helpers from Android

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

I defer to Gerv for the license aspect.

Reuben, is this code working on all releases >= KK ?
Attachment #8683790 - Flags: feedback?(fabrice) → feedback?(gerv)
Importing Apache-licensed code as whole files is fine - no licensing actions to be taken.

Gerv
(In reply to [:fabrice] Fabrice Desré from comment #4)
> Reuben, is this code working on all releases >= KK ?

Yep, all of this stuff was introduced with KK and hasn't changed. I just need to make sure I import the KK version of those MMS files.
Attachment #8683790 - Flags: feedback?(gerv) → feedback+
Duplicate of this bug: 1214561
This is the supporting code needed to save incoming WAP messages into the Android content provider, minus the DRM stuff which requires access to (even more) Android internals.
Attachment #8683790 - Attachment is obsolete: true
Attachment #8695670 - Flags: review?(fabrice)
With this patch b2gdroid can be set as the default messaging app and handle the relevant intents. The "default messaging app" API (if you can even call it that) is incredibly poorly designed and basically requires copying the code from Android.

Basically there's a few things you need to handle as the default app:

- Different intents for receiving messages. These are called *_DELIVER (vs RECEIVED), and they're only sent to the default app. When you're the default app you're required to persist incoming messages.
- Intents for other apps to trigger the SMS compose UI. This is done by creating an equivalent web activity on the Gecko side.

The extra code needed to persist SMS messages is in GeckoSmsManager.java. The code to persist MMS messages is added to b2gdroid only. Most of the MMS stuff is adapted from the Android telephony framework: https://android.googlesource.com/platform/frameworks/opt/telephony
Attachment #8683793 - Attachment is obsolete: true
Attachment #8695672 - Flags: review?(fabrice)
Comment on attachment 8695672 [details] [diff] [review]
Handle default SMS/MMS intents

James, can you review these changes, in particular the mobile/android/base/ stuff?
Attachment #8695672 - Flags: review?(snorp)
Oh, and if we ever bump the minimum compatibility to Lollipop we can get rid of a lot of this code, since they added an auto-persist option to SmsManager that takes care of most of the boring work, and lets apps focus on their actual features.
Attachment #8695672 - Flags: review?(snorp) → review+
Comment on attachment 8695672 [details] [diff] [review]
Handle default SMS/MMS intents

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

::: mobile/android/b2gdroid/app/src/main/AndroidManifest.xml
@@ +72,5 @@
>  
>          <meta-data android:name="com.sec.android.support.multiwindow" android:value="true"/>
>  
> +        <!-- Listen for incoming SMS messages -->
> +        <receiver android:name="org.mozilla.gecko.GeckoSmsManager"

nit: ".GeckoSmsManager" ?

@@ +119,5 @@
>              </intent-filter>
>  
> +            <!-- Handle SMS intents -->
> +            <intent-filter>
> +                <action android:name="android.intent.action.SEND" />                

nit: trailing whitespace

::: mobile/android/b2gdroid/app/src/main/java/org/mozilla/b2gdroid/Launcher.java
@@ +275,5 @@
> +            JSONObject obj = new JSONObject();
> +            try {
> +                obj.put("action", "send_sms");
> +                if (Intent.ACTION_SENDTO.equals(action)) {
> +                    String number = intent.getData().getPath();

nit: final String (or inline in obj.put)

@@ +278,5 @@
> +                if (Intent.ACTION_SENDTO.equals(action)) {
> +                    String number = intent.getData().getPath();
> +                    obj.put("number", number);
> +                }
> +                String body = intent.getStringExtra("sms_body");

same here.

::: mobile/android/b2gdroid/app/src/main/java/org/mozilla/b2gdroid/MmsService.java
@@ +14,5 @@
> +import android.net.Uri;
> +import android.os.Build;
> +import android.provider.Telephony;
> +import android.telephony.SmsManager;
> +import android.telephony.TelephonyManager;

that seems unused.
Attachment #8695672 - Flags: review?(fabrice) → review+
Attachment #8695670 - Flags: review?(fabrice) → review+
(In reply to [:fabrice] Fabrice Desré from comment #12)
> ::: mobile/android/b2gdroid/app/src/main/AndroidManifest.xml
> @@ +72,5 @@
> >  
> >          <meta-data android:name="com.sec.android.support.multiwindow" android:value="true"/>
> >  
> > +        <!-- Listen for incoming SMS messages -->
> > +        <receiver android:name="org.mozilla.gecko.GeckoSmsManager"
> 
> nit: ".GeckoSmsManager" ?

The "package" property of the manifest is "org.mozilla.b2gdroid", so that would resolve to org.mozilla.b2gdroid.GeckoSmsManager.

I've addressed the other comments.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29e263016b67
https://hg.mozilla.org/mozilla-central/rev/219dee692172
https://hg.mozilla.org/mozilla-central/rev/9a03868366df
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.