606235 - Android file picker should honor file type filters

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

Its currently not implemented: 
http://mxr.mozilla.org/mozilla-central/source/widget/src/android/nsFilePicker.cpp#51

Comment 1

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

Attachment: patch

Comment 2

[Michael Wu [:mwu]]

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


>diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java
>--- a/embedding/android/GeckoAppShell.java
>+++ b/embedding/android/GeckoAppShell.java
>@@ -418,7 +418,28 @@ class GeckoAppShell
>     }
> 
>     static String getMimeTypeFromExtension(String aFileExt) {
should probably be called getMimeTypeFromExtensions.

>-        return android.webkit.MimeTypeMap.getSingleton().getMimeTypeFromExtension(aFileExt);
>+        android.webkit.MimeTypeMap mtm =
>+            android.webkit.MimeTypeMap.getSingleton();
>+        StringTokenizer st = new StringTokenizer(aFileExt, "., ");
>+        String type = "";
>+        String subType = "";
Test for null instead of empty string.

>+        while (st.hasMoreElements()) {
>+            String ext = st.nextToken();
>+            String mt = mtm.getMimeTypeFromExtension(ext);
>+            if (mt != null) {
if (mt == null) continue;

Would the string tokenizer really give us null if it says there's more elements?

>+                int slash = mt.indexOf('/');
>+                if (!type.equalsIgnoreCase(mt.substring(0,slash)))
Space after the comma.

>+                    type = type.equals("") ? mt.substring(0, slash) : "*";
>+                if (!subType.equalsIgnoreCase(mt.substring(slash + 1)))
>+                    subType = subType.equals("") ?
>+                        mt.substring(slash + 1) : "*";
Store mt.substring(0, slash) and mt.substring(slash + 1) in local variables.

>+            }
>+        }
>+        if (type.equals(""))
>+            type = "*";
>+        if (subType.equals(""))
>+            subType = "*";
>+        return type + "/" + subType;
>     }
> 
>     static boolean openUriExternal(String aUriSpec, String aMimeType, String aPackageName, 
>diff --git a/widget/src/android/AndroidBridge.cpp b/widget/src/android/AndroidBridge.cpp
>--- a/widget/src/android/AndroidBridge.cpp
>+++ b/widget/src/android/AndroidBridge.cpp
> void
>-AndroidBridge::ShowFilePicker(nsAString& aFilePath)
>+AndroidBridge::ShowFilePicker(nsAString& aFilePath, nsAString& aFilters)
> {
>+    jstring jstrFilers = mJNIEnv->NewString(nsPromiseFlatString(aFilters).get(),
>+                                            aFilters.Length());
>     jstring jstr =  static_cast<jstring>(mJNIEnv->CallStaticObjectMethod(
>-                                             mGeckoAppShellClass, jShowFilePicker));
>+                                             mGeckoAppShellClass,
>+                                             jShowFilePicker, jstrFilers));
>     aFilePath.Assign(nsJNIString(jstr));
> }
> 
Not sure how I missed this the first time. You need an autoframe here.

>diff --git a/widget/src/android/nsFilePicker.cpp b/widget/src/android/nsFilePicker.cpp
>--- a/widget/src/android/nsFilePicker.cpp
>+++ b/widget/src/android/nsFilePicker.cpp
> 
>@@ -48,14 +51,66 @@ NS_IMETHODIMP nsFilePicker::Init(nsIDOMW
>     return nsIFilePicker::modeOpen == mode ? NS_OK : NS_ERROR_NOT_IMPLEMENTED;
> }
> 
>-NS_IMETHODIMP nsFilePicker::AppendFilters(PRInt32 filterMask)
>+NS_IMETHODIMP nsFilePicker::AppendFilters(PRInt32 aFilterMask)
> {
Let the base class do this.

Comment 3

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

Attachment: patch

(In reply to comment #2)
> Comment on attachment 487220 [details] [diff] [review]
> patch
> >+            String mt = mtm.getMimeTypeFromExtension(ext);
> >+            if (mt != null) {
> if (mt == null) continue;
> 
> Would the string tokenizer really give us null if it says there's more
> elements?

mt is the return value of getMimeTypeFromExtension(), not nextToken()

> >-NS_IMETHODIMP nsFilePicker::AppendFilters(PRInt32 filterMask)
> >+NS_IMETHODIMP nsFilePicker::AppendFilters(PRInt32 aFilterMask)
> > {
> Let the base class do this.

this doesn't inherit from nsBaseFilePicker, are you suggesting that it should?

Comment 4

[Michael Wu [:mwu]]

(In reply to comment #3)
> Created attachment 487284 [details] [diff] [review]
> patch
> 
> (In reply to comment #2)
> > Comment on attachment 487220 [details] [diff] [review] [details]
> > patch
> > >+            String mt = mtm.getMimeTypeFromExtension(ext);
> > >+            if (mt != null) {
> > if (mt == null) continue;
> > 
> > Would the string tokenizer really give us null if it says there's more
> > elements?
> 
> mt is the return value of getMimeTypeFromExtension(), not nextToken()
> 
Ah right, misread.

> > >-NS_IMETHODIMP nsFilePicker::AppendFilters(PRInt32 filterMask)
> > >+NS_IMETHODIMP nsFilePicker::AppendFilters(PRInt32 aFilterMask)
> > > {
> > Let the base class do this.
> 
> this doesn't inherit from nsBaseFilePicker, are you suggesting that it should?
Yes.

Comment 5

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

Attachment: patch

Comment 6

[Michael Wu [:mwu]]

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

Close. Just one issue that needs to be addressed for parity with other widgets.

>diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java
>--- a/embedding/android/GeckoAppShell.java
>+++ b/embedding/android/GeckoAppShell.java
>@@ -417,8 +417,29 @@ class GeckoAppShell
>             return new Intent(Intent.ACTION_VIEW);
>     }
> 
>-    static String getMimeTypeFromExtension(String aFileExt) {
>-        return android.webkit.MimeTypeMap.getSingleton().getMimeTypeFromExtension(aFileExt);
>+    static String getMimeTypeFromExtensions(String aFileExt) {
>+        android.webkit.MimeTypeMap mtm =
>+            android.webkit.MimeTypeMap.getSingleton();
>+        StringTokenizer st = new StringTokenizer(aFileExt, "., ");
>+        String type = null;
>+        String subType = null;
Hmm, does Java require this? I remember java initializes everything to sane values, but sometimes it just complains and make you set it instead.

>+        while (st.hasMoreElements()) {
>+            String ext = st.nextToken();
>+            String mt = mtm.getMimeTypeFromExtension(ext);
>+            if (mt == null) continue;
continue on the next line.

>diff --git a/widget/src/android/nsFilePicker.h b/widget/src/android/nsFilePicker.h
>--- a/widget/src/android/nsFilePicker.h
>+++ b/widget/src/android/nsFilePicker.h
>@@ -38,16 +38,17 @@
> #ifndef NSFILEPICKER_H
> #define NSFILEPICKER_H
> 
>-#include "nsIFilePicker.h"
>+#include "nsBaseFilePicker.h"
> #include "nsString.h"
> 
>-class nsFilePicker : public nsIFilePicker
>+class nsFilePicker : public nsBaseFilePicker
> {
> public:
>   NS_DECL_ISUPPORTS
>   NS_DECL_NSIFILEPICKER
>-
> private:
>+  void InitNative(nsIWidget*, const nsAString&, short int) {}
>   nsString mFilePath;
>+  nsString mFilters;
> };
> #endif
Check out the nsFilePicker.h in other widgets. They don't use NS_DECL_NSIFILEPICKER so they don't need to implement any functions already in the base impl.

Comment 7

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

Attachment: patch

Comment 8

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

(In reply to comment #6)
> Comment on attachment 487383 [details] [diff] [review]
> patch

> >+        String type = null;
> >+        String subType = null;
> Hmm, does Java require this? I remember java initializes everything to sane
> values, but sometimes it just complains and make you set it instead.

yea, using a variable before its initialized is an error in java.

Comment 9

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

pushed http://hg.mozilla.org/mozilla-central/rev/847df5a157c9