741284 - add async file request method

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

Attachment: patch

this is needed for the async getUserMedia API

Comment 1

[Kartikaya Gupta (email:[e-mail])]

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

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

No major problems but lots of little things that could be improved.

::: mobile/android/base/GeckoApp.java
@@ +2639,5 @@
> +    }
> +
> +    class ActivityResultHandlerMap {
> +        Vector<ActivityResultHandler> mVector = new Vector<ActivityResultHandler>();
> +        int mNumHandlers = 0;

Make instance variables private.

Also, consider implementing this as:

private Map<Integer, ActivityResultHandler> mMap = new HashMap<Integer, ActivityResultHandler>();
private int mCounter = 0;

synchronized int put(ActivityResultHandler handler) {
    mMap.put(mCounter, handler);
    return mCounter++;
}

synchronized ActivityResultHandler getAndRemove(int i) {
    remove mMap.remove(i);
}

It's much simpler and is probably about the same for efficiency.

@@ +2642,5 @@
> +        Vector<ActivityResultHandler> mVector = new Vector<ActivityResultHandler>();
> +        int mNumHandlers = 0;
> +        synchronized int put(ActivityResultHandler handler) {
> +                mNumHandlers++;
> +                if (mNumHandlers == 1) {

Method body is over-indented.

@@ +2659,5 @@
> +            }
> +
> +        synchronized ActivityResultHandler getAndRemove(int i) {
> +                ActivityResultHandler handler = mVector.get(i);
> +                mNumHandlers--;

Method body is over-indented.

@@ +2668,5 @@
> +                return handler;
> +            }
> +    }
> +
> +    ActivityResultHandlerMap mActivityResultHandlerMap = new ActivityResultHandlerMap();

Make private and move to top of file.

@@ +2737,5 @@
>              }
> +        }
> +    }
> +
> +    FilePickerResultHandler mFilePickerResultHandler = new FilePickerResultHandler();

Ditto.

@@ +2752,5 @@
>              }
> +        }
> +    }
> +
> +    AwesomebarResultHandler mAwesomebarResultHandler = new AwesomebarResultHandler();

Ditto.

@@ +2755,5 @@
> +
> +    AwesomebarResultHandler mAwesomebarResultHandler = new AwesomebarResultHandler();
> +
> +    class CameraImageResultHandler implements ActivityResultHandler {
> +        public void onActivityResult(int resultCode, Intent data) {            try {

Missing a line break.

@@ +2770,5 @@
>              }
> +        }
> +    }
> +
> +    CameraImageResultHandler mCameraImageResultHandler = new CameraImageResultHandler();

Make private and move to top of file.

@@ +2796,3 @@
>      }
>  
> +    CameraVideoResultHandler mCameraVideoResultHandler = new CameraVideoResultHandler();

Ditto.

::: mobile/android/base/GeckoAppShell.java
@@ +2063,5 @@
>          GeckoScreenOrientationListener.getInstance().unlockScreenOrientation();
>      }
> +
> +    static class AsyncResultHandler implements GeckoApp.ActivityResultHandler {
> +        long mId;

Make instance variable private.

@@ +2070,5 @@
> +        }
> +
> +        public void onActivityResult(int resultCode, Intent data) {
> +            String filePickerResult = "";
> +            if (data != null && resultCode == Activity.RESULT_OK) {

Ehh.. this is a huge copy/paste. Why not make this class extend GeckoApp.FilePickerResultHandler with necessary changes? Also not really sure why this code makes a copy of the file but that's a separate issue.

::: widget/android/AndroidJNI.cpp
@@ +102,4 @@
>      NS_EXPORT void JNICALL Java_org_mozilla_gecko_GeckoAppShell_notifyListCreated(JNIEnv* jenv, jclass, jint, jint, jstring, jstring, jstring, jlong, jint, jlong);
>      NS_EXPORT void JNICALL Java_org_mozilla_gecko_GeckoAppShell_notifyGotNextMessage(JNIEnv* jenv, jclass, jint, jstring, jstring, jstring, jlong, jint, jlong);
>      NS_EXPORT void JNICALL Java_org_mozilla_gecko_GeckoAppShell_notifyReadingMessageListFailed(JNIEnv* jenv, jclass, jint, jint, jlong);
> +    NS_EXPORT void JNICALL Java_org_mozilla_gecko_GeckoAppShell_notifyFilePickerResult(JNIEnv* jenv, jclass, jstring fileDir, jlong id);

nit: s/id/callback/

@@ +910,5 @@
>      nsWindow::ScheduleResumeComposition();
>  }
>  
> +NS_EXPORT void JNICALL
> +Java_org_mozilla_gecko_GeckoAppShell_notifyFilePickerResult(JNIEnv* jenv, jclass, jstring filePath, jlong id)

s/id/callback/

@@ +921,5 @@
> +            nsFilePickerCallback* handler = (nsFilePickerCallback*)mId;
> +            handler->handleResult(mFileDir);
> +            handler->Release();
> +            return NS_OK;
> +            }

indentation of close-brace is wrong

@@ +924,5 @@
> +            return NS_OK;
> +            }
> +    private:
> +        nsString mFileDir;
> +        long mId;

s/mId/mCallback/

Comment 2

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

Attachment: patch

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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

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

r=me with nit addressed

::: mobile/android/base/GeckoAppShell.java
@@ +2076,5 @@
> +    static native void notifyFilePickerResult(String filePath, long id);
> +
> +    public static void showFilePickerAsync(String aMimeType, long id) {
> +        if (!GeckoApp.mAppContext.showFilePicker(aMimeType, new AsyncResultHandler(id)))
> +            GeckoAppShell.notifyFilePickerResult("", id);

nit: add a comment that this method is invoked by JNI, so changes to the method signature should be kept in sync.

Comment 4

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

(In reply to Kartikaya Gupta (:kats) from comment #3)
> Comment on attachment 611895 [details] [diff] [review]
> patch
> 
> Review of attachment 611895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nit addressed
> 
> ::: mobile/android/base/GeckoAppShell.java
> @@ +2076,5 @@
> > +    static native void notifyFilePickerResult(String filePath, long id);
> > +
> > +    public static void showFilePickerAsync(String aMimeType, long id) {
> > +        if (!GeckoApp.mAppContext.showFilePicker(aMimeType, new AsyncResultHandler(id)))
> > +            GeckoAppShell.notifyFilePickerResult("", id);
> 
> nit: add a comment that this method is invoked by JNI, so changes to the
> method signature should be kept in sync.
Why would this method be different than any other we call from JNI?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

It isn't, and we should add those comments on all of them. But for now I'd like to at least see them added on new code.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Also I just realized that the original reason I started doing this was to prevent breaking XUL. Which this patch does, by not ifdef'ing the stuff in AndroidBridge::Init or adding a stub method to XUL. That should be fixed too.

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdaa03c8f244

Comment 8

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/bdaa03c8f244