616075 - HTML5 offline event support for Android

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Makoto Kato [:m_kato]]

Fennec for Android doesn't work HTML5 offline event

- Implement nsINetworkLinkService that uses ConnectivityManager
- Use BroadcastReceiver for network link status

Comment 1

[Makoto Kato [:m_kato]]

Attachment: fix v1

Comment 2

[Michael Wu [:mwu]]

Looks good.

Can we use <receiver> to register the receiver instead? The GeckoApp gets destroyed and recreated when rotating, so we should avoid creating a new receiver on new GeckoApps. Another alternative would be to make mConnectivityReceiver static and only create/register it if it's null.

Are these offline events being forwarded to the child?

Do we know if nsINetworkLinkService is only used in the parent process? (otherwise we risk crashing the child process)

We tend to import everything instead of specific classes, so something like import android.content.*; in GeckoConnectivityReceiver.java would be better.

There should be no need to check for the bridge in the event handler in nsAppShell.cpp. If it's the child process, the event will never be sent, and if it's the parent process, the bridge should already be setup by that point.

Comment 3

[Stuart Parmenter]

What is the status here?

Comment 4

[Makoto Kato [:m_kato]]

(In reply to comment #2)
> Looks good.
> 
> Can we use <receiver> to register the receiver instead? The GeckoApp gets
> destroyed and recreated when rotating, so we should avoid creating a new
> receiver on new GeckoApps. Another alternative would be to make
> mConnectivityReceiver static and only create/register it if it's null.
> 
> Are these offline events being forwarded to the child?
> 
> Do we know if nsINetworkLinkService is only used in the parent process?
> (otherwise we risk crashing the child process)

I believe that it should work on parent process only.

> We tend to import everything instead of specific classes, so something like
> import android.content.*; in GeckoConnectivityReceiver.java would be better.

OK.

> There should be no need to check for the bridge in the event handler in
> nsAppShell.cpp. If it's the child process, the event will never be sent, and if
> it's the parent process, the bridge should already be setup by that point.

Since Bug 619670 is landed, I will use nsAppShell::NotifyObservers directly.


(In reply to comment #3)
> What is the status here?
I will post new patch soon

Comment 5

[Makoto Kato [:m_kato]]

Attachment: fix v2

Comment 6

[Michael Wu [:mwu]]

Comment on attachment 502770 [details] [diff] [review]
fix v2


Looks good. Most of my comments are nits, but there is one part that could potentially crash, and one string leak. Both of them are easy to fix so r=me with comments addressed.

>@@ -21,6 +22,15 @@
>     <application android:label="@MOZ_APP_DISPLAYNAME@"
> 		 android:icon="@drawable/icon"
> 		 android:debuggable="true">
>+
>+        <!-- for network link status -->
>+        <receiver android:name="org.mozilla.gecko.GeckoConnectivityReceiver"
>+                  android:enabled="true">

Shouldn't android:enabled default to true?

>+            <intent-filter>
>+                <action android:name="android.net.conn.CONNECTIVITY_CHANGE" />
>+            </intent-filter>
>+        </receiver>
>+

I suspect this receiver will catch any connectivity change regardless of whether our app has started. Have you checked whether that's safe? Calling into GeckoAppShell before loading libraries could have bad results, so we might have to check whether the app is initialized before calling into GeckoAppShell. Something like GeckoApp.checkLaunchState(GeckoApp.LaunchState.GeckoRunning) should work.

Also, it might look nicer if we put this receiver part near the other receiver section in this file.

>         <activity android:name="App"
>                   android:label="@MOZ_APP_DISPLAYNAME@"
>                   android:configChanges="keyboard|keyboardHidden|mcc|mnc"
>diff -r bcab2d6652cb embedding/android/GeckoAppShell.java
>--- a/embedding/android/GeckoAppShell.java	Tue Jan 11 17:31:38 2011 +1300
>+++ b/embedding/android/GeckoAppShell.java	Mon Jan 10 23:55:09 2011 -0800
>@@ -59,6 +59,8 @@
> 
> import android.util.*;
> import android.net.Uri;
>+import android.net.ConnectivityManager;
>+import android.net.NetworkInfo;
> 

Should replace all this with android.net.*; if possible.

>diff -r bcab2d6652cb embedding/android/GeckoConnectivityReceiver.java
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/embedding/android/GeckoConnectivityReceiver.java	Mon Jan 10 23:55:09 2011 -0800
>@@ -0,0 +1,63 @@
>+/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
>+ * ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * 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) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *

Happy new year! :)

>+ * Contributor(s):
>+ *   Makoto Kato <[e-mail]> (Original Author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+package org.mozilla.gecko;
>+
>+import android.content.*;
>+import android.net.ConnectivityManager;
>+import android.net.NetworkInfo;
>+

import android.net.*;

>+public class GeckoConnectivityReceiver extends BroadcastReceiver

I think in most cases we put extends on the next line, though there seem to be a few exceptions.

>+{
>+    @Override
>+    public void onReceive(Context context, Intent intent) {
>+        String status;
>+        ConnectivityManager cm = (ConnectivityManager)
>+            GeckoApp.mAppContext.getSystemService(Context.CONNECTIVITY_SERVICE);
>+        NetworkInfo info = cm.getActiveNetworkInfo();
>+        if (info == null) {
>+            status = "unknown";
>+        } else {
>+            if(!info.isConnected())

Add a space after if.

>+                status = "down";
>+            else
>+                status = "up";
>+        }
>+
>+        GeckoAppShell.onChangeNetworkLinkStatus(status);
>+    }
>+}
>diff -r bcab2d6652cb widget/src/android/AndroidJNI.cpp
>--- a/widget/src/android/AndroidJNI.cpp	Tue Jan 11 17:31:38 2011 +1300
>+++ b/widget/src/android/AndroidJNI.cpp	Mon Jan 10 23:55:09 2011 -0800
>@@ -132,3 +134,16 @@
> 
>     nsAppShell::gAppShell->RemoveObserver(sObserverKey);
> }
>+
>+NS_EXPORT void JNICALL
>+Java_org_mozilla_gecko_GeckoAppShell_onChangeNetworkLinkStatus(JNIEnv *jenv, jclass, jstring jStatus)
>+{
>+    if (nsAppShell::gAppShell) {

if (!nsAppShell::gAppShell)
    return;

>+        const jchar *status = jenv->GetStringChars(jStatus, NULL);
>+        nsString sStatus(status);
>+        sStatus.SetLength(jenv->GetStringLength(jStatus));

There is a nsJNIString class which will automatically convert jstrings to nsStrings for you. Otherwise you'd have to manually release the buffer returned by GetStringChars.

>+        nsAppShell::gAppShell->NotifyObservers(nsnull,
>+                                               NS_NETWORK_LINK_TOPIC,
>+                                               sStatus.get());
>+    }
>+}

Comment 7

[Makoto Kato [:m_kato]]

Michael, I'm sorry.  I forget attaching some files into patch!  I will attach new patch soon.

Comment 8

[Makoto Kato [:m_kato]]

Attachment: fix v3

Comment 9

[Makoto Kato [:m_kato]]

Attachment: fix v3.1

Comment 10

[Makoto Kato [:m_kato]]

Comment on attachment 504385 [details] [diff] [review]
fix v3.1

I forgot attaching network/system/android/*.

Comment 11

[Michael Wu [:mwu]]

Comment on attachment 504385 [details] [diff] [review]
fix v3.1

The new files look fine to me.

>@@ -21,6 +22,15 @@
>     <application android:label="@MOZ_APP_DISPLAYNAME@"
> 		 android:icon="@drawable/icon"
> 		 android:debuggable="true">
>+
>+        <!-- for network link status -->
>+        <receiver android:name="org.mozilla.gecko.GeckoConnectivityReceiver"
>+                  android:enabled="true">

Are you sure android:enabled="true" is necessary here? I think the documentation says it defaults to true, so this shouldn't be necessary.

>+public class GeckoConnectivityReceiver
>+    extends BroadcastReceiver
>+{
>+    @Override
>+    public void onReceive(Context context, Intent intent) {
>+        String status;
>+        ConnectivityManager cm = (ConnectivityManager)
>+            GeckoApp.mAppContext.getSystemService(Context.CONNECTIVITY_SERVICE);
>+        NetworkInfo info = cm.getActiveNetworkInfo();
>+        if (info == null) {
>+            status = "unknown";
>+        } else {
>+            if (!info.isConnected())
>+                status = "down";
>+            else
>+                status = "up";
>+        }
>+
>+        GeckoAppShell.onChangeNetworkLinkStatus(status);

As mentioned in the previous review, I would like a check here to make sure that gecko is still running before executing the rest of the function. This code can run even if the user hasn't started gecko, so it's probably worth being careful here. I'm not sure what happens if we call a native method without the library being loaded, but it's probably safer not to do it.

r=me with comments addressed

Comment 12

[Michael Wu [:mwu]]

Comment on attachment 504385 [details] [diff] [review]
fix v3.1

>+public class GeckoConnectivityReceiver
>+    extends BroadcastReceiver
>+{
>+    @Override
>+    public void onReceive(Context context, Intent intent) {
>+        String status;
>+        ConnectivityManager cm = (ConnectivityManager)
>+            GeckoApp.mAppContext.getSystemService(Context.CONNECTIVITY_SERVICE);
>+        NetworkInfo info = cm.getActiveNetworkInfo();
>+        if (info == null) {
>+            status = "unknown";
>+        } else {
>+            if (!info.isConnected())
>+                status = "down";
>+            else
>+                status = "up";
>+        }
>+

Oh, one more thing I just noticed - we should be able to simplify this to:

        if (info == null)
            status = "unknown";
        else if (!info.isConnected())
            status = "down";
        else
            status = "up";

Comment 13

[Makoto Kato [:m_kato]]

http://hg.mozilla.org/mozilla-central/rev/b78620dc5239