HTML5 offline event support for Android

RESOLVED FIXED in mozilla2.0b10

Status

()

defect
P1
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

(Blocks 1 bug)

Trunk
mozilla2.0b10
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0+)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

9 years ago
Fennec for Android doesn't work HTML5 offline event

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

Updated

9 years ago
blocking2.0: --- → ?
Assignee

Updated

9 years ago
blocking2.0: ? → ---
tracking-fennec: --- → ?
Assignee

Comment 1

9 years ago
Posted patch fix v1 (obsolete) — Splinter Review

Comment 2

9 years ago
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.
Blocks: 616348
tracking-fennec: ? → 2.0+
Priority: -- → P1

Comment 3

9 years ago
What is the status here?
Assignee

Comment 4

9 years ago
(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
Assignee

Comment 5

9 years ago
Posted patch fix v2 (obsolete) — Splinter Review
Attachment #494647 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Attachment #502770 - Flags: review?(mwu)

Comment 6

9 years ago
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 <m_kato@ga2.so-net.ne.jp> (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());
>+    }
>+}
Attachment #502770 - Flags: review?(mwu) → review+
Assignee

Comment 7

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

Comment 8

9 years ago
Posted patch fix v3 (obsolete) — Splinter Review
Assignee

Comment 9

9 years ago
Posted patch fix v3.1Splinter Review
Attachment #504384 - Attachment is obsolete: true
Assignee

Comment 10

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

I forgot attaching network/system/android/*.
Attachment #504385 - Attachment is patch: true
Attachment #504385 - Attachment mime type: application/octet-stream → text/plain
Attachment #504385 - Flags: review?(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
Attachment #504385 - Flags: review?(mwu) → review+

Updated

9 years ago
Attachment #502770 - Attachment is obsolete: true
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";
Assignee

Comment 13

9 years ago
http://hg.mozilla.org/mozilla-central/rev/b78620dc5239
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10

Updated

9 years ago
Blocks: 627853

Updated

9 years ago
Blocks: 632526
You need to log in before you can comment on or make changes to this bug.