webapps OS level integration : Android

VERIFIED FIXED

Status

VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

(Blocks: 1 bug)

Trunk
All
Android

Details

Attachments

(4 attachments, 9 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
Assignee: nobody → fabrice
(Assignee)

Updated

8 years ago
Blocks: 583750
(Assignee)

Comment 1

8 years ago
Created attachment 471879 [details] [diff] [review]
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.
Attachment #471879 - Flags: review?(blassey.bugs)
(Assignee)

Comment 2

8 years ago
Created attachment 471881 [details] [diff] [review]
UI patch V1

Android implementation for the UI handler. It simply makes a jsctypes call to the createShortcut function from the platform patch.
Attachment #471881 - Flags: review?(blassey.bugs)

Comment 3

8 years ago
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!)
Attachment #471879 - Flags: review?(blassey.bugs)
(Assignee)

Comment 4

8 years ago
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 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
Attachment #471881 - Flags: review?(blassey.bugs) → review+

Comment 6

8 years ago
(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.
(Assignee)

Comment 7

8 years ago
Created attachment 474064 [details] [diff] [review]
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.
Attachment #471879 - Attachment is obsolete: true
Attachment #474064 - Flags: feedback?(blassey.bugs)
(Assignee)

Updated

8 years ago
Attachment #474064 - Flags: feedback?(mwu)

Comment 8

8 years ago
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.
Attachment #474064 - Flags: feedback?(ted.mielczarek)
Attachment #474064 - Flags: feedback?(mwu)
Attachment #474064 - Flags: feedback?(khuey)
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?
In particular, you can use the makefile bits of http://hg.mozilla.org/mozilla-central/rev/416b9b6ffc77 as an example of what to do.
(Assignee)

Comment 11

8 years ago
(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 on attachment 474064 [details] [diff] [review]
WIP plaform patch

Clearing flags since I think you got the pointers you wanted.
Attachment #474064 - Flags: feedback?(ted.mielczarek)
Attachment #474064 - Flags: feedback?(khuey)
(Assignee)

Comment 13

8 years ago
Created attachment 475880 [details] [diff] [review]
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
Attachment #471881 - Attachment is obsolete: true
Attachment #474064 - Attachment is obsolete: true
Attachment #475880 - Flags: review?(mwu)
Attachment #475880 - Flags: review?(blassey.bugs)
Attachment #474064 - Flags: feedback?(blassey.bugs)

Comment 14

8 years ago
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é <fabrice@mozilla.com>
>+ *
>+ * 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.
Attachment #475880 - Flags: review?(mwu)
(Assignee)

Comment 15

8 years ago
Created attachment 477888 [details] [diff] [review]
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?
Attachment #475880 - Attachment is obsolete: true
Attachment #477888 - Flags: review?(mwu)
Attachment #477888 - Flags: review?(blassey.bugs)
Attachment #475880 - Flags: review?(blassey.bugs)
(Assignee)

Comment 16

8 years ago
Created attachment 477889 [details]
browser window when opened as a second window
(Assignee)

Comment 17

8 years ago
Created attachment 477901 [details]
logcat when opening new window

Here's the logcat relevant output when we open a new window, leading to the previous screenshot.
(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

8 years ago
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__
Attachment #477888 - Flags: review?(vladimir)
Attachment #477888 - Flags: review?(mwu)
Attachment #477888 - Flags: review?(blassey.bugs)
Attachment #477888 - Flags: review+
Whiteboard: [fennec-checkin-postb1]
(Assignee)

Comment 20

8 years ago
Created attachment 479744 [details] [diff] [review]
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.
Attachment #477888 - Attachment is obsolete: true
Attachment #479744 - Flags: review?
Attachment #477888 - Flags: review?(vladimir)
(Assignee)

Updated

8 years ago
Attachment #479744 - Flags: review? → review?(vladimir)
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.
Attachment #479744 - Flags: review?(vladimir)
Attachment #479744 - Flags: review?(blassey.bugs)
Attachment #479744 - Flags: review+

Updated

8 years ago
Attachment #479744 - Flags: review?(blassey.bugs) → review+
tracking-fennec: --- → ?
Whiteboard: [fennec-checkin-postb1]
(Assignee)

Comment 22

8 years ago
Created attachment 480598 [details] [diff] [review]
better commit string
Attachment #479744 - Attachment is obsolete: true
Attachment #480598 - Flags: review?(blassey.bugs)
Attachment #480598 - Flags: review?(blassey.bugs) → review+
pushed http://hg.mozilla.org/mozilla-central/rev/7e10dcf5f763
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Backed out due to bustage:
http://hg.mozilla.org/mozilla-central/rev/70fe427ed0fc
http://hg.mozilla.org/mozilla-central/rev/60f4509b6e08
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 25

8 years ago
Created attachment 480743 [details] [diff] [review]
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/fdesre@mozilla.com-2835d362cce1
Attachment #480743 - Flags: review?(blassey.bugs)
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é <fabrice@mozilla.com>
>+#
>+# 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é <fabrice@mozilla.com>
>+ *
>+ * 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é <fabrice@mozilla.com>
>+ *
>+ * 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é <fabrice@mozilla.com>
>+ *
>+ * 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)
> {
tracking-fennec: ? → 2.0+
(Assignee)

Comment 27

8 years ago
(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
(Assignee)

Comment 28

8 years ago
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.

It's green on http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/fdesre@mozilla.com-1a71e6618790
Attachment #480743 - Attachment is obsolete: true
Attachment #482659 - Flags: review?(blassey.bugs)
Attachment #480743 - Flags: review?(blassey.bugs)
(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
(Assignee)

Comment 30

8 years ago
Created attachment 482671 [details] [diff] [review]
updated patch

More robust filtering, using |ifeq (mobile,$(MOZ_BUILD_APP))| to build the idl part.
Attachment #482659 - Attachment is obsolete: true
Attachment #482671 - Flags: review?(blassey.bugs)
Attachment #482659 - Flags: review?(blassey.bugs)
(Assignee)

Updated

8 years ago
Attachment #482671 - Flags: review?(khuey)
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.
(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.
Ah.  I misunderstood then; I thought this was generic enough that anything could use it.
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é <fabrice@mozilla.com>
>+#
>+# 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.
Attachment #482671 - Flags: review?(khuey) → review+
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.
(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
Status: REOPENED → ASSIGNED
(Assignee)

Comment 37

8 years ago
Created attachment 482985 [details] [diff] [review]
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.
Attachment #482671 - Attachment is obsolete: true
Attachment #482985 - Flags: review?(blassey.bugs)
Attachment #482671 - Flags: review?(blassey.bugs)
Attachment #482985 - Flags: review?(blassey.bugs) → review+
pushed http://hg.mozilla.org/mozilla-central/rev/d30d0303cd0f
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Verified on Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110919 Firefox/9.0a1 Fennec/9.0a1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.