Last Comment Bug 701996 - Merge widget/src/android from birch back into mozilla-central
: Merge widget/src/android from birch back into mozilla-central
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: All Android
-- normal (vote)
: ---
Assigned To: Doug Turner (:dougt)
: Jim Chen [:jchen] [:darchons]
Depends on:
  Show dependency treegraph
Reported: 2011-11-12 01:37 PST by Doug Turner (:dougt)
Modified: 2011-11-17 10:24 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v.1 (54.11 KB, patch)
2011-11-12 01:37 PST, Doug Turner (:dougt)
blassey.bugs: review-
Details | Diff | Splinter Review
patch v.2 (58.79 KB, patch)
2011-11-12 10:34 PST, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
patch v.2 (58.49 KB, patch)
2011-11-14 11:52 PST, Doug Turner (:dougt)
blassey.bugs: review+
Details | Diff | Splinter Review

Description User image Doug Turner (:dougt) 2011-11-12 01:37:55 PST
Created attachment 574016 [details] [diff] [review]
patch v.1

This is the merge from birch back to mozilla-central.  We want to support both the native client as well as the xul client.  Much of this has been reviewed previously, but it doesn't hurt to get another review.

The one todo is figuring out where to move the AndroidHistory stuff.  Right now The implementation of AndroidHistory is in the toolkit places directory -- what we need to do is move it and have an idl instead of directly linking from widget... I will do this in a followup.

the next todo is merge the new stuff back into embedding/android on mozilla.  When I do, i can uncomment jGetAccessibilityEnabled and the other new methods+properties.  Another followup.

This hasn't been tested on the native client yet -- only on the xul client.  We will test the native client when we begin bring it back over to mozilla-central.
Comment 1 User image Brad Lassey [:blassey] (use needinfo?) 2011-11-12 05:51:40 PST
Comment on attachment 574016 [details] [diff] [review]
patch v.1

Review of attachment 574016 [details] [diff] [review]:
----------------------------------------------------------------- and should be landable right away. With those landing along with this AndroidBridge.cpp and AndroidJavaWrappers.cpp should be landable without commented or ifdef'd out code. 

If that is not the case, please use #ifdefs with sensible flags rather than comments or if (0)

Finally, the java compositor is a new feature, please add a ifdef for it

::: widget/src/android/AndroidBridge.cpp
@@ +166,5 @@
> +    jGetAccessibilityEnabled = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "getAccessibilityEnabled", "()Z");
> +    jHandleGeckoMessage = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "handleGeckoMessage", "(Ljava/lang/String;)Ljava/lang/String;");
> +    jCheckUriVisited = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "checkUriVisited", "(Ljava/lang/String;)V");
> +    jMarkUriVisited = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "markUriVisited", "(Ljava/lang/String;)V");
> +    */

use an #ifdef rather than comment this out. Also, use that #ifdef around the functions that call these and in the header.

::: widget/src/android/AndroidJNI.cpp
@@ +53,5 @@
>  #include "mozilla/Services.h"
>  #include "nsINetworkLinkService.h"
> +// Android history hasn't landed yet.
> +// #include "nsAndroidHistory.h"


@@ +204,5 @@
> +Java_org_mozilla_gecko_GeckoAppShell_notifyUriVisited(JNIEnv *jenv, jclass, jstring uri)
> +{
> +    // XXX - Android History hasn't landed on mozilla-central yet.
> +    //    nsAndroidHistory::NotifyURIVisited(nsJNIString(uri, jenv));


::: widget/src/android/AndroidJavaWrappers.cpp
@@ +167,5 @@
>      jRectField = getField("mRect", "Landroid/graphics/Rect;");
>      jCharactersField = getField("mCharacters", "Ljava/lang/String;");
> +    // This can be enabled when the embedding/android bits land on m-c
> +    //    jCharactersExtraField = getField("mCharactersExtra", "Ljava/lang/String;");

the patch to this file and the patch to should land together

::: widget/src/android/nsWindow.cpp
@@ +307,5 @@
> +{
> +    nsIntRect entireRect(0, 0, TILE_WIDTH, TILE_HEIGHT);
> +    AndroidGeckoEvent *event = new AndroidGeckoEvent(AndroidGeckoEvent::DRAW,
> +                                                     entireRect);
> +    nsAppShell::gAppShell->PostEvent(event);

we may need an ifdef/else here. The old code sent "new AndroidGeckoEvent(-1, -1, -1, -1);"

@@ +1150,5 @@
>          return;
>      }
>      AndroidBridge::AutoLocalJNIFrame jniFrame;
> +    if (0) { // java compositor

#ifdef JAVA_COMPOSITOR please
Comment 2 User image Doug Turner (:dougt) 2011-11-12 10:34:17 PST
Created attachment 574073 [details] [diff] [review]
patch v.2

this has the embedding/android bits so that we don't have to comment out stuff in widget/
Comment 3 User image Doug Turner (:dougt) 2011-11-12 10:39:31 PST

I don't want to have a #ifdef.  Instead, lets just define an interface, have it implemented by the native ui, and not the xul ui.  It will keep the widget code cleaner by not having #ifdefs.  We can keep the comments in the code, land this, and fix it up doubletime (e.g. days)


I don't want to have a #ifdef.  Instead, we can test to see if the class exists.  If it does, we can use it.  Otherwise, we will clear the exception and move on with life.  This is what the current patch does.
Comment 4 User image Doug Turner (:dougt) 2011-11-12 11:44:57 PST

thinking about this more, we might need this because we have #ifdefs in places now to disable the default history store.  Either way, we can do this as a follow up.
Comment 5 User image Doug Turner (:dougt) 2011-11-14 09:10:09 PST
that follow up is bug 702037.
Comment 6 User image Doug Turner (:dougt) 2011-11-14 11:52:07 PST
Created attachment 574365 [details] [diff] [review]
patch v.2

Note You need to log in before you can comment on or make changes to this bug.