590225 - webapps OS level integration : Android

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

We want to be able to "install" web apps on Android like on Maemo.

On Maemo we install a .deb package generated by fennec. The equivalent for Android would be to generate an .apk package and install it. Another option could be to use ACTION_CREATE_SHORTCUT but I'm not sure the user experience wouldbe comparable.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: platform patch V1

This patch adds functionnality to allow web apps installation by the way of desktop shortcuts. It reuses the window from a web app if we reselect it  after the first launch.

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: UI patch V1

Android implementation for the UI handler. It simply makes a jsctypes call to the createShortcut function from the platform patch.

Comment 3

[Michael Wu [:mwu]]

Comment on attachment 471879 [details] [diff] [review]
platform patch V1

Please add showfunc=true to the diff section of your hgrc.

I've reviewed this patch but they are mostly nits. My main concern is the android specific interface that you're introducing - AndroidCreateShortcut. Are you sure you want to do this? A normal idl/xpcom interface that covers most/all platforms would be preferred over a platform specific ctypes interface.

There's also the window mediator stuff which seems fairly sketchy to me. Finding the right window to focus seems like a job for the frontend which can do it for all platforms.

>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
>--- a/embedding/android/GeckoApp.java
>+++ b/embedding/android/GeckoApp.java
>@@ -143,6 +143,18 @@
>             GeckoAppShell.sendEventToGecko(new GeckoEvent(uri));
>             Log.i("GeckoApp","onNewIntent: "+uri);
>         }
>+        else if (action.equals("org.mozilla.fennec.WEBAPP")) {
>+            //String uri = intent.getDataString();
Remove

>+            String uri = intent.getStringExtra("args");
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent(uri));
>+            Log.i("GeckoApp","Intent : WEBAPP - " + uri);
>+        }
>+        else if (Intent.ACTION_MAIN.equals(action)) {
action.equals(Intent.ACTION_MAIN) to keep things consistent.

>+            Log.i("GeckoApp", "Intent : ACTION_MAIN");
>+            // find main window (windowtype == "navigator:browser") and focus it
>+            // hackish, but using JNI failed with different threads reusing the same JNIEnv
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent("action://raiseBrowser"));
>+        }
>     }
> 
>     @Override

>diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java
>--- a/embedding/android/GeckoAppShell.java
>+++ b/embedding/android/GeckoAppShell.java
>@@ -357,6 +357,27 @@
>         Log.i("GeckoAppJava", "scheduling restart");
>         gRestartScheduled = true;        
>     }
>+ 
>+    // Creates a shortcut using an intent
>+    static void createShortcut(String aURI, String aTitle, String aIconData) {
>+        Log.w("GeckoAppJava", "createShortcut for " + aURI + " [" + aTitle + "]");
>+        
>+        // the intent to be launched by the shortcut
>+        Intent shortcutIntent = new Intent("org.mozilla.fennec.WEBAPP");
>+        shortcutIntent.setClassName(GeckoApp.mAppContext,
>+                                    "org.mozilla." + GeckoApp.mAppContext.getAppName() + ".App");
>+        shortcutIntent.putExtra("args", "--webapp=" + aURI);
>+        
>+        Intent intent = new Intent();
>+        intent.putExtra(Intent.EXTRA_SHORTCUT_INTENT, shortcutIntent);
>+        intent.putExtra(Intent.EXTRA_SHORTCUT_NAME, aTitle);
>+        byte[] raw = android.util.Base64.decode(aIconData, android.util.Base64.DEFAULT);
android.util.* is already imported.

>+        Bitmap bitmap = BitmapFactory.decodeByteArray(raw, 0, raw.length);
>+        intent.putExtra(Intent.EXTRA_SHORTCUT_ICON, bitmap);
>+        //intent.setAction("com.android.launcher.action.INSTALL_SHORTCUT");
Remove

>+        intent.setAction(Intent.ACTION_CREATE_SHORTCUT);
>+        GeckoApp.mAppContext.sendBroadcast(intent);
>+    }
>     
>     static String[] getHandlersForMimeType(String aMimeType) {
>         PackageManager pm = 

>diff --git a/widget/src/android/AndroidBridge.h b/widget/src/android/AndroidBridge.h
>--- a/widget/src/android/AndroidBridge.h
>+++ b/widget/src/android/AndroidBridge.h
>@@ -87,6 +87,10 @@
>     static JNIEnv *JNIForThread() {
>         return sBridge->AttachThread();
>     }
>+    
>+    static jclass GetGeckoAppShellClass() {
>+        return sBridge->mGeckoAppShellClass;
>+    }
> 
>     // The bridge needs to be constructed via ConstructBridge first,
>     // and then once the Gecko main thread is spun up (Gecko side),
>@@ -214,5 +218,6 @@
> 
> extern "C" JNIEnv * GetJNIForThread();
> extern PRBool mozilla_AndroidBridge_SetMainThread(void *);
>+extern jclass GetGeckoAppShellClass();
> 
> #endif /* AndroidBridge_h__ */

>diff --git a/widget/src/android/nsAppShell.cpp b/widget/src/android/nsAppShell.cpp
>--- a/widget/src/android/nsAppShell.cpp
>+++ b/widget/src/android/nsAppShell.cpp
>@@ -44,6 +44,10 @@
> #include "nsIAppStartup.h"
> #include "nsIGeolocationProvider.h"
> #include "nsIPrefService.h"
>+#include "nsIWindowMediator.h"
>+#include "nsIDOMWindowInternal.h"
>+#include "nsIDOMElement.h"
>+#include "nsIDOMDocument.h"
> 
> #include "mozilla/Services.h"
> #include "prenv.h"
>@@ -135,6 +139,51 @@
> 
> 
> PRBool
>+nsAppShell::ReopenWebappWindow(const nsAString& webapp)
>+{
>+    nsString start(webapp);
>+    start.Truncate(9);
>+    if (!start.Equals(NS_LITERAL_STRING("--webapp=")))
Compare to the substring instead. Something like Substring(webapp, 0, 0).Equals

>+        return false;
>+    
>+    nsresult rv;
>+    nsCOMPtr<nsIWindowMediator> wm = do_GetService(NS_WINDOWMEDIATOR_CONTRACTID, &rv);
>+    if (rv != NS_OK)
NS_SUCCEEDED(rv)

>+        return false;
>+    
>+    nsCOMPtr<nsISimpleEnumerator> windowEnumerator;
>+    wm->GetEnumerator(NS_LITERAL_STRING("navigator:webapp").get(), getter_AddRefs(windowEnumerator));
>+    PRBool more;
>+    windowEnumerator->HasMoreElements(&more);
HasMoreElements can fail in theory.

>+    while (more) {
while (NS_SUCCEEDED(windowEnumerator->HasMoreElements(&more)) && more)

>+        nsCOMPtr<nsISupports> window;
>+        windowEnumerator->GetNext(getter_AddRefs(window));
>+        if (window) {
if (!window) continue;

>+            nsCOMPtr<nsIDOMWindowInternal> domWindow = do_QueryInterface(window);
>+            if (domWindow) {
if (!domWindow) continue;

>+                nsCOMPtr<nsIDOMDocument> domdoc;
>+                domWindow->GetDocument(getter_AddRefs(domdoc));
>+                nsCOMPtr<nsIDOMElement> aRoot;
>+                domdoc->GetElementById(NS_LITERAL_STRING("main-window"), getter_AddRefs(aRoot));
>+                PRBool bWebApp;
For locally defined things we normally don't prefix. In this case I would name it isWebApp.

>+                aRoot->HasAttribute(NS_LITERAL_STRING("webapp"), &bWebApp);
>+                if (bWebApp) {
if (!isWebApp) continue;

>+                    nsAutoString attUri;
>+                    aRoot->GetAttribute(NS_LITERAL_STRING("webapp"), attUri);
>+                    attUri.Insert(NS_LITERAL_STRING("--webapp="), 0);
>+                    if (attUri.Equals(webapp)) {
if (!attUri.Equals(webapp)) continue;


>+                        domWindow->Focus();
>+                        return true;
>+                    }
>+                }
>+            }
>+        }
>+        windowEnumerator->HasMoreElements(&more);
>+    }
>+    return false;
>+}
>+
>+PRBool
> nsAppShell::ProcessNextNativeEvent(PRBool mayWait)
> {
>     EVLOG("nsAppShell::ProcessNextNativeEvent %d", mayWait);
>diff --git a/widget/src/android/nsAppShell.h b/widget/src/android/nsAppShell.h
>--- a/widget/src/android/nsAppShell.h
>+++ b/widget/src/android/nsAppShell.h
>@@ -78,7 +78,7 @@
> protected:
>     virtual void ScheduleNativeEventCallback();
>     virtual ~nsAppShell();
>-
>+    PRBool ReopenWebappWindow(const nsAString &webapp);
bool (Looks like you used true/false in the impl already - great!)

Comment 4

[[:fabrice] Fabrice Desré]

Thanks for your review Michael,

> I've reviewed this patch but they are mostly nits. My main concern is the
> android specific interface that you're introducing - AndroidCreateShortcut. Are
> you sure you want to do this? A normal idl/xpcom interface that covers most/all
> platforms would be preferred over a platform specific ctypes interface.

I'm not a big fan either of adding web apps specific stuff in the platform code for now. What I need is a way to call java code from js. Writing a dedicated xpcom for this is indeed a solution, but what about a more generic solution using intents for this?

From the JS side, we'll have something like :
let bridge = Cc["mozilla.org/android/bridge;1].getService(Ci.AndroidIntentBridge);
let intent = Cc["mozilla.org/android/intent;1].createInstance(Ci.AndroidIntent);
intent.action = Ci.AndroidIntent.ACTION_MAIN;
intent.data = aURI;
intent.category = ...
intent.component = ...
intent.putExtra("arg1", "hello");  // don't plan to support all the java datatypes here
intent.putExtra("arg2", 10);

bridge.broadcastIntent(intent);

With the c++ side implementing the intent bridge, and using custom intents to point to specific java classes.
This should allow a better decoupling when we want to add new features. I don't know if this would implies performance penalties that we can't live with.

In my case, the web app specific code would involve writing a java class listening for a org.mozilla.fennec.INSTALL_WEBAPP intent, with 3 string parameters (title, uri, and base64 encoded icon).

> There's also the window mediator stuff which seems fairly sketchy to me.
> Finding the right window to focus seems like a job for the frontend which can
> do it for all platforms.

Yes, you're obviously right. The update to bug 584767 does this.

Comment 5

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

Comment on attachment 471881 [details] [diff] [review]
UI patch V1


>+    Cu.reportError("Android::installWebApp " + aURI);
don't use reportError if its not an error, instead use nsIConsoleService's logStringMessage

>+    try  {
>+    let libxul = ctypes.open("/data/data/org.mozilla.fennec/lib/libxul.so");
>+    let AndroidCreateShortcut = libxul.declare("AndroidCreateShortcut", ctypes.default_abi, ctypes.void_t,
>+                                    ctypes.jschar.ptr, ctypes.jschar.ptr, ctypes.jschar.ptr);

This obviously depends on the platform patch, may I suggest moving that function into the AndroidBridge

Comment 6

[Michael Wu [:mwu]]

(In reply to comment #4)
> Thanks for your review Michael,
> 
> > I've reviewed this patch but they are mostly nits. My main concern is the
> > android specific interface that you're introducing - AndroidCreateShortcut. Are
> > you sure you want to do this? A normal idl/xpcom interface that covers most/all
> > platforms would be preferred over a platform specific ctypes interface.
> 
> I'm not a big fan either of adding web apps specific stuff in the platform code
> for now. What I need is a way to call java code from js. Writing a dedicated
> xpcom for this is indeed a solution, but what about a more generic solution
> using intents for this?
> 
We should be presenting an interface to the frontend that hides the fact that we're calling java code and using intents. Using intents is not any more generic. Once we have this xpcom interface, the implementation can be js or c++ or whatever makes the most sense for the given platform.

As far as I can tell, you already have a cross platform interface for achieving this: |install: function(aURI, aTitle, aIconURI, aIconData)|. Just stick that into an idl and then the android implementation can be all C++ without any need to juggle data across androidbridge/ctypes/etc. You'll also be able to get rid of a nest of ifdefs since platforms that don't implement webapps can't return a service/object to implement this install function.

> From the JS side, we'll have something like :
> let bridge =
> Cc["mozilla.org/android/bridge;1].getService(Ci.AndroidIntentBridge);
> let intent =
> Cc["mozilla.org/android/intent;1].createInstance(Ci.AndroidIntent);
> intent.action = Ci.AndroidIntent.ACTION_MAIN;
> intent.data = aURI;
> intent.category = ...
> intent.component = ...
> intent.putExtra("arg1", "hello");  // don't plan to support all the java
> datatypes here
> intent.putExtra("arg2", 10);
> 
> bridge.broadcastIntent(intent);
> 
> With the c++ side implementing the intent bridge, and using custom intents to
> point to specific java classes.
> This should allow a better decoupling when we want to add new features. I don't
> know if this would implies performance penalties that we can't live with.
> 
I understand the appeal of making as much code JS as possible but I don't think it is worth the extra complexity in this case. In fact, I'd argue for moving as much code to the Java wrapper side as possible as that's the most natural place to write code working with Java/Android APIs. Any other place requires more work and code punching data and APIs through various layers.

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: WIP plaform patch

WIP do move to a component approach. Since we need to link with AndroidBridge, we end up having a static library (in obj-i686-pc-linux-gnu/toolkit/components/webapps/libwebapps.a and obj-i686-pc-linux-gnu/staticlib/components/libwebapps.a), but it does not appear to be linked into libxul, so we fail to instanciate the component.

I probably miss something in the webapps/Makefile.in but can't see what.

Comment 8

[Michael Wu [:mwu]]

Comment on attachment 474064 [details] [diff] [review]
WIP plaform patch

Requesting feedback from some build peers as they'll probably know best on how to get your component linking into libxul.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Add your component's registration hooks in at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/build/nsToolkitCompsModule.cpp rather than defining your own module.

Is this intended to be something that eventually gets implemented on other platforms, or is this intended to be Android only?

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

In particular, you can use the makefile bits of http://hg.mozilla.org/mozilla-central/rev/416b9b6ffc77 as an example of what to do.

Comment 11

[[:fabrice] Fabrice Desré]

(In reply to comment #9)
> 
> Is this intended to be something that eventually gets implemented on other
> platforms, or is this intended to be Android only?

It's not Android only : for instance, I also have a (javascript) implementation for Maemo.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 474064 [details] [diff] [review]
WIP plaform patch

Clearing flags since I think you got the pointers you wanted.

Comment 13

[[:fabrice] Fabrice Desré]

Attachment: patch with xpcom component

New patch that uses an XPCOM component to provide the functionnality :
- we expose GetGeckoAppShellClass() from the AndroidBridge to call back into java from our xpcom.
- a new intent action is used to recognize that we are launching a web app.
- we don't really create applications, but merely install new shortcuts to the desktop.
- there's a resizing issue when we open a new window, that messes up the rendering. It's fine after an orientation change that itself triggers a resize event.

To test this patch, apply also the UI patch in bug 584767

Comment 14

[Michael Wu [:mwu]]

Comment on attachment 475880 [details] [diff] [review]
patch with xpcom component

Looks pretty close this time.

>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
>--- a/embedding/android/GeckoApp.java
>+++ b/embedding/android/GeckoApp.java
>@@ -147,6 +147,15 @@ abstract public class GeckoApp
>             GeckoAppShell.sendEventToGecko(new GeckoEvent(uri));
>             Log.i("GeckoApp","onNewIntent: "+uri);
>         }
>+        else if (Intent.ACTION_MAIN.equals(action)) {
>+            Log.i("GeckoApp", "Intent : ACTION_MAIN");
>+            GeckoAppShell.sendEventToGecko(new GeckoEvent("action://raiseBrowser"));
>+        }
Hmm I'm still not understanding this. This just ends up getting converted to "" in the end so why not GeckoAppShell.sendEventToGecko(new GeckoEvent("")); ?

What sends this intent?

>diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java
>--- a/embedding/android/GeckoAppShell.java
>+++ b/embedding/android/GeckoAppShell.java
>@@ -357,6 +357,27 @@ class GeckoAppShell
>         Log.i("GeckoAppJava", "scheduling restart");
>         gRestartScheduled = true;        
>     }
>+ 
>+    // "Installs" an application by creating a shortcut
>+    static void installWebApplication(String aURI, String aTitle, String aIconData) {
>+        Log.w("GeckoAppJava", "installWebApplication for " + aURI + " [" + aTitle + "]");
>+
>+        // the intent to be launched by the shortcut
>+        Intent shortcutIntent = new Intent("org.mozilla.fennec.WEBAPP");
>+        shortcutIntent.setClassName(GeckoApp.mAppContext,
>+                                    "org.mozilla." + GeckoApp.mAppContext.getAppName() + ".App");
>+        shortcutIntent.putExtra("args", "--webapp=" + aURI);
>+        
>+        Intent intent = new Intent();
>+        intent.putExtra(Intent.EXTRA_SHORTCUT_INTENT, shortcutIntent);
>+        intent.putExtra(Intent.EXTRA_SHORTCUT_NAME, aTitle);
>+        byte[] raw = android.util.Base64.decode(aIconData.substring(22), android.util.Base64.DEFAULT);
s/android.util.//g

>+        Bitmap bitmap = BitmapFactory.decodeByteArray(raw, 0, raw.length);
>+        intent.putExtra(Intent.EXTRA_SHORTCUT_ICON, bitmap);
>+        intent.setAction("com.android.launcher.action.INSTALL_SHORTCUT");
>+        //intent.setAction(Intent.ACTION_CREATE_SHORTCUT);
remove. (how odd that ACTION_CREATE_SHORTCUT doesn't work)

>+        GeckoApp.mAppContext.sendBroadcast(intent);
>+    }
>     
>     static String[] getHandlersForMimeType(String aMimeType) {
>         PackageManager pm = 
>diff --git a/toolkit/components/build/nsToolkitCompsCID.h b/toolkit/components/build/nsToolkitCompsCID.h
>--- a/toolkit/components/build/nsToolkitCompsCID.h
>+++ b/toolkit/components/build/nsToolkitCompsCID.h
>@@ -119,6 +119,11 @@
> #define NS_APPSTARTUP_CONTRACTID \
>   "@mozilla.org/toolkit/app-startup;1"
> 
>+#ifdef ANDROID
>+#define NS_WEBAPPSSUPPORT_CONTRACTID \
>+  "@mozilla.org/webapps/installer;1"
>+#endif
>+
Seems like we could safely define this all the time? (unless the prevailing style of the file is to ifdef everything, in which case this is fine)

> /////////////////////////////////////////////////////////////////////////////
> 
> // {A0CCAAF8-09DA-44D8-B250-9AC3E93C8117}
>@@ -202,3 +207,8 @@
> // {6fb0c970-e1b1-11db-8314-0800200c9a66}
> #define NS_PLACESIMPORTEXPORTSERVICE_CID \
> { 0x6fb0c970, 0xe1b1, 0x11db, { 0x83, 0x14, 0x08, 0x00, 0x20, 0x0c, 0x9a, 0x66 } }
>+
>+#ifdef ANDROID
>+#define NS_WEBAPPSSUPPORT_CID \
>+{ 0xd0b62752, 0x88be, 0x4c88, {0x94, 0xe5, 0xc6, 0x9e, 0x15, 0xa1, 0x0c, 0x4e} }
>+#endif
Ditto

>diff --git a/toolkit/components/webapps/Makefile.in b/toolkit/components/webapps/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/webapps/Makefile.in
>@@ -0,0 +1,71 @@
>+# ***** 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
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# The Original Code is Mozilla Webapp code.
>+#
>+# The Initial Developer of the Original Code is
>+# the Mozilla Foundation <http://www.mozilla.org/>.
no uri after Foundation.

>+# Portions created by the Initial Developer are Copyright (C) 2008
2010

>diff --git a/toolkit/components/webapps/nsWebappsSupport.cpp b/toolkit/components/webapps/nsWebappsSupport.cpp
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/webapps/nsWebappsSupport.cpp
>@@ -0,0 +1,69 @@
>+/* -*- 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
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Mozilla Webapp code.
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Fabrice Desré <[e-mail]>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#define MOZILLA_INTERNAL_API
>+
I suspect this is wrong but I have no reason for why I think so.

>+#include "AndroidBridge.h"
>+#include "nsCRTGlue.h"
>+#include "nsWebappsSupport.h"
>+
>+NS_IMPL_ISUPPORTS1(nsWebappsSupport, nsIWebappsSupport)
>+
>+NS_IMETHODIMP
>+nsWebappsSupport::InstallApplication(const PRUnichar *aTitle, const PRUnichar *aURI, const PRUnichar *aIconURI, const PRUnichar *aIconData)
>+{
>+  ALOG("in nsWebappsSupport::InstallApplication()\n");
>+  JNIEnv *jEnv = GetJNIForThread();
>+  jclass jGeckoAppShellClass = GetGeckoAppShellClass();
>+  jmethodID jInstallWebApplication = jEnv->GetStaticMethodID(jGeckoAppShellClass, "installWebApplication", "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)V");
>+  jstring jstrURI = jEnv->NewString(aURI, NS_strlen(aURI));
>+  jstring jstrTitle = jEnv->NewString(aTitle, NS_strlen(aTitle));
>+  jstring jstrIconData = jEnv->NewString(aIconData, NS_strlen(aIconData));
>+  jEnv->CallStaticVoidMethod(jGeckoAppShellClass, jInstallWebApplication, jstrURI, jstrTitle, jstrIconData);
>+  return NS_OK;
>+}
You'll need a AutoLocalJNIFrame here. (or you'll leak all your strings)

Also a lot of these things can return null.

Comment 15

[[:fabrice] Fabrice Desré]

Attachment: updated patch

Addressing comments. 

Details

> >+        else if (Intent.ACTION_MAIN.equals(action)) {
> >+            Log.i("GeckoApp", "Intent : ACTION_MAIN");
> >+            GeckoAppShell.sendEventToGecko(new GeckoEvent("action://raiseBrowser"));
> >+        }
> Hmm I'm still not understanding this. This just ends up getting converted to ""
> in the end so why not GeckoAppShell.sendEventToGecko(new GeckoEvent("")); ?
> 
> What sends this intent?

Indeed using new GeckoEvent("") is enough, and this removes all the code from nsAppShell.cpp. We need this when we have a web app running, but want to start the "classic" browser. We then receive the ACTION_MAIN intent, and must open a new window or at least put it in the foreground. If we don't focus it ourselves, android displays the last web app in use.

> >+
> >+#define MOZILLA_INTERNAL_API
> >+
> I suspect this is wrong but I have no reason for why I think so.

That was a remain from an old patch, it's not needed now that the web app component is using the appropriate build system.

The only platform issue remaining is that when you open a new window, some  resize event seems to not reach the front end, and the layout is messed up. See the screenshot attached, showing the main browser window. Triggering a resize (eg. by changing the device orientation) fixes the layout. This could be worked around in the front end, but rather looks like a platform window issue?

Comment 16

[[:fabrice] Fabrice Desré]

Attachment: browser window when opened as a second window

Comment 17

[[:fabrice] Fabrice Desré]

Attachment: logcat when opening new window

Here's the logcat relevant output when we open a new window, leading to the previous screenshot.

Comment 18

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

(In reply to comment #15)

> The only platform issue remaining is that when you open a new window, some 
> resize event seems to not reach the front end, and the layout is messed up. See
> the screenshot attached, showing the main browser window. Triggering a resize
> (eg. by changing the device orientation) fixes the layout. This could be worked
> around in the front end, but rather looks like a platform window issue?

Bug 556393?

Comment 19

[Michael Wu [:mwu]]

Comment on attachment 477888 [details] [diff] [review]
updated patch

Nice. Just nits this time. Switched review from blassey to vlad since I think the android part is reviewed enough. Just need review from a toolkit peer. Not sure if putting the android specific code in toolkit is the right thing.

>diff --git a/embedding/android/AndroidManifest.xml.in b/embedding/android/AndroidManifest.xml.in
>--- a/embedding/android/AndroidManifest.xml.in
>+++ b/embedding/android/AndroidManifest.xml.in
>@@ -9,9 +9,10 @@
>     <uses-sdk android:minSdkVersion="5"
>               android:targetSdkVersion="5"/>
> 
>-    <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/> 
>-    <uses-permission android:name="android.permission.INTERNET"/> 
>-    <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/> 
>+    <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/>
>+    <uses-permission android:name="android.permission.INTERNET"/>
>+    <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/>
>+    <uses-permission android:name="com.android.launcher.permission.INSTALL_SHORTCUT"/>
> 

Since we're using a somewhat less standard feature here, we'll probably want to test this on htc and samsung firmware to see if their launcher code handles this right. I can test on HTC firmware.

>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
>--- a/embedding/android/GeckoApp.java
>+++ b/embedding/android/GeckoApp.java
>@@ -138,7 +138,7 @@ abstract public class GeckoApp
> 
>         super.onCreate(savedInstanceState);
>     }
>-
>+    

No whitespace on blank lines is preferred.

>diff --git a/toolkit/components/build/nsToolkitCompsCID.h b/toolkit/components/build/nsToolkitCompsCID.h
>--- a/toolkit/components/build/nsToolkitCompsCID.h
>+++ b/toolkit/components/build/nsToolkitCompsCID.h
>@@ -202,3 +205,8 @@
> // {6fb0c970-e1b1-11db-8314-0800200c9a66}
> #define NS_PLACESIMPORTEXPORTSERVICE_CID \
> { 0x6fb0c970, 0xe1b1, 0x11db, { 0x83, 0x14, 0x08, 0x00, 0x20, 0x0c, 0x9a, 0x66 } }
>+
>+#ifdef ANDROID
>+#define NS_WEBAPPSSUPPORT_CID \
>+{ 0xd0b62752, 0x88be, 0x4c88, {0x94, 0xe5, 0xc6, 0x9e, 0x15, 0xa1, 0x0c, 0x4e} }
>+#endif

This ifdef can be removed.

>diff --git a/toolkit/components/build/nsToolkitCompsModule.cpp b/toolkit/components/build/nsToolkitCompsModule.cpp
>--- a/toolkit/components/build/nsToolkitCompsModule.cpp
>+++ b/toolkit/components/build/nsToolkitCompsModule.cpp
>@@ -72,6 +72,12 @@
> 
> #include "nsBrowserStatusFilter.h"
> 
>+#ifdef ANDROID
>+#include "nsWebappsSupport.h"
>+
>+NS_GENERIC_FACTORY_CONSTRUCTOR(nsWebappsSupport)
>+#endif
>+

Probably want to stick the NS_GENERIC_FACTORY_CONSTRUCTOR in the section full of NS_GENERIC_FACTORY_CONSTRUCTORs instead of with the includes.

>diff --git a/toolkit/components/webapps/nsWebappsSupport.cpp b/toolkit/components/webapps/nsWebappsSupport.cpp
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/webapps/nsWebappsSupport.cpp
>+#include "AndroidBridge.h"
>+#include "nsCRTGlue.h"
nsCRTGlue.h is very rarely used AFAICT. I suspect the right header to include is something else..

>diff --git a/toolkit/components/webapps/nsWebappsSupport.h b/toolkit/components/webapps/nsWebappsSupport.h
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/webapps/nsWebappsSupport.h
>+#ifndef nsWebappsSupport_h__
>+#define nsWebappsSupport_h__
>+
>+#include "nsIWebappsSupport.h"
>+
>+class nsWebappsSupport : public nsIWebappsSupport
>+{
>+public:
>+
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIWEBAPPSSUPPORT
>+
>+  nsWebappsSupport() {};
>+  ~nsWebappsSupport(){};

Add a space after ().

>+
>+};
>+
>+#endif

#endif // nsWebappsSupport_h__

Comment 20

[[:fabrice] Fabrice Desré]

Attachment: addressing comments

Updated patch with comments from Michael addressed, except for the  nsCTRGlue.h include. I need it for NS_strlen(), and all other files using this function also  included it.

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 479744 [details] [diff] [review]
addressing comments

So this looks good to me, but I think you want blassey or mwu to take a look at the android bits as well.

Comment 22

[[:fabrice] Fabrice Desré]

Attachment: better commit string

Comment 23

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

pushed http://hg.mozilla.org/mozilla-central/rev/7e10dcf5f763

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Backed out due to bustage:
http://hg.mozilla.org/mozilla-central/rev/70fe427ed0fc
http://hg.mozilla.org/mozilla-central/rev/60f4509b6e08

Comment 25

[[:fabrice] Fabrice Desré]

Attachment: updated patch

Better isolation of Android specific sections in webapps/Makefile.in

Checked on try, green on all platforms :
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-2835d362cce1

Comment 26

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

Comment on attachment 480743 [details] [diff] [review]
updated patch


>diff --git a/toolkit/components/Makefile.in b/toolkit/components/Makefile.in
>--- a/toolkit/components/Makefile.in
>+++ b/toolkit/components/Makefile.in
>@@ -70,6 +70,7 @@ PARALLEL_DIRS += \
>   typeaheadfind \
>   urlformatter \
>   viewconfig \
>+  webapps \
>   $(NULL)


I'd recommend having:

ifdef ANDROID
PARALLEL_DIRS += webapps
endif

> 
> ifdef BUILD_CTYPES
>diff --git a/toolkit/components/build/Makefile.in b/toolkit/components/build/Makefile.in
>--- a/toolkit/components/build/Makefile.in
>+++ b/toolkit/components/build/Makefile.in
>@@ -80,6 +80,12 @@ LOCAL_INCLUDES += \
> 	$(NULL)
> endif
> 
>+ifeq (Android,$(OS_TARGET))
>+LOCAL_INCLUDES += \
>+	-I$(srcdir)/../webapps \
>+	$(NULL)
>+endif
>+
> SHARED_LIBRARY_LIBS = \
> 	../find/src/$(LIB_PREFIX)mozfind_s.$(LIB_SUFFIX) \
> 	../typeaheadfind/src/$(LIB_PREFIX)fastfind_s.$(LIB_SUFFIX) \
>@@ -120,6 +126,10 @@ ifdef MOZ_FEEDS
> SHARED_LIBRARY_LIBS += ../feeds/src/$(LIB_PREFIX)feed_s.$(LIB_SUFFIX)
> endif
> 
>+ifeq (Android,$(OS_TARGET))
>+SHARED_LIBRARY_LIBS += ../webapps/$(LIB_PREFIX)webapps_s.$(LIB_SUFFIX)
>+endif
>+
> EXTRA_DSO_LIBS = gkgfx
> 
> EXTRA_DSO_LDOPTS += \
>diff --git a/toolkit/components/build/nsToolkitCompsCID.h b/toolkit/components/build/nsToolkitCompsCID.h
>--- a/toolkit/components/build/nsToolkitCompsCID.h
>+++ b/toolkit/components/build/nsToolkitCompsCID.h
>@@ -119,6 +119,9 @@
> #define NS_APPSTARTUP_CONTRACTID \
>   "@mozilla.org/toolkit/app-startup;1"
> 
>+#define NS_WEBAPPSSUPPORT_CONTRACTID \
>+  "@mozilla.org/webapps/installer;1"
>+
> /////////////////////////////////////////////////////////////////////////////
> 
> // {A0CCAAF8-09DA-44D8-B250-9AC3E93C8117}
>@@ -202,3 +205,7 @@
> // {6fb0c970-e1b1-11db-8314-0800200c9a66}
> #define NS_PLACESIMPORTEXPORTSERVICE_CID \
> { 0x6fb0c970, 0xe1b1, 0x11db, { 0x83, 0x14, 0x08, 0x00, 0x20, 0x0c, 0x9a, 0x66 } }
>+
>+#define NS_WEBAPPSSUPPORT_CID \
>+{ 0xd0b62752, 0x88be, 0x4c88, {0x94, 0xe5, 0xc6, 0x9e, 0x15, 0xa1, 0x0c, 0x4e} }
>+
>diff --git a/toolkit/components/build/nsToolkitCompsModule.cpp b/toolkit/components/build/nsToolkitCompsModule.cpp
>--- a/toolkit/components/build/nsToolkitCompsModule.cpp
>+++ b/toolkit/components/build/nsToolkitCompsModule.cpp
>@@ -72,6 +72,10 @@
> 
> #include "nsBrowserStatusFilter.h"
> 
>+#ifdef ANDROID
>+#include "nsWebappsSupport.h"
>+#endif
>+
> /////////////////////////////////////////////////////////////////////////////
> 
> NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsAppStartup, Init)
>@@ -125,6 +129,10 @@ NS_GENERIC_FACTORY_CONSTRUCTOR(nsScripta
> 
> NS_GENERIC_FACTORY_CONSTRUCTOR(nsBrowserStatusFilter)
> 
>+#ifdef ANDROID
>+NS_GENERIC_FACTORY_CONSTRUCTOR(nsWebappsSupport)
>+#endif
>+
> NS_DEFINE_NAMED_CID(NS_TOOLKIT_APPSTARTUP_CID);
> NS_DEFINE_NAMED_CID(NS_USERINFO_CID);
> #ifdef ALERTS_SERVICE
>@@ -151,6 +159,10 @@ NS_DEFINE_NAMED_CID(NS_SCRIPTABLEUNESCAP
> NS_DEFINE_NAMED_CID(NS_BROWSERSTATUSFILTER_CID);
> NS_DEFINE_NAMED_CID(NS_CHARSETMENU_CID);
> 
>+#ifdef ANDROID
>+NS_DEFINE_NAMED_CID(NS_WEBAPPSSUPPORT_CID);
>+#endif
>+
> static const mozilla::Module::CIDEntry kToolkitCIDs[] = {
>   { &kNS_TOOLKIT_APPSTARTUP_CID, false, NULL, nsAppStartupConstructor },
>   { &kNS_USERINFO_CID, false, NULL, nsUserInfoConstructor },
>@@ -177,6 +189,9 @@ static const mozilla::Module::CIDEntry k
> #endif
>   { &kNS_BROWSERSTATUSFILTER_CID, false, NULL, nsBrowserStatusFilterConstructor },
>   { &kNS_CHARSETMENU_CID, false, NULL, NS_NewCharsetMenu },
>+#ifdef ANDROID
>+  { &kNS_WEBAPPSSUPPORT_CID, false, NULL, nsWebappsSupportConstructor },
>+#endif
>   { NULL }
> };
> 
>@@ -207,6 +222,9 @@ static const mozilla::Module::ContractID
> #endif
>   { NS_BROWSERSTATUSFILTER_CONTRACTID, &kNS_BROWSERSTATUSFILTER_CID },
>   { NS_RDF_DATASOURCE_CONTRACTID_PREFIX NS_CHARSETMENU_PID, &kNS_CHARSETMENU_CID },
>+#ifdef ANDROID
>+  { NS_WEBAPPSSUPPORT_CONTRACTID, &kNS_WEBAPPSSUPPORT_CID },
>+#endif
>   { NULL }
> };
> 
>diff --git a/toolkit/components/webapps/Makefile.in b/toolkit/components/webapps/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/webapps/Makefile.in
>@@ -0,0 +1,72 @@
>+# ***** 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
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# The Original Code is Mozilla Webapp code.
>+#
>+# The Initial Developer of the Original Code is
>+# the Mozilla Foundation.
>+# Portions created by the Initial Developer are Copyright (C) 2010
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#    Fabrice Desré <[e-mail]>
>+#
>+# Alternatively, the contents of this file may be used under the terms of
>+# either the GNU General Public License Version 2 or later (the "GPL"), or
>+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+# in which case the provisions of the GPL or the LGPL are applicable instead
>+# of those above. If you wish to allow use of your version of this file only
>+# under the terms of either the GPL or the LGPL, and not to allow others to
>+# use your version of this file under the terms of the MPL, indicate your
>+# decision by deleting the provisions above and replace them with the notice
>+# and other provisions required by the GPL or the LGPL. If you do not delete
>+# the provisions above, a recipient may use your version of this file under
>+# the terms of any one of the MPL, the GPL or the LGPL.
>+#
>+# ***** END LICENSE BLOCK *****
>+
>+DEPTH		= ../../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
>+VPATH		= @srcdir@
>+
>+include $(DEPTH)/config/autoconf.mk
>+
>+MODULE_NAME     = webapps
>+MODULE          = webapps
>+                  
>+ifeq (Android,$(OS_TARGET))
>+LIBRARY_NAME    = webapps_s
>+LIBXUL_LIBRARY  = 1
>+IS_COMPONENT	  = 1
>+EXPORT_LIBRARY  = 1
>+
>+REQUIRES	= xpcom \
>+		  string \
>+		  $(NULL)
>+
>+CPPSRCS   = \
>+	nsWebappsSupport.cpp \
>+	$(NULL)
>+
>+EXTRA_DSO_LDOPTS += \
>+  $(MOZ_COMPONENT_LIBS) \
>+	$(XPCOM_GLUE_LDOPTS) \
>+	$(NSPR_LIBS) \
>+	$(NULL)
>+endif
>+
>+XPIDLSRCS = nsIWebappsSupport.idl
>+
>+include $(topsrcdir)/config/rules.mk
>+
>diff --git a/toolkit/components/webapps/nsIWebappsSupport.idl b/toolkit/components/webapps/nsIWebappsSupport.idl
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/webapps/nsIWebappsSupport.idl
>@@ -0,0 +1,60 @@
>+/* -*- 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
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Mozilla Webapp code.
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Fabrice Desré <[e-mail]>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#include "nsISupports.idl"
>+
>+[scriptable, uuid(adb91273-0cf1-4bbe-a37b-22e660192e2a)]
>+interface nsIWebappsSupport : nsISupports
>+{
>+  /**
>+   * This method installs a web app.
>+   *
>+   * @param title     the user-friendly name of the application.
>+   * @param uri       the uri of the web app.
>+   * @param iconData  a base64 encoded representation of the application's icon.
>+   */
>+  void installApplication(in wstring title, in wstring uri, in wstring iconUri, in wstring iconData);
>+  
>+  /**
>+   * Checks is a web app is already installed
>+   *
>+   * @param uri the uri of the web app
>+   * @return true if the web app is installed, false if it's not installed
>+   */
>+  boolean isApplicationInstalled(in wstring uri);
>+};
>+
>diff --git a/toolkit/components/webapps/nsWebappsSupport.cpp b/toolkit/components/webapps/nsWebappsSupport.cpp
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/webapps/nsWebappsSupport.cpp
>@@ -0,0 +1,78 @@
>+/* -*- 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
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Mozilla Webapp code.
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Fabrice Desré <[e-mail]>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#include "AndroidBridge.h"
>+#include "nsCRTGlue.h"
>+#include "nsWebappsSupport.h"
>+
>+using namespace mozilla;
>+
>+NS_IMPL_ISUPPORTS1(nsWebappsSupport, nsIWebappsSupport)
>+
>+NS_IMETHODIMP
>+nsWebappsSupport::InstallApplication(const PRUnichar *aTitle, const PRUnichar *aURI, const PRUnichar *aIconURI, const PRUnichar *aIconData)
>+{
>+  ALOG("in nsWebappsSupport::InstallApplication()\n");
>+  AndroidBridge::AutoLocalJNIFrame jniFrame;
>+  JNIEnv *jEnv = GetJNIForThread();
>+  jclass jGeckoAppShellClass = GetGeckoAppShellClass();
>+  
>+  if (!jEnv || !jGeckoAppShellClass)
>+    return NS_ERROR_FAILURE;
>+    
>+  jmethodID jInstallWebApplication = jEnv->GetStaticMethodID(jGeckoAppShellClass, "installWebApplication", "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)V");
>+  jstring jstrURI = jEnv->NewString(aURI, NS_strlen(aURI));
>+  jstring jstrTitle = jEnv->NewString(aTitle, NS_strlen(aTitle));
>+  jstring jstrIconData = jEnv->NewString(aIconData, NS_strlen(aIconData));
>+  
>+  if (!jstrURI || !jstrTitle || !jstrIconData)
>+    return NS_ERROR_FAILURE;
>+    
>+  jEnv->CallStaticVoidMethod(jGeckoAppShellClass, jInstallWebApplication, jstrURI, jstrTitle, jstrIconData);
>+  return NS_OK;
>+}
>+
>+/*
>+ * we have no way to know if an application is installed, so pretend it's not installed
>+ */
>+NS_IMETHODIMP
>+nsWebappsSupport::IsApplicationInstalled(const PRUnichar *aURI, PRBool *_retval)
>+{
>+  *_retval = PR_FALSE;
>+  return NS_OK;
>+}
>+
>diff --git a/toolkit/components/webapps/nsWebappsSupport.h b/toolkit/components/webapps/nsWebappsSupport.h
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/webapps/nsWebappsSupport.h
>@@ -0,0 +1,56 @@
>+/* -*- 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
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Mozilla Webapp code.
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Fabrice Desré <[e-mail]>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#ifndef nsWebappsSupport_h__
>+#define nsWebappsSupport_h__
>+
>+#include "nsIWebappsSupport.h"
>+
>+class nsWebappsSupport : public nsIWebappsSupport
>+{
>+public:
>+
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIWEBAPPSSUPPORT
>+
>+  nsWebappsSupport() {};
>+  ~nsWebappsSupport() {};
>+
>+};
>+
>+#endif // nsWebappsSupport_h__
>+
>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
>@@ -562,3 +562,8 @@ extern "C" JNIEnv * GetJNIForThread()
> {
>     return mozilla::AndroidBridge::JNIForThread();
> }
>+
>+jclass GetGeckoAppShellClass()
>+{
>+    return mozilla::AndroidBridge::GetGeckoAppShellClass();
>+}
>diff --git a/widget/src/android/AndroidBridge.h b/widget/src/android/AndroidBridge.h
>--- a/widget/src/android/AndroidBridge.h
>+++ b/widget/src/android/AndroidBridge.h
>@@ -90,6 +90,10 @@ public:
>           return sBridge->AttachThread();
>         return nsnull;
>     }
>+    
>+    static jclass GetGeckoAppShellClass() {
>+        return sBridge->mGeckoAppShellClass;
>+    }
> 
>     // The bridge needs to be constructed via ConstructBridge first,
>     // and then once the Gecko main thread is spun up (Gecko side),
>@@ -230,5 +234,6 @@ protected:
> 
> extern "C" JNIEnv * GetJNIForThread();
> extern PRBool mozilla_AndroidBridge_SetMainThread(void *);
>+extern jclass GetGeckoAppShellClass();
> 
> #endif /* AndroidBridge_h__ */
>diff --git a/widget/src/android/nsAppShell.cpp b/widget/src/android/nsAppShell.cpp
>--- a/widget/src/android/nsAppShell.cpp
>+++ b/widget/src/android/nsAppShell.cpp
>@@ -129,7 +129,6 @@ nsAppShell::ScheduleNativeEventCallback(
>     PostEvent(new AndroidGeckoEvent(AndroidGeckoEvent::NATIVE_POKE));
> }
> 
>-
> PRBool
> nsAppShell::ProcessNextNativeEvent(PRBool mayWait)
> {

Comment 27

[[:fabrice] Fabrice Desré]

(In reply to comment #26)
> Comment on attachment 480743 [details] [diff] [review]
> updated patch
> 
> 
> >diff --git a/toolkit/components/Makefile.in b/toolkit/components/Makefile.in
> >--- a/toolkit/components/Makefile.in
> >+++ b/toolkit/components/Makefile.in
> >@@ -70,6 +70,7 @@ PARALLEL_DIRS += \
> >   typeaheadfind \
> >   urlformatter \
> >   viewconfig \
> >+  webapps \
> >   $(NULL)
> 
> 
> I'd recommend having:
> 
> ifdef ANDROID
> PARALLEL_DIRS += webapps
> endif
> 

I didn't do this because we want the idl -> xpt to be done even for non android platform. For instance, the Maemo implementation is a js xpcom.

Or would a directory layout like this be better : 

toolkit/components/webapps
                      |- Makefile.in
                      |- nsWebappsSupports.idl
                      |- android
                            |- Makefile.in
                            |- webAppsSupport.cpp
                            |- webAppsSupport.h

Comment 28

[[:fabrice] Fabrice Desré]

Attachment: updated patch

> I'd recommend having:
>
> ifdef ANDROID
> PARALLEL_DIRS += webapps
> endif

This patch implements this idea, but uses MOZ_GFX_OPTIMIZE_MOBILE to build the IDL parts for all mobile platforms and only build the .cpp parts if we are targetting Android.

It's green on http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-1a71e6618790

Comment 29

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

(In reply to comment #28)
> Created attachment 482659 [details] [diff] [review]
> updated patch
> 
> > I'd recommend having:
> >
> > ifdef ANDROID
> > PARALLEL_DIRS += webapps
> > endif
> 
> This patch implements this idea, but uses MOZ_GFX_OPTIMIZE_MOBILE to build the
> IDL parts for all mobile platforms and only build the .cpp parts if we are
> targetting Android.

Does this mean it won't build correctly for desktop Linux?

I guess we could set a flag in mozconfig to trigger MOZ_GFX_OPTIMIZE_MOBILE

Comment 30

[[:fabrice] Fabrice Desré]

Attachment: updated patch

More robust filtering, using |ifeq (mobile,$(MOZ_BUILD_APP))| to build the idl part.

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'd prefer that you wrote the logic of when to turn this on and off into configure.in and then just ifdef MOZ_WEBAPPS it.

Comment 32

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

(In reply to comment #31)
> I'd prefer that you wrote the logic of when to turn this on and off into
> configure.in and then just ifdef MOZ_WEBAPPS it.

Don't you think that's overkill? It's not like any random Mozilla app can just enable it and get it to work.

Comment 33

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ah.  I misunderstood then; I thought this was generic enough that anything could use it.

Comment 34

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 482671 [details] [diff] [review]
updated patch

>diff --git a/toolkit/components/webapps/Makefile.in b/toolkit/components/webapps/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/webapps/Makefile.in
>@@ -0,0 +1,72 @@
>+# ***** 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
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# The Original Code is Mozilla Webapp code.
>+#
>+# The Initial Developer of the Original Code is
>+# the Mozilla Foundation.
>+# Portions created by the Initial Developer are Copyright (C) 2010
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#    Fabrice Desré <[e-mail]>
>+#
>+# Alternatively, the contents of this file may be used under the terms of
>+# either the GNU General Public License Version 2 or later (the "GPL"), or
>+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+# in which case the provisions of the GPL or the LGPL are applicable instead
>+# of those above. If you wish to allow use of your version of this file only
>+# under the terms of either the GPL or the LGPL, and not to allow others to
>+# use your version of this file under the terms of the MPL, indicate your
>+# decision by deleting the provisions above and replace them with the notice
>+# and other provisions required by the GPL or the LGPL. If you do not delete
>+# the provisions above, a recipient may use your version of this file under
>+# the terms of any one of the MPL, the GPL or the LGPL.
>+#
>+# ***** END LICENSE BLOCK *****
>+
>+DEPTH		= ../../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
>+VPATH		= @srcdir@
>+
>+include $(DEPTH)/config/autoconf.mk
>+
>+MODULE_NAME     = webapps
>+MODULE          = webapps
>+                  
>+ifeq (Android,$(OS_TARGET))
>+LIBRARY_NAME    = webapps_s
>+LIBXUL_LIBRARY  = 1
>+IS_COMPONENT	  = 1
>+EXPORT_LIBRARY  = 1
>+
>+REQUIRES	= xpcom \
>+		  string \
>+		  $(NULL)

Remove this, REQUIRES is dead and has been for some time.

>+
>+CPPSRCS   = \
>+	nsWebappsSupport.cpp \
>+	$(NULL)
>+
>+EXTRA_DSO_LDOPTS += \
>+  $(MOZ_COMPONENT_LIBS) \
>+	$(XPCOM_GLUE_LDOPTS) \
>+	$(NSPR_LIBS) \
>+	$(NULL)

MOZ_COMPONENT_LIBS should be all you need here

>+endif
>+
>+XPIDLSRCS = nsIWebappsSupport.idl
>+
>+include $(topsrcdir)/config/rules.mk
>+

Please remove all of the tabs from this file and replace them with spaces.  With that, r=me on the build parts.

Comment 35

[(not currently active) Ted Mielczarek]

FWIW, it seems weird to have a file named nsWebappsSupport that only actually works on Android. Also FWIW, I would probably build the interface unconditionally (it's not going to hurt anything if there's no implementation), and just build the implementation on Android.

Comment 36

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

(In reply to comment #35)
> FWIW, it seems weird to have a file named nsWebappsSupport that only actually
> works on Android.

The Maemo (Linux) version is implemented in /mobile

Comment 37

[[:fabrice] Fabrice Desré]

Attachment: addressing comments

Changes in this patch :
- addressing comments from Kyle.
- according to IRC conversation with Brad, reverted to building the idl part on all platforms.

Comment 38

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

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

Comment 39

[Catalin Suciu [:csuciu]]

Verified on Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110919 Firefox/9.0a1 Fennec/9.0a1