Last Comment Bug 584767 - webapps frontend
: webapps frontend
Status: VERIFIED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: x86 All
: -- enhancement (vote)
: Firefox 8
Assigned To: Mark Finkle (:mfinkle) (use needinfo?)
:
Mentors:
Depends on: 593782 670677 671258 672391 673906 678517
Blocks: 583750
  Show dependency treegraph
 
Reported: 2010-08-05 10:33 PDT by [:fabrice] Fabrice Desré
Modified: 2012-02-19 07:07 PST (History)
26 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP (8.86 KB, patch)
2010-08-05 10:36 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
Site menu entry (98.33 KB, image/png)
2010-08-05 10:40 PDT, [:fabrice] Fabrice Desré
no flags Details
awesomescreen (50.05 KB, image/png)
2010-08-05 10:42 PDT, [:fabrice] Fabrice Desré
no flags Details
webapps list (16.44 KB, image/png)
2010-08-05 10:43 PDT, [:fabrice] Fabrice Desré
no flags Details
WIP 2 (15.19 KB, patch)
2010-08-06 10:00 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
webapps screen (18.01 KB, image/png)
2010-08-06 10:01 PDT, [:fabrice] Fabrice Desré
no flags Details
WIP 3 (13.93 KB, patch)
2010-08-09 08:00 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
awesomescreen (57.14 KB, image/png)
2010-08-09 08:01 PDT, [:fabrice] Fabrice Desré
no flags Details
webapps list (38.14 KB, image/png)
2010-08-09 08:03 PDT, [:fabrice] Fabrice Desré
no flags Details
WIP 4 (15.99 KB, patch)
2010-08-12 08:55 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
WIP 5 (17.01 KB, patch)
2010-08-19 16:35 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
TAKE 2 - WIP 1 (12.52 KB, patch)
2010-08-21 19:22 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
updated patch on current m-b (12.48 KB, patch)
2010-08-24 07:51 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
TAKE 2 - WIP 3 (20.09 KB, patch)
2010-08-25 07:20 PDT, [:fabrice] Fabrice Desré
mark.finkle: feedback+
Details | Diff | Review
composite screenshot (679.34 KB, image/png)
2010-08-25 07:21 PDT, [:fabrice] Fabrice Desré
no flags Details
TAKE 2 - comments addressed (20.16 KB, patch)
2010-09-03 09:27 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
updated for new command line handling (21.22 KB, patch)
2010-09-06 07:07 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
updated patch (23.27 KB, patch)
2010-09-10 01:19 PDT, [:fabrice] Fabrice Desré
mark.finkle: review-
Details | Diff | Review
addressing comments (24.35 KB, patch)
2010-09-15 10:24 PDT, [:fabrice] Fabrice Desré
mark.finkle: review+
Details | Diff | Review
updated patch (23.00 KB, patch)
2010-09-20 04:09 PDT, [:fabrice] Fabrice Desré
mark.finkle: review-
Details | Diff | Review
updated patch (22.96 KB, patch)
2010-09-21 09:56 PDT, [:fabrice] Fabrice Desré
mark.finkle: review+
Details | Diff | Review
updated patch on current m-b (19.70 KB, patch)
2010-11-04 11:22 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
updated patch (24.15 KB, patch)
2011-06-10 17:02 PDT, [:fabrice] Fabrice Desré
mark.finkle: review-
Details | Diff | Review
patch 2 (31.16 KB, patch)
2011-06-17 22:23 PDT, Mark Finkle (:mfinkle) (use needinfo?)
wjohnston2000: review+
Details | Diff | Review
patch 3 (17.28 KB, patch)
2011-06-21 14:55 PDT, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details | Diff | Review
patch 3 (real) (32.06 KB, patch)
2011-06-21 20:13 PDT, Mark Finkle (:mfinkle) (use needinfo?)
wjohnston2000: review+
Details | Diff | Review
patch with openURI changes (34.90 KB, patch)
2011-07-05 22:17 PDT, Mark Finkle (:mfinkle) (use needinfo?)
fabrice: review+
Details | Diff | Review
patch without IDL changes (34.96 KB, patch)
2011-07-07 09:01 PDT, Mark Finkle (:mfinkle) (use needinfo?)
fabrice: review+
Details | Diff | Review
screenshot with permission requested (89.52 KB, image/png)
2011-07-18 03:59 PDT, [:fabrice] Fabrice Desré
no flags Details
patch for permission string (3.11 KB, patch)
2011-08-05 13:38 PDT, Mark Finkle (:mfinkle) (use needinfo?)
fabrice: review+
Details | Diff | Review

Description [:fabrice] Fabrice Desré 2010-08-05 10:33:25 PDT
Implement the frontend pieces to expose webapps in Fennec.
Comment 1 [:fabrice] Fabrice Desré 2010-08-05 10:36:34 PDT
Created attachment 463218 [details] [diff] [review]
WIP

This patch implements the following features:
- new "Set Page as Web App" entry in the site menu
- "See all Web Apps" entry in the awesomscreen
- A list-oriented display of the web apps.
Comment 2 [:fabrice] Fabrice Desré 2010-08-05 10:40:10 PDT
Created attachment 463221 [details]
Site menu entry
Comment 3 [:fabrice] Fabrice Desré 2010-08-05 10:42:31 PDT
Created attachment 463222 [details]
awesomescreen
Comment 4 [:fabrice] Fabrice Desré 2010-08-05 10:43:34 PDT
Created attachment 463224 [details]
webapps list
Comment 5 [:fabrice] Fabrice Desré 2010-08-06 10:00:54 PDT
Created attachment 463579 [details] [diff] [review]
WIP 2

This patch moves from a list view to a grid with icons and labels.
Comment 6 [:fabrice] Fabrice Desré 2010-08-06 10:01:48 PDT
Created attachment 463580 [details]
webapps screen
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2010-08-06 18:33:56 PDT
Fabrice - Note that the awesomescreen is changing, very soon:
https://wiki.mozilla.org/Mobile/Projects/AwesomeScreen2.0
Comment 8 [:fabrice] Fabrice Desré 2010-08-09 08:00:47 PDT
Created attachment 464053 [details] [diff] [review]
WIP 3

This patch must be applied on top of bug 436069
A new categoryis added to the awesomescreen, that lead to the icon view.
The context menu allows the user to edit the title of the app and to remove it.
Comment 9 [:fabrice] Fabrice Desré 2010-08-09 08:01:58 PDT
Created attachment 464055 [details]
awesomescreen
Comment 10 [:fabrice] Fabrice Desré 2010-08-09 08:03:27 PDT
Created attachment 464056 [details]
webapps list
Comment 11 [:fabrice] Fabrice Desré 2010-08-12 08:55:48 PDT
Created attachment 465267 [details] [diff] [review]
WIP 4

Updated patch that will open a web app in a new window.
Comment 12 [:fabrice] Fabrice Desré 2010-08-19 16:35:18 PDT
Created attachment 467595 [details] [diff] [review]
WIP 5

Diff from previous version :
- icons are scaled to the same size as the mask
- better favicon  detection
Comment 13 [:fabrice] Fabrice Desré 2010-08-21 19:22:22 PDT
Created attachment 468096 [details] [diff] [review]
TAKE 2 - WIP 1

Implements the needed parts to :
- add an "Install web app" entry in the site menu when not in a web app
- handle a --webapp switch on the command line
- minimize the chrome when in web app mode. The icon is still clickable to show the site menu.
Comment 14 [:fabrice] Fabrice Desré 2010-08-24 07:51:23 PDT
Created attachment 468676 [details] [diff] [review]
updated patch on current m-b
Comment 15 [:fabrice] Fabrice Desré 2010-08-25 07:20:09 PDT
Created attachment 469046 [details] [diff] [review]
TAKE 2 - WIP 3

Adds a configuration dialog before installing the application, that allows to change the application title and set the permissions for offline mode and geolocation.
Comment 16 [:fabrice] Fabrice Desré 2010-08-25 07:21:20 PDT
Created attachment 469047 [details]
composite screenshot
Comment 17 Mark Finkle (:mfinkle) (use needinfo?) 2010-09-03 07:16:03 PDT
Comment on attachment 469046 [details] [diff] [review]
TAKE 2 - WIP 3

>     this._setURI(caption);
>+    if (document.getElementById("main-window").getAttribute("windowtype") == "navigator:webapp")
>+      window.document.title = caption;

Add a blank line before the new code

>+  // Switching to Web App mode, by:
>+  // - changing the window type, to let all the use of navigator:browser go to
>+  //   the main window
>+  // - hide some UI elements (reload button, tab & tools sidebars)
>+  // - deactivate some UI elements (favicon display, title bar)
>+  // - deactivate some commands
>+  // - remove "Open in new tab" from the context menu
>+  enterAppMode: function() {

I don't like the name, but we'll think of something :)

>       case "DOMLinkAdded":
>+        // checks for an icon to use for a web app
>+        // priority is : icon < apple-touch-icon
>+        let rel = json.rel.toLowerCase().split(" ");
>+        if (rel.indexOf("icon") != -1) {
>+        // We should also use the sizes attribute if available
>+        // see http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#rel-icon
>+          if (!browser.appIcon)
>+            browser.appIcon = json.href;
>+        }
>+        else if (rel.indexOf("apple-touch-icon") != -1) {
>+        // XXX should we support apple-touch-icon-precomposed ?
>+        // see http://developer.apple.com/safari/library/documentation/appleapplications/reference/safariwebcontent/configuringwebapplications/configuringwebapplications.html
>+          browser.appIcon = json.href;
>+        }

Maybe we could move this to the browser binding? Just a thought. I don't know if we want webapp stuff moving into the toolkit code yet. Maybe in the future

>         if (Browser.selectedBrowser == browser)
>           this._updateIcon(Browser.selectedBrowser.mIconURL);
>         break;
>@@ -936,7 +991,8 @@
>         this.activePanel = RemoteTabsList;
>         break;
>       case "cmd_quit":
>-        goQuitApplication();
>+        // only close one window
>+        this._closeOrQuit();

We do this because closing the "app" would actually close all apps, and Fennec itself - if running?

>     this.register("pageaction-saveas", this.updatePageSaveAs, this);
>     this.register("pageaction-share", this.updateShare, this);
>     this.register("pageaction-search", BrowserSearch.updatePageSearchEngines, BrowserSearch);
>+    this.register("pageaction-webapps-install", this.updateWebappsInstall, this);
>+    this.register("pageaction-webapps-config", this.updateWebappsConfig, this);

Might be nice to have a WebApp JS helper object instead of adding this to PageActions. Maybe even the WebappsUI object?

>+  updateWebappsInstall: function updateWebappsInstall(node) {

>+    node.onclick = function(event) {
>+      BrowserUI._hidePopup();

Do we still need to call this? I was hoping we wouldn't

>+  updateWebappsConfig: function updateWebappsConfig(node) {

>+    node.onclick = function(event) {
>+      BrowserUI._hidePopup();

Same

>+var WebappsUI = {

>+  show: function show(aURI, aTitle, aIcon) {

>+    if (aTitle)
>+      document.getElementById("webapps-title").value = aTitle;
>+    else {
>+      document.getElementById("webapps-title-box").hidden = true;
>+      document.getElementById("webapps-perm-box").className = "prompt-header";
>+    }

Use {} around the "if" when the "else" needs them

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>+          // check for "app" flag

"webapp"

>   startLoading: function startLoading() {
>     if (this._loading) throw "Already Loading!";
>-
>+    this._browser.appIcon = null;
>     this._loading = true;

Move this to onLocationChange? I have a feeling this method is going away. (Oh and don't kill nice blank lines)

>diff --git a/components/BrowserCLH.js b/components/BrowserCLH.js

>+    // web app support
>+    let app = aCmdLine.handleFlagWithParam("webapp", false);
>+    if (app) {
>+      let uri = resolveURIInternal(aCmdLine, app);
>+      if (uri) {
>+        win.browserDOMWindow.openURI(uri, null, Ci.nsIBrowserDOMWindow.OPEN_NEWWINDOW, null);
>+        return;
Refresh my memory. Why do we still need some commandline code in browser.js too?

Looks good. I don't know what we want to do for the webapp sitemenu. Right now you keep it mostly the same, adding the "Configure" action. Not sure if we want that or not. We can talk to madhava.
Comment 18 [:fabrice] Fabrice Desré 2010-09-03 09:27:39 PDT
Created attachment 471876 [details] [diff] [review]
TAKE 2 - comments addressed

> >@@ -936,7 +991,8 @@
> >         this.activePanel = RemoteTabsList;
> >         break;
> >       case "cmd_quit":
> >-        goQuitApplication();
> >+        // only close one window
> >+        this._closeOrQuit();
> 
> We do this because closing the "app" would actually close all apps, and Fennec
> itself - if running?

Exactly.
 
> >     this.register("pageaction-saveas", this.updatePageSaveAs, this);
> >     this.register("pageaction-share", this.updateShare, this);
> >     this.register("pageaction-search", BrowserSearch.updatePageSearchEngines, BrowserSearch);
> >+    this.register("pageaction-webapps-install", this.updateWebappsInstall, this);
> >+    this.register("pageaction-webapps-config", this.updateWebappsConfig, this);
> 
> Might be nice to have a WebApp JS helper object instead of adding this to
> PageActions. Maybe even the WebappsUI object?

I moved them into WebappsUI.
 
> >+  updateWebappsInstall: function updateWebappsInstall(node) {
> 
> >+    node.onclick = function(event) {
> >+      BrowserUI._hidePopup();
> 
> Do we still need to call this? I was hoping we wouldn't

We don't! hooray!
 
> >+    // web app support
> >+    let app = aCmdLine.handleFlagWithParam("webapp", false);
> >+    if (app) {
> >+      let uri = resolveURIInternal(aCmdLine, app);
> >+      if (uri) {
> >+        win.browserDOMWindow.openURI(uri, null, Ci.nsIBrowserDOMWindow.OPEN_NEWWINDOW, null);
> >+        return;
> Refresh my memory. Why do we still need some commandline code in browser.js
> too?

We need to support several launching patterns : 
1) launch fennec first, then a web app. When we launch the app, the CLH finds that we already have a window, and so calls nsIBrowserDOMWindow.OPEN_NEWWINDOW() for the app and get the URI as a string.

2) launch a web app first. Since we don't have a "navigator:browser" window, the CLH bails out early and transmits the command line arguments to browser.js.

All this must also support opening a new tab when calling an already running fennec  with just a url and also the 'url' param that is processed in browser.js

That said, we may be able to handle all CLH handling by refactoring both browser.js and the CLH component. I'd prefer to do it in a separate bug however.
Comment 19 [:fabrice] Fabrice Desré 2010-09-06 07:07:22 PDT
Created attachment 472392 [details] [diff] [review]
updated for new command line handling

Building on top of bug 593782, doing all the command line handling in BrowserCLH.js

It also ensure that we only run one instance of a given web app, and focuses the window as needed.
Comment 20 Mark Finkle (:mfinkle) (use needinfo?) 2010-09-07 08:53:06 PDT
Comment on attachment 472392 [details] [diff] [review]
updated for new command line handling


>       case "DOMLinkAdded":
>+        // checks for an icon to use for a web app
>+        // priority is : icon < apple-touch-icon
>+        let rel = json.rel.toLowerCase().split(" ");
>+        if (rel.indexOf("icon") != -1) {
>+        // We should also use the sizes attribute if available
>+        // see http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#rel-icon
>+          if (!browser.appIcon)
>+            browser.appIcon = json.href;
>+        }
>+        else if (rel.indexOf("apple-touch-icon") != -1) {
>+        // XXX should we support apple-touch-icon-precomposed ?
>+        // see http://developer.apple.com/safari/library/documentation/appleapplications/reference/safariwebcontent/configuringwebapplications/configuringwebapplications.html
>+          browser.appIcon = json.href;
>+        }
>+ 

nit: indent the comments

>         if (Browser.selectedBrowser == browser)
>           this._updateIcon(Browser.selectedBrowser.mIconURL);
>         break;
>@@ -972,7 +1029,8 @@ var BrowserUI = {
>         this.activePanel = RemoteTabsList;
>         break;
>       case "cmd_quit":
>-        goQuitApplication();
>+        // only close one window
>+        this._closeOrQuit();
>         break;
>       case "cmd_close":
>         this._closeOrQuit();
>@@ -1085,6 +1143,8 @@ var PageActions = {
>     this.register("pageaction-saveas", this.updatePageSaveAs, this);
>     this.register("pageaction-share", this.updateShare, this);
>     this.register("pageaction-search", BrowserSearch.updatePageSearchEngines, BrowserSearch);
>+    this.register("pageaction-webapps-install", WebappsUI.updateWebappsInstall, this);
>+    this.register("pageaction-webapps-config", WebappsUI.updateWebappsConfig, this);
>   },
> 
>   /**
>@@ -2492,3 +2552,77 @@ var SharingUI = {
>   ]
> };
> 
>+var WebappsUI = {
>+  _dialog: null,
>+  _uri: null,
>+  _icon: null,
>+
>+  show: function show(aURI, aTitle, aIcon) {
>+    this._uri = aURI;
>+    this._icon = aIcon;
>+    this._dialog = importDialog(window, "chrome://browser/content/webapps.xul", null);
>+
>+    if (aTitle) {
>+      document.getElementById("webapps-title").value = aTitle;
>+    }
>+    else {
>+      document.getElementById("webapps-title-box").hidden = true;
>+      document.getElementById("webapps-perm-box").className = "prompt-header";
>+    }
>+
>+    document.getElementById("webapps-offline-checkbox").checked = Services.perms.testExactPermission(aURI, "offline-app") == Ci.nsIPermissionManager.ALLOW_ACTION;
>+    document.getElementById("webapps-geoloc-checkbox").checked = Services.perms.testExactPermission(aURI, "geo") == Ci.nsIPermissionManager.ALLOW_ACTION;
>+
>+    BrowserUI.pushPopup(this, this._dialog);
>+    this._dialog.waitForClose();
>+  },
>+
>+  hide: function hide() {
>+    this._dialog.close();
>+    this._dialog = null;
>+    BrowserUI.popPopup();
>+  },
>+
>+  _updatePermission: function updatePermission(id, perm) {

aId, aPerm

>+  updateWebappsInstall: function updateWebappsInstall(node) {

aNode

>+  updateWebappsConfig: function updateWebappsConfig(node) {

aNode

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>--- a/chrome/content/browser.js
>+++ b/chrome/content/browser.js
>@@ -521,6 +521,7 @@ var Browser = {
>       else {
>         // This window could have been opened by nsIBrowserDOMWindow.openURI
>         whereURI = window.arguments[0];
>+        BrowserUI.enterAppMode(whereURI);

Can we skip this call? It's in BrowserCLH.js when we explicitly use --webapp
I am worried we could enter webapp mode when we shouldn't

>+let WebappsManager  = {
>+  // creates a resized png version of the icon, and proxy it to _install()
>+  install: function(aURI, aTitle, aIcon) {
>+    const kIconSize = 64;
>+    Cu.import("resource://gre/modules/Services.jsm");
>+    let win = Services.wm.getMostRecentWindow("navigator:browser");
>+    let canvas = win.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
>+    canvas.setAttribute("style", "display: none");
>+    canvas = win.document.getElementById("main-window").appendChild(canvas);

I would love a different way to handle this. Components maybe? Does Prism have code that we could use?

BTW, the latest changes for hiding the favicon button in the URLBar _might_ have broken this patch. I'm not 100% sure, just wanted you to try it.

Almost there!!!!
Comment 21 Madhava Enros [:madhava] 2010-09-08 15:00:30 PDT
first take at some UI review - more to come, but I didn't want to keep you waiting:

http://www.flickr.com/photos/madhava_work/4971782185/
Comment 22 Madhava Enros [:madhava] 2010-09-08 15:01:56 PDT
or, better, in legible form: http://www.flickr.com/photos/madhava_work/4971788093/sizes/o/
Comment 23 Mark Finkle (:mfinkle) (use needinfo?) 2010-09-08 21:14:14 PDT
Fabrice - Can you also update the patch with Madhava's string changes? I assume the Maemo/Meego task switcher and app-close buttons will "just work". The only part I don't know how to handle without some research is hiding the "Install as App" button _if_ the web app is already installed. We would want to track the actual application installed on the device, not just a simple flag that could get out of sync when the user uninstalls the app.

That could be a followup bug IMO
Comment 24 [:fabrice] Fabrice Desré 2010-09-10 01:19:08 PDT
Created attachment 473982 [details] [diff] [review]
updated patch

Patch updated :
- new strings
- CLH adapted to the new component
- Moved from a js module to a component for actual web app installation support : this is needed since on Android this is a c++ component.

I confirm that the Maemo/Meego task switcher and app-close buttons work like usually.
Tracking installation/removal on the device may not be easy depending on the OS, but at least on Maemo we could check if we have a desktop file installed corresponding to the relevant URI.
Comment 25 Mark Finkle (:mfinkle) (use needinfo?) 2010-09-13 20:04:41 PDT
Comment on attachment 473982 [details] [diff] [review]
updated patch

I am suddenly worried about changing "windowtype" to "navigator:webapp". Too many components use "navigator:browser" to find the current window and would break. nsIAlertsService is one example. We should not change the "windowtype" and I think we will still be fine.

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+    
>+    if (document.getElementById("main-window").getAttribute("windowtype") == "navigator:webapp")
>+      window.document.title = caption;

Use

if (document.getElementById("main-window").hasAttribute("webapp"))

>+  enterAppMode: function(uri) {

>+    document.getElementById("main-window").setAttribute("windowtype", "navigator:webapp");

Don't do this anymore
>       case "DOMLinkAdded":
>+        // checks for an icon to use for a web app
>+        // priority is : icon < apple-touch-icon

Does this priority mean "icon is less important than apple-touch-icon" ? If so, I think I agree, but the code seems to say "icon is more important than apple-icon-touch". I think we want to use the larger apple-icon-touch, if available. What do you think?

>+        let rel = json.rel.toLowerCase().split(" ");
>+        if (rel.indexOf("icon") != -1) {
>+          // We should also use the sizes attribute if available
>+          // see http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#rel-icon

Let's file a bug for using "sizes"

>+  updateWebappsInstall: function updateWebappsInstall(aNode) {
>+    if (document.getElementById("main-window").getAttribute("windowtype") == "navigator:webapp")

Use:
if (document.getElementById("main-window").hasAttribute("webapp"))

>+  updateWebappsConfig: function updateWebappsConfig(aNode) {
>+    if (document.getElementById("main-window").getAttribute("windowtype") != "navigator:webapp")

Use:
if (document.getElementById("main-window").hasAttribute("webapp"))

>+  install: function(aURI, aTitle, aIcon) {
>+    const kIconSize = 64;
>+    Cu.import("resource://gre/modules/Services.jsm");

Put the Cu.import above the const line and add a line break after both

>+    let win = Services.wm.getMostRecentWindow("navigator:browser");
>+    let canvas = win.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
>+    canvas.setAttribute("style", "display: none");
>+    canvas = win.document.getElementById("main-window").appendChild(canvas);
>+    let self = this;
>+    let image = new win.Image();

Let's file a bug to somehow do this without needing a temporary canvas

>diff --git a/chrome/content/webapps.xul b/chrome/content/webapps.xul

>+</dialog>
>\ No newline at end of file

add a linebreak


>diff --git a/components/BrowserCLH.js b/components/BrowserCLH.js

>+    // retrieve the webapp param, if any
>+    let app = aCmdLine.handleFlagWithParam("webapp", false);
>+
>+    // only allow a single instance for a web app
>+    if (app) {
>+      let uri = resolveURIInternal(aCmdLine, app).spec;
>+      let apps = Services.wm.getEnumerator("navigator:webapp");

Use "navigator:browser" here. You will get the Fennec window too, but it will not match the "webapp" tests anyway

>     // Open the main browser window, if we don't already have one
>     let win;
>+    let newWin = false;

I don't like this

>     try {
>       win = Services.wm.getMostRecentWindow("navigator:browser");
>       if (!win) {
>@@ -194,6 +213,7 @@ BrowserCLH.prototype = {
>         }
> 

You should be checking for "app" right here and passing the uri.spec

>         win = openWindow(null, "chrome://browser/content/browser.xul", "_blank", "chrome,dialog=no,all", defaultURL);
>+        newWin = true;
>       }
> 
>       win.focus();
>@@ -202,6 +222,28 @@ BrowserCLH.prototype = {
>       aCmdLine.preventDefault = true;
>     } catch (e) { }
> 
>+    // web app support
>+    if (app) {

This check should be above, where we create the new window

>+          win = openWindow(null, "chrome://browser/content/browser.xul", "_blank", "chrome,dialog=no,all", uri.spec);

You could pass in an array of params, like [uri.spec, "webapp"], and browser.js could call BrowserUI.enterAppMode for you

Here is an example of how we can convert an Array to something you can pass to openWindow
http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#261

>+          // we need to make sure that BrowserUI is initialized...
>+          while (!(win.BrowserUI && win.BrowserUI._edit))
>+            Services.tm.currentThread.processNextEvent(true);

Really don't like

>+        }
>+        else {
>+          while (!(win.BrowserUI && win.BrowserUI._edit && win.Browser))
>+            Services.tm.currentThread.processNextEvent(true);
>+          win.Browser.loadURI(uri.spec);

Why pass this twice? once when we open the window and now again?

>+        }
>+        win.BrowserUI.enterAppMode(uri.spec);

Let's try to pass a flag to browser.js and have it call enterAppMode

>diff --git a/components/webapps/Makefile.in b/components/webapps/Makefile.in

>+# 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 Android code.

Not exactly true. "Mozilla Webapp code."

>diff --git a/components/webapps/nsIWebappsSupport.idl b/components/webapps/nsIWebappsSupport.idl

>+ * 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 Android code.

Same

r- mainly for the "navigator:browser" changes and the BrowserCLH.js changes
Comment 26 [:fabrice] Fabrice Desré 2010-09-15 10:24:33 PDT
Created attachment 475553 [details] [diff] [review]
addressing comments

Comments addressed.

about:

> Does this priority mean "icon is less important than apple-touch-icon" ? If so,
> I think I agree, but the code seems to say "icon is more important than
> apple-icon-touch". I think we want to use the larger apple-icon-touch, if
> available. What do you think?

We use the larger one : the code uses "icon" only if |browser.appIcon| is null, so it won't override a previously seen apple-icon-touch, if any.
Comment 27 Mark Finkle (:mfinkle) (use needinfo?) 2010-09-15 11:43:45 PDT
Comment on attachment 475553 [details] [diff] [review]
addressing comments

looks good
Comment 28 [:fabrice] Fabrice Desré 2010-09-20 04:09:12 PDT
Created attachment 476764 [details] [diff] [review]
updated patch

Updated patch implementing :
1. do not go fullscreen on maemo so the normal maemo application bar is used across the top
2. completely hide the fennec URL bar
3. no site menu at all

on maemo, the [x] in the OS app bar only closes the window and not the entire app.

try server builds are available here :
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/fdesre@mozilla.com-2315b3fd58c7
Comment 29 Mark Finkle (:mfinkle) (use needinfo?) 2010-09-21 08:08:34 PDT
Comment on attachment 476764 [details] [diff] [review]
updated patch

* Remove any WebappsUI.updateWebappsConfig related code. Madhava feels that the user can uninstall and reinstall to update the permissions for now. We also have no menu in the webapp anymore.
* Add support for desktop web notifications in the permissions dialog

r- for adding desktop notifications support, but we might as well remove the WebappsUI.updateWebappsConfig code too.
Comment 30 [:fabrice] Fabrice Desré 2010-09-21 09:56:06 PDT
Created attachment 477170 [details] [diff] [review]
updated patch

Changes implementing comment 29
Comment 31 [:fabrice] Fabrice Desré 2010-11-04 11:22:08 PDT
Created attachment 488233 [details] [diff] [review]
updated patch on current m-b

Apart from working on current m-b, some changes compared to previous version :
- instead of carrying lots of parameters to WebappsUI.show(), pack them all in a js object.
- there's a platform bug on gtk that prevent going from fullscreen to maximized mode if there are no width and height specified on the window, so I,m setting them even on maemo.
- fixed a bug with pushPopup() missing parameter.
Comment 32 Mark Finkle (:mfinkle) (use needinfo?) 2010-11-10 10:41:26 PST
Moving to post fennec4
Comment 33 [:fabrice] Fabrice Desré 2011-06-10 17:02:16 PDT
Created attachment 538638 [details] [diff] [review]
updated patch
Comment 34 Matt Brubeck (:mbrubeck) 2011-06-17 14:30:07 PDT
Comment on attachment 538638 [details] [diff] [review]
updated patch

>+++ b/mobile/chrome/content/common-ui.js
>     this.register("pageaction-share", this.updateShare, this);
>     this.register("pageaction-search", BrowserSearch.updatePageSearchEngines, BrowserSearch);
>+    this.register("pageaction-webapps-install", WebappsUI.updateWebappsInstall, this);

Drive-by comment: The last argument should be "WebappsUI" instead of "this".
Comment 35 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-17 14:39:06 PDT
Comment on attachment 538638 [details] [diff] [review]
updated patch

Note: We do not want to make webapps launch chromeless browser windows yet. We want to land the first phase and see how the system works before adding chromeless support. There are other issues we'll have to deal with (and Fabrice has tried to deal with in this patch and others) that we can ignore while we act like "another browser window", not a chromeless app.

>diff --git a/mobile/chrome/content/browser-ui.js b/mobile/chrome/content/browser-ui.js

>+
>+    if (document.getElementById("main-window").hasAttribute("webapp"))
>+      window.document.title = caption;

Not needed yet

>+      WebappsUI.init();

Empty, so lets drop it

>+  // Switching to Web App mode, by:
>+  // - hide some the URL bar
>+  // - deactivate some commands
>+  // - remove "Open in new tab" from the context menu
>+  enterAppMode: function(uri) {
>+    document.getElementById("main-window").setAttribute("webapp", uri);
>+    document.getElementById("toolbar-container").style.display = "none";
>+
>+#ifdef MOZ_PLATFORM_MAEMO
>+    document.getElementById("main-window").setAttribute("sizemode", "maximized");
>+#endif
>+
>+    document.getElementById("tabs-container").style.display = "none";
>+    document.getElementById("browser-controls").parentNode.style.display = "none";
>+
>+    document.getElementById("cmd_forceReload").setAttribute("disabled", "true");
>+    document.getElementById("cmd_reload").setAttribute("disabled", "true");
>+    document.getElementById("cmd_newTab").setAttribute("disabled", "true");
>+    document.getElementById("cmd_back").setAttribute("disabled", "true");
>+    document.getElementById("cmd_forward").setAttribute("disabled", "true");
>+    document.getElementById("cmd_openLocation").setAttribute("disabled", "true");
>+    document.getElementById("cmd_menu").setAttribute("disabled", "true");
>+
>+    let tabMenu = document.getElementById("context-openinnewtab");
>+    tabMenu.parentNode.removeChild(tabMenu);
>+  },

Let's remove all of this for now, but keep the function and the "webapp" attribute. Just kill the UI disabling.

>       case "cmd_quit":
>-        GlobalOverlay.goQuitApplication();
>+        //GlobalOverlay.goQuitApplication();
>+        // only close one window
>+        this._closeOrQuit();

This seems safe. We have a real quit method now as well. We'll need to see how we want the "quit" action to work.

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js
>--- a/mobile/chrome/content/browser.js
>+++ b/mobile/chrome/content/browser.js
>@@ -334,8 +334,16 @@ var Browser = {
>     // page as an argument. commandURL _should_ never be empty, but we protect against it
>     // below. However, we delay trying to get the fallback homepage until we really need it.
>     let commandURL = null;
>-    if (window.arguments && window.arguments[0])
>-      commandURL = window.arguments[0];
>+    let isWebapp = false;
>+    if (window.arguments && window.arguments[0]) {
>+       commandURL = window.arguments[0];
>+      
>+      // check if the second argument is "webapp"
>+      if ((window.arguments.length > 1) && (window.arguments[1] == "webapp")) {
>+        BrowserUI.enterAppMode(commandURL);
>+        isWebapp = true;
>+      }
>+    }

We don't use "isWebapp so lets drop it
Lets drop the enterAppMode for now and just set the "webapp" attribute here

>+#ifdef ANDROID
>+    // workaround Bug 610834. Need to resize any new window
>+    let winEnum = Services.wm.getEnumerator("navigator:browser");
>+    winEnum.getNext();
>+    if (winEnum.hasMoreElements())
>+      resizeHandler({ target: window });
>+#endif

Why can't we just use "window"? Why the getEnumerator?

>diff --git a/mobile/chrome/content/browser.xul b/mobile/chrome/content/browser.xul
>         <pageaction id="pageaction-charset" title="&pageactions.charEncoding;" onclick="CharsetMenu.show();"/>
>+	<pageaction id="pageaction-webapps-install" title="&pageactions.webapps.install;"/>

Indent

>diff --git a/mobile/chrome/content/common-ui.js b/mobile/chrome/content/common-ui.js

>+var WebappsUI = {

>+  init: function() {
>+  },

Drop it

>+  install: function(aURI, aTitle, aIcon) {
>+    Cu.import("resource://gre/modules/Services.jsm");

Should already be loaded

>+    const kIconSize = 64;
>+    
>+    let win = Services.wm.getMostRecentWindow("navigator:browser");
>+    let canvas = win.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");

Since this code is not in a component, you don't need to look for a window, you are in a window

>+        if (WebappsUI._manifest.successCallback)
>+          WebappsUI._manifest.successCallback(WebappsUI._manifest);
>+      }
>+      catch(e) {
>+        if (WebappsUI._manifest.errorCallback) {
>+          WebappsUI._manifest.errorMessage = e.toString();
>+          WebappsUI._manifest.errorCallback(WebappsUI._manifest);

This is the WebappUI object so use "this" or "self" if in an anon function

>diff --git a/mobile/components/BrowserCLH.js b/mobile/components/BrowserCLH.js

>+    // only allow a single instance for a web app, and checks if a classic browser is opened
>+    let apps = Services.wm.getEnumerator("navigator:browser");
>+    while (apps.hasMoreElements()) {
>+      let appWin = apps.getNext().QueryInterface(Ci.nsIDOMWindowInternal);
>+      let root = appWin.document.getElementById("main-window");

appWin.document.documentElement should be better

>diff --git a/mobile/locales/en-US/chrome/browser.dtd b/mobile/locales/en-US/chrome/browser.dtd

>+<!ENTITY pageactions.webapps.install "Install as App">

Might need a different string here

>diff --git a/mobile/themes/core/browser.css b/mobile/themes/core/browser.css

Add to gingerbread css

>+.webapps-noperm description:last-child{
>+  color: rgb(192, 0, 0);

No color, I think

>+  font-weight: bold;

Not sure about bold either. Lets leave it off

>diff --git a/mobile/themes/core/platform.css b/mobile/themes/core/platform.css
> 
>+dialog > .prompt-section {
>+  -moz-border-radius: 0;
>+}

This should be in gingerbread css too, if we need ti at all
Comment 36 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-17 22:23:54 PDT
Created attachment 540209 [details] [diff] [review]
patch 2

This patch needs the patch in bug 663571

* Addresses review comments
  * Removes all of the chromeless window changes
  * Add CSS for gingerbread
  * Tweaks to the install dialog (scrollbox and other layout to be more like other dialogs)
* Found a CSS issue with button-checkbox style where the button moved on :hover. Needed to fix the padding
Comment 37 Wesley Johnston (:wesj) 2011-06-20 13:47:23 PDT
Comment on attachment 540209 [details] [diff] [review]
patch 2

Review of attachment 540209 [details] [diff] [review]:
-----------------------------------------------------------------

I think there's a bug with the CID for the WebappsSupport service that keeps this from working. With that fixed, this works.

I know this is just a part of a larger patch, but having multiple Fennec windows open, but not available to flip through really really confusing. I think I would prefer if (for now) BrowserCLH just opened the webapp in the default browser window until we have a way to open "chromeless" webapps of some sort (with whatever webapp specific UI we decided to give them), but a big part of that may just be my expectations for how this works.

::: mobile/chrome/content/browser.js
@@ +339,5 @@
> +      
> +      // check if the second argument is "webapp"
> +      if ((window.arguments.length > 1) && (window.arguments[1] == "webapp"))
> +        document.documentElement.setAttribute("webapp", commandURL);
> +    }

Depending on order seems fragile to me.

@@ +375,5 @@
> +    winEnum.getNext();
> +    if (winEnum.hasMoreElements())
> +      resizeHandler({ target: window });
> +#endif
> +

We have additional code to do this at:

http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/browser.js#384

Can we remove it now?

::: mobile/chrome/content/common-ui.js
@@ +1277,5 @@
> +      elem.classList.add("webapps-perm");
> +    });
> +
> +    BrowserUI.pushPopup(this, this._dialog);
> +    this._dialog.waitForClose();

We're not doing anything after this waitForClose. Do we need this?

@@ +1326,5 @@
> +        uri: browser.currentURI.spec,
> +        name: browser.contentTitle,
> +        icon: icon
> +      });
> +    }

Is there any reason to do this each time updateWebappsInstall is called?

@@ +1348,5 @@
> +      canvas.parentNode.removeChild(canvas);
> +      canvas = null;
> +      try {
> +        let webapp = Cc["@mozilla.org/webapps/support;1"].getService(Ci.nsIWebappsSupport);
> +        webapp.installApplication(aTitle, aURI, aIcon, data);

I think this might be @mozilla.org/webapps/installer;1

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/build/nsToolkitCompsCID.h#123

With that and the widget installed this works for me.

::: mobile/themes/core/browser.css
@@ +1333,5 @@
> +}
> +
> +.webapps-perm description:last-child {
> +  display: none;
> +}

Nitpicky, but is there a reason that we aren't using a class or something here? I think its better to save using last-child where there's a good reason something is the last child (i.e. the last item of a list).

::: mobile/themes/core/gingerbread/browser.css
@@ +1317,5 @@
> +}
> +
> +.webapps-perm description:last-child {
> +  display: none;
> +}

Same here.
Comment 38 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-21 14:55:17 PDT
(In reply to comment #37)

> I think there's a bug with the CID for the WebappsSupport service that keeps
> this from working. With that fixed, this works.

You need patch in bug 663571 applied too

> I know this is just a part of a larger patch, but having multiple Fennec
> windows open, but not available to flip through really really confusing. I
> think I would prefer if (for now) BrowserCLH just opened the webapp in the
> default browser window until we have a way to open "chromeless" webapps of
> some sort (with whatever webapp specific UI we decided to give them), but a
> big part of that may just be my expectations for how this works.

Definitely food for thought. Let's get this landed so we can get more feedback.

> ::: mobile/chrome/content/browser.js
> @@ +339,5 @@
> > +      
> > +      // check if the second argument is "webapp"
> > +      if ((window.arguments.length > 1) && (window.arguments[1] == "webapp"))
> > +        document.documentElement.setAttribute("webapp", commandURL);
> > +    }
> 
> Depending on order seems fragile to me.

Yeah. Changed to window.arguments.indexOf("webapp")

> > +    winEnum.getNext();
> > +    if (winEnum.hasMoreElements())
> > +      resizeHandler({ target: window });
> > +#endif
> > +
> 
> We have additional code to do this at:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/browser.
> js#384
> 
> Can we remove it now?

I am testing without the opener check. I'll report back.

> ::: mobile/chrome/content/common-ui.js

> > +    BrowserUI.pushPopup(this, this._dialog);
> > +    this._dialog.waitForClose();
> 
> We're not doing anything after this waitForClose. Do we need this?

We need it for a modal dialog. Otherwise the dialog closes if you tap outside of it.

> > +        uri: browser.currentURI.spec,
> > +        name: browser.contentTitle,
> > +        icon: icon
> > +      });
> > +    }
> 
> Is there any reason to do this each time updateWebappsInstall is called?

Nope. I changed the code to only do the "webapp" check in WebappUI.updateWebappsInstall and moved the manifest creation to WebappUI.show

> > +        let webapp = Cc["@mozilla.org/webapps/support;1"].getService(Ci.nsIWebappsSupport);
> > +        webapp.installApplication(aTitle, aURI, aIcon, data);
> 
> I think this might be @mozilla.org/webapps/installer;1
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/build/
> nsToolkitCompsCID.h#123

We are not using that component. We are using an sqlite-based component to store the webapp entries.

I think we should remove the older "installer" code.

> ::: mobile/themes/core/browser.css

> > +.webapps-perm description:last-child {
> > +  display: none;
> > +}
> 
> Nitpicky, but is there a reason that we aren't using a class or something
> here? I think its better to save using last-child where there's a good
> reason something is the last child (i.e. the last item of a list).

done
Comment 39 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-21 14:55:57 PDT
Created attachment 540903 [details] [diff] [review]
patch 3

Enough changed to warrant a new review
Comment 40 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-21 20:13:58 PDT
Created attachment 540963 [details] [diff] [review]
patch 3 (real)

OK. I got the patches folded correctly now.
Comment 41 Wesley Johnston (:wesj) 2011-06-23 16:14:55 PDT
Comment on attachment 540963 [details] [diff] [review]
patch 3 (real)

Review of attachment 540963 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/chrome/content/browser.js
@@ +367,5 @@
>        ss.restoreLastSession(bringFront);
>      } else {
>        this.addTab(commandURL || this.getHomePage(), true);
>      }
>  

We should probably also skip all of the sessionstore stuff in this case as well.

::: mobile/chrome/content/common-ui.js
@@ +1348,5 @@
> +      let data = canvas.toDataURL("image/png", "");
> +      canvas.parentNode.removeChild(canvas);
> +      canvas = null;
> +      try {
> +        let webapp = Cc["@mozilla.org/webapps/support;1"].getService(Ci.nsIWebappsSupport);

This doesn't match what's in https://bug663571.bugzilla.mozilla.org/attachment.cgi?id=540126.

::: mobile/chrome/content/webapps.xul
@@ +49,5 @@
> +
> +  <commandset>
> +    <command id="cmd_cancel" oncommand="WebappsUI.hide();"/>
> +    <command id="cmd_ok" oncommand="WebappsUI.launch();"/>
> +  </commandset>

As a personal pet peeve, I'm hoping we can move away from these. If (for some reason) two dialogs are imported we run into id overlap. (i.e. bug 621506). But that should probably just be mine to fix in that bug.
Comment 42 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-05 22:17:41 PDT
Created attachment 544152 [details] [diff] [review]
patch with openURI changes

This patch simplifies the way we open the webapp a bit more: It loads the webapp in a tab, not in a new window. If the webapp is already open in a tab, we re-use it and do not open/load a new tab. The /mobile code changes to browser.js and BrowserCLH.js make this happen.

We add a new flag to nsIBrowserDOMWindow so we can get the special context.
Comment 43 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-05 22:21:19 PDT
Comment on attachment 544152 [details] [diff] [review]
patch with openURI changes

Boris - does the nsIBrowserDOMWindow.idl change look ok?
Comment 44 [:fabrice] Fabrice Desré 2011-07-06 00:40:23 PDT
Comment on attachment 544152 [details] [diff] [review]
patch with openURI changes

Review of attachment 544152 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with small canvas nit corrected

::: mobile/chrome/content/common-ui.js
@@ +1760,5 @@
> +    const kIconSize = 64;
> +    
> +    let canvas = document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
> +    canvas.setAttribute("style", "display: none");
> +    canvas = document.documentElement.appendChild(canvas);

no need to add the <canvas> to the DOM

@@ +1769,5 @@
> +      canvas.width = canvas.height = kIconSize; // clears the canvas
> +      let ctx = canvas.getContext("2d");
> +      ctx.drawImage(image, 0, 0, kIconSize, kIconSize);
> +      let data = canvas.toDataURL("image/png", "");
> +      canvas.parentNode.removeChild(canvas);

so we can also remove this line
Comment 45 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-06 09:19:53 PDT
> Boris - does the nsIBrowserDOMWindow.idl change look ok?

I can't make any sense of the comments, so no...
Comment 46 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-07 08:57:32 PDT
(In reply to comment #44)

> > +    let canvas = document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
> > +    canvas.setAttribute("style", "display: none");
> > +    canvas = document.documentElement.appendChild(canvas);
> 
> no need to add the <canvas> to the DOM

> >     let data = canvas.toDataURL("image/png", "");
> > +      canvas.parentNode.removeChild(canvas);
> 
> so we can also remove this line

Done.
Comment 47 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-07 09:01:00 PDT
Created attachment 544508 [details] [diff] [review]
patch without IDL changes

This patch has Fabrice's comments addressed. It also uses a slight hack to get the "open app tab" behavior we want. I create a fake openURI constant and use it. We discussed making a new API method in nsIBrowserDOMWindow to check to see if a tab was open with a given URI. If so, the impl would select it. If not, we would call openURI using the normal constants.

The problem with that is, we need to "tag" app tab - not non-app tabs. So we still need a context for openURI to know this URI is an app tab and not some normal website. Until we get that kind of API, the hack works fine.
Comment 48 [:fabrice] Fabrice Desré 2011-07-07 09:09:08 PDT
Comment on attachment 544508 [details] [diff] [review]
patch without IDL changes

Review of attachment 544508 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 49 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-07 15:00:35 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/5179f07def0c
Comment 50 Daniel Holbert [:dholbert] 2011-07-07 16:02:36 PDT
Backed out because this push turned Android talos perma-orange (in a way that can be caused by bugs in code, according to aki):
http://hg.mozilla.org/integration/mozilla-inbound/rev/cfada2de45d4
Comment 51 Daniel Holbert [:dholbert] 2011-07-07 16:29:39 PDT
(looks like the push turned all unittests orange, actually - talos was just the first to finish. http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=89ef5bf1e3d2 )
Comment 52 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-07 19:39:51 PDT
This patch was not the cause of the failures

http://hg.mozilla.org/integration/mozilla-inbound/rev/3e5c94427f4a
Comment 53 Marco Bonardo [::mak] 2011-07-08 05:59:19 PDT
http://hg.mozilla.org/mozilla-central/rev/3e5c94427f4a
Comment 54 Staś Małolepszy :stas 2011-07-15 23:38:54 PDT
The following strings are really hard to localize:

> <description class="prompt-checkbox-label" flex="1">&webapps.perm.geolocation;</description>
> <description class="prompt-checkbox-label webapps-perm-requested-hint" id="webapps-geoloc-app">&webapps.perm.requested;</description>

> <description class="prompt-checkbox-label" flex="1">&webapps.perm.offline;</description>
> <description class="prompt-checkbox-label webapps-perm-requested-hint" id="webapps-offline-app">&webapps.perm.requested;</description>

> <description class="prompt-checkbox-label" flex="1">&webapps.perm.notifications;</description>
> <description class="prompt-checkbox-label webapps-perm-requested-hint" id="webapps-notifications-app">&webapps.perm.requested;</description>

Note that they all use &webapps.perm.requested; while the object that has been requested varies: geolocation, offline, notifications.  Depending on the gender and the grammatical number of these objects, in some languages (yay for Polish!) it might be required to change the form of the past participle "requested."

CC'ing Pike as I'm on vacation this week and can't really help a lot.
Comment 55 Axel Hecht [:Pike] 2011-07-16 00:18:48 PDT
Also noun-verb order is hard-coded, and maybe other things like whitespace.

I don't think that the screenshots here are current, can we get one?

Reopening, I suggest to back this out until we know what to do here.
Comment 56 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-16 07:14:20 PDT
(In reply to comment #55)
> Also noun-verb order is hard-coded, and maybe other things like whitespace.

Noun-verb order is not hard coded. The "requested" string is a hint, not part of the sentence. We could just as easily say "requested by application", but that is a bit long. Think of the "requested" as you would the "required" hint used in data form entry.

> I don't think that the screenshots here are current, can we get one?

I don't have a screenshot with the "requested" hint, but you can visualize it under the permission names in this screen shot:
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-webapp-02.png

> Reopening, I suggest to back this out until we know what to do here.

I don't think we need to reopen or back out. Let's discuss the nature of the hint and if we need to change the text to make it more hint-like, we can in a followup patch.
Comment 57 [:fabrice] Fabrice Desré 2011-07-18 03:59:16 PDT
Created attachment 546488 [details]
screenshot with permission requested

Here's a screenshot of the install dialog for Angry Birds : this app needs offline storage, so we add the "requested" hint if the permission is not checked.

We may need a better UX there, but I agree with Mark that we don't need to backout the patch.
Comment 58 Axel Hecht [:Pike] 2011-07-18 04:26:56 PDT
Comment on attachment 546488 [details]
screenshot with permission requested

And here I am, naively thinking that a screenshot would tell me what this is all about. Sadly, it does not. Not even to an extent that I could suggest a follow-up bug.
Comment 59 [:fabrice] Fabrice Desré 2011-07-18 06:09:39 PDT
Thanks for this constructive comment. It's not like if you could ping me on IRC to get additional details...

Now we know that you don't understand something, but what?
Comment 60 Axel Hecht [:Pike] 2011-07-18 06:31:54 PDT
If I'd see that screen, I'd do one of two things: if I wanted that webapp at all cost, I'd select all boxes and hit OK, and otherwise I'd hit cancel.

I wouldn't (and won't) reverse engineer why they're three checkboxens on one whatever-button.

Also, it's completely irrelevant for this UI if you can explain it to me on irc.
Comment 61 Zibi Braniecki [:gandalf][:zibi] 2011-08-04 17:02:53 PDT
l10n warning. the word "requested" added at the end of permission is extremely hard to localize to most latin locales (the passive form, one word etc.)

Could we limit the list of permissions to the ones that are actually requested by the app and by default check the ones that the app requests allowing the user to uncheck the ones that he does not want to give?

This would allow us to remove the "requested" flag in UI.
Comment 62 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-04 17:16:49 PDT
(In reply to comment #61)
> l10n warning. the word "requested" added at the end of permission is
> extremely hard to localize to most latin locales (the passive form, one word
> etc.)
> 
> Could we limit the list of permissions to the ones that are actually
> requested by the app and by default check the ones that the app requests
> allowing the user to uncheck the ones that he does not want to give?
> 
> This would allow us to remove the "requested" flag in UI.

We could look at changing the way permission are shown, but I wanted to point out again that "requested" is not at the end of the permission string.

The string is NOT:
[ ] Offline storage requested

The string is:
[ ] Offline storage
    requested

"requested" is NOT used to make an English sentence
"requested" is a hint used on a separate line

It's quite possible we'll rework this dialog after we get some feedback from users as well.
Comment 63 Zibi Braniecki [:gandalf][:zibi] 2011-08-05 01:00:03 PDT
@Mark: yes, I'm aware it'll be a separate line, but it's still hard to localize properly (in many latin-based languages you will want to adjust the form of the word "requested" to the sentence it refers to (Offline storage, etc.) AND to the app name (gender may differ between Angry Birds and Zenonia).
Comment 64 Staś Małolepszy :stas 2011-08-05 10:45:14 PDT
To be frank, I'm not sure about Latin languages.  If you mean Romance languages, I think there's a good chance this can be translate alright.  The word 'requested' can adjust to the form of the word 'permission.'  For example, if 'permission' is feminine, the word 'requested' would take its feminine form.  For this, I believe that the current UI should work well.  CC'ing Philippe, flod and Ricardo for their input.

I am concerned about Slavic languages.  I write this comment as the Polish localizer for Fennec.  In Polish, there is not such participle as 'requested' that is used here.  In Polish, 'requested' refers to the usage as in the following example: 'I was requested to do something,'  not as in 'Something was requested of me.'  When used in the passive voice, the Polish verb 'requested' means the person, not the object of the request.

I wonder if this is the same for other Slavic languages.  CC'ing Pavel, Matjaž, Vlado, Unghost, Milos and Tim for a quick survey.

Thanks in advance for chiming in.
Comment 65 [:rickiees] Ricardo Palomares 2011-08-05 12:16:37 PDT
(In reply to Staś Małolepszy :stas from comment #64)
> To be frank, I'm not sure about Latin languages.  If you mean Romance
> languages, I think there's a good chance this can be translate alright.  The
> word 'requested' can adjust to the form of the word 'permission.'  For
> example, if 'permission' is feminine, the word 'requested' would take its
> feminine form.  For this, I believe that the current UI should work well. 
> CC'ing Philippe, flod and Ricardo for their input.


Adding Guillermo as he is the actual localizer of Fennec.

First, Mark, you have had to point out twice that "requested" is not part of the string even with a sample screenshot, so maybe it's not so clear, don't you think so? Perhaps enclosing "requested" in parenthesis, or using bold face could stress the isolation between the permission names and the fact of the app having asked for them.

Second. I'm not so sure about making an implicit "permissions" in the string. I'm trying to imagine the string in Spanish and I think I would expect that "requested" for first and third permissions used a female gender, while the second one use a male one. Though we could live without two separate strings if absolutely needed, I guess, and I reckon that taking into account gender would double the list of strings.

JM2C
Comment 66 Alexander L. Slovesnik 2011-08-05 12:33:34 PDT
"requested" string is definitely confusing for user. It should be clear for user that it's separate string, so it should be enclosed in parenthesis or displayed in italic font.
Also it's not entirely clear to me who requested this permission, so I think it should be changed to "requested by app" or smth. like that.
I don't think that gender is problem for Russian, as it's implied that full string is "Permission is requested by app". So "requested" gender depends on "permission" gender (neuter for Russian).
Comment 67 Tim Babych 2011-08-05 12:51:12 PDT
As long as 'requested' under discussion will be separated from the permission name and is not supposed to make proper sentence, I do not think I will have problem translating that dialog into Ukrainian.

I am going to use words like "needed", "a must". They are in neuter gender.
Comment 68 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-05 13:33:28 PDT
Sounds like wrapping the word in parenthesis is a good improvement. I'll add a patch for that.
Comment 69 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-05 13:38:52 PDT
Created attachment 551142 [details] [diff] [review]
patch for permission string

"requested" -> "(requested)" and I updated the entity name
Comment 70 Francesco Lodolo [:flod] - OFFLINE Jun 26-Jul 3 2011-08-05 21:40:38 PDT
I would personally prefer an interface where requested permissions are displayed separated from optional ones*, but I don't see any problem translating that "requested" as referred to "permission" (even if implicit).

* Something like

Requested permissions
[x] aaaaa

Allow also access to
[] bbbbb
[] ccccc
Comment 71 Marco Bonardo [::mak] 2011-08-06 03:03:37 PDT
http://hg.mozilla.org/mozilla-central/rev/2ea2772ce148
Comment 72 Aaron Train [:aaronmt] 2011-08-07 14:19:18 PDT
Verified Fixed

Mozilla/5.0 (Android; Linux armv7l; rv:8.0a1) Gecko/20110807 Firefox/8.0a1 Fennec/8.0a1
Comment 73 Girish Sharma [:Optimizer] 2011-08-16 15:11:14 PDT
I have my Galaxy s2 with latest Nightly build of Fennec . 
On any site I get an option of install as app. After that it asks for permissions to allow. 
But after that nothing happens , and there is no way to view my installed apps.
I went through the comments on this bug , but all the comments lead to some kind of screen that show the list of installed apps . But I could not find one in my Nightly build.
Please help and solve my problem.
Comment 74 Aaron Train [:aaronmt] 2011-08-16 15:22:47 PDT
(In reply to scrapmachines from comment #73)
> I have my Galaxy s2 with latest Nightly build of Fennec . 
> On any site I get an option of install as app. After that it asks for
> permissions to allow. 
> But after that nothing happens , and there is no way to view my installed
> apps.
> I went through the comments on this bug , but all the comments lead to some
> kind of screen that show the list of installed apps . But I could not find
> one in my Nightly build.
> Please help and solve my problem.

That would be bug 670677, to make things more discoverable. If you're looking for them now, on your Android homescreen access the shortcuts menu (tap and hold, or menu), and look for 'Nightly Web Apps'.

Note You need to log in before you can comment on or make changes to this bug.