Closed
Bug 741284
Opened 13 years ago
Closed 13 years ago
add async file request method
Categories
(Core Graveyard :: Widget: Android, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla14
People
(Reporter: blassey, Assigned: blassey)
References
Details
Attachments
(1 file, 1 obsolete file)
18.17 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
this is needed for the async getUserMedia API
Attachment #611364 -
Flags: review?(bugmail.mozilla)
Comment 1•13 years ago
|
||
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-
Assignee | ||
Comment 2•13 years ago
|
||
Assignee: nobody → blassey.bugs
Attachment #611364 -
Attachment is obsolete: true
Attachment #611895 -
Flags: review?(bugmail.mozilla)
Comment 3•13 years ago
|
||
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+
Assignee | ||
Comment 4•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 7•13 years ago
|
||
Whiteboard: [inbound]
Target Milestone: --- → mozilla14
Comment 8•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•