Closed Bug 697309 Opened 13 years ago Closed 12 years ago

Add support for the Open Web Apps API

Categories

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

All
Android
defect

Tracking

(firefox14 verified)

VERIFIED FIXED
Firefox 14
Tracking Status
firefox14 --- verified

People

(Reporter: fabrice, Assigned: mfinkle)

References

Details

(Whiteboard: [webapp])

Attachments

(7 files, 8 obsolete files)

15.54 KB, patch
fabrice
: review+
mbrubeck
: review+
Details | Diff | Splinter Review
127.25 KB, image/png
Details
75.64 KB, image/png
Details
133.85 KB, image/png
Details
104.30 KB, image/png
Details
355.97 KB, image/png
Details
10.40 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
Attached patch wip (obsolete) — Splinter Review
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.
Attachment #569538 - Flags: feedback?(mark.finkle)
Assignee: nobody → fabrice
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.
Attachment #569538 - Flags: feedback?(mark.finkle) → feedback+
Attached patch patch (obsolete) — Splinter Review
The install dialog is a confirm prompt with a checkbox to add apps to the home screen. Lots of code removed, which feel good ;)
Attachment #569538 - Attachment is obsolete: true
Attachment #569844 - Flags: review?(mark.finkle)
Priority: -- → P4
Comment on attachment 569844 [details] [diff] [review]
patch

Waiting on the owa API cleanup before trying to land.
Attachment #569844 - Flags: review?(mark.finkle)
Attached patch part 1 : UI glue (obsolete) — Splinter Review
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.
Attachment #569844 - Attachment is obsolete: true
Attachment #578690 - Flags: review?(mark.finkle)
Attached patch part 2 : about:apps (obsolete) — Splinter Review
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
Attachment #578691 - Flags: review?(mark.finkle)
Attached image dashboard screenshot (obsolete) —
Attached patch part 1 : UI glue (obsolete) — Splinter Review
Updated to use the async version of getManifestFor()
Attachment #578690 - Attachment is obsolete: true
Attachment #578690 - Flags: review?(mark.finkle)
Attachment #581379 - Flags: review?(mark.finkle)
Attached patch part 2 : about:apps (obsolete) — Splinter Review
updated to register the about:apps component correctly
Attachment #578691 - Attachment is obsolete: true
Attachment #578691 - Flags: review?(mark.finkle)
Attachment #581380 - Flags: review?(mark.finkle)
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.
Attachment #581379 - Flags: review?(mark.finkle) → 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.
Attachment #581380 - Flags: review?(mark.finkle) → review-
(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).
Whiteboard: [webapp]
Depends on: 738526
Depends on: 738527
Attached patch part 1: UI glue v2 (obsolete) — Splinter Review
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
Assignee: fabrice → mark.finkle
Attachment #581379 - Attachment is obsolete: true
Attachment #608603 - Flags: review?(fabrice)
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
Attachment #581380 - Attachment is obsolete: true
Attachment #608604 - Flags: review?(fabrice)
Attachment #608604 - Flags: review?(mbrubeck)
Priority: P4 → P2
Attached image Your Apps (empty)
Empty page. The URL needs to be changed to whatever we plan on using.
Attachment #578694 - Attachment is obsolete: true
Attached image Installing an App
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?
Attachment #608603 - Flags: review?(fabrice) → review+
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
Attachment #608604 - Flags: review?(fabrice) → review+
(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.
Attachment #608604 - Flags: review?(mbrubeck) → review+
(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.
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
Note to QA: I have been using this web site to "install" webapps:
https://apps.mozillalabs.com/appdir/
Priority: P2 → P4
Target Milestone: Firefox 14 → ---
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.
Status: NEW → ASSIGNED
Priority: P4 → P2
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.
Attachment #608603 - Attachment is obsolete: true
Attachment #610703 - Flags: review?(mbrubeck)
Attachment #610703 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/mozilla-central/rev/7c8f5618b789
https://hg.mozilla.org/mozilla-central/rev/7f483ad72e7c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Verified Fixed

Mozilla/5.0 (Android; Mobile; rv:14.0) Gecko/14.0 Firefox/14.0a1
Status: RESOLVED → VERIFIED
Flags: in-litmus?(aaron.train)
Blocks: 660802
Flags: in-litmus?(aaron.train)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: