Closed Bug 597041 Opened 9 years ago Closed 9 years ago

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

Categories

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

ARM
Android

Tracking

(fennec2.0b2+)

VERIFIED FIXED
Tracking Status
fennec 2.0b2+ ---

People

(Reporter: nhirata, Assigned: fabrice)

References

Details

Attachments

(2 files, 2 obsolete files)

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
Note:
1) shows on the N900 as well as the Mac OS X version of Fennec
nominated as block; blocks fennec beta testing for android.
tracking-fennec: --- → ?
Duplicate of this bug: 597044
Duplicate of this bug: 597392
tracking-fennec: ? → 2.0b2+
Why is this a b2 blocker? Shouldn't it be blocking b1?
Summary: Feedback is missing in the control panel on the Android platform → [Feedback] Feedback is missing in the control panel on the Android platform
I feel it's too much work for b1
We can definitely try to land this ahead of b2 so it will be in the nightlies for a while.
What's needed to get this done?
Assignee: nobody → fabrice
Attached patch platform patch (obsolete) — Splinter Review
We add .xpis at the root of the .apk, and copy them to /data/data/org.mozilla.fennec/extensions
Attached patch m-b patch (obsolete) — Splinter Review
Creates a packaged version of the extensions if we compile for android.
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.
Attachment #477963 - Flags: review+
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
Attachment #477961 - Flags: review?(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.
Priority: -- → P1
Attached patch m-b patch v2Splinter Review
We now create a .xpi for all platforms.
Attachment #477963 - Attachment is obsolete: true
Attachment #478248 - Flags: review?(mark.finkle)
Addressing comments, this now works for Maemo & Android.
Attachment #477961 - Attachment is obsolete: true
Attachment #478249 - Flags: review?(mwu)
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?
Attachment #478248 - Flags: review?(mark.finkle) → review+
Comment on attachment 478249 [details] [diff] [review]
platform patch v2

Huh, didn't know <? extends ZipEntry> would work.
Attachment #478249 - Flags: review?(mwu) → review+
Whiteboard: [fennec-checkin-postb1]
pushed http://hg.mozilla.org/mozilla-central/rev/ea09bb0212c4 and http://hg.mozilla.org/mobile-browser/rev/c0915b9c0abb
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fennec-checkin-postb1]
verified :
Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101011 Firefox/4.0b8pre Fennec/4.0b2pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.