Last Comment Bug 741284 - add async file request method
: add async file request method
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Brad Lassey [:blassey] (use needinfo?)
:
Mentors:
Depends on:
Blocks: 738528
  Show dependency treegraph
 
Reported: 2012-04-01 23:31 PDT by Brad Lassey [:blassey] (use needinfo?)
Modified: 2012-04-05 11:28 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (17.71 KB, patch)
2012-04-01 23:31 PDT, Brad Lassey [:blassey] (use needinfo?)
bugmail.mozilla: review-
Details | Diff | Review
patch (18.17 KB, patch)
2012-04-03 11:13 PDT, Brad Lassey [:blassey] (use needinfo?)
bugmail.mozilla: review+
Details | Diff | Review

Description Brad Lassey [:blassey] (use needinfo?) 2012-04-01 23:31:52 PDT
Created attachment 611364 [details] [diff] [review]
patch

this is needed for the async getUserMedia API
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-02 09:31:16 PDT
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?) 2012-04-03 11:13:45 PDT
Created attachment 611895 [details] [diff] [review]
patch
Comment 3 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-03 12:35:33 PDT
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?) 2012-04-03 14:35:49 PDT
(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:kats@mozilla.com) 2012-04-03 14:51:53 PDT
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:kats@mozilla.com) 2012-04-03 15:08:02 PDT
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?) 2012-04-04 23:03:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdaa03c8f244
Comment 8 :Ehsan Akhgari (out sick) 2012-04-05 11:28:29 PDT
http://hg.mozilla.org/mozilla-central/rev/bdaa03c8f244

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