564327 - Add Android widgets backend

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, enhancement)

Comment 1

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

Attachment: patch

related configure changes are being reviewed by ted in bug 561471

Comment 2

[Michael Wu [:mwu]]

Attachment: Add Android widget backend

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 444149 [details] [diff] [review]
Add Android widget backend

I don't want to be the reviewer of record here since I know almost nothing about Android or the Android port. The general structure looks OK though. The GLES code should be replaced by GL layers code, but I assume that's the plan?

Comment 4

[Michael Wu [:mwu]]

(In reply to comment #3)
> (From update of attachment 444149 [details] [diff] [review])
> I don't want to be the reviewer of record here since I know almost nothing
> about Android or the Android port. The general structure looks OK though. The
> GLES code should be replaced by GL layers code, but I assume that's the plan?

Yup. Just waiting for the layers magic to happen.

Will we need a reviewer for landing this then? It seems that anyone who knows about Android or the port probably already wrote some code for the widgets backend.

Comment 5

[Michael Wu [:mwu]]

Attachment: Add Java wrapper in embedding/android

This java wrapper code works with the widgets code to make things actually work.

Comment 6

[Michael Wu [:mwu]]

Attachment: Add Android widget backend, v2

Added some fixes from mozilla-droid.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 446069 [details] [diff] [review]
Add Android widget backend, v2


>+static PRUintn sJavaEnvThreadIndex = 0;
>+
>+AndroidBridge *AndroidBridge::sBridge = 0;

Comment usage and lifetime.

>+
>+static void
>+JavaThreadDetachFunc(void *arg)
>+{

Is arg ever null? maybe add: NS_ASSERTION(arg, "blah");

>+    /* NSS hack -- bionic doesn't handle recursive unloads correctly,
>+     * because library finalizer functions are called with the dynamic
>+     * linker lock still held.  This results in a deadlock when trying
>+     * to call dlclose() while we're already inside dlclose().
>+     * Conveniently, NSS has an env var that can prevent it from unloading.
>+     */
>+    putenv(strdup("NSS_DISABLE_UNLOAD=1"));

Is there a bug on file for this?


>+JNIEnv *
>+AndroidBridge::AttachThread(PRBool asDaemon)
>+{

is this ever called as a daemon?


>+#if 0
>+    ALOG("Attach success: %p", (void*)jEnv);
>+#endif

drop

>+void
>+AndroidBridge::SetSurfaceView(jobject obj)
>+{
>+    mSurfaceView.Init(obj);
>+}

Does it matter if SetSurfaceView is called multiple times?

>+
>+extern "C" JNIEnv * GetJNIForThread()
>+{
>+  return mozilla::AndroidBridge::JNIForThread();
>+}

4 spaces indentation in this file.  (now is the time to change it back to what I use!  2sp FTW!)


>+// Some debug #defines
>+#define ANDROID_DEBUG_EVENTS
>+#undef ANDROID_DEBUG_WIDGET

comment these out?


>+    static JNIEnv *JNI() {
>+        sBridge->AssertJNIThread();
>+        return sBridge->mJNIEnv;
>+    }

What is the Assert for?



>+    /* These are all implemented in Java */

For these methods - do do you want to have them follow some sort of naming convention?  It is not clear which are just stubs for java implemented functions.

>diff --git a/widget/src/android/AndroidJNI.cpp b/widget/src/android/AndroidJNI.cpp

And end here... (for a bit)

Comment 8

[Michael Wu [:mwu]]

(In reply to comment #7)
> >+
> >+static void
> >+JavaThreadDetachFunc(void *arg)
> >+{
> 
> Is arg ever null? maybe add: NS_ASSERTION(arg, "blah");
> 
Never. PR_NewThreadPrivateIndex guarantees that this destructor is only called with non null arg.

> >+    /* NSS hack -- bionic doesn't handle recursive unloads correctly,
> >+     * because library finalizer functions are called with the dynamic
> >+     * linker lock still held.  This results in a deadlock when trying
> >+     * to call dlclose() while we're already inside dlclose().
> >+     * Conveniently, NSS has an env var that can prevent it from unloading.
> >+     */
> >+    putenv(strdup("NSS_DISABLE_UNLOAD=1"));
> 
> Is there a bug on file for this?
> 
I couldn't find a bug for this in the android bugtracker - http://code.google.com/p/android/issues/list

> 
> >+JNIEnv *
> >+AndroidBridge::AttachThread(PRBool asDaemon)
> >+{
> 
> is this ever called as a daemon?
> 
Yup. It defaults to asDaemon=PR_TRUE;

> >+void
> >+AndroidBridge::SetSurfaceView(jobject obj)
> >+{
> >+    mSurfaceView.Init(obj);
> >+}
> 
> Does it matter if SetSurfaceView is called multiple times?
> 
Hmm.. we need to drop the reference to the old object if SetSurfaceView is called multiple times. Shouldn't happen but I'll add that in case we want to in the future.

> 
> >+    static JNIEnv *JNI() {
> >+        sBridge->AssertJNIThread();
> >+        return sBridge->mJNIEnv;
> >+    }
> 
> What is the Assert for?
> 
It's not an assertion. It ensures that we've attached this thread to the JVM, which is required to use sBridge->mJNIEnv. I guess something like EnsureJNIThread would make more sense.

> 
> 
> >+    /* These are all implemented in Java */
> 
> For these methods - do do you want to have them follow some sort of naming
> convention?  It is not clear which are just stubs for java implemented
> functions.
> 
Well, AndroidBridge is generally the gathering place for stubs that call into java. The functions which aren't stubs are mostly used outside AndroidBridge for creating other wrappers. I suspect we can limit the public methods to just java wrappers and a single function used for creating other wrappers.

> >diff --git a/widget/src/android/AndroidJNI.cpp b/widget/src/android/AndroidJNI.cpp
> 
> And end here... (for a bit)

\o/

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 446069 [details] [diff] [review]
Add Android widget backend, v2


>+extern "C" {
>+    NS_EXPORT void JNICALL Java_org_mozilla_gecko_GeckoAppShell_nativeInit(JNIEnv *, jclass);
>+    NS_EXPORT void JNICALL Java_org_mozilla_gecko_GeckoAppShell_notifyGeckoOfEvent(JNIEnv *, jclass, jobject event);
>+    NS_EXPORT void JNICALL Java_org_mozilla_gecko_GeckoAppShell_setSurfaceView(JNIEnv *jenv, jclass, jobject sv);
>+    NS_EXPORT void JNICALL Java_org_mozilla_gecko_GeckoAppShell_setInitialSize(JNIEnv *jenv, jclass, int width, int height);
>+    NS_EXPORT void JNICALL Java_org_mozilla_gecko_GeckoAppShell_putenv(JNIEnv *jenv, jclass, jstring map);
>+    NS_EXPORT int JNICALL Java_org_mozilla_gecko_GeckoApp_isInEmulator(JNIEnv *jenv, jclass);

nit: just add a space in front of the int so that things line up better.


>+    __android_log_print(ANDROID_LOG_INFO, "GeckoJNI", "putenv(%s)", str);

no need, or debug only, or print for each of the GeckoAppShell calls.

>+NS_EXPORT int JNICALL
>+Java_org_mozilla_gecko_GeckoApp_isInEmulator(JNIEnv *jenv, jclass)
>+{
>+    if (pget == NULL)
>+        pget = (property_get_func*) dlsym(NULL, "property_get_func");
>+
>+    if (pget == NULL)
>+        return 0;
>+
>+    char propval[92]; // PROPERTY_VALUE_MAX
>+    (*pget)("ro.kernel.qemu", propval, "");

Add a comment.  Like other variables, pget should be prefix to let us know it is a variable.

e.g.  +static property_get_func *s_pget = NULL; or something.  Also, use nsnull instead of NULL.

>+#undef initInit
>+#undef initClassGlobalRef
>+#undef getField
>+#undef getMethod  

Why?  Also, getMethod has some trailing white space (there are 50+ places in this patch that also have trailing whitespace)

>+    AndroidRect r(jenv, jenv->GetObjectField(wrappedObject(), jRectField));
>+    if (!r.isNull()) {
>+        mRect.SetRect(r.Left(),
>+                      r.Top(),
>+                      r.Right() - r.Left(),
>+                      r.Bottom() - r.Top());
>+    } else {
>+        mRect.Empty();
>+    }

what happens if r is null?  Does r.Left() throw, or just return 0?  In other words, can you drop the check and assign mRect


>+void
>+AndroidPoint::Init(JNIEnv *jenv, jobject jobj)
>+{
>+    NS_ASSERTION(wrapped_obj == nsnull, "Init called on non-null wrapped_obj!");
>+
>+    wrapped_obj = jobj;

i would prefer you use a prefix on globals.

>diff --git a/widget/src/android/Makefile.in b/widget/src/android/Makefile.in

all makefiles derived from one.  but lets be kind and update the copyright.


>+PRBool
>+nsAppShell::ProcessNextNativeEvent(PRBool mayWait)
>+{
>+    EVLOG("nsAppShell::ProcessNextNativeEvent %d", mayWait);
>+
>+    PR_Lock(mCondLock);
>+
>+    nsAutoPtr<AndroidGeckoEvent> curEvent;
>+    AndroidGeckoEvent *nextEvent;
>+
>+    curEvent = GetNextEvent();
>+    if (!curEvent && mayWait) {
>+        // hmm, should we really hardcode this 10s?
>+#if defined(ANDROID_DEBUG_EVENTS) && 0
>+        PRTime t0, t1;
>+        EVLOG("nsAppShell: waiting on mQueueCond");
>+        t0 = PR_Now();
>+#endif

drop if 0 code.

>+
>+        PR_WaitCondVar(mQueueCond, PR_MillisecondsToInterval(10000));

How did you come up with 10000?  Pref this?


>diff --git a/widget/src/android/nsAppShell.h b/widget/src/android/nsAppShell.h

2009 copyright


>+bool
>+nsIdleServiceAndroid::PollIdleTime(PRUint32 *aIdleTime)
>+{
>+    return false;
>+}

Is there plan to fix this?

>+// why do we have this?
>+static PRUintn gToolkitTLSIndex = 0;

okay, i give up. why


<and end again here>
>diff --git a/widget/src/android/nsToolkit.h b/widget/src/android/nsToolkit.h

Comment 10

[Michael Wu [:mwu]]

Attachment: Add Android widget backend, v3

Comment 11

[Michael Wu [:mwu]]

Attachment: Add Java wrapper in embedding/android, v2

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 447154 [details] [diff] [review]
Add Java wrapper in embedding/android, v2

>diff --git a/embedding/android/Makefile.in b/embedding/android/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/embedding/android/Makefile.in
>+JAVA=java
>+JAVAC=javac

Do we not check for these in configure? That's usually safer, because otherwise you get weird build errors instead of helpful configure errors.


>+JAVAFILES = \
>+	GeckoApp.java \
>+	GeckoAppShell.java \
>+	GeckoEvent.java \
>+	GeckoSurfaceView.java \
>+	$(NULL)

Prefer two-space indent for line continuations in Makefiles.

>+
>+DEFINES += -DMOZ_APP_DISPLAYNAME=$(MOZ_APP_DISPLAYNAME) \
>+	-DMOZ_APP_NAME=$(MOZ_APP_NAME)

If you're going to do line-continuations, it's nicer to do like you did above:
FOO += \
 blah \
 blah \
 $(NULL)

>+
>+GARBAGE = \

+=

>+GARBAGE_DIRS = res libs dist classes

+=

>+
>+ifdef JARSIGNER
>+    APKBUILDER_FLAGS += -u
>+endif

Where does JARSIGNER get set?

>+
>+ifeq ($(MOZ_APP_NAME),fennec)
>+ICON_PATH = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/fennec_48x48.png
>+ICON_PATH_HI = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/fennec_72x72.png
>+else
>+ICON_PATH = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/icon48.png
>+ICON_PATH_HI = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/icon64.png
>+endif

Ew. Can we make this a configure variable set in confvars.sh or something? We shouldn't be switching on appname in core Makefiles.

>+# We'll strip all the libs by default, due to size, but we might
>+# want to have a non-stripped version for debugging.  We should
>+# really pull out debuginfo and stuff.
>+DO_STRIP=$(STRIP)
>+ifdef NO_STRIP
>+DO_STRIP=echo not stripping
>+endif

I'd write this as
ifdef NO_STRIP
DO_STRIP=echo not stripping
else
DO_strip=$(STRIP)
endif

>+# rules.mk has some java stuff, but we're going to ignore it

Is it too hard to fix the rules.mk stuff to do what you want? I understand that it's focused on solving a different problem, but still.

>+# Note that we're going to set up a dependency directly between embed_android.dex and the java files
>+# Instead of on the .class files, since more than one .class file might be produced per .java file
>+classes.dex: $(JAVAFILES) $(MOZ_APP_NAME).java $(MOZ_APP_NAME)Restarter.java
>+	mkdir -p classes

$(NSINSTALL) -D classes (here and elsewhere)

>+AndroidManifest.xml :  AndroidManifest.xml.in
>+	$(PYTHON) $(topsrcdir)/config/Preprocessor.py \
>+             $(AUTOMATION_PPARGS) $(DEFINES) $(ACDEFINES) $< > $@
>+
>+$(MOZ_APP_NAME).java :  App.java.in
>+	$(PYTHON) $(topsrcdir)/config/Preprocessor.py \
>+             $(AUTOMATION_PPARGS) $(DEFINES) $(ACDEFINES) $< > $@
>+
>+$(MOZ_APP_NAME)Restarter.java :  Restarter.java.in
>+	$(PYTHON) $(topsrcdir)/config/Preprocessor.py \
>+             $(AUTOMATION_PPARGS) $(DEFINES) $(ACDEFINES) $< > $@

I think you can just use a static pattern rule for this, something like:

AndroidManifest.xml $(MOZ_APP_NAME).java $(MOZ_APP_NAME)Restarter.java : % : %.in
	$(PYTHON) $(topsrcdir)/config/Preprocessor.py \
             $(AUTOMATION_PPARGS) $(DEFINES) $(ACDEFINES) $< > $@

which says that for all those files, to build 'foo' from 'foo.in', run this rule.

http://www.gnu.org/software/make/manual/make.html#Static-Pattern


>+
>+dist: FORCE

Why bother making this FORCE? You could just make this a tools:: rule instead.

>+	mkdir -p dist
>+	$(DIST)/host/bin/host_xpt_link gecko.xpt $(DIST)/bin/components/*.xpt
>+	rm $(DIST)/bin/components/*.xpt
>+	mv gecko.xpt $(DIST)/bin/components
>+	@(for MANIFEST in $(DIST)/bin/chrome/*.manifest ; do cat "$$MANIFEST" >> chrome.manifest ; echo >> chrome.manifest ; rm "$$MANIFEST" ; done )
>+	mv chrome.manifest $(DIST)/bin/chrome/
>+	@(for PREF in $(DIST)/bin/defaults/pref/*.js ; do cat "$$PREF" >> prefs.js ; echo >> prefs.js ; rm "$$PREF" ; done )
>+	mv prefs.js $(DIST)/bin/defaults/
>+	@(for f in $(DIST_LINK_FILES) ; do if [ -e $(DIST)/bin/$$f ] ; then echo $$f ; ln -sf ../$(DIST)/bin/$$f dist ; fi ; done)

This is a little weird, since it's basically duplicating the stage-package code... Can you at least file a followup on making this use the packager code instead?

r- for a few things that should be cleaned up.

Comment 13

[Michael Wu [:mwu]]

Attachment: Add Java wrapper in embedding/android, v3

I think I addressed most of the review comments, but I couldn't get tools:: to work.

Comment 14

[Michael Wu [:mwu]]

(In reply to comment #12)
> >+ifdef JARSIGNER
> >+    APKBUILDER_FLAGS += -u
> >+endif
> 
> Where does JARSIGNER get set?
> 
It's not necessarily set. It's set on build/signing machines for signing with specific keys. Bug 562842 has more details.

> >+
> >+ifeq ($(MOZ_APP_NAME),fennec)
> >+ICON_PATH = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/fennec_48x48.png
> >+ICON_PATH_HI = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/fennec_72x72.png
> >+else
> >+ICON_PATH = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/icon48.png
> >+ICON_PATH_HI = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/icon64.png
> >+endif
> 
> Ew. Can we make this a configure variable set in confvars.sh or something? We
> shouldn't be switching on appname in core Makefiles.
> 
Definitely not. I'll file a bug for this.

> >+# rules.mk has some java stuff, but we're going to ignore it
> 
> Is it too hard to fix the rules.mk stuff to do what you want? I understand that
> it's focused on solving a different problem, but still.
> 
I don't know. Will need to investigate.

> I think you can just use a static pattern rule for this, something like:
> 
Done. Had to change some class names to get it to work.

> >+
> >+dist: FORCE
> 
> Why bother making this FORCE? You could just make this a tools:: rule instead.
> 
Replaced that line with tools:: - didn't seem to work.

> This is a little weird, since it's basically duplicating the stage-package
> code... Can you at least file a followup on making this use the packager code
> instead?
> 
Filed 567873.

Comment 15

[Michael Wu [:mwu]]

Attachment: Add Java wrapper in embedding/android, v4

updates to the dist dir generation rule.

Comment 16

[Michael Wu [:mwu]]

Attachment: Add Android widget backend, v4

Some minor cleanups

Comment 17

[Doug Turner (:dougt)]

Comment on attachment 447375 [details] [diff] [review]
Add Java wrapper in embedding/android, v4

>diff --git a/configure.in b/configure.in

leaving build/config changes for ted.  hi ted!

>+        }
>+        GeckoAppShell.runGecko(getApplication().getPackageResourcePath(), i.getStringExtra("args"), i.getDataString());

wrap

>+    }
>+
>+    /** Called when the activity is first created. */
>+    @Override
>+    public void onCreate(Bundle savedInstanceState)
>+    {
>+        super.onCreate(savedInstanceState);
>+
>+        appContext = this;


Do you mind using some member prefix, like mAppContext?

>+                                                  ViewGroup.LayoutParams.FILL_PARENT));
>+
>+        useSoftwareDrawing = true; //isInEmulator() == 1;

It wasn't clear why this was set here (maybe a comment to say that the default is to use software drawing).  Maybe you should use a preferences set in the xml packaging?  

>+            if (useLaunchButton) {

Nit.  I am not very happy with this.  Can we drop it, or support it with some sort of pref? 


>+    @Override
>+    public void onPause()
>+    {
>+        // The user is navigating away from this activity, but nothing
>+        // has come to the foreground yet; for Gecko, we may want to
>+        // stop repainting, for example.
>+
>+        // Whatever we do here should be fast, because we're blocking
>+        // the next activity from showing up until we finish.
>+
>+        // onPause will be followed by either onResume or onStop.
>+        super.onPause();
>+    }

Do we have to override if we do nothing?

>+
>+    @Override
>+    public void onResume()
>+    {
>+        // After an onPause, the activity is back in the foreground.
>+        // Undo whatever we did in onPause.
>+        super.onResume();
>+    }


Same.  Do we have to override if we do nothing?  There are a few other places here.


>+        // in onXreExit.
>+        if (GeckoAppShell.sGeckoRunning) {
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.ACTIVITY_STOPPING));
>+        }

You do not need to check sGeckoRunning here (sendEventToGecko does that for you).


>+            File componentsDir = new File("/data/data/org.mozilla." + getAppName() +"/components");

Don't have a device handy to verify this -- but "/data/data" or "/data"

>+        } catch (Exception e) {
>+            Log.i("GeckoAppJava", e.toString());
>+            return;

How about one catch over this entire method.  

>+        ZipEntry componentsList = zip.getEntry("components/components.list");
>+        if (componentsList == null) {

And throw here.


>+    public String getEnvString() {

I really don't understand this function/Why that some keys are



>+    public void doRestart() {
>+        try {
>+            String action = "org.mozilla.gecko.restart" + getAppName();
>+            String amCmd = "/system/bin/am broadcast -a " + action + getEnvString() + " -n org.mozilla." + getAppName() + "/org.mozilla." + getAppName() + ".Restarter";

wrap.  Also, isnt' a space required between action and getEnvString()?


>+        // if we need to split things up by version number, we can do things this way --
>+        // it would mean pulling out all android-specific code into its own shared lib
>+//        if (Build.VERSION.SDK_INT == Build.VERSION_CODES.DONUT) {
>+//            System.loadLibrary("GeckoAndroidBridge-a16");
>+//        } else if (Build.VERSION.SDK_INT == 5 /*Build.VERSION_CODES.ECLAIR*/ ||
>+//                   Build.VERSION.SDK_INT == 6 /*Build.VERSION_CODES.ECLAIR_MR1*/)
>+//        {
>+//            System.loadLibrary("GeckoAndroidBridge-a20");
>+//        }


If you really want to keep this code here, please keep the comments aligned, but I would rather you drop this code.

>+        // First argument is the .apk path
>+        String combinedArgs = "";
>+        if (apkPath != null)
>+            combinedArgs += apkPath;

String combinedArgs = apkgPath;


>+    public static void sendEventToAndroid() {
>+        // todo
>+    }

Tell me more.

>+    public static void scheduleRedraw() {
>+        scheduleRedraw(0, -1, -1, -1, -1);
>+    }

What is this magic.  does -1 for h and w have some known behavior?


>+        GeckoApp.surfaceView.mIMEState = state;
>+        if (state != 0)
>+            Log.i("GeckoAppJava", "requestFocus - " + GeckoApp.surfaceView.requestFocus());

No branching for log statements.

>+
>+    static void onXreExit() {
>+        sGeckoRunning = false;
>+        Log.i("GeckoAppJava", "XRE exited");
>+        if (gRestartScheduled) {
>+            GeckoApp.appContext.doRestart();
>+        } else {
>+            Log.i("GeckoAppJava", "we're done, good bye");
>+            System.exit(0);
>+        }
>+
>+    }
>+    static void scheduleRestart() {
>+        Log.i("GeckoAppJava", "scheduling restart");
>+        gRestartScheduled = true;        
>+    }

Where is gRestartScheduled defined?


>+/*
>+    public static void ShowWindow(GeckoView vw) {
>+        if (GeckoApp.mainLayout.indexOfChild(vw) == -1) {
...

>+*/
>+}

Drop dead code.



>+    public int mType;
>+    public int mAction;
>+    public long mTime;
>+    public Point mP0, mP1;
>+    public Rect mRect;
>+    public float mX, mY, mZ;
>+
>+    public int mMetaState, mFlags;
>+    public int mKeyCode, mUnicodeChar;
>+    public int mCount, mCount2;
>+    public String mCharacters;
>+

I asked about this in person.  It wasn't plainly obvious that these attributes are really the attributes that android provides in their event structure.  A comment would be helpful.


>+class GeckoSurfaceView

jeff/joe would be a better reviewer than I on this.  jeff, could you review the GeckoSurfaceView class for correctness?


mwu, r+ with comments addressed

Comment 18

[Michael Wu [:mwu]]

(In reply to comment #17)
> >+    }
> >+
> >+    /** Called when the activity is first created. */
> >+    @Override
> >+    public void onCreate(Bundle savedInstanceState)
> >+    {
> >+        super.onCreate(savedInstanceState);
> >+
> >+        appContext = this;
> 
> 
> Do you mind using some member prefix, like mAppContext?
> 
Will do. 

> >+                                                  ViewGroup.LayoutParams.FILL_PARENT));
> >+
> >+        useSoftwareDrawing = true; //isInEmulator() == 1;
> 
> It wasn't clear why this was set here (maybe a comment to say that the default
> is to use software drawing).  Maybe you should use a preferences set in the xml
> packaging?  
> 
Vlad put in fallback code for when GLES2 isn't available (when we're in an emulator). The fallback code ended up performing better than GLES2 so this switched to always using it, with the old check commented out. If vlad gets this working fast, we'll want to do proper detection here.

> >+            if (useLaunchButton) {
> 
> Nit.  I am not very happy with this.  Can we drop it, or support it with some
> sort of pref? 
> 
The launch button isn't shown by default.

> 
> >+    @Override
> >+    public void onPause()
> >+    {
> >+        // The user is navigating away from this activity, but nothing
> >+        // has come to the foreground yet; for Gecko, we may want to
> >+        // stop repainting, for example.
> >+
> >+        // Whatever we do here should be fast, because we're blocking
> >+        // the next activity from showing up until we finish.
> >+
> >+        // onPause will be followed by either onResume or onStop.
> >+        super.onPause();
> >+    }
> 
> Do we have to override if we do nothing?
> 
@Override is an annotation saying we're overriding some other method so javac complains if we're not actually doing that.

> >+        // in onXreExit.
> >+        if (GeckoAppShell.sGeckoRunning) {
> >+            GeckoAppShell.sendEventToGecko(new GeckoEvent(GeckoEvent.ACTIVITY_STOPPING));
> >+        }
> 
> You do not need to check sGeckoRunning here (sendEventToGecko does that for
> you).
> 
Ok.

> 
> >+            File componentsDir = new File("/data/data/org.mozilla." + getAppName() +"/components");
> 
> Don't have a device handy to verify this -- but "/data/data" or "/data"
> 
/data/data is in fact correct. (first data is for the data partition, second data is application data)

> >+        } catch (Exception e) {
> >+            Log.i("GeckoAppJava", e.toString());
> >+            return;
> 
> How about one catch over this entire method.  
> 
> >+        ZipEntry componentsList = zip.getEntry("components/components.list");
> >+        if (componentsList == null) {
> 
> And throw here.
> 
> 
I'll see - sounds like a better plan.

> >+    public String getEnvString() {
> 
> I really don't understand this function/Why that some keys are
> 
what

> 
> 
> >+    public void doRestart() {
> >+        try {
> >+            String action = "org.mozilla.gecko.restart" + getAppName();
> >+            String amCmd = "/system/bin/am broadcast -a " + action + getEnvString() + " -n org.mozilla." + getAppName() + "/org.mozilla." + getAppName() + ".Restarter";
> 
> wrap.  Also, isnt' a space required between action and getEnvString()?
> 

Nope that's just how brad wrote this code.

> 
> >+        // if we need to split things up by version number, we can do things this way --
> >+        // it would mean pulling out all android-specific code into its own shared lib
> >+//        if (Build.VERSION.SDK_INT == Build.VERSION_CODES.DONUT) {
> >+//            System.loadLibrary("GeckoAndroidBridge-a16");
> >+//        } else if (Build.VERSION.SDK_INT == 5 /*Build.VERSION_CODES.ECLAIR*/ ||
> >+//                   Build.VERSION.SDK_INT == 6 /*Build.VERSION_CODES.ECLAIR_MR1*/)
> >+//        {
> >+//            System.loadLibrary("GeckoAndroidBridge-a20");
> >+//        }
> 
> 
> If you really want to keep this code here, please keep the comments aligned,
> but I would rather you drop this code.
> 
Will drop. Having alternate libs will make our bloat problem worse.

> >+        // First argument is the .apk path
> >+        String combinedArgs = "";
> >+        if (apkPath != null)
> >+            combinedArgs += apkPath;
> 
> String combinedArgs = apkgPath;
> 
Ah. Will do.

> 
> >+    public static void sendEventToAndroid() {
> >+        // todo
> >+    }
> 
> Tell me more.
> 
I would if I knew. Vlad probably had some plan for this. Going to drop for now.

> >+    public static void scheduleRedraw() {
> >+        scheduleRedraw(0, -1, -1, -1, -1);
> >+    }
> 
> What is this magic.  does -1 for h and w have some known behavior?
> 
I think it means redraw everything.

> 
> >+        GeckoApp.surfaceView.mIMEState = state;
> >+        if (state != 0)
> >+            Log.i("GeckoAppJava", "requestFocus - " + GeckoApp.surfaceView.requestFocus());
> 
> No branching for log statements.
> 
I'll just drop this. It's old debugging code I left in.

> >+
> >+    static void onXreExit() {
> >+        sGeckoRunning = false;
> >+        Log.i("GeckoAppJava", "XRE exited");
> >+        if (gRestartScheduled) {
> >+            GeckoApp.appContext.doRestart();
> >+        } else {
> >+            Log.i("GeckoAppJava", "we're done, good bye");
> >+            System.exit(0);
> >+        }
> >+
> >+    }
> >+    static void scheduleRestart() {
> >+        Log.i("GeckoAppJava", "scheduling restart");
> >+        gRestartScheduled = true;        
> >+    }
> 
> Where is gRestartScheduled defined?
> 
line 69: static private boolean gRestartScheduled = false;

> 
> >+/*
> >+    public static void ShowWindow(GeckoView vw) {
> >+        if (GeckoApp.mainLayout.indexOfChild(vw) == -1) {
> ...
> 
> >+*/
> >+}
> 
> Drop dead code.
> 
Ok. Wish I knew what this code was about.

> 
> 
> >+    public int mType;
> >+    public int mAction;
> >+    public long mTime;
> >+    public Point mP0, mP1;
> >+    public Rect mRect;
> >+    public float mX, mY, mZ;
> >+
> >+    public int mMetaState, mFlags;
> >+    public int mKeyCode, mUnicodeChar;
> >+    public int mCount, mCount2;
> >+    public String mCharacters;
> >+
> 
> I asked about this in person.  It wasn't plainly obvious that these attributes
> are really the attributes that android provides in their event structure.  A
> comment would be helpful.
> 
Ok.

Comment 19

[Michael Wu [:mwu]]

Comment on attachment 447375 [details] [diff] [review]
Add Java wrapper in embedding/android, v4

Clearing review. We expect a large amount of the drawing code to be rewritten for gles/layers.

Comment 20

[Doug Turner (:dougt)]

Comment on attachment 447560 [details] [diff] [review]
Add Android widget backend, v4

>+    /* NSS hack -- bionic doesn't handle recursive unloads correctly,
>+     * because library finalizer functions are called with the dynamic
>+     * linker lock still held.  This results in a deadlock when trying
>+     * to call dlclose() while we're already inside dlclose().
>+     * Conveniently, NSS has an env var that can prevent it from unloading.
>+     */
>+    putenv(strdup("NSS_DISABLE_UNLOAD=1"));
>+

Please create a new mozilla bug and cite it here.  It isn't really clear how we are going to fix this other than hack around it.

>+        KEYCODE_MEDIA_REWIND    = 89,
>+        KEYCODE_MEDIA_FAST_FORWARD = 90,
>+        KEYCODE_MUTE            = 91,
>+        ACTION_DOWN             = 0,
>+        ACTION_UP               = 1,
>+        ACTION_MULTIPLE         = 2,
>+        META_ALT_ON = 0x02,

It looked so pretty until you go to the META keys.  Please align the = chars.



>+    mQueueLock = PR_NewLock();
>+    mCondLock = PR_NewLock();
>+    mQueueCond = PR_NewCondVar(mCondLock);

These can fail.

>+#if defined(ANDROID_DEBUG_EVENTS)
>+        PRTime t0, t1;
>+        EVLOG("nsAppShell: waiting on mQueueCond");
>+        t0 = PR_Now();
>+#endif
>+
>+#if defined(ANDROID_DEBUG_EVENTS)

you can remove that #ifdef here.


>+    nextEvent = PeekNextEvent();
>+
>+    while (nextEvent) {
>+        int curType = curEvent->Type();
>+        int nextType = nextEvent->Type();
>+
>+        while (nextType == AndroidGeckoEvent::DRAW &&
>+               mNumDraws > 1)
>+        {
>+            // skip this draw, since there's a later one already in the queue.. this will let us
>+            // deal with sequences that look like:
>+            //   MOVE DRAW MOVE DRAW MOVE DRAW
>+            // and end up with just
>+            //   MOVE DRAW
>+            // when we process all the events.
>+            RemoveNextEvent();
>+
>+#if defined(ANDROID_DEBUG_EVENTS)
>+            ALOG("# Removing DRAW event (%d outstanding)", mNumDraws);
>+#endif
>+
>+            nextEvent = PeekNextEvent();
>+            nextType = nextEvent->Type();
>+        }
>+

What do you think about refactoring this so that mQueueLock is only accessed once.




in the look and feel code:

>+#if 0

drop dead code..

>+
>+NS_IMETHODIMP
>+nsScreenAndroid::GetPixelDepth(PRInt32 *aPixelDepth)
>+{
>+    // XXX do we need to lie here about 16bpp?  Or
>+    // should we actually check and return the right thing?
>+    *aPixelDepth = 24;
>+    return NS_OK;
>+}

Can't you get the screen's depth?  I would imagine it would be 16 on a nexus one, right?


>+++ b/widget/src/android/nsWidgetFactory.cpp


remove commented out lines from this file.


>+nsWindow::nsWindow()
>+{
>+    mIsVisible = PR_FALSE;
>+    mParent = nsnull;
>+    mSpecialKeyTracking = 0;
>+}

Use constructor initialization





>+    if (drawType == AndroidGeckoSurfaceView::DRAW_SOFTWARE) {
>+        int bufCap;
>+        unsigned char *buf = sview.GetSoftwareDrawBuffer(&bufCap);
>+        if (!buf || bufCap != mBounds.width * mBounds.height * 4) {

why *4?  are we assuming 24/32 bits here?

>+        DrawTo(targetSurface);
>+
>+        // need to swap B and R channels, to get ABGR instead of ARGB
>+        unsigned int *ibuf = (unsigned int*) buf;
>+        unsigned int *ibufMax = ibuf + mBounds.width * mBounds.height;
>+        while (ibuf < ibufMax) {
>+            *ibuf++ = (*ibuf & 0xff00ff00) | ((*ibuf & 0x00ff0000) >> 16) | ((*ibuf & 0x000000ff) << 16);
>+        }


Use Pixman?  Maybe get some neon going on here!

Jeff should also review the Drawing code here.

>+/*
>+    if (pt.x < 0 || pt.x >= mBounds.width ||
>+        pt.y < 0 || pt.y >= mBounds.height)
>+    {
>+        ALOG("Motion event dispatched to %p but final coordinates %d %d aren't within bounds", (void*)this, pt.x, pt.y);
>+        return;
>+    }

dead code.


>+void
>+nsWindow::InitKeyEvent(nsKeyEvent& event, AndroidGeckoEvent& key)
>+{
>+    switch (key.KeyCode()) {


Can you special case the alphanumberics here to avoid the dozens of compares?



>diff --git a/widget/src/xpwidgets/nsWidgetAtomList.h b/widget/src/xpwidgets/nsWidgetAtomList.h
>--- a/widget/src/xpwidgets/nsWidgetAtomList.h
>+++ b/widget/src/xpwidgets/nsWidgetAtomList.h
>@@ -87,6 +87,7 @@ WIDGET_ATOM(key, "key") // The key eleme
> WIDGET_ATOM(label, "label")
> WIDGET_ATOM(max, "max")
> WIDGET_ATOM(maxpos, "maxpos")
>+WIDGET_ATOM(Menu, "Menu") // AppCommand to open a menu
> WIDGET_ATOM(menu, "menu") // Represents an XP menu
> WIDGET_ATOM(menuitem, "menuitem") // Represents an XP menu item
> WIDGET_ATOM(menupopup, "menupopup") // The XP menu's children.
>@@ -124,4 +125,6 @@ WIDGET_ATOM(tree, "tree")
> WIDGET_ATOM(treecolpicker, "treecolpicker")
> WIDGET_ATOM(type, "type")
> WIDGET_ATOM(value, "value")
>+WIDGET_ATOM(VolumeUp, "VolumeUp")
>+WIDGET_ATOM(VolumeDown, "VolumeDown")
> 

Ask roc about these two new items.  Getting close.  I want to do another pass.

Comment 21

[Michael Wu [:mwu]]

Attachment: Add Java wrapper in embedding/android, v5

comments addressed

Comment 22

[Michael Wu [:mwu]]

Attachment: Add Java wrapper in embedding/android, v5.1

This one even builds

Comment 23

[(not currently active) Ted Mielczarek]

Comment on attachment 448662 [details] [diff] [review]
Add Java wrapper in embedding/android, v5.1

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in

>+if test -n "${JAVA_BIN_PATH}" || test "$OS_TARGET" = Android; then
>+  if test -z "$JAVA" || test "$JAVA" = ":" || test -z "$JAVAC" || test "$JAVAC" = ":" || test -z "$JAR" || test "$JAR" = ":"; then
>+    AC_MSG_ERROR([The programs java, javac and jar were not found.  Set \$JAVA_HOME to your java sdk directory, use --with-java-bin-path={java-bin-dir}, or reconfigure with --disable-javaxpcom.])

This error message will be a little weird if you're targeting Android. Maybe just have a different error message when targeting Android?

>diff --git a/embedding/android/Makefile.in b/embedding/android/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/embedding/android/Makefile.in
>+GARBAGE = \

+=

>+GARBAGE_DIRS = res libs dist classes

+=

>+ifeq ($(MOZ_APP_NAME),fennec)
>+ICON_PATH = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/fennec_48x48.png
>+ICON_PATH_HI = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/fennec_72x72.png
>+else
>+ICON_PATH = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/icon48.png
>+ICON_PATH_HI = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/icon64.png
>+endif

Can you add the bug you filed here in a TODO comment?

>+
>+# We're using this to get this to build, but we have a custom rule for it below
>+##JAVA_LIBRARY = classes.dex

Just remove this if it's going to be commented out.


>+res/drawable/icon.png: $(MOZ_APP_ICON)
>+	$(NSINSTALL) -D res/drawable
>+	cp $(ICON_PATH) res/drawable/icon.png
>+
>+res/drawable-hdpi/icon.png: $(MOZ_APP_ICON)
>+	$(NSINSTALL) -D res/drawable-hdpi
>+	cp $(ICON_PATH_HI) res/drawable-hdpi/icon.png
>+
>+gecko.ap_: AndroidManifest.xml res/drawable/icon.png res/drawable-hdpi/icon.png
>+	$(AAPT) package -f -M AndroidManifest.xml -I $(ANDROID_SDK)/android.jar  -S res -F gecko.ap_

Use $@ in the commands here instead of repeating the target name.

>+dist: FORCE

Add another TODO comment with the bug number about packaging. I'm still not wild about all this code being a half-assed stage-package reimplementation.


>+gecko-unsigned-unaligned.apk: gecko.ap_ classes.dex dist $(FULL_LIBS)
>+	$(APKBUILDER) gecko-unsigned-unaligned.apk -v $(APKBUILDER_FLAGS) -z gecko.ap_ -f classes.dex -nf `pwd`/libs -rf dist
>+
>+gecko-unaligned.apk: gecko-unsigned-unaligned.apk
>+	cp  gecko-unsigned-unaligned.apk gecko-unaligned.apk
>+ifdef JARSIGNER
>+	$(JARSIGNER) gecko-unaligned.apk
>+endif
>+
>+$(MOZ_APP_NAME).apk: gecko-unaligned.apk
>+	$(ZIPALIGN) -f -v 4 gecko-unaligned.apk $(MOZ_APP_NAME).apk

Use more $@.

r=me with those changes.

Comment 24

[Michael Wu [:mwu]]

Embedding patch committed. Thanks for the reviews!

http://hg.mozilla.org/mozilla-central/rev/67d932ee2153

Comment 25

[Michael Wu [:mwu]]

Attachment: Add Android widget backend, v5

Comment 26

[Doug Turner (:dougt)]

Comment on attachment 448915 [details] [diff] [review]
Add Android widget backend, v5


>+#define initInit()                              \
>+    jclass jClass
>+
>+// note that this also sets jClass
>+#define getClassGlobalRef(cname)                                        \
>+    (jClass = jclass(jEnv->NewGlobalRef(jEnv->FindClass(cname))))

Lets bring those \ in a bit.


>+    nextEvent = PeekNextEvent();
>+
>+    while (nextEvent) {
>+        int curType = curEvent->Type();
>+        int nextType = nextEvent->Type();
>+
>+        while (nextType == AndroidGeckoEvent::DRAW &&
>+               mNumDraws > 1)
>+        {
>+            // skip this draw, since there's a later one already in the queue.. this will let us
>+            // deal with sequences that look like:
>+            //   MOVE DRAW MOVE DRAW MOVE DRAW
>+            // and end up with just
>+            //   MOVE DRAW
>+            // when we process all the events.
>+            RemoveNextEvent();
>+
>+#if defined(ANDROID_DEBUG_EVENTS)
>+            ALOG("# Removing DRAW event (%d outstanding)", mNumDraws);
>+#endif
>+
>+            nextEvent = PeekNextEvent();
>+            nextType = nextEvent->Type();
>+        }
>+
>+        // If the next type of event isn't the same as the current type,
>+        // we don't coalesce.
>+        if (nextType != curType)
>+            break;
>+
>+        // Can only coalesce motion move events, for motion events
>+        if (curType != AndroidGeckoEvent::MOTION_EVENT)
>+            break;
>+
>+        if (!(curEvent->Action() == AndroidMotionEvent::ACTION_MOVE &&
>+              nextEvent->Action() == AndroidMotionEvent::ACTION_MOVE))
>+            break;
>+
>+#if defined(ANDROID_DEBUG_EVENTS)
>+        ALOG("# Removing % 2d event", curType);
>+#endif
>+
>+        RemoveNextEvent();
>+        curEvent = nextEvent;
>+        nextEvent = PeekNextEvent();
>+    }

Again, multiple lock / unlocks....  feel free to fix this as a follow up.


great work.

Comment 27

[Michael Wu [:mwu]]

(In reply to comment #20)
> Can't you get the screen's depth?  I would imagine it would be 16 on a nexus
> one, right?
> 
(forgot to reply to this before)

The screen is most likely 16 bit but android will draw 16bit or 32bit surfaces. We'll probably just switch everything to 16bit later when it's supported on our side.

Comment 28

[Michael Wu [:mwu]]

Thanks for the reviews!

http://hg.mozilla.org/mozilla-central/rev/1c416ab33c8e

Comment 29

[Wolfgang Rosenauer [:wolfiR]]

The configure change added a hard dependency to a Java SDK on other platforms as plain Linux but I can't see what would use it.
Should I open a new bug about it?

Comment 30

[Wolfgang Rosenauer [:wolfiR]]

Hmm, no, that's probably bug 570440