697309 - Add support for the Open Web Apps API

Status: VERIFIED FIXED

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

Description

[[:fabrice] Fabrice Desré]

Attachment: wip

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 1

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

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.

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: patch

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 3

[[:fabrice] Fabrice Desré]

Comment on attachment 569844 [details] [diff] [review]
patch

Waiting on the owa API cleanup before trying to land.

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: part 1 : UI glue

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.

Comment 5

[[:fabrice] Fabrice Desré]

Attachment: part 2 : about:apps

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

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: dashboard screenshot

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: part 1 : UI glue

Updated to use the async version of getManifestFor()

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: part 2 : about:apps

updated to register the about:apps component correctly

Comment 9

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

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é <[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 ***** */

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 10

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

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 11

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

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.

Comment 12

[[:fabrice] Fabrice Desré]

(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).

Comment 13

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

Attachment: part 1: UI glue v2

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

Comment 14

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

Attachment: part 2: about:apps v2

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

Comment 15

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

Attachment: Your Apps (empty)

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

Comment 16

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

Attachment: Installing an App

Install dialog

Comment 17

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

Attachment: Your Apps (with an app)

Comment 18

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

Attachment: Your Apps (context menu)

Long tap on an app

Comment 19

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

Attachment: App on Home Screen

Comment 20

[[:fabrice] Fabrice Desré]

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 21

[[:fabrice] Fabrice Desré]

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

Comment 22

[Jason Smith [:jsmith]]

(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 23

[Matt Brubeck (:mbrubeck)]

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.

Comment 24

[[:fabrice] Fabrice Desré]

(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.

Comment 25

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

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

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 27

[:Margaret Leibovic]

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

Comment 28

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

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

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 30

[Phil Ringnalda (:philor)]

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.

Comment 31

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

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

Comment 32

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

Attachment: part 1: UI glue v3

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.

Comment 33

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

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

Comment 34

[Ed Morley [:emorley]]

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

Comment 35

[Aaron Train [:aaronmt]]

Verified Fixed

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