585958 - webapps OS level integration : Maemo

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

Each webapp should appear as an entry in the application list at the OS level.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: WIP

First pass at OS level support, with support for linux & maemo only :
- when adding or removing a web app, it is added to the OS application list
- the CLH recognizes --app=URI as a new parameter type, and opens a new window in this case.

For now, opening a web app when fennec is not already running doesn't work (Browser.startup() doesn't get called)

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: WIP 2

This patch provides :
* install/uninstall launcher (Linux/Maemo)

* correct command line handling
  - webapps always open their own windows.
  - all other uris are grouped in the "real" fennec window.

* minimal UI for webapps
  - show the content and a stripped down title bar + favicon.
  - always hide the tab and tools bar.
  - update the window's title with the content title, to not get 10 "fennec"
    windows.

Comment 3

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

Comment on attachment 465661 [details] [diff] [review]
WIP 2


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

>     this._setURI(caption);
>+    if (Browser.isApp)
>+      window.document.title = caption;

Why?

>+  enterAppMode: function() {
>+    this._edit.removeEventListener("click", this, false);
>+    this._edit.removeEventListener("mousedown", this, false);
>+    this._edit.disabled = true;
>+    let idBox = document.getElementById("identity-box");
>+    idBox.removeAttribute("onclick");
>+    idBox.removeAttribute("onkeypress");
>+    document.getElementById("urlbar-icons").style.display = "none";
>+  },

We'll probably need more than this. For example, we don't want the tabsidebar to be visible, I think.

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

>+          // check for "app" flag
>+          var appFlag = cmdLine.handleFlagWithParam("app", false);

I think we should try to get this handler in the BrowserCLH, where we handle other commands. The CLH is also called while the app is running.

Also, let's use "webapp" as the flag. Mozilla already has an "app" flag.

>     // away panning for the total scroll offset.
>     doffset.add(panOffset);
>     Browser.tryFloatToolbar(doffset.x, 0);
>-    this._panScroller(Browser.controlsScrollboxScroller, doffset);
>+    if (!Browser.isApp)
>+      this._panScroller(Browser.controlsScrollboxScroller, doffset);

Why?

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

>-      win = Services.wm.getMostRecentWindow("navigator:browser");
>+    // we can only reuse the single "navigator:browser" window wich is not displaying a webapp

I wonder if we should change the windowtype to "navigator:webapp" of something. Need to check to see what Prism had to do (I forget)

>+    // web app support
>+    let app = aCmdLine.handleFlagWithParam("app", false);
>+    if (app) {
>+      let uri = resolveURIInternal(aCmdLine, app);
>+      if (uri) {
>+        win.browserDOMWindow.openURI(uri, null, Ci.nsIBrowserDOMWindow.OPEN_NEWWINDOW, null);
>+        return;

OK, you have some support in the CLH, so I am wondering why you need any support in browser.js?

>diff --git a/modules/webapps.jsm b/modules/webapps.jsm
>         itemIds.forEach(function(aItem) {
>-		  if (bkmsvc.getFolderIdForItem(aItem) == self.rootId)
>+		  if (bkmsvc.getFolderIdForItem(aItem) == self.rootId) {
> 		    bkmsvc.removeItem(aItem);
>+        self._removeFromOS(aURI, aItem);
>+      }

You have some TABs in this patch
>+    let execd = Cc["@mozilla.org/file/directory_service;1"].  
>+                getService(Ci.nsIProperties).get("CurProcD", Ci.nsIFile);
>+    execd.append("fennec");
>+    let foStream = Cc["@mozilla.org/network/file-output-stream;1"].  
>+                    createInstance(Ci.nsIFileOutputStream);
>+    foStream.init(file, 0x02 | 0x08 | 0x20, 0666, 0); 
>+    let converter = Cc["@mozilla.org/intl/converter-output-stream;1"].
>+                    createInstance(Ci.nsIConverterOutputStream);

Feel free to not wrap these lines

>+      wbp.persistFlags &= ~Components.interfaces.nsIWebBrowserPersist.PERSIST_FLAGS_NO_CONVERSION;

Ci

>+    let file = Cc["@mozilla.org/file/directory_service;1"].  
>+                  getService(Ci.nsIProperties).get("Home", Ci.nsIFile);

No wrap


Just a quick first pass. I'll look deeper later (and at the other patches too)

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to comment #3)
> Comment on attachment 465661 [details] [diff] [review]
> WIP 2
> 
> 
> >diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
> 
> >     this._setURI(caption);
> >+    if (Browser.isApp)
> >+      window.document.title = caption;
> 
> Why?

This sets the window title to the document's title. Without this, all the webapps windows (and the main fennec one) have the same title ("Fennec") and are hard to distinguish in the WM.

> >+  enterAppMode: function() {
> >+    this._edit.removeEventListener("click", this, false);
> >+    this._edit.removeEventListener("mousedown", this, false);
> >+    this._edit.disabled = true;
> >+    let idBox = document.getElementById("identity-box");
> >+    idBox.removeAttribute("onclick");
> >+    idBox.removeAttribute("onkeypress");
> >+    document.getElementById("urlbar-icons").style.display = "none";
> >+  },
> 
> We'll probably need more than this. For example, we don't want the tabsidebar
> to be visible, I think.

Sure, but it's taken care elsewhere

 
> >     // away panning for the total scroll offset.
> >     doffset.add(panOffset);
> >     Browser.tryFloatToolbar(doffset.x, 0);
> >-    this._panScroller(Browser.controlsScrollboxScroller, doffset);
> >+    if (!Browser.isApp)
> >+      this._panScroller(Browser.controlsScrollboxScroller, doffset);
> 
> Why?

This prevents the tabs and tools sidebars to appear.
 
> >diff --git a/components/BrowserCLH.js b/components/BrowserCLH.js
> 
> >-      win = Services.wm.getMostRecentWindow("navigator:browser");
> >+    // we can only reuse the single "navigator:browser" window wich is not displaying a webapp
> 
> I wonder if we should change the windowtype to "navigator:webapp" of something.
> Need to check to see what Prism had to do (I forget)
> 

I tried this, but it didn't work - this needs thorough investigation however since  it could simplify the command line handling.

> >+    // web app support
> >+    let app = aCmdLine.handleFlagWithParam("app", false);
> >+    if (app) {
> >+      let uri = resolveURIInternal(aCmdLine, app);
> >+      if (uri) {
> >+        win.browserDOMWindow.openURI(uri, null, Ci.nsIBrowserDOMWindow.OPEN_NEWWINDOW, null);
> >+        return;
> 
> OK, you have some support in the CLH, so I am wondering why you need any
> support in browser.js?

I found this needed to support all these scenarii: 
- start fennec, then open one or several web apps.
- start a web app, then open another one or the "classic" fennec.
- launch a web app from within fennec.

Thanks for the feedback!

Comment 5

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

OK, OK - Good answers, although we should work on pushing all commandline stuff to BrowserCLH and we should try to avoid Browser.isApp (having to check means it's easy for people to break the "app" mode without knowing)

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: WIP 3

Addresses previous comments :
- no more Browser.isApp
- new windows have their own windowtype (navigator:webapp)

The chromeless mode is also improved, with deactivated commands when needed.

Current patch supports linux desktop & maemo. On maemo, webapps appear in the application list, but not in the shortcut list so can't be put on the desktop. This is tracked in https://bugs.maemo.org/show_bug.cgi?id=11163

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: TAKE 2 - WIP 1

Web apps installation support for Maemo. We create a .deb file which is then installed by the application manager.

The deb is build by :
- creating all the needed files in a temporary directory.
- create the control.tar.gz & data.tar.gz files by calling /bin/tar
- finally create the .deb ourselves in JS since |dpkg-deb -b| is broken on Maemo (it expects GNU tar which is not what busybox provides)

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: TAKE 2 - WIP 2

This patch splits up the Debian packaging in its own module, and fixes a couple of bugs :
- be sure to generate a valid package name.
- fallback to default favicon in chrome if loading the image fails.

Note: this patch works also on ubuntu, with the limitation that the icon doesn't show up in the menus, since maemo and ubuntu look for icons in different directories.

Comment 9

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

Comment on attachment 468678 [details] [diff] [review]
TAKE 2 - WIP 2


>diff --git a/modules/Makefile.in b/modules/Makefile.in

> EXTRA_JS_MODULES = \
>   linuxTypes.jsm \
>+  debian.jsm \

We should think about our naming convention for our modules. "linuxTypes.jsm" is kinda vague, considering what is in the file."debian.jsm" is vague too. "debianPackager.jsm" ?

>diff --git a/modules/debian.jsm b/modules/debian.jsm
>+  build: function(aBaseDir, aPackageDir, aPackageName, aControl) {
>+      }
>+    }
>+    // create control.tar.gz and data.tar.gz
>+    let controlTgz = this._createArchive(controlDir, aBaseDir, "control.tar.gz");

Use a blank line to separate some of the sections a little bit more. Just a little, here and there.

>+      let header = "!<arch>\n";
>+      // we need a buffered stream to use the writeFrom method
>+      let boStream = Cc["@mozilla.org/network/buffered-output-stream;1"].createInstance(Ci.nsIBufferedOutputStream);

Here too (above the comment)

>+      boStream.write(header, header.length);
>+      // hard code the debian-binary file entry, since tt's a simple text file
>+      this._appendPaddedString(boStream, "debian-binary", 16);

and here

>diff --git a/modules/webapps.jsm b/modules/webapps.jsm
>   _install: function(aURI, aTitle, aIconURI, aIconData) {

>+    let tmpDir = Cc["@mozilla.org/file/directory_service;1"].  
>+                  getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile);

No wrap needed

>+    while (base64.length >= 64) {
>+      iconString += " " + base64.substring(0, 64) + "\n";
>+      base64 = base64.substring(64);
>+    }
>+    if (base64.length > 0)
>+      iconString += " " + base64 + "\n";

Blank line before "if"

>+    let aControl = {
>+      "Package": "moz-" + safeName,

You use the "moz-" prefix a few times, to create a unqiue name for the package, desktop and icon, I think - maybe more. I wonder if we should use something more like "webapp-"? I also wonder if these strings will be seen by the user during the install or uninstall?

>+    let iconListener = {
Add a short comment about why we need the listener, so other know why it's here

>+      QueryInterface: function(aIID) {
>+        if (aIID.equals(Ci.nsIWebProgressListener) ||
>+          aIID.equals(Components.interfaces.nsISupports))

No wrap

>+      onLocationChange: function() {return 0;},
>+      onProgressChange: function() {return 0;},
>+      onStatusChange: function() {return 0;},
>+      onSecurityChange: function() {return 0;},

Can we just remove the "return 0;" ?

>+    file.append("moz-" + safeName + "." + ext);
>+    let wbp = Cc['@mozilla.org/embedding/browser/nsWebBrowserPersist;1'].createInstance(Ci.nsIWebBrowserPersist);
>+    wbp.persistFlags = Ci.nsIWebBrowserPersist.PERSIST_FLAGS_REPLACE_EXISTING_FILES | Ci.nsIWebBrowserPersist.PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION;
>+    wbp.progressListener = iconListener;
>+    let ios = Cc['@mozilla.org/network/io-service;1'].getService(Ci.nsIIOService);
>+    wbp.saveURI(ios.newURI(uri, "UTF8", null), null, null, null, null, file);

Add a blank line separator and a comment about what the webbrowserpersist is doing, actually saving the icon to a file

Great, clean patch! Just small nits, nothing major.

Comment 10

[[:fabrice] Fabrice Desré]

Attachment: addressing comments

I renamed the packager module to debianPackager.jsm, and addressed other comments.

About :

> You use the "moz-" prefix a few times, to create a unqiue name for the package,
> desktop and icon, I think - maybe more. I wonder if we should use something
> more like "webapp-"? I also wonder if these strings will be seen by the user
> during the install or uninstall?

Indeed the "moz-" prefix is used to create unique names for debian packages names, desktop files and icons. I'm not sure we should use the more generic "webapp-" or not : there is theoretically more chance of name conflicts with "webapp-". Chrome on linux uses a "chrome-" prefix, so using either we're safe with this.

The user never sees these ugly names. Both in the debian control file and in the desktop file we set an appropriate user-friendly display name, which is the application name that you can change when installing the web app.

Comment 11

[[:fabrice] Fabrice Desré]

Attachment: addressing comments

uploading correct version of the patch

Comment 12

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

Comment on attachment 472347 [details] [diff] [review]
addressing comments

OK, thanks for the "moz-" explanation.

Comment 13

[[:fabrice] Fabrice Desré]

Attachment: updated patch

Patch updated to move to a component based approach. No change in the code.

Comment 14

[[:fabrice] Fabrice Desré]

Attachment: updated patch on current m-b

Comment 15

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

Comment on attachment 488237 [details] [diff] [review]
updated patch on current m-b

Let's not install any of the Maemo/Linux webapps components for Android. We should be able to just skip the components/webapps folder for Android, right?

Comment 16

[[:fabrice] Fabrice Desré]

Attachment: better android filtering

Prevents building components/webapps on android, and also remove linuxTypes.jsm and debianPackager.jsm from android.

Comment 17

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

Moving to post fennec4

Comment 18

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

tickly to mfinkle are we ready to look at this one?

Comment 19

[Kevin Brosnan [Ex-Mozilla]]

Mameo/MeeGo is not being developed by Mozilla. This patch and bug will be driven by the MeeGo team. It is unclear if they have any plans in turning this into submitted code.

Comment 20

[Justin Dolske [:Dolske]]

Shall we just WONTFIX this unless someone wants to be an active owner here? I don't see why we'd want to waste time on this.

Comment 21

[Kevin Brosnan [Ex-Mozilla]]

Romaxa works on the community version of Firefox for Mameo/Meego. Thoughts?

Comment 22

[[:fabrice] Fabrice Desré]

(In reply to Kevin Brosnan [:kbrosnan] from comment #21)
> Romaxa works on the community version of Firefox for Mameo/Meego. Thoughts?

Also, some non-UI parts (the .deb generator) could be reused for desktop linux on some distributions.

Comment 23

[Oleg Romashin (:romaxa)]

> Also, some non-UI parts (the .deb generator) could be reused for desktop
> linux on some distributions.
IIUC we should generate deb package on fly, and invoke system app handler, which would suggest to install package.

Comment 24

[[:fabrice] Fabrice Desré]

(In reply to Oleg Romashin (:romaxa) from comment #23)
> IIUC we should generate deb package on fly, and invoke system app handler,
> which would suggest to install package.

Yes, that's exactly what this patch provided for xul fennec on maemo.

Comment 25

[Oleg Romashin (:romaxa)]

so what prevents us to land it?

Comment 26

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

sec review is won't fix so removing the flag here

Comment 27

[BMO Automation]

Closing all opened bug in a graveyard component