565142 - Support startup on Android

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, enhancement)

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This bug needs more description -- no idea what this means.  Also, why does this block the merge into m-c?

Comment 2

[Michael Wu [:mwu]]

This bug is for the code added to toolkit/xre. Patch coming soon.

Comment 3

[Michael Wu [:mwu]]

Attachment: Add Android support to toolkit/xre

The code here is mostly vlad's with a bunch of changes by myself, blassey, and alexp.

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 445897 [details] [diff] [review]
Add Android support to toolkit/xre


>+extern "C" {
>+    NS_EXPORT void JNICALL Java_org_mozilla_gecko_GeckoAppShell_nativeRun(JNIEnv *, jclass, jstring);
>+}

Can you just add extern "C" in front of the function declaration below and not have this forward declare?

>+static pthread_t gGeckoThread = 0;

comment its usage and lifetime.

>+static void *GeckoStart(void *);

comment its usage

>+NS_EXPORT void JNICALL
>+Java_org_mozilla_gecko_GeckoAppShell_nativeRun(JNIEnv *jenv, jclass jc, jstring jargs)
>+{

are any of these parameters null?  Specifically jenv.

>+    if (pthread_create(&gGeckoThread, NULL, GeckoStart, args) != 0) {
>+        LOG("pthread_create failed!");

Is there anything better to do than to LOG.  (same for the other failures in GeckoStart()


>+    setenv("TMPDIR", "/data/data/org.mozilla." MOZ_APP_NAME, 1);

Why?


>+        arg = strtok(NULL, " ");

s/NULL/nsnull

>+#if defined(ANDROID)
>+  Output(PR_FALSE, "scheduling restart");

Unneeded

>diff --git a/toolkit/xre/nsXREDirProvider.cpp b/toolkit/xre/nsXREDirProvider.cpp
>--- a/toolkit/xre/nsXREDirProvider.cpp
>+++ b/toolkit/xre/nsXREDirProvider.cpp
>@@ -1175,6 +1175,12 @@ nsXREDirProvider::GetUserDataDirectoryHo
> 
>   rv = NS_NewNativeLocalFile(nsDependentCString(appDir), PR_TRUE,
>                              getter_AddRefs(localDir));
>+#elif defined(ANDROID)
>+  // XXX temporary

Explain.

>+#elif defined(ANDROID)
>+  // XXX ANDROID hack
>+  rv = aFile->AppendNative(nsDependentCString("mozilla"));

Explain

Comment 5

[Michael Wu [:mwu]]

(In reply to comment #4)
> >+NS_EXPORT void JNICALL
> >+Java_org_mozilla_gecko_GeckoAppShell_nativeRun(JNIEnv *jenv, jclass jc, jstring jargs)
> >+{
> 
> are any of these parameters null?  Specifically jenv.
> 
Never. This function is called by the java VM/our java wrapper. We're screwed if jenv is null.

> >+    if (pthread_create(&gGeckoThread, NULL, GeckoStart, args) != 0) {
> >+        LOG("pthread_create failed!");
> 
> Is there anything better to do than to LOG.  (same for the other failures in
> GeckoStart()
> 
We're hosed if this fails. Same for most of the other LOGs in this file, except for the log reporting the return code of XRE_main.

> 
> >+    setenv("TMPDIR", "/data/data/org.mozilla." MOZ_APP_NAME, 1);
> 
> Why?
> 
No global /tmp directory is provided on Android. We set the temporary directory here so code depending on a temporary directory have a place to write to.

> >+#elif defined(ANDROID)
> >+  // XXX ANDROID hack
> >+  rv = aFile->AppendNative(nsDependentCString("mozilla"));
> 
> Explain
Probably not needed anymore, though it would be nice to use mozilla instead of .mozilla

Comment 6

[Michael Wu [:mwu]]

Attachment: Add Android support to toolkit/xre, v2

Forward declarations eliminated, some comments added.

Comment 7

[Michael Wu [:mwu]]

Attachment: Add Android support to toolkit/xre, v2.1

This one actually compiles

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 446848 [details] [diff] [review]
Add Android support to toolkit/xre, v2.1

Usually we don't like reaching into other modules this way.  Can you think of a better way?


> No global /tmp directory is provided on Android. We set the temporary directory here so code depending on a temporary directory have a place to write to.

What code depends on TMP in the environment?  If it is our code, we should change that to use the directory service.


The buffer you pass to GeckoStart() was created by ToNewUTF8String.  Based on the comments, it needs to be freed with |nsMemory::Free|:

http://mxr.mozilla.org/mozilla-central/source/xpcom/string/public/nsReadableUtils.h#114

Also add an assertion because I think ToNewUTF8String can fail.

What is up with the XXX's in the patch…. follow up bugs?


With those things fixed, r+.

Comment 9

[Michael Wu [:mwu]]

(In reply to comment #8)
> (From update of attachment 446848 [details] [diff] [review])
> Usually we don't like reaching into other modules this way.  Can you think of a
> better way?
> 
> 
? Which way are you talking about?

> > No global /tmp directory is provided on Android. We set the temporary directory here so code depending on a temporary directory have a place to write to.
> 
> What code depends on TMP in the environment?  If it is our code, we should
> change that to use the directory service.
> 
> 
IIRC that was necessary to make extensions install. I'll try removing it and see what breaks.

> What is up with the XXX's in the patch…. follow up bugs?
> 
Possibly. We need to make a decision as to whether to stick with those locations or use the standard unix profile location code.

> 
> With those things fixed, r+.
\o/

Comment 10

[Michael Wu [:mwu]]

Attachment: Add Android support to toolkit/xre, v3

Comment 11

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/9065d777cf62