597041 - [Feedback] Feedback is missing in the control panel on the Android platform

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

on the android platform:
1) go to the control panel
2) look for the Feedback panel

Expected:
beta button for the Feedback panel

Actual:
No beta button

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Note:
1) shows on the N900 as well as the Mac OS X version of Fennec

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

nominated as block; blocks fennec beta testing for android.

Comment 5

[Aakash Desai [:aakashd]]

Why is this a b2 blocker? Shouldn't it be blocking b1?

Comment 6

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

I feel it's too much work for b1

Comment 7

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

We can definitely try to land this ahead of b2 so it will be in the nightlies for a while.

Comment 8

[Aakash Desai [:aakashd]]

What's needed to get this done?

Comment 9

[[:fabrice] Fabrice Desré]

Attachment: platform patch

We add .xpis at the root of the .apk, and copy them to /data/data/org.mozilla.fennec/extensions

Comment 10

[[:fabrice] Fabrice Desré]

Attachment: m-b patch

Creates a packaged version of the extensions if we compile for android.

Comment 11

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

Comment on attachment 477963 [details] [diff] [review]
m-b patch

>Bug 597041 : creates an .xpi for android
>
>
>diff --git a/app/profile/extensions/Makefile.in b/app/profile/extensions/Makefile.in
>--- a/app/profile/extensions/Makefile.in
>+++ b/app/profile/extensions/Makefile.in
>@@ -54,6 +54,17 @@ define _INSTALL_EXTENSIONS
> 
> endef # do not remove the blank line!
> 
>+#ifdef ANDROID
>+define _PACKAGE_EXTENSIONS
>+jar cfM $(DIST)/bin/extensions/$(dir).xpi -C $(srcdir)/$(dir)/ .
>+
>+endef 
>+#endif

Add the "# do not remove the blank line!" after the "endef" (like the one above) and add a blank line after the "endef" just to be sure.

Comment 12

[Michael Wu [:mwu]]

Comment on attachment 477961 [details] [diff] [review]
platform patch

>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
>--- a/embedding/android/GeckoApp.java
>+++ b/embedding/android/GeckoApp.java
>@@ -317,6 +317,17 @@ abstract public class GeckoApp
>         unpackFile(zip, buf, null, getContentProcessName());
>         unpackFile(zip, buf, null, "update.locale");
> 
>+        // copy any .xpi file into an extensions/ directory
>+        // the extension/ directory is created if needed by unpackFile()
I think this comment about unpackFile isn't necessary.

>+        Enumeration<?> zipEntries = zip.entries();
Enumeration<ZipEntry> should be what you want here. It should get rid of the cast in the while loop.

>+        while (zipEntries.hasMoreElements()) {
>+          ZipEntry entry = (ZipEntry) zipEntries.nextElement();
>+          if (entry.getName().endsWith(".xpi")) {
We should also check if it begins with extensions/. It looks like the xpis might not be in that directory, but if they aren't, they should be placed there.

>+            Log.i("GeckoAppJava", "installing extension : " + entry.getName());
>+            unpackFile(zip, buf, entry, "extensions/" + entry.getName());
>+          }
>+        }
>+        
>         try {
>             ZipEntry componentsList = zip.getEntry("components/components.manifest");
>             if (componentsList == null) {
>diff --git a/embedding/android/Makefile.in b/embedding/android/Makefile.in
>--- a/embedding/android/Makefile.in
>+++ b/embedding/android/Makefile.in
>@@ -121,6 +121,8 @@ DIST_LINK_FILES = \
>   chrome.manifest \
>   $(NULL)
> 
>+DIST_LINK_FILES += $(shell ls $(DIST)/bin/extensions/*.xpi)
>+
I suspect you can just add extensions to DIST_LINK_FILES and use zip -r9m to pack the extensions in your other patch. This will move files into the extension so only the .xpis are left. Also consider doing this on all platforms instead of just Maemo, since the code to read packed extensions is enabled on all platforms.

> ifdef MOZ_IPC
> DIST_LINK_FILES += $(MOZ_CHILD_PROCESS_NAME)
> endif

Comment 13

[Michael Wu [:mwu]]

(In reply to comment #12)
> Also consider doing this on all platforms
> instead of just Maemo, since the code to read packed extensions is enabled on
> all platforms.
And by Maemo I actually mean Android.

Comment 14

[[:fabrice] Fabrice Desré]

Attachment: m-b patch v2

We now create a .xpi for all platforms.

Comment 15

[[:fabrice] Fabrice Desré]

Attachment: platform patch v2

Addressing comments, this now works for Maemo & Android.

Comment 16

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

Comment on attachment 478248 [details] [diff] [review]
m-b patch v2

I assume you tested that Fennec loads the XPI from the bin/extensions folder correctly?

Comment 17

[Michael Wu [:mwu]]

Comment on attachment 478249 [details] [diff] [review]
platform patch v2

Huh, didn't know <? extends ZipEntry> would work.

Comment 18

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

pushed http://hg.mozilla.org/mozilla-central/rev/ea09bb0212c4 and http://hg.mozilla.org/mobile-browser/rev/c0915b9c0abb

Comment 19

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

verified :
Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101011 Firefox/4.0b8pre Fennec/4.0b2pre