631760 - don't hard code "/data/data/<package name>"

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

We hard code the /data/data/org.mozilla.fennec path in a few places in the code, to future proof we should be using the java APIs to get the correct path and set a GRE_HOME env var

Comment 1

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

Attachment: patch

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 510312 [details] [diff] [review]
patch

># HG changeset patch
># User Brad Lassey <[e-mail]>
># Date 1297102386 18000
># Node ID a044a3638a50ddff438bcb2eb905f3458be38d1a
># Parent  ea0ea576816fd658bc235f47506040b38574a58f
>[mq]: dont_hardcode_data_data
>
>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
>--- a/embedding/android/GeckoApp.java
>+++ b/embedding/android/GeckoApp.java
>@@ -69,6 +69,7 @@ abstract public class GeckoApp
>     public static GeckoSurfaceView surfaceView;
>     public static GeckoApp mAppContext;
>     public static boolean mFullscreen = false;
>+    public static File sGREDir = null;
>     static Thread mLibLoadThread = null;

the other public static variables are prefixed with 'm'.  Make this mGREDir, or convert the others.  Also, for extra points, surfaceView -> mSurfaceView.  Maybe others.

>     enum LaunchState {PreLaunch, Launching, WaitButton,
>@@ -170,6 +171,16 @@ abstract public class GeckoApp
>         Log.i("GeckoApp", "create");
>         super.onCreate(savedInstanceState);
> 
>+        if (sGREDir == null) {
>+            // If application.ini already exists in the old dir, use its parent.
>+            File app_ini =
>+                new File("/data/data/" + getPackageName() + "/application.ini");
>+            if (app_ini.exists())
>+                sGREDir = app_ini.getParentFile();
>+            else
>+                sGREDir = this.getDir("gre", 0);
>+        }

Does it make sense to turn this around and try to call this.getDir() and if it is null, then stat for /data/data/<package>/application.ini?


>+    char* greHome = getenv("GRE_HOME");
>+    if (!greHome) {
>+        LOG("Failed to get GRE_HOME from the env vars");
>+        return 0;
>+    }
>+    NS_ConvertUTF8toUTF16 gre_path(greHome);
>+    nsAutoString appini_path(gre_path);
>+    appini_path.AppendLiteral("/application.ini");

I hate our string classes, but there has to be a better way?

nsCString a;
a.Append(greHome);
a.AppendLiteral("/application.ini");

and use NS_NewNativeLocalFile?

Comment 3

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

(In reply to comment #2)
> Comment on attachment 510312 [details] [diff] [review]
> patch
> 
> ># HG changeset patch
> ># User Brad Lassey <[e-mail]>
> ># Date 1297102386 18000
> ># Node ID a044a3638a50ddff438bcb2eb905f3458be38d1a
> ># Parent  ea0ea576816fd658bc235f47506040b38574a58f
> >[mq]: dont_hardcode_data_data
> >
> >diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
> >--- a/embedding/android/GeckoApp.java
> >+++ b/embedding/android/GeckoApp.java
> >@@ -69,6 +69,7 @@ abstract public class GeckoApp
> >     public static GeckoSurfaceView surfaceView;
> >     public static GeckoApp mAppContext;
> >     public static boolean mFullscreen = false;
> >+    public static File sGREDir = null;
> >     static Thread mLibLoadThread = null;
> 
> the other public static variables are prefixed with 'm'.  Make this mGREDir, or
> convert the others.  Also, for extra points, surfaceView -> mSurfaceView. 
> Maybe others.
s is correct for static variables, I believe most of the instances of using m are from when we went from having one instance of GeckoApp through our app's life cycle to many. I'm happy to refactor in a follow up bug, but let's not muddy the waters here.

> 
> >     enum LaunchState {PreLaunch, Launching, WaitButton,
> >@@ -170,6 +171,16 @@ abstract public class GeckoApp
> >         Log.i("GeckoApp", "create");
> >         super.onCreate(savedInstanceState);
> > 
> >+        if (sGREDir == null) {
> >+            // If application.ini already exists in the old dir, use its parent.
> >+            File app_ini =
> >+                new File("/data/data/" + getPackageName() + "/application.ini");
> >+            if (app_ini.exists())
> >+                sGREDir = app_ini.getParentFile();
> >+            else
> >+                sGREDir = this.getDir("gre", 0);
> >+        }
> 
> Does it make sense to turn this around and try to call this.getDir() and if it
> is null, then stat for /data/data/<package>/application.ini?
Nope, we've been doing it "wrong" up to this point. The logic behind this is if we already have a functional GRE home, use it. If not use the correct/safe one (which will end up being /data/data/org.mozilla.fennec/app_gre).

> 
> 
> >+    char* greHome = getenv("GRE_HOME");
> >+    if (!greHome) {
> >+        LOG("Failed to get GRE_HOME from the env vars");
> >+        return 0;
> >+    }
> >+    NS_ConvertUTF8toUTF16 gre_path(greHome);
> >+    nsAutoString appini_path(gre_path);
> >+    appini_path.AppendLiteral("/application.ini");
> 
> I hate our string classes, but there has to be a better way?
> 
> nsCString a;
> a.Append(greHome);
> a.AppendLiteral("/application.ini");
> 
> and use NS_NewNativeLocalFile?

NS_NewNativeLocalFile does get rid of the need to convert the strings, new patch for that coming up.

Comment 4

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

Attachment: patch

Comment 5

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

pushed http://hg.mozilla.org/mozilla-central/rev/9f036ae7dd5d

Comment 6

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

pushed a follow up http://hg.mozilla.org/mozilla-central/rev/e24d6f8127b2

turns out we make some assumptions about the libpath in the child process