add async file request method

RESOLVED FIXED in mozilla14

Status

()

Core
Widget: Android
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: blassey, Assigned: blassey)

Tracking

unspecified
mozilla14
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 611364 [details] [diff] [review]
patch

this is needed for the async getUserMedia API
Attachment #611364 - Flags: review?(bugmail.mozilla)
(Assignee)

Updated

5 years ago
Blocks: 738528
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/
Attachment #611364 - Flags: review?(bugmail.mozilla) → review-
Created attachment 611895 [details] [diff] [review]
patch
Assignee: nobody → blassey.bugs
Attachment #611364 - Attachment is obsolete: true
Attachment #611895 - Flags: review?(bugmail.mozilla)
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.
Attachment #611895 - Flags: review?(bugmail.mozilla) → review+
(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?
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.
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdaa03c8f244
Whiteboard: [inbound]
Target Milestone: --- → mozilla14
http://hg.mozilla.org/mozilla-central/rev/bdaa03c8f244
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.