We need some tweaks to the e10s implementation. In this wip I use a simple confirmation prompt for the install phase. We probably need to remote this to a Java dialog.

Comment on attachment 569538 [details] [diff] [review] wip Not bad. I have been thinking that we could just use a simple confirmation (maybe with a checkbox for "add to home screen") instead of the full permissions dialog. Maybe even a doorhanger? The app can just set permissions up like a normal web site, as the user needs them. This might help us remove some more code from WebappsUI.js and make it even easier for users - less to think about.

The install dialog is a confirm prompt with a checkbox to add apps to the home screen. Lots of code removed, which feel good ;)

Comment on attachment 569844 [details] [diff] [review] patch Waiting on the owa API cleanup before trying to land.

updated patch working on current m-c. I'm still using a simple prompt dialog when installing. We'll need to move to a better java solution when we'll have a clear idea of which UX we want for permissions.

Implements an about:apps dashboard. Tapping on an app launches it. A long tap fires up a context menu to delete the app. There is a test page at : http://people.mozilla.com/~fdesre/openwebapps/test.html

Updated to use the async version of getManifestFor()

updated to register the about:apps component correctly

Comment on attachment 581379 [details] [diff] [review] part 1 : UI glue Bug 697006 contains a similar patch for Desktop Firefox. Some of my comments will be based on what landed in that bug. >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js >+ WebappsUI.init(); >+ WebappsUI.uninit(); >+ <script type="application/javascript" src="chrome://browser/content/webapps.js"/> We use a module and lazyload code in the desktop patch. It might be better for mobile to consider using those techniques, but we can do that later. When we delay load the scripts, we don't really need to use modules anyway. XUL Fennec uses JS scripts. >diff --git a/mobile/android/chrome/content/webapps.js b/mobile/android/chrome/content/webapps.js >+/* ***** 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 Webapps. >+ * >+ * The Initial Developer of the Original Code is >+ * Ben Goodger. >+ * Portions created by the Initial Developer are Copyright (C) 2011 >+ * 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 ***** */ Switch to the new MPL 2 boilerplate. >+ doInstall: function(aData) { >+ let manifest = new DOMApplicationManifest(aData.app.manifest, aData.app.origin); >+ let checkbox = { value: false }; >+ if (Services.prompt.confirm(null, Strings.browser.GetStringFromName("webapps.installTitle"), >+ manifest.name ? manifest.name : manifest.fullLaunchPath())) >+ DOMApplicationRegistry.confirmInstall(aData); >+ }, We have access to "doorhangers" now, so let's use the same approach desktop does in patch: https://bug697006.bugzilla.mozilla.org/attachment.cgi?id=605234 >+ openURL: function(aURI, aOrigin) { >+ let tabs = BrowserApp.tabs; >+ let tab = null; >+ for (let i = 0; i < tabs.length; i++) { >+ if (tabs[i].appOrigin == aOrigin) >+ tab = tabs[i]; >+ } >+ if (tab) { >+ BrowserApp.selectTab(tab); >+ } >+ else { >+ tab = BrowserApp.addTab(aURI); >+ tab.appOrigin = aOrigin; >+ } >+ } Let's use the SessionStore.get/setTabData approach used in the desktop patch to hold the "appOrigin". See: https://bug697006.bugzilla.mozilla.org/attachment.cgi?id=605234 >diff --git a/mobile/android/locales/en-US/chrome/browser.properties b/mobile/android/locales/en-US/chrome/browser.properties >+#Webapps >+webapps.installTitle=Install this application? Let's use the same string used in the desktop door hanger: #LOCALIZATION NOTE (webapps.requestInstall) %1$S is the application name, %2$S is the site from which the web app is installed webapps.requestInstall = Do you want to install "%1$S" from this site (%2$S)? Overall this patch is fine, but we just need to unbitrot it and tweak it to be more like the version used in desktop. I (or someone) can post a new version of the patch for another review.

Comment on attachment 581379 [details] [diff] [review] part 1 : UI glue Actually, I'd be interested to know if the modal dialog approach might be better than the modeless+timeout method used in the desktop patch.

Comment on attachment 581380 [details] [diff] [review] part 2 : about:apps >diff --git a/mobile/android/chrome/content/aboutApps.js b/mobile/android/chrome/content/aboutApps.js >+/* ***** BEGIN LICENSE BLOCK ***** >+ * ***** END LICENSE BLOCK ***** */ Use the new minimal boilerplate >+ container.setAttribute("draggable", "true"); What's this attr for? >diff --git a/mobile/android/chrome/content/aboutApps.xhtml b/mobile/android/chrome/content/aboutApps.xhtml >+# ***** BEGIN LICENSE BLOCK ***** >+# ***** END LICENSE BLOCK ***** Use the new minimal boilerplate >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js >+ this.add(Strings.browser.GetStringFromName("contextmenu.deleteApp"), >+ this.SelectorContext("div[mozApp]"), >+ function(aTarget) { >+ Cu.import("resource://gre/modules/Webapps.jsm"); >+ DOMApplicationRegistry.uninstall({ origin: aTarget.getAttribute("mozApp") }) >+ }); This should be added in the aboutApps.js file Also, I wonder if we could add the "Add to Home Screen" context menu? r- for a new patch, but this is pretty good.

(In reply to Mark Finkle (:mfinkle) from comment #10) > Comment on attachment 581379 [details] [diff] [review] > part 1 : UI glue > > Actually, I'd be interested to know if the modal dialog approach might be > better than the modeless+timeout method used in the desktop patch. My initial implementation for desktop was using a tab-modal dialog (this was based on Faaborg's mockups). This has then been rejected by fx-team in favor of the doorhanger but I still think that a modal dialog is the way to go (especially when we'll add proper permission hints in the manifest).

This patch addresses my comments, but keeps the modal dialog prompt for installing apps. The patch: * Uses the SessionStore to store the appOrigin, like the desktop patch does. * Removes webapps.dtd since it is unused in native fennec * Removes unused openwebapps files from package-manifest.in Asking Fabrice to check over the webapps parts

This patch addresses the comments for the original patch. It also: * Uses the latest mozApps API * Adds "Add to Home Screen" menu * Moves the context menus into aboutApps.js * Themes the background of the page to match the other Fennec about pages

Empty page. The URL needs to be changed to whatever we plan on using.

Install dialog

Long tap on an app

Comment on attachment 608603 [details] [diff] [review] part 1: UI glue v2 Review of attachment 608603 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/webapps.js @@ +37,5 @@ > + }, > + > + doInstall: function(aData) { > + let manifest = new DOMApplicationManifest(aData.app.manifest, aData.app.origin); > + let checkbox = { value: false }; unused here. I guess you want to use it for the "Add to homescreen" option?

Comment on attachment 608604 [details] [diff] [review] part 2: about:apps v2 Review of attachment 608604 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/aboutApps.xhtml @@ +33,5 @@ > + <body dir="&locale.dir;" onload="onLoad(event)" onunload="onUnload(event)"> > + <div id="main-container"> > + <div>&aboutApps.title;</div> > + <div id="noapps" class="hidden"> > + &aboutApps.noApps.pre;<a href="https://apps-preview.mozilla.org/">&aboutApps.noApps.middle;</a>&aboutApps.noApps.post; it looks like apps-preview.mozilla.og is gone. Using a pref will give us more flexibility. ::: mobile/android/locales/en-US/chrome/aboutApps.dtd @@ +1,4 @@ > +<!ENTITY aboutApps.title "Your Apps"> > +<!ENTITY aboutApps.noApps.pre "No web apps installed. Get some from the "> > +<!ENTITY aboutApps.noApps.middle "app store"> > +<!ENTITY aboutApps.noApps.post "."> we should add a localization note to explain how we build the UI string

(In reply to Fabrice Desré [:fabrice] from comment #21) > Comment on attachment 608604 [details] [diff] [review] > part 2: about:apps v2 > > Review of attachment 608604 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/chrome/content/aboutApps.xhtml > @@ +33,5 @@ > > + <body dir="&locale.dir;" onload="onLoad(event)" onunload="onUnload(event)"> > > + <div id="main-container"> > > + <div>&aboutApps.title;</div> > > + <div id="noapps" class="hidden"> > > + &aboutApps.noApps.pre;<a href="https://apps-preview.mozilla.org/">&aboutApps.noApps.middle;</a>&aboutApps.noApps.post; > > it looks like apps-preview.mozilla.og is gone. Using a pref will give us > more flexibility. FYI - The link for the permanent marketplace to point to is https://marketplace.mozilla.org.

Comment on attachment 608604 [details] [diff] [review] part 2: about:apps v2 Looks good! Just a couple of nits: >+++ b/mobile/android/chrome/content/aboutApps.js >@@ -0,0 +1,154 @@ >+/* ***** BEGIN LICENSE BLOCK ***** >+ * >+ * This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file, >+ * You can obtain one at http://mozilla.org/MPL/2.0/. >+ * >+ * ***** END LICENSE BLOCK ***** */ >+ >+let Ci = Components.interfaces, Cc = Components.classes, Cu = Components.utils; I'd like to start adding "use strict"; to the start of all new JS files. >+ &aboutApps.noApps.pre;<a href="https://apps-preview.mozilla.org/">&aboutApps.noApps.middle;</a>&aboutApps.noApps.post; >+<!ENTITY aboutApps.title "Your Apps"> >+<!ENTITY aboutApps.noApps.pre "No web apps installed. Get some from the "> >+<!ENTITY aboutApps.noApps.middle "app store"> >+<!ENTITY aboutApps.noApps.post "."> This probably deserves an explanatory comment for l10n. I'm also curious if the the l10n community has any feedback about whether splitting the string in this way is workable and/or whether there's a preferred way.

(In reply to Matt Brubeck (:mbrubeck) from comment #23) > > This probably deserves an explanatory comment for l10n. I'm also curious if > the the l10n community has any feedback about whether splitting the string > in this way is workable and/or whether there's a preferred way. I've been asked by the reviewers to do that for the desktop patch, since it is the common usage in the l10n community.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f20ab3487db https://hg.mozilla.org/integration/mozilla-inbound/rev/add087add91a

These patches were backed out due to test failures on patches beneath these patches that had conflicts with these patches. Please rebase and reland. https://hg.mozilla.org/integration/mozilla-inbound/rev/f8e563013a61 https://hg.mozilla.org/integration/mozilla-inbound/rev/2989466dba9e

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccb20a1169f1 https://hg.mozilla.org/integration/mozilla-inbound/rev/47afa45afdfb

Note to QA: I have been using this web site to "install" webapps: https://apps.mozillalabs.com/appdir/

Mark: Did you unset TM and change priority on purpose?

Backed out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/254382ca03c1 - either this or bug 736278 was busting native talos, hard though it was to see through all the other bustage and the normal bustage. Retriggered on https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=36586538ef3b in between the backout and the relanding, got nothing but green, retriggered on the relanding in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=47afa45afdfb and it was back to red.

backed out again: https://hg.mozilla.org/integration/mozilla-inbound/rev/17f586740c3b

In order to get this to pass the talos tests on Try, I had to move the webapps.js code into browser.js I have no idea why this was needed, but I don't want to wait to land this until I find out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c8f5618b789 https://hg.mozilla.org/integration/mozilla-inbound/rev/7f483ad72e7c

https://hg.mozilla.org/mozilla-central/rev/7c8f5618b789 https://hg.mozilla.org/mozilla-central/rev/7f483ad72e7c

Verified Fixed Mozilla/5.0 (Android; Mobile; rv:14.0) Gecko/14.0 Firefox/14.0a1