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

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)

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

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

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)

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

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)

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

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.

uploading correct version of the patch

Comment on attachment 472347 [details] [diff] [review] addressing comments OK, thanks for the "moz-" explanation.

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

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?

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

Moving to post fennec4

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

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.

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.

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

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

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

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

so what prevents us to land it?

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

Closing all opened bug in a graveyard component