Closed Bug 701996 Opened 8 years ago Closed 8 years ago

Merge widget/src/android from birch back into mozilla-central

Categories

(Core :: Widget: Android, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dougt, Assigned: dougt)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch v.1 (obsolete) — Splinter Review
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.
Attachment #574016 - Flags: review?(blassey.bugs)
Comment on attachment 574016 [details] [diff] [review]
patch v.1

Review of attachment 574016 [details] [diff] [review]:
-----------------------------------------------------------------

GeckoEvent.java and GeckoAppShell.java 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"

#ifdef ANDROID_HISTORY

@@ +204,5 @@
>  NS_EXPORT void JNICALL
> +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));

#ifdef ANDROID_HISTORY

::: 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 GeckoEvents.java 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
Attachment #574016 - Flags: review?(blassey.bugs) → review-
Attached patch patch v.2 (obsolete) — Splinter Review
this has the embedding/android bits so that we don't have to comment out stuff in widget/
Attachment #574016 - Attachment is obsolete: true
Attachment #574073 - Flags: review?(blassey.bugs)
> #ifdef ANDROID_HISTORY

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)

> #ifdef JAVA_COMPOSITOR

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.
> #ifdef ANDROID_HISTORY

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.
that follow up is bug 702037.
Attached patch patch v.2Splinter Review
Attachment #574073 - Attachment is obsolete: true
Attachment #574073 - Flags: review?(blassey.bugs)
Attachment #574365 - Flags: review?(blassey.bugs)
Attachment #574365 - Flags: review?(blassey.bugs) → review+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.