569497 - need finger friendly file picker for android

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: current file picker

the default picker is almost unusable on a capacitive screen

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

That image looks like the XUL nsIFilePicker. Are we defaulting to that on Android? Does Android have a native file picker? If so, let's wrap it.

Comment 3

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

I'm marking this as blocking? until we figure out if we actually need a file picker on android.

Comment 4

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

I can't see a need to give the user access to the file system, marking as won't fix. We can reopen if we find a need.

Comment 5

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

file uploads are a good enough use case to reopen this

Comment 6

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

Attachment: WIP patch

Here's a work in progress that allows the user to get a file through intents (for example picking a photo from the gallery app). This processes is async for android but needs to be synchronous for the existing file picker api and this patch does nothing to make that work.

Comment 7

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

Attachment: WIP patch

Comment 8

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

Attachment: patches

This works, but there are three main issues with this patch

1) I'm copying what gets returned to a temp file because otherwise its a content:// uri that gecko doesn't understand
2) The while loop used to wait for the sub intent to return is kinda ugly
3) When the sub intent returns, we have a back screen

Interestingly, turning the screen off and back on rescues us from the black screen.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #8)
> Created attachment 476194 [details] [diff] [review]

> 1) I'm copying what gets returned to a temp file because otherwise its a
> content:// uri that gecko doesn't understand

If it makes life easier, not just for this patch but in general on Android, we could make a protocol handler to work with content:// uris

Comment 10

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

(In reply to comment #9)
> (In reply to comment #8)
> > Created attachment 476194 [details] [diff] [review] [details]
> 
> > 1) I'm copying what gets returned to a temp file because otherwise its a
> > content:// uri that gecko doesn't understand
> 
> If it makes life easier, not just for this patch but in general on Android, we
> could make a protocol handler to work with content:// uris

perhaps in general, but I don't think that helps us with the file picker since all the code that depends on it expects a file on the local file system

Comment 11

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

(In reply to comment #8)
> Created attachment 476194 [details] [diff] [review]
> patches
y
> 3) When the sub intent returns, we have a back screen
> 
> Interestingly, turning the screen off and back on rescues us from the black
> screen.

this is fixed by bug 597412

Comment 12

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

Attachment: patch

bug 597412 solved the black screens and this patch uses a wait/notify rather than a busy loop. I don't think there is anything we can do about copying the file to a tmp file, short of mapping /content to these content urls (so they appear as file normal file paths).

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

This patch only supports "opening" a file, correct? I think we should check the aMode param passed to nsIFilePicker::Init and return NS_NOT_IMPLEMENTED for all modes except nsIFilePicker::modeOpen.

What do you think?

Comment 14

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

Makes sense

Comment 15

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

Attachment: patch

Comment 16

[Michael Wu [:mwu]]

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

>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
>--- a/embedding/android/GeckoApp.java
>+++ b/embedding/android/GeckoApp.java
>@@ -522,4 +522,65 @@ abstract public class GeckoApp
>             return false;
>         }
>     }
>+
>+    static int FILE_PICKER_REQUEST = 4;
Why 4?

The standard way to write constants in Java is static final.

>+    private  String mFilePickerResult;
Extra space.

>+    private Object mFilePickerLock = new Object();
Use java.util.concurrent.SynchronousQueue

>+    public String showFilePicker() {
>+        mFilePickerResult = null;
>+        new Thread(new Runnable(){public void run(){
>+            Intent intent = new Intent(Intent.ACTION_GET_CONTENT);
>+            intent.addCategory(Intent.CATEGORY_OPENABLE);
>+            intent.setType("*/*");
>+            GeckoApp.this.
>+                startActivityForResult(
>+                    Intent.createChooser(intent,"choose a file"),
>+                    FILE_PICKER_REQUEST);
>+        }}).start();
Why do you need a new thread? If startActivityForResult is asynchronous, you don't need a new thread, and if it's synchronous, you don't need to go out of your way to synchronize with the callback.

>+        synchronized (mFilePickerLock) {
>+            while (mFilePickerResult == null)
>+                try {mFilePickerLock.wait();} 
>+                catch(Exception e){Log.i("GeckoApp", "error: " + e);}
>+        }
>+        
>+        return mFilePickerResult;
>+    }
>+    
>+    @Override
>+    protected void onActivityResult(int requestCode, int resultCode, 
>+                                    Intent data) {
>+        if (data != null && resultCode == RESULT_OK) {
>+            try {
>+                ContentResolver cr = getContentResolver();
>+                android.net.Uri uri = data.getData();
s/android.net.//

>+                String mimeType = cr.getType(uri);
>+                String fileExt = "." + 
>+                    mimeType.substring(mimeType.lastIndexOf('/') + 1);
>+                File file = 
>+                    File.createTempFile("tmp_" + 
>+                                        (int)Math.floor(1000 * Math.random()), 
>+                                        fileExt, 
>+                                        new File("/data/data/org.mozilla." +
>+                                                 getAppName()));
>+                
>+                FileOutputStream fos = new FileOutputStream(file);
>+                InputStream is = cr.openInputStream(uri);
>+                byte[] buf = new byte[1024];
4096

>+                int len = is.read(buf);
>+                while (len != -1) {
>+                    fos.write(buf, 0, len);
>+                    len = is.read(buf);
>+                }
>+                fos.close();
Oh how sad.

So, Java will give us a fd. There's no guarantee the fd is backed by an actual file. If it's backed by a file, there's no guarantee it's the whole file. It could even be compressed.

Fortunately, we can work with that. khuey's work in bug 583863 will allow you hack html input to accept files backed by nothing but a piece of memory. The fd can be mmaped for that purpose.

Unfortunately, this will only work with html input. We could wrap a nsIInputStream around the fd for other cases but that requires more extensive changes everywhere.

Up to you whether this is worth pursuing now. Seems like a case that could be hit without much trouble if someone tries to upload some large video they recorded.

>diff --git a/widget/src/android/nsClipboard.h b/widget/src/android/nsClipboard.h
>--- a/widget/src/android/nsClipboard.h
>+++ b/widget/src/android/nsClipboard.h
>@@ -1,4 +1,5 @@
>-/* ***** BEGIN LICENSE BLOCK *****
>+/* -*- Mode: c++; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
>+ * ***** BEGIN LICENSE BLOCK *****
>  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>  *
>  * The contents of this file are subject to the Mozilla Public License Version
Unrelated change

>diff --git a/widget/src/android/nsFilePicker.cpp b/widget/src/android/nsFilePicker.cpp
>new file mode 100644
>--- /dev/null
>+++ b/widget/src/android/nsFilePicker.cpp
>+NS_IMPL_ISUPPORTS1(nsFilePicker, nsIFilePicker)
>+
>+/* void init (in nsIDOMWindow parent, in AString title, in short mode); */
A bit excessive.. guess you copied this from somewhere else?

>+NS_IMETHODIMP nsFilePicker::Init(nsIDOMWindow *parent, const nsAString& title, 
>+                                 PRInt16 mode)
>+{
>+    __android_log_print(ANDROID_LOG_INFO, "Gecko" , "%s", __PRETTY_FUNCTION__);
ALOG?

Comment 17

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

Attachment: patch

Comment 18

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

(In reply to comment #16)
> Comment on attachment 476712 [details] [diff] [review]
> patch
> 
> >diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
> >--- a/embedding/android/GeckoApp.java
> >+++ b/embedding/android/GeckoApp.java
> >@@ -522,4 +522,65 @@ abstract public class GeckoApp
> >             return false;
> >         }
> >     }
> >+
> >+    static int FILE_PICKER_REQUEST = 4;
> Why 4?
doesn't matter, just need something thst's greater than 1 

> >+    private Object mFilePickerLock = new Object();
> Use java.util.concurrent.SynchronousQueue
done, but why?

> 
> >+    public String showFilePicker() {
> >+        mFilePickerResult = null;
> >+        new Thread(new Runnable(){public void run(){
> >+            Intent intent = new Intent(Intent.ACTION_GET_CONTENT);
> >+            intent.addCategory(Intent.CATEGORY_OPENABLE);
> >+            intent.setType("*/*");
> >+            GeckoApp.this.
> >+                startActivityForResult(
> >+                    Intent.createChooser(intent,"choose a file"),
> >+                    FILE_PICKER_REQUEST);
> >+        }}).start();
> Why do you need a new thread? If startActivityForResult is asynchronous, you
> don't need a new thread, and if it's synchronous, you don't need to go out of
> your way to synchronize with the callback.

its an artifact of a previous impl, dropped
> >+                int len = is.read(buf);
> >+                while (len != -1) {
> >+                    fos.write(buf, 0, len);
> >+                    len = is.read(buf);
> >+                }
> >+                fos.close();
> Oh how sad.
yes it is
> 
> So, Java will give us a fd. There's no guarantee the fd is backed by an actual
> file. If it's backed by a file, there's no guarantee it's the whole file. It
> could even be compressed.
> 
> Fortunately, we can work with that. khuey's work in bug 583863 will allow you
> hack html input to accept files backed by nothing but a piece of memory. The fd
> can be mmaped for that purpose.
> 
> Unfortunately, this will only work with html input. We could wrap a
> nsIInputStream around the fd for other cases but that requires more extensive
> changes everywhere.
> 
> Up to you whether this is worth pursuing now. Seems like a case that could be
> hit without much trouble if someone tries to upload some large video they
> recorded.
nope, not worth it now. We can file a follow up bug to figure something out.

> >diff --git a/widget/src/android/nsClipboard.h b/widget/src/android/nsClipboard.h
> >--- a/widget/src/android/nsClipboard.h
> >+++ b/widget/src/android/nsClipboard.h
> >@@ -1,4 +1,5 @@
> >-/* ***** BEGIN LICENSE BLOCK *****
> >+/* -*- Mode: c++; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
> >+ * ***** BEGIN LICENSE BLOCK *****
> >  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
> >  *
> >  * The contents of this file are subject to the Mozilla Public License Version
> Unrelated change

think of it as a freebee

> >diff --git a/widget/src/android/nsFilePicker.cpp b/widget/src/android/nsFilePicker.cpp
> >new file mode 100644
> >--- /dev/null
> >+++ b/widget/src/android/nsFilePicker.cpp
> >+NS_IMPL_ISUPPORTS1(nsFilePicker, nsIFilePicker)
> >+
> >+/* void init (in nsIDOMWindow parent, in AString title, in short mode); */
> A bit excessive.. guess you copied this from somewhere else?
> 
> >+NS_IMETHODIMP nsFilePicker::Init(nsIDOMWindow *parent, const nsAString& title, 
> >+                                 PRInt16 mode)
> >+{
> >+    __android_log_print(ANDROID_LOG_INFO, "Gecko" , "%s", __PRETTY_FUNCTION__);
> ALOG?

just dropped

Comment 19

[Michael Wu [:mwu]]

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


>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
>--- a/embedding/android/GeckoApp.java
>+++ b/embedding/android/GeckoApp.java
>@@ -54,6 +54,8 @@ import android.widget.*;
> import android.hardware.*;
> 
> import android.util.*;
>+import java.util.concurrent.SynchronousQueue;
>+import android.net.Uri;
The style in this file is .* . The java.* and android.* imports are grouped with each other.

> 
> abstract public class GeckoApp
>     extends Activity
>@@ -499,4 +501,63 @@ abstract public class GeckoApp
>         if (statusCode == 0)
>             System.exit(0);
>     }
>+
>+    static final int FILE_PICKER_REQUEST = 4;
>+    private String mFilePickerResult;
>+    private SynchronousQueue mFilePickerLock = new SynchronousQueue();
>+    public String showFilePicker() {
>+        mFilePickerResult = null;
>+        Intent intent = new Intent(Intent.ACTION_GET_CONTENT);
>+        intent.addCategory(Intent.CATEGORY_OPENABLE);
>+        intent.setType("*/*");
>+        GeckoApp.this.
>+            startActivityForResult(
>+                Intent.createChooser(intent,"choose a file"),
>+                FILE_PICKER_REQUEST);
>+        synchronized (mFilePickerLock) {
>+            while (mFilePickerResult == null)
>+                try {mFilePickerLock.wait();} 
>+                catch(Exception e){Log.i("GeckoApp", "error: " + e);}
>+        }
No. I mean, actually use the SynchronousQueue object. See GeckoInputConnection for an example of how it's used.

Comment 20

[Michael Wu [:mwu]]

(In reply to comment #18)
> (In reply to comment #16)
> > Comment on attachment 476712 [details] [diff] [review] [details]
> > patch
> > 
> > >diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
> > >--- a/embedding/android/GeckoApp.java
> > >+++ b/embedding/android/GeckoApp.java
> > >@@ -522,4 +522,65 @@ abstract public class GeckoApp
> > >             return false;
> > >         }
> > >     }
> > >+
> > >+    static int FILE_PICKER_REQUEST = 4;
> > Why 4?
> doesn't matter, just need something thst's greater than 1 
> 
The API doc says you can use anything >= 0. I would suggest 0 in this case. 4 almost implies that we're trying to match some magic number elsewhere in Android when we really just want some enums local to our app.. which Java does have these days if you want to use them.

Comment 21

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

(In reply to comment #20)
> (In reply to comment #18)
> > (In reply to comment #16)
> > > Comment on attachment 476712 [details] [diff] [review] [details] [details]
> > > patch
> > > 
> > > >diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
> > > >--- a/embedding/android/GeckoApp.java
> > > >+++ b/embedding/android/GeckoApp.java
> > > >@@ -522,4 +522,65 @@ abstract public class GeckoApp
> > > >             return false;
> > > >         }
> > > >     }
> > > >+
> > > >+    static int FILE_PICKER_REQUEST = 4;
> > > Why 4?
> > doesn't matter, just need something thst's greater than 1 
> > 
> The API doc says you can use anything >= 0. I would suggest 0 in this case. 4
> almost implies that we're trying to match some magic number elsewhere in
> Android when we really just want some enums local to our app.. which Java does
> have these days if you want to use them.

I remember reading the 0 had special meaning (although don't see it in the docs now). I'd be fine with 1 though.


> > abstract public class GeckoApp
> >     extends Activity
> >@@ -499,4 +501,63 @@ abstract public class GeckoApp
> >         if (statusCode == 0)
> >             System.exit(0);
> >     }
> >+
> >+    static final int FILE_PICKER_REQUEST = 4;
> >+    private String mFilePickerResult;
> >+    private SynchronousQueue mFilePickerLock = new SynchronousQueue();
> >+    public String showFilePicker() {
> >+        mFilePickerResult = null;
> >+        Intent intent = new Intent(Intent.ACTION_GET_CONTENT);
> >+        intent.addCategory(Intent.CATEGORY_OPENABLE);
> >+        intent.setType("*/*");
> >+        GeckoApp.this.
> >+            startActivityForResult(
> >+                Intent.createChooser(intent,"choose a file"),
> >+                FILE_PICKER_REQUEST);
> >+        synchronized (mFilePickerLock) {
> >+            while (mFilePickerResult == null)
> >+                try {mFilePickerLock.wait();} 
> >+                catch(Exception e){Log.i("GeckoApp", "error: " + e);}
> >+        }
> No. I mean, actually use the SynchronousQueue object. See GeckoInputConnection
> for an example of how it's used.

why?

Comment 22

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

Attachment: patch

Comment 23

[Michael Wu [:mwu]]

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


>@@ -499,4 +501,59 @@ abstract public class GeckoApp
>         if (statusCode == 0)
>             System.exit(0);
>     }
>+
>+    static final int FILE_PICKER_REQUEST = 1;
>+    private String mFilePickerResult;
>+    private SynchronousQueue mFilePickerLock = new SynchronousQueue();

private SynchronousQueue<String> mFilePickerResult = new SynchronousQueue<String>();

>+    public String showFilePicker() {
>+        mFilePickerResult = null;
>+        Intent intent = new Intent(Intent.ACTION_GET_CONTENT);
>+        intent.addCategory(Intent.CATEGORY_OPENABLE);
>+        intent.setType("*/*");
>+        GeckoApp.this.
>+            startActivityForResult(
>+                Intent.createChooser(intent,"choose a file"),
>+                FILE_PICKER_REQUEST);
>+        try {mFilePickerLock.put(this);} 
>+        catch(Exception e){Log.i("GeckoApp", "error: " + e);}
>+        
>+        return mFilePickerResult;

String filePickerResult;
try {
    filePickerResult = mFilePickerResult.take();
} catch (InterruptedException e) {
    Log.i("GeckoApp", "error: " + e);
}

return filePickerResult;

>+    }
>+    
>+    @Override
>+    protected void onActivityResult(int requestCode, int resultCode, 
>+                                    Intent data) {

String filePickerResult = "";

>+        if (data != null && resultCode == RESULT_OK) {
>+            try {
>+                ContentResolver cr = getContentResolver();
>+                Uri uri = data.getData();
>+                String mimeType = cr.getType(uri);
>+                String fileExt = "." + 
>+                    mimeType.substring(mimeType.lastIndexOf('/') + 1);
>+                File file = 
>+                    File.createTempFile("tmp_" + 
>+                                        (int)Math.floor(1000 * Math.random()), 
>+                                        fileExt, 
>+                                        new File("/data/data/org.mozilla." +
>+                                                 getAppName()));
>+                
>+                FileOutputStream fos = new FileOutputStream(file);
>+                InputStream is = cr.openInputStream(uri);
>+                byte[] buf = new byte[4096];
>+                int len = is.read(buf);
>+                while (len != -1) {
>+                    fos.write(buf, 0, len);
>+                    len = is.read(buf);
>+                }
>+                fos.close();
>+                mFilePickerResult =  file.getAbsolutePath();
>+            }catch (Exception e) {
>+                Log.e("GeckoApp", "error : "+ e);
>+            }
>+        }
>+        if (mFilePickerResult == null)
>+            mFilePickerResult = "";
>+        try {mFilePickerLock.take();} 
>+        catch(Exception e){Log.i("GeckoApp", "error: " + e);}

try {
    mFilePickerResult.put(filePickerResult);
} catch (InterruptedException e) {
    Log.i("GeckoApp", "error: " + e);
}

Comment 24

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

Attachment: patch

Comment 25

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

pushed http://hg.mozilla.org/mozilla-central/rev/0a01860cef28

Comment 26

[Matt Brubeck (:mbrubeck)]

Backed out because launchMode="singleTop" caused black screens when Fennec receives an intent from the system or other activities (bug 601314):
http://hg.mozilla.org/mozilla-central/rev/f88f8337cef1
http://hg.mozilla.org/mozilla-central/rev/1776cbcf2b31

Comment 27

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

Attachment: patch with singleTask

I don't see these issues with singleTask. Either way I think we should check this in after beta 2

Comment 28

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

pushed http://hg.mozilla.org/mozilla-central/rev/b71ffa5cf123

Comment 29

[Ayan Shah[:ashah]]

Verified fixed on:

Mozilla/5.0 (Android; Linux arm7vl; rv:2.0b8pre) Gecko/20101119 Firefox/4.0b8pre Fennec/4.0b3pre

Comment 30

[Ayan Shah[:ashah]]

https://litmus.mozilla.org/show_test.cgi?id=13820