Closed Bug 584767 Opened 14 years ago Closed 13 years ago

webapps frontend

Categories

(Firefox for Android Graveyard :: General, enhancement)

x86
All
enhancement
Not set
normal

Tracking

(firefox8 fixed, fennec8+)

VERIFIED FIXED
Firefox 8
Tracking Status
firefox8 --- fixed
fennec 8+ ---

People

(Reporter: fabrice, Assigned: mfinkle)

References

Details

Attachments

(3 files, 27 obsolete files)

34.96 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
89.52 KB, image/png
Details
3.11 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
Implement the frontend pieces to expose webapps in Fennec.
Blocks: 583750
Attached patch WIP (obsolete) — Splinter Review
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.
Assignee: nobody → fabrice
Attachment #463218 - Flags: ui-review?(madhava)
Attached image Site menu entry (obsolete) —
Attached image awesomescreen (obsolete) —
Attached image webapps list (obsolete) —
Attached patch WIP 2 (obsolete) — Splinter Review
This patch moves from a list view to a grid with icons and labels.
Attachment #463218 - Attachment is obsolete: true
Attachment #463579 - Flags: ui-review?(madhava)
Attachment #463218 - Flags: ui-review?(madhava)
Attached image webapps screen (obsolete) —
Attachment #463224 - Attachment is obsolete: true
Attachment #463580 - Flags: ui-review?(madhava)
Fabrice - Note that the awesomescreen is changing, very soon:
https://wiki.mozilla.org/Mobile/Projects/AwesomeScreen2.0
Depends on: 584765
Attached patch WIP 3 (obsolete) — Splinter Review
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.
Attachment #463579 - Attachment is obsolete: true
Attachment #464053 - Flags: ui-review?(madhava)
Attachment #463579 - Flags: ui-review?(madhava)
Attachment #464053 - Flags: ui-review?(madhava)
Attached image awesomescreen (obsolete) —
Attachment #463222 - Attachment is obsolete: true
Attached image webapps list (obsolete) —
Attachment #463580 - Attachment is obsolete: true
Attachment #464056 - Flags: ui-review?(madhava)
Attachment #463580 - Flags: ui-review?(madhava)
Attached patch WIP 4 (obsolete) — Splinter Review
Updated patch that will open a web app in a new window.
Attachment #464053 - Attachment is obsolete: true
Depends on: 586624
Attached patch WIP 5 (obsolete) — Splinter Review
Diff from previous version :
- icons are scaled to the same size as the mask
- better favicon  detection
Attached patch TAKE 2 - WIP 1 (obsolete) — Splinter Review
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.
Attachment #465267 - Attachment is obsolete: true
Attachment #467595 - Attachment is obsolete: true
Attached patch updated patch on current m-b (obsolete) — Splinter Review
Attachment #468096 - Attachment is obsolete: true
Attached patch TAKE 2 - WIP 3 (obsolete) — Splinter Review
Adds a configuration dialog before installing the application, that allows to change the application title and set the permissions for offline mode and geolocation.
Attachment #468676 - Attachment is obsolete: true
Attached image composite screenshot (obsolete) —
Attachment #463221 - Attachment is obsolete: true
Attachment #464055 - Attachment is obsolete: true
Attachment #464056 - Attachment is obsolete: true
Attachment #464056 - Flags: ui-review?(madhava)
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.
Attachment #469046 - Flags: feedback+
Attached patch TAKE 2 - comments addressed (obsolete) — Splinter Review
> >@@ -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.
Attachment #469046 - Attachment is obsolete: true
Attachment #471876 - Flags: ui-review?(madhava)
Attachment #471876 - Flags: review?(mark.finkle)
Depends on: 593782
No longer depends on: 584765, 586624
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.
Attachment #471876 - Attachment is obsolete: true
Attachment #472392 - Flags: review?(mark.finkle)
Attachment #471876 - Flags: ui-review?(madhava)
Attachment #471876 - Flags: review?(mark.finkle)
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!!!!
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/
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
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #472392 - Attachment is obsolete: true
Attachment #473982 - Flags: review?(mark.finkle)
Attachment #472392 - Flags: review?(mark.finkle)
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
Attachment #473982 - Flags: review?(mark.finkle) → review-
Attached patch addressing comments (obsolete) — Splinter Review
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.
Attachment #473982 - Attachment is obsolete: true
Attachment #475553 - Flags: review?(mark.finkle)
Comment on attachment 475553 [details] [diff] [review]
addressing comments

looks good
Attachment #475553 - Flags: review?(mark.finkle) → review+
Attached patch updated patch (obsolete) — Splinter Review
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
Attachment #469047 - Attachment is obsolete: true
Attachment #475553 - Attachment is obsolete: true
Attachment #476764 - Flags: review?(mark.finkle)
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.
Attachment #476764 - Flags: review?(mark.finkle) → review-
Attached patch updated patch (obsolete) — Splinter Review
Changes implementing comment 29
Attachment #476764 - Attachment is obsolete: true
Attachment #477170 - Flags: review?(mark.finkle)
Attachment #477170 - Flags: review?(mark.finkle) → review+
Whiteboard: [fennec-checkin-postb1]
tracking-fennec: --- → ?
Whiteboard: [fennec-checkin-postb1]
tracking-fennec: ? → 2.0+
tracking-fennec: 2.0+ → 2.0b3+
Attached patch updated patch on current m-b (obsolete) — Splinter Review
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.
Attachment #477170 - Attachment is obsolete: true
Attachment #488233 - Flags: review?(mark.finkle)
Whiteboard: [has-patch]
Moving to post fennec4
tracking-fennec: 2.0b3+ → 2.0-
tracking-fennec: 2.0- → 2.0next+
tracking-fennec: 2.0next+ → 6+
tracking-fennec: 6+ → 7+
Attached patch updated patch (obsolete) — Splinter Review
Attachment #488233 - Attachment is obsolete: true
Attachment #488233 - Flags: review?(mark.finkle)
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 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
Attachment #538638 - Flags: review-
Attached patch patch 2 (obsolete) — Splinter Review
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
Assignee: fabrice → mark.finkle
Attachment #538638 - Attachment is obsolete: true
Attachment #540209 - Flags: review?(wjohnston)
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.
Attachment #540209 - Flags: review?(wjohnston) → review+
(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
Attached patch patch 3 (obsolete) — Splinter Review
Enough changed to warrant a new review
Attachment #540209 - Attachment is obsolete: true
Attachment #540903 - Flags: review?(wjohnston)
Attached patch patch 3 (real) (obsolete) — Splinter Review
OK. I got the patches folded correctly now.
Attachment #540903 - Attachment is obsolete: true
Attachment #540963 - Flags: review?(wjohnston)
Attachment #540903 - Flags: review?(wjohnston)
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.
Attachment #540963 - Flags: review?(wjohnston) → review+
Attached patch patch with openURI changes (obsolete) — Splinter Review
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.
Attachment #540963 - Attachment is obsolete: true
Attachment #544152 - Flags: review?(fabrice)
Comment on attachment 544152 [details] [diff] [review]
patch with openURI changes

Boris - does the nsIBrowserDOMWindow.idl change look ok?
Attachment #544152 - Flags: review?(bzbarsky)
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
Attachment #544152 - Flags: review?(fabrice) → review+
> Boris - does the nsIBrowserDOMWindow.idl change look ok?

I can't make any sense of the comments, so no...
(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.
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.
Attachment #544152 - Attachment is obsolete: true
Attachment #544508 - Flags: review?(fabrice)
Attachment #544152 - Flags: review?(bzbarsky)
Comment on attachment 544508 [details] [diff] [review]
patch without IDL changes

Review of attachment 544508 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #544508 - Flags: review?(fabrice) → review+
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
(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 )
This patch was not the cause of the failures

http://hg.mozilla.org/integration/mozilla-inbound/rev/3e5c94427f4a
Whiteboard: [has-patch] → [inbound]
http://hg.mozilla.org/mozilla-central/rev/3e5c94427f4a
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
Depends on: 670677
Depends on: 671258
tracking-fennec: 7+ → 8+
Whiteboard: [inbound]
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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
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 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.
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?
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.
Depends on: 672391
Depends on: 673906
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.
(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.
@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).
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.
(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
"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).
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.
Sounds like wrapping the word in parenthesis is a good improvement. I'll add a patch for that.
"requested" -> "(requested)" and I updated the entity name
Attachment #551142 - Flags: review?(fabrice)
Attachment #551142 - Flags: review?(fabrice) → review+
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
Verified Fixed

Mozilla/5.0 (Android; Linux armv7l; rv:8.0a1) Gecko/20110807 Firefox/8.0a1 Fennec/8.0a1
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Flags: in-litmus? → in-litmus?(aaron.train)
Depends on: 678517
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.
(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'.
Flags: in-litmus?(aaron.train)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: